Improved pattern for registerAddedBall

[jonathanolson]

I saw a pattern repeated a few times in the various registerAddedBall methods (TotalKineticEnergyProperty, CenterOfMass, BallValuesPanelColumnNode) that could potentially be improved.

A stripped-down (no docs, etc.) example of what this could look like would be in TotalKineticEnergyProperty:

class TotalKineticEnergyProperty extends NumberProperty {
  constructor( balls ) {
    super( 0, {
      isValidValue: value => value >= 0
    } );

    // @private {function}
    this.recomputeListener = () => {
      // OR inline this into the compute function
      this.value = this.computeTotalBallKineticEnergy();
    };

    balls.addItemAddedListener( this.onBallAdded.bind( this ) );
    balls.addItemRemovedListener( this.onBallRemoved.bind( this ) );
    balls.forEach( ball => this.onBallAdded( ball ) );
  }

  computeTotalBallKineticEnergy() { /* ... */ }

  onBallAdded( ball ) {
    ball.kineticEnergyProperty.link( this.recomputeListener );
  }
  onBallRemoved( ball ) {
    ball.kineticEnergyProperty.unlink( this.recomputeListener );
  }
}

where instead of adding multiple listeners to the ObservableArray (with checks to see if it's the same ball), we can just have one listener that gets added to each. The general onBallAdded/onBallRemoved can be used for other actions (e.g. BallValuesPanelColumnNode). We don't need to store the balls in a property, or do a ew other things.

For TotalKineticEnergyProperty in particular, things could be inlined into the constructor due to simplicity:

class TotalKineticEnergyProperty extends NumberProperty {
  constructor( balls ) {
    super( 0, {
      isValidValue: value => value >= 0
    } );

    const recompute = () => {
      this.value = _.sum( balls.map( ball => ball.kineticEnergy ) );
    };

    const onBallAdded = ball => ball.kineticEnergyProperty.link( this.recomputeListener );
    const onBallRemoved = ball => ball.kineticEnergyProperty.unlink( this.recomputeListener );

    balls.addItemAddedListener( onBallAdded );
    balls.addItemRemovedListener( onBallRemoved );
    balls.forEach( onBallAdded );
  }
}

OR if we change the design, we could reconsider writing that type as an inline DerivedProperty:

new DerivedProperty( prepopulatedBalls.map( ball => ball.kineticEnergyProperty ), () => {
  return _.sum( balls.map( ball => ball.kineticEnergy ) );
} );

(since the CPU time to recompute if a non-added ball changes is unlikely to be a factor).

Let me know if these ideas or helpful, or if there's anything good to discuss related to it.

[brandonLi8]

Thanks @jonathanolson.

I really like the inlined DerivedProperty for TotalKineticEnergyProperty!

That also makes me wonder if I could use a similar strategy for CenterOfMass, where I pass in the prepulatedBalls and the balls array and do something like

const massProperties = prepopulatedBalls.map( ball => ball.massProperty );
const positionProperties = prepopulatedBalls.map( ball => ball.positionProperty );
const velocityProperties = prepopulatedBalls.map( ball => ball.velocityProperty );

this.positionProperty = new DerivedProperty( [ ...massProperties, ...positionProperties ], () => {
  return this.updatePosition();
} );

this.velocityProperty = new DerivedProperty( [ ...massProperties, ...velocityProperties ], () => {
  return this.updateVelocity();
} );

There is no way for a non-added ball to change (since there is no UI for it to change and they aren't stepped so they can't collide), so I believe that this would, in the long-run, improve performance slightly, and it is more elegant. @jonathanolson what do you think?

EDIT: for the properties i wrote above and for the KineticEnergyProperty that you wrote with the inlinedProperty, we also need to add the numberOfBalls property as a dependency, since removing/adding a ball changes the total KE, and the COM position/velocity.

[brandonLi8]

As for BallValuesPanelColumnNode, I think ill switch to the onBallAdded and onBallRemoved pattern.

[brandonLi8]

I also noticed that the playAreaUserControlledProperty could be better written with a derived Property. I was already using the logic of listening to the prepopulatedBalls but only use the balls in the system in the calculation.

It is now written as

    // @public {DerivedProperty.<boolean>} - indicates if there are any Balls that are being controlled by the user. Use
    //                                       all possible userControlledProperties as dependencies to update the
    //                                       derivation but only the balls in the play-area are used in the calculation.
    this.playAreaUserControlledProperty = new DerivedProperty( ballUserControlledProperties, () => {
        return this.balls.some( ball => ball.userControlledProperty.value );
      }, { valueType: 'boolean' } );

[brandonLi8]

For the COM, the position/velocity aren't recomputed when the COM isn't visible for performance purposes.

Thus, now the positionProperty of the COM now looks like (done in the commit below)

    // @public (read-only) {DerivedProperty.<Vector2>} - Property of the position of the COM, in meter coordinates.
    //
    // For the dependencies, we use:
    //  - centerOfMassVisibleProperty; for performance reasons, the COM position isn't calculated if it isn't visible.
    //  - The position Properties of the prepopulatedBalls. Only the balls in the play-area are used in the calculation.
    //  - The mass Properties of the prepopulatedBalls. Only the balls in the play-area are used in the calculation.
    //  - balls.lengthProperty, since removing or adding a Ball changes the position of the COM.
    //
    // This DerivedProperty is never disposed and lasts for the lifetime of the sim.
    this.positionProperty = new DerivedProperty(
      [ centerOfMassVisibleProperty, ...ballMassProperties, ...ballPositionProperties, balls.lengthProperty ],
      centerOfMassVisible => {
        return centerOfMassVisible ? this.computePosition() : this.position; // Don't recompute if not visible.
      }, {
        valueType: Vector2
      } );

[brandonLi8]

All cases that used the registerAddedBall patterns have been removed and all substitutes have been documented. My work here is done. Back to @jonathanolson.

[jonathanolson]

Looks great, thanks!