search

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

Link to soccerBallsEnabledProperty under soccerBallNodes #254

Closed marlitas closed 3 months ago

marlitas commented 4 months ago

Discussion Is this a change that would affect soccer-common? Check with MK if this changes the migration (MS: I don't think it will)

marlitas commented 4 months ago

This change affected the center-and-variability phet-io API, but did not require any changes in the migration wrapper since we are adding a Property tandem path. It did not change the location or remove any other Properties that would require a migration rule. I tested by running the migration wrapper and found no errors.

Ready for review by @amanda-phet, and tagging @zepumph for code review to confirm my migration diagnosis from above.

marlitas commented 4 months ago

I have absolutely no clue how this happened, but somehow I linked the commits to a buoyancy issue I've never heard of? https://github.com/phetsims/buoyancy/issues/64

Really not sure. Here are the commits: https://github.com/phetsims/soccer-common/commit/96b2d224504bc1af552400d9224a664e5ac3dfff https://github.com/phetsims/phet-io-sim-specific/commit/21cf4fb7c774c4812182c67d17d511cd5a4b5a33

zepumph commented 4 months ago

Looks perfect thanks. There are multiple reasons why migration doesn't matter here:

  1. You only added an element (no backward compatibility problems)
  2. LinkedElements do not have any migration needs. They don't apply any data to the new sim, they are just in the state for informational purposes (like derivedProperties).
amanda-phet commented 4 months ago

Rather thank link this seven times (under each ball - meanShareAndBalance.balancePointScreen.view.sceneView.soccerBallNodes.soccerBallNode*), I thought it would be linked once under soccerBallNodes.

If that's hard to do I'm fine leaving this as it is, but I think it would be cleaner to have this property at the same level as soccerBallNode*.

marlitas commented 4 months ago

I was able to make the above request happen by customizing the tandem. I added documentation to make that clear in the code, and documentation in PhetioObject makes it seem like this is a behavior that is not recommended but not unprecedented either: If tandem is specified in the options (not recommended), it will use that tandem and nest it anywhere that tandem exists. Use this option with caution since it allows you to nest the tandem anywhere in the tree.

Over to @amanda-phet to confirm and close if all looks well.

amanda-phet commented 3 months ago

Looks good, thanks!