Skip to content

Commit

Permalink
Document the need of unlink or dispose for #51
Browse files Browse the repository at this point in the history
  • Loading branch information
tmildemberger committed Feb 17, 2020
1 parent d9d75db commit 5afbfce
Show file tree
Hide file tree
Showing 12 changed files with 36 additions and 5 deletions.
1 change: 1 addition & 0 deletions js/common/model/Spring.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ define( require => {
this.rightMass = rightMass;

// @public {Property.<boolean>} determines the visibility of the spring
// dispose is unnecessary because all masses and springs exist for the lifetime of the sim
this.visibilityProperty = new DerivedProperty(
[ this.leftMass.visibilityProperty, this.rightMass.visibilityProperty ],
function( leftVisible, rightVisible ) {
Expand Down
2 changes: 2 additions & 0 deletions js/common/view/MassNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,12 @@ define( require => {
this.size = 20;

// @public {Property.<boolean>} determines the visibility of the MassNode
// TODO - this property is unnecessary (see https://github.com/phetsims/normal-modes/issues/45)
this.visibilityProperty = new DerivedProperty( [ this.mass.visibilityProperty ], function( massVisible ) {
return massVisible;
} );

// dispose is unnecessary, the MassNode and the dependencies exist for the lifetime of the sim
Property.multilink( [ this.mass.equilibriumPositionProperty, this.mass.displacementProperty ],
( massPosition, massDisplacement ) => {
this.translation = this.modelViewTransform.modelToViewPosition( massPosition.plus( massDisplacement ) );
Expand Down
2 changes: 2 additions & 0 deletions js/common/view/SpringNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ define( require => {
this.model = model;

// @public {Property.<boolean>} determines the visibility of the SpringNode
// dispose is unnecessary because the SpringNode and the dependencies exist for the lifetime of the sim
this.visibilityProperty = new DerivedProperty(
[ this.spring.visibilityProperty, this.model.springsVisibilityProperty ],
function( mySpringVisible, springsVisible ) {
Expand All @@ -61,6 +62,7 @@ define( require => {

let currentXScaling = 1;

// dispose is unnecessary because the SpringNode and the dependencies exist for the lifetime of the sim
Property.multilink(
[ this.spring.leftMass.equilibriumPositionProperty,
this.spring.leftMass.displacementProperty,
Expand Down
12 changes: 9 additions & 3 deletions js/one-dimension/model/OneDimensionModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ define( require => {
range: new Range( OneDimensionConstants.MIN_MODE_PHASE, OneDimensionConstants.MAX_MODE_PHASE )
} );

// dispose is unnecessary, since this class owns the dependency
this.modeFrequencyProperty[ i ] = new DerivedProperty( [ this.numVisibleMassesProperty ], function( numMasses ) {
const k = OneDimensionConstants.SPRING_CONSTANT_VALUE;
const m = OneDimensionConstants.MASSES_MASS_VALUE;
Expand All @@ -112,9 +113,6 @@ define( require => {
this.springs = new Array( MAX_SPRINGS );
this.createDefaultSprings();

this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) );
this.amplitudeDirectionProperty.link( this.changedAmplitudeDirection.bind( this ) );

// @public {Property.<number>} the index of the mass being dragged
this.draggingMassIndexProperty = new NumberProperty( 0, {
tandem: tandem.createTandem( 'draggingMassIndexProperty' )
Expand All @@ -124,6 +122,12 @@ define( require => {
this.arrowsVisibilityProperty = new BooleanProperty( true, {
tandem: tandem.createTandem( 'arrowsVisibilityProperty' )
} );

// unlink is unnecessary, exists for the lifetime of the sim
this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) );

// unlink is unnecessary, exists for the lifetime of the sim
this.amplitudeDirectionProperty.link( this.changedAmplitudeDirection.bind( this ) );
}

/**
Expand Down Expand Up @@ -183,6 +187,7 @@ define( require => {
for ( let i = 0; i < MAX_MASSES; i++ ) {
const visible = ( i <= defaultMassesNum );

// All the masses needed are created at once, and exist for the lifetime of the sim
this.masses[ i ] = new Mass( new Vector2( x, 0 ), visible, tandem.createTandem( 'mass' + i ) );

if ( x < xFinal ) {
Expand All @@ -197,6 +202,7 @@ define( require => {
*/
createDefaultSprings() {
for ( let i = 0; i < MAX_SPRINGS; i++ ) {
// All the springs needed are created at once, and exist for the lifetime of the sim
this.springs[ i ] = new Spring( this.masses[ i ], this.masses[ i + 1 ] );
}
}
Expand Down
2 changes: 2 additions & 0 deletions js/one-dimension/view/MassNode1D.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,11 @@ define( require => {
} );

this.addInputListener( this.dragListener );
// unlink is unnecessary, the MassNode1D and the depencency exist for the lifetime of the sim
this.model.arrowsVisibilityProperty.link( arrowsVisible => {
const callback = this.overUpCallback.bind( this );
if ( arrowsVisible ) {
// unlink is needed when the arrows become invisible
this.dragListener.isOverProperty.link( callback );
}
else {
Expand Down
2 changes: 2 additions & 0 deletions js/one-dimension/view/NormalModeSpectrumAccordionBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ define( require => {

super( contentNode, options );

// unlink is unnecessary, exists for the lifetime of the sim
model.phasesVisibilityProperty.link( phasesVisibility => {
for ( let i = 1; i < panelColumns.length; ++i ) {
const j = i - 1;
Expand Down Expand Up @@ -240,6 +241,7 @@ define( require => {
this.bottom = options.bottom;
} );

// unlink is unnecessary, exists for the lifetime of the sim
model.numVisibleMassesProperty.link( numMasses => {
for ( let i = 0; i < numMasses; i++ ) {
const k = OneDimensionConstants.SPRING_CONSTANT_VALUE;
Expand Down
2 changes: 2 additions & 0 deletions js/one-dimension/view/NormalModesAccordionBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ define( require => {
for ( let i = 0; i < normalModeGraphs.length; i++ ) {
normalModeGraphs[ i ] = new ModeGraphCanvasNode( model, i );

// dispose is unnecessary, exists for the lifetime of the sim
Property.multilink( [ model.timeProperty, model.modeAmplitudeProperty[ i ], model.modePhaseProperty[ i ] ], function( time, amp, phase ) {
normalModeGraphs[ i ].update();
} );
Expand All @@ -89,6 +90,7 @@ define( require => {

super( graphContainer, options );

// dispose is unnecessary, exists for the lifetime of the sim
Property.multilink( [ model.numVisibleMassesProperty, this.expandedProperty ], ( numMasses, isExpanded ) => {
graphContainer.children = normalModeGraphs.slice( 0, numMasses );
graphContainer.children.forEach( graph => graph.update() );
Expand Down
1 change: 1 addition & 0 deletions js/one-dimension/view/WallNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ define( require => {
} );
this.addChild( this.rect );

// dispose is unnecessary, the WallNode and the dependencies exist for the lifetime of the sim
Property.multilink( [ this.mass.equilibriumPositionProperty, this.mass.displacementProperty ], ( massPosition, massDisplacement ) => {
this.translation = this.modelViewTransform.modelToViewPosition( massPosition.plus( massDisplacement ) ).subtract( new Vector2( this.rect.rectWidth / 2, this.rect.rectHeight / 2 ) );
} );
Expand Down
8 changes: 6 additions & 2 deletions js/two-dimensions/model/TwoDimensionsModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ define( require => {
range: new Range( TwoDimensionsConstants.MIN_MODE_PHASE, TwoDimensionsConstants.MAX_MODE_PHASE )
} );

// dispose is unnecessary, since this class owns the dependency
this.modeFrequencyProperty[ i ][ j ] = new DerivedProperty( [ this.numVisibleMassesProperty ], function( numMasses ) {
const k = TwoDimensionsConstants.SPRING_CONSTANT_VALUE;
const m = TwoDimensionsConstants.MASSES_MASS_VALUE;
Expand Down Expand Up @@ -135,8 +136,6 @@ define( require => {
}
this.createDefaultSprings();

this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) );

// @public {Property.<number[]|null>} the indexes of the mass being dragged (an object with and 'i' and a 'j')
this.draggingMassIndexesProperty = new Property( null, {
tandem: tandem.createTandem( 'draggingMassIndexesProperty' )
Expand All @@ -151,6 +150,9 @@ define( require => {
this.amplitudeDirectionProperty = new EnumerationProperty( AmplitudeDirection, AmplitudeDirection.VERTICAL, {
tandem: tandem.createTandem( 'amplitudeDirectionProperty' )
} );

// unlink is unnecessary, exists for the lifetime of the sim
this.numVisibleMassesProperty.link( this.changedNumberOfMasses.bind( this ) );
}

/**
Expand Down Expand Up @@ -238,6 +240,7 @@ define( require => {
//TODO see https://github.com/phetsims/normal-modes/issues/17
// const massTandem = tandem.createTandem( 'mass' + i + '_' + j );
const massTandem = Tandem.OPTIONAL;
// All the masses needed are created at once, and exist for the lifetime of the sim
this.masses[ i ][ j ] = new Mass( new Vector2( x, y ), visible, massTandem );

if ( x < xFinal - xStep / 2 ) {
Expand All @@ -260,6 +263,7 @@ define( require => {
for ( let i = 0; i < MAX_SPRINGS; i++ ) {
for ( let j = 0; j < MAX_SPRINGS; ++j ) {

// All the springs needed are created at once, and exist for the lifetime of the sim
if ( i !== MAX_SPRINGS - 1 ) {
this.springsX[ i ][ j ] = new Spring( this.masses[ i + 1 ][ j ], this.masses[ i + 1 ][ j + 1 ] );
}
Expand Down
4 changes: 4 additions & 0 deletions js/two-dimensions/view/AmplitudeSelectorRectangle.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,19 @@ define( require => {
this.amplitudeChanged( axisAmplitudesProperty.get()[ row ][ col ].get(), amplitudeDirection );
};

// unlink is unnecessary, exists for the lifetime of the sim
model.modeXAmplitudeProperty[ row ][ col ].link( amplitude => {
this.amplitudeChanged( amplitude, AmplitudeDirection.HORIZONTAL );
} );
// unlink is unnecessary, exists for the lifetime of the sim
model.modeYAmplitudeProperty[ row ][ col ].link( amplitude => {
this.amplitudeChanged( amplitude, AmplitudeDirection.VERTICAL );
} );

// unlink is unnecessary, exists for the lifetime of the sim
model.numVisibleMassesProperty.link( this.numMassesChanged );

// unlink is unnecessary, exists for the lifetime of the sim
model.amplitudeDirectionProperty.link( this.amplitudeDirectionChanged );

const isNear = function( n1, n2 ) {
Expand Down
2 changes: 2 additions & 0 deletions js/two-dimensions/view/MassNode2D.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,11 @@ define( require => {
} );

this.addInputListener( this.dragListener );
// unlink is unnecessary, the MassNode2D and the depencency exist for the lifetime of the sim
this.model.arrowsVisibilityProperty.link( arrowsVisible => {
const callback = this.overUpCallback.bind( this );
if ( arrowsVisible ) {
// unlink is needed when the arrows become invisible
this.dragListener.isOverProperty.link( callback );
}
else {
Expand Down
3 changes: 3 additions & 0 deletions js/two-dimensions/view/NormalModeAmplitudesAccordionBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,17 @@ define( require => {

const amplitudeDirectionRadioButtonGroup = new AmplitudeDirectionRadioButtonGroup( model.amplitudeDirectionProperty );

// dispose is unnecessary, exists for the lifetime of the sim
const axisAmplitudesProperty = new DerivedProperty( [ model.amplitudeDirectionProperty ], amplitudeDirection => {
return ( amplitudeDirection === AmplitudeDirection.VERTICAL ) ? model.modeYAmplitudeProperty : model.modeXAmplitudeProperty;
} );

// dispose is unnecessary, exists for the lifetime of the sim
const maxAmpProperty = new DerivedProperty( [ model.numVisibleMassesProperty ], numMasses => {
return TwoDimensionsConstants.MAX_MODE_AMPLITUDE[ numMasses - 1 ];
} );

// dispose is unnecessary, exists for the lifetime of the sim
const gridSizeProperty = new DerivedProperty( [ model.numVisibleMassesProperty ], numMasses => {
return PANEL_SIZE / ( 1 + ( RECT_GRID_UNITS + PADDING_GRID_UNITS ) * numMasses );
} );
Expand Down

0 comments on commit 5afbfce

Please sign in to comment.