move isSettingPhetioStateProperty to this repo

phetsims / tandem

Simulation-side code for PhET-iO

MIT License

0 stars 5 forks source link

move isSettingPhetioStateProperty to this repo #294

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

From https://github.com/phetsims/chipper/issues/1004, this would cleanup a lot of common code that needs to check for the existence of this Property before using it as a global. Thanks @samreid

Also tagging the backlog project board.

zepumph commented 1 year ago

There are currently 76 usages of phet.joist.sim.isSettingPhetioStateProperty, this seems like a really nice global to factor out!

zepumph commented 1 year ago

[x] We probably would want the same pattern for isClearingPhetioDynamicElementsProperty.
[x] Check in audioManager for sim.isSettingPhetioStateProperty
[x] Check in number-compare/play-main for sim.isSettingPhetioStateProperty

zepumph commented 1 year ago

@samreid, a spot check here would be good. It was pretty straight forward.

My process was to start with the new TinyProperty called newIsSettingPhetioStateProperty, which helped me confirm that all old usages were handled, then I renamed new back to the original name.

samreid commented 1 year ago

I spot checked some of the commits above and this all seems great, thanks! Closing.

phet-dev commented 1 year ago

Reopening because there is a TODO marked for this issue.

zepumph commented 1 year ago

^^^^^^^^^^^^^^^^^^^^

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

So fun!

zepumph commented 1 year ago

@samreid can you give this one more spot check. I believe that CAV has some sounds that you may be able to confirm are still working in the state wrapper.

samreid commented 1 year ago

The preceding commit is really great. CAV state wrapper sounds correct to me as far as I can tell. It has 2 sound effects that are triggered in step(dt) (soccer ball landing and continuous sound for the interval tool node) and I do hear those doubled in the downstream sim. I'm not sure how to address that but will open a side issue in CAV. Can this issue be closed?