Instrumented stateful Properties should only accept values from the state during state set

[samreid]

@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

[samreid]

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.

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<T> ).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:

• Discussion/review of that proposal with @zepumph
• Reviewing the 88 occurrences of isSettingPhetioStateProperty across 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]

I believe that you are on a great track! Thanks for implementing. I believe that many cases will indeed be fixed by this, but in general there are more stateful items than just Properties. We don't want to be adding/removing/mutating dynamic elements during state setting, or also updating observable arrays etc. So we will likely not be able to cover all usages.

For example these usages in ProjectileMotion don't seem to be fixed by the patch.

Index: js/common/model/ProjectileMotionModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/ProjectileMotionModel.js b/js/common/model/ProjectileMotionModel.js
--- a/js/common/model/ProjectileMotionModel.js	(revision 5fab9a1da11a852d6c6eb12e79e1ce3190bde26d)
+++ b/js/common/model/ProjectileMotionModel.js	(date 1661366912287)
@@ -12,8 +12,8 @@
 import Emitter from '../../../../axon/js/Emitter.js';
 import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
 import NumberProperty from '../../../../axon/js/NumberProperty.js';
-import VarianceNumberProperty from '../../../../axon/js/VarianceNumberProperty.js';
 import Property from '../../../../axon/js/Property.js';
+import VarianceNumberProperty from '../../../../axon/js/VarianceNumberProperty.js';
 import Range from '../../../../dot/js/Range.js';
 import Vector2 from '../../../../dot/js/Vector2.js';
 import EventTimer from '../../../../phet-core/js/EventTimer.js';
@@ -302,20 +302,14 @@
 
     // if any of the global Properties change, update the status of moving projectiles
     this.airDensityProperty.link( () => {
-      if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
-        this.updateTrajectoriesWithMovingProjectiles();
-      }
+      this.updateTrajectoriesWithMovingProjectiles();
     } );
     this.gravityProperty.link( () => {
-      if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
-        this.updateTrajectoriesWithMovingProjectiles();
-      }
+      this.updateTrajectoriesWithMovingProjectiles();
     } );
     this.selectedProjectileObjectTypeProperty.link(
       selectedProjectileObjectType => {
-        if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) {
-          this.setProjectileParameters( selectedProjectileObjectType );
-        }
+        this.setProjectileParameters( selectedProjectileObjectType );
       }
     );
   }

[zepumph]

I made a couple of adjustments, ensuring the unguarded set was private. Looks great though! Committed below. Thoughts?

[samreid]

After the commit, instrumented DerivedProperty instances cannot get updated during PhET-iO state set. So I'll add a check for that.

[zepumph]

Love it!

[samreid]

Here's a chip-away for the remaining cases. The main rule:

If there is a guard like isSettingPhetioStateProperty.value that is

• powered by an instrumented Property
• which has phetioState: true
• and is not a DerivedProperty
• and is OK for it to take the correct value from the state
• and doesn't create or dispose dynamic elements. (or effect the state of any other PhET-iO stateful, non-Property entities).

then the guard can probably be removed. The sim state should be tested before (to make sure it was already working OK) and after removal of the guard (to make sure it still works correctly after the change). I'll do a few examples then bring it to dev meeting for kicking off a chip-away.

• axon @samreid (3)
• beers-law-lab @pixelzoom (4)
• capacitor-lab-basics @jonathanolson (1)
• center-and-variability @samreid and @chrisklus (5)
• charges-and-fields @jonathanolson (1)
• density-buoyancy-common @jonathanolson (3)
• energy-forms-and-changes (10)
• energy-skate-park @jessegreenberg (1)
• fourier-making-waves @pixelzoom (1)
• friction @zepumph (3)
• gas-properties @pixelzoom (1)
• graphing-quadratics @pixelzoom (3)
• gravity-and-orbits @samreid (2)
• greenhouse-effect @jbphet (2)
• john-travoltage @jessegreenberg (1)
• joist @zepumph (1)
• natural-selection @pixelzoom (15)
• ph-scale @pixelzoom (1)
• projectile-motion @zepumph (3)
• scenery-phet @pixelzoom (1)
• scenery @jonathanolson (2)
• states-of-matter @jbphet (6)
• sun @jbphet (2)
• tandem @zepumph (3)
• wave-on-a-string @jonathanolson (1)

Also the names above are from my memory and partly from the responsible devs list when I didn't recall. That doesn't mean the responsible dev needs to be the implementer---we can adjust accordingly at the dev meeting.

UPDATE: The cases in axon and center-and-variability could not be removed. But both in gravity-and-orbits could be removed, please see the commits below. Ready for team discussion and chip-away.

[pixelzoom]

Self assigning to chip away.

[pixelzoom]

Discussed with @samreid. I'm relucant to remove isSettingPhetioStateProperty checks from existing code. First, the behavior of the guard that was added to ReadOnlyProperty.set is not equivalent -- listeners are still called once, so code that was guarded by isSettingPhetioStateProperty will be called once instead of not at all. Second, @samreid reported that he was only able to remove isSettingPhetioStateProperty checks in a couple of places. So I feel that the return on investment here is likely to be low, and very likely to introduce problems. I'll certainly give this a try in new code. Self unassigning.

[pixelzoom]

In ReadOnlyProperty.ts:

  private unguardedSet( value: T ): void {
    if ( !this.isDisposed ) {
      if ( this.isDeferred ) {
        this.deferredValue = value;
        this.hasDeferredValue = true;
      }
      else if ( !this.equalsValue( value ) ) {
        const oldValue = this.get();
        this.setPropertyValue( value );
        this._notifyListeners( oldValue );
      }
    }
  }

Why are we not checking this.equalsValue( value ) earlier in this method? We may be deferring a value that is effectively a no-op.

[samreid]

Why are we not checking this.equalsValue( value ) earlier in this method? We may be deferring a value that is effectively a no-op.

@zepumph I'm unclear about whether the deferred value can be the same. What do you think?

[jonathanolson]

@samreid will document this.

[zepumph]

We determined today that this was not worth the time to go back to change/remove this workaround. It also has a low rate of success (most aren't removed). As a result, we will document this nuance in PhET-iO doc, and try to be more sparring with usages of isSettingPhetioStateProperty in the future.

[zepumph]

Doc updated. Closing

[zepumph]

Just noting here that we the guard on ReadOnlyProperty added here was WAY too powerful, and we had to tamper it down to allow Properties to be changed during stateSetEmitter over in https://github.com/phetsims/phet-io/issues/1958.

[zepumph]

@pixelzoom and I were got by this no-op today in ReadOnlyProperty.set(). We both totally forgot about this feature, and how clever it is. I added some doc above. I hope that helps future us.