phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Setting the max for the number of cups/plates #246

Closed marlitas closed 1 month ago

marlitas commented 1 month ago

Related to: https://github.com/phetsims/mean-share-and-balance/issues/187

Just be able to set the max. Call it maxCupsProperty and maxPlatesProperty. min: 1 max: 7

When we set this property, sim resets. Keep this consistent.

jbphet commented 1 month ago

I've got a few comments and observations on these changes. First, maxKicksProperty is created in BalancePointModel like this:

    const maxKicksProperty = new NumberProperty( MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_DATA_SETS, {
      range: new Range( 0, 7 ),
      tandem: options.tandem.createTandem( 'maxKicksProperty' )
    } );

The use of a hard-coded '7' seems odd. Why not use MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_DATA_SETS for the max value? It might make sense to change the name to now include DEFAULT.

Second, it is created here, then passed into BalancePointSceneModel and never directly used again in BalanceModel. Why create it in BalanceModel and not in BalancePointSceneModel?

Next, there is the following linkage in `BalancePointSceneModel:

    maxKicksProperty.link( maxKicks => {
      this.clearData();
      MeanShareAndBalanceConstants.NUMBER_OF_KICKS_RANGE_PROPERTY.value = new Range(
        MeanShareAndBalanceConstants.NUMBER_OF_KICKS_RANGE_PROPERTY.value.min,
        maxKicks
      );
    } );

This appears to be changing a value of what is presumably a constant from another file (MeanShareAndBalanceConstants). I don't think we should do this. There should just be an instance of numberOfKicksRangeProperty somewhere that can be accessed by all, but it shouldn't be presented as a constant if it isn't. If it needs to be fairly global, the pattern used for isResettingAllProperty in ResetAllButton might be helpful.

Back to @marlitas for next steps.

marlitas commented 1 month ago

The use of a hard-coded '7' seems odd.

It looks like I updated that to use MeanShareAndBalanceConstants.NUMBER_OF_KICKS_RANGE_PROPERTY.value in a later commit. And has been updated further to address the third request.

Second, [maxKicksProperty is created in BalancePointModel], then passed into BalancePointSceneModel and never directly used again in BalanceModel. Why create it in BalanceModel and not in BalancePointSceneModel?

I did this based on the use case precedent in Center and Variability. If we ever do want to add a second scene to Balance Point the maxKicksProperty should live in the model and not the sceneModel since it is not a scene specific Property. I agree it's a little strange since we only have one scene here, but it feels more accurate to the spirit of the Property and the scene model.

I went ahead and added the numberOfKicksRangeProperty as a static to BalancePointModel. @jbphet let me know what you think of the changes.

amanda-phet commented 1 month ago

This is working well on my end!

jbphet commented 1 month ago

Looks good to me. Closing.