Potentially remove PlayArea from Ball
Closed this issue · 1 comments
For #153 @jonathanolson left a review comment:
//REVIEW: This feels like something that the ball itself should not be tracking. This is listed as a Property, but
//REVIEW: I see no link or observers listed for it. It's requiring an external playArea (which ideally we should be
//REVIEW: able to create a Ball without a playArea, but that's not a problem), but also assumes what
//REVIEW: playArea.containsAnyPartOfBall does. Furthermore, because that method uses the top/left/etc., the value of
//REVIEW: this COULD change based on the radiusProperty, BUT the radiusProperty is not included in the dependencies.
//REVIEW: Having this be a check on the PlayArea or elsewhere can prevent this class of bug, and I don't see an
//REVIEW: advantage to having a Property to track this.
// @public {DerivedProperty.<boolean>} - indicates if ANY part of the Ball is inside the PlayArea's bounds.
this.insidePlayAreaProperty = new DerivedProperty( [ this.positionProperty ],
() => playArea.containsAnyPartOfBall( this ),
@jonathanolson this Property is indeed used in BallSystem:
// @public {DerivedProperty.<boolean>} - indicates if all of the Balls in the system are NOT inside of the PlayArea.
// Uses the insidePlayAreaProperty of all possible Balls but only the Balls in
// the system are considered in the derivation function.
this.ballsNotInsidePlayAreaProperty = new DerivedProperty(
[ this.balls.lengthProperty, ...this.prepopulatedBalls.map( ball => ball.insidePlayAreaProperty ) ],
length => this.balls.every( ball => !ball.insidePlayAreaProperty.value ), {
valueType: 'boolean'
} );
This Property is used for the 'Return Balls' button when every ball has escaped the PlayArea.
Although I agree that it doesn't feel like ball should have to be created with a PlayArea passed-in. It also feels like the this.ballsNotInsidePlayAreaProperty
should belong to PlayArea, with the BallSystem passed into it.
Currently, BallSystem requires a PlayArea only to pass it into Ball, which uses it for this Property and also the dragToPosition
method.
One proposal for removing this could be
- Pass the PlayArea into
BallNode
only and have BallNode pass the play-area into thedragToPosition
. This would remove the need for passing PlayArea into Ball. - Change the order or creation to create the BallSystem first and pass that into PlayArea. Then, move the
this.ballsNotInsidePlayAreaProperty
to PlayArea (which would observe the position/radius properties of the prepopulatedBalls).
@jonathanolson thoughts?
I also remembered that the insidePlayAreaProperty
is used in two listeners in BallSystem.
// Observe when Balls are added to the system and save the states of all balls in the system. Listener lasts for the
// life-time of the simulation since BallSystems are never disposed.
this.balls.elementAddedEmitter.addListener( ball => {
this.balls.forEach( ball => ball.insidePlayAreaProperty.value && ball.saveState() );
} );
// Observe when the user is done controlling any of the Balls to:
// 1. Save the states of all Balls that are both in the system and fully inside the PlayArea's bounds.
// 2. Clear the trailing Paths of all Balls and the Path of the CenterOfMass.
// 3. Reset the rotation of Balls relative to their centers.
//
// Link lasts for the life-time of the sim as BallSystems are never disposed.
this.ballSystemUserControlledProperty.lazyLink( ballSystemUserControlled => {
if ( !ballSystemUserControlled ) {
this.balls.forEach( ball => {
// Save the state of each Ball in the BallSystem that is fully inside the PlayArea.
ball.insidePlayAreaProperty.value && ball.saveState();
ball.path.clear();
ball.rotationProperty.reset();
} );
this.centerOfMass.path.clear();
}
} );
If we go with the proposal above, these could be moved to PlayArea.