phetsims / collision-lab

"Collision Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

Potentially remove PlayArea from Ball #161

Closed brandonLi8 closed 3 years ago

brandonLi8 commented 4 years ago

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

  1. Pass the PlayArea into BallNode only and have BallNode pass the play-area into the dragToPosition. This would remove the need for passing PlayArea into Ball.
  2. 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?

brandonLi8 commented 4 years ago

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.