samreid
commented
1 year ago This patch works great. I was able to remove all of the isSettingPhetioStateProperty guards in Mean: Share and Balance and it still worked without any of the bugs those workarounds addressed.
```diff
Index: main/axon/js/ReadOnlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/ReadOnlyProperty.ts b/main/axon/js/ReadOnlyProperty.ts
--- a/main/axon/js/ReadOnlyProperty.ts (revision a5961582e9fed7de7a0df002257961872cec4d7c)
+++ b/main/axon/js/ReadOnlyProperty.ts (date 1661307101650)
@@ -214,9 +214,23 @@
/**
* Sets the value and notifies listeners, unless deferred or disposed. You can also use the es5 getter
* (property.value) but this means is provided for inner loops or internal code that must be fast. If the value
- * hasn't changed, this is a no-op.
+ * hasn't changed, this is a no-op. For PhET-iO instrumented Properties that are phetioState: true, the value is only
+ * set by the state and cannot be modified by other code while isSettingPhetioStateProperty === true
*/
protected set( value: T ): void {
+ if ( window.phet && phet?.joist?.sim?.isSettingPhetioStateProperty.value
+ && this.isPhetioInstrumented() && this.phetioState ) {
+ // state is managed by the state engine
+ }
+ else {
+ this.unguardedSet( value );
+ }
+ }
+
+ /**
+ * For usage by the IO Type during PhET-iO state setting.
+ */
+ public unguardedSet( value: T ): void {
if ( !this.isDisposed ) {
if ( this.isDeferred ) {
this.deferredValue = value;
@@ -540,7 +554,7 @@
const units = NullableIO( StringIO ).fromStateObject( stateObject.units );
assert && assert( property.units === units, 'Property units do not match' );
assert && assert( property.isSettable(), 'Property should be settable' );
- ( property as Property ).set( parameterType.fromStateObject( stateObject.value ) );
+ ( property ).unguardedSet( parameterType.fromStateObject( stateObject.value ) );
if ( stateObject.validValues ) {
property.validValues = stateObject.validValues.map( ( validValue: StateType ) => parameterType.fromStateObject( validValue ) );
Index: main/mean-share-and-balance/js/intro/model/IntroModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/mean-share-and-balance/js/intro/model/IntroModel.ts b/main/mean-share-and-balance/js/intro/model/IntroModel.ts
--- a/main/mean-share-and-balance/js/intro/model/IntroModel.ts (revision c28937ffa489f3069e9857c3fa35a57d7564596f)
+++ b/main/mean-share-and-balance/js/intro/model/IntroModel.ts (date 1661306333349)
@@ -232,11 +232,9 @@
* Reset 2D waterLevelProperty to 3D waterLevelProperty.
*/
private matchCupWaterLevels(): void {
- if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
- this.iterateCups( ( cup2D, cup3D ) => {
- cup2D.waterLevelProperty.set( cup3D.waterLevelProperty.value );
- } );
- }
+ this.iterateCups( ( cup2D, cup3D ) => {
+ cup2D.waterLevelProperty.set( cup3D.waterLevelProperty.value );
+ } );
}
/**
@@ -257,19 +255,15 @@
this.assertConsistentState();
this.stepWaterLevels( dt );
this.pipeArray.forEach( pipe => pipe.step( dt ) );
-
- assert && assert( !phet.joist.sim.isSettingPhetioStateProperty.value, 'Cannot step while setting state' );
}
private assertConsistentState(): void {
- if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
- const numberOfCups = this.numberOfCupsProperty.value;
- assert && assert( numberOfCups === this.getNumberOfActiveCups(), `Expected ${numberOfCups} cups, but found: ${this.getNumberOfActiveCups()}.` );
- assert && assert( numberOfCups > 0, 'There should always be at least 1 cup' );
- assert && assert( this.getNumberOfActiveCups() - 1 === this.getActivePipes().length, `The length of pipes is: ${this.getActivePipes().length}, but should be one less the length of water cups or: ${this.getNumberOfActiveCups() - 1}.` );
- assert && assert( this.waterCup3DArray.length === MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS, `There should be ${MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS}, but there were actually ${this.waterCup3DArray.length} cups` );
- assert && assert( this.waterCup2DArray.length === MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS, `There should be ${MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS}, but there were actually ${this.waterCup2DArray.length} cups` );
- }
+ const numberOfCups = this.numberOfCupsProperty.value;
+ assert && assert( numberOfCups === this.getNumberOfActiveCups(), `Expected ${numberOfCups} cups, but found: ${this.getNumberOfActiveCups()}.` );
+ assert && assert( numberOfCups > 0, 'There should always be at least 1 cup' );
+ assert && assert( this.getNumberOfActiveCups() - 1 === this.getActivePipes().length, `The length of pipes is: ${this.getActivePipes().length}, but should be one less the length of water cups or: ${this.getNumberOfActiveCups() - 1}.` );
+ assert && assert( this.waterCup3DArray.length === MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS, `There should be ${MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS}, but there were actually ${this.waterCup3DArray.length} cups` );
+ assert && assert( this.waterCup2DArray.length === MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS, `There should be ${MeanShareAndBalanceConstants.MAXIMUM_NUMBER_OF_CUPS}, but there were actually ${this.waterCup2DArray.length} cups` );
}
public reset(): void {
@@ -292,14 +286,12 @@
* @param oldWaterLevel - the old water level from the 3D cup's listener
*/
public changeWaterLevel( cup3DModel: WaterCup, waterLevel: number, oldWaterLevel: number ): void {
- if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
- const delta = waterLevel - oldWaterLevel;
- const cup2D = this.waterCup2DArray[ cup3DModel.linePlacement ];
- const cup2DWaterLevel = Utils.clamp( cup2D.waterLevelProperty.value + delta, 0, 1 );
- cup2D.waterLevelProperty.set( cup2DWaterLevel );
+ const delta = waterLevel - oldWaterLevel;
+ const cup2D = this.waterCup2DArray[ cup3DModel.linePlacement ];
+ const cup2DWaterLevel = Utils.clamp( cup2D.waterLevelProperty.value + delta, 0, 1 );
+ cup2D.waterLevelProperty.set( cup2DWaterLevel );
- this.arePipesOpenProperty.value && this.distributeWaterRipple( this.getActive2DCups(), cup2D, delta );
- }
+ this.arePipesOpenProperty.value && this.distributeWaterRipple( this.getActive2DCups(), cup2D, delta );
}
}
```
Next steps:
- [x] Discussion/review of that proposal with @zepumph
- [ ] Reviewing the 88 occurrences of
isSettingPhetioStatePropertyacross around 25 repos and see which can be eliminated base on this change. - [ ] ~Should we optimize the implementation at all, by caching values that cannot change?~
zepumph
pixelzoom
jonathanolson
@marlitas and I discovered this during working on Mean: Share and Balance and @zepumph says it sounds promising. Instrumented stateful Properties should only accept values from the state during state set.
Hopefully we can get rid of all of the manual guards for
joist.isSettingPhetioStateProperty