The Whisker Plot state isn't always correct in downstream sim of State wrapper

[Nancy-Salpepi]

Test device
MacBook Air M1 chip/ Dell

Operating System
14.0/Win10

Browser
Safari 17/Chrome

Problem description
For phetsims/qa#985, in the State Wrapper:
After kicking a ball in the downstream sim, pressing the Set State Now button, doesn't update the Whisker plot to match the upstream sim.

Steps to reproduce

1. In the state wrapper, go to the Variability Screen
2. Set State rate =0
3. Press the Kick 5 button once in the upstream sim
4. Select the IQR radio button in the upstream sim
5. Press Set State Now
6. Press Kick 1 button in the downstream sim
7. Press Set State Now

Visuals

[Nancy-Salpepi]

This also occurs with the MAD scene:

[marlitas]

The data measures were not being updated while setting state which caused this error.

I believe this is fixed above. @Nancy-Salpepi can you check the behavior on main?

I would also like to check in with @samreid or @zepumph about this. Does using isSettingPhetioStateProperty in this way introduce any brittleness? Not sure if there's a different strategy I should be looking at.

[marlitas]

@zepumph and I worked on this for a bit today. I added a new updated commit but there's still work to be done here. Unassigning @Nancy-Salpepi until that's completed.

[zepumph]

I'm happy to pair with you more if you'd like. Let me know! It seems like a classic case of "doing it right may cause other hidden bugs."

[marlitas]

I talked through the various options and performance impacts with @chrisklus and we decided that the current implementation works well and keeps us away from relying on the isSettingPhetioStateProperty. This is now ready for code review!

[samreid]

I reviewed the commits and it seems reasonable to me. Having @Nancy-Salpepi test sounds like a good next step to me.

[Nancy-Salpepi]

Looks good to me on main 🎉. Closing.

[Nancy-Salpepi]

Sorry @marlitas, I'm going to have to reopen this. For #580, I found a case where the IRQ value above the bracket in the downstream sim is missing. Sorry I didn't catch this earlier.

Steps:

1. Set state rate =0
2. In upstream sim: Select the IRQ scene, check the IRQ checkbox, and press the kick 5 button
3. Press Set State Now
4. In the downstream sim, press the Eraser button
5. Press Set State Now

[marlitas]

This fixes it, but I'm still not clear why it's only happening after we press the eraser button. I'm also not understanding why it's not affecting the interval bar since the intervalBarLabel and intervalBar are getting updated in the same function...

Is it a listener order thing? The updateIntervalBar function is running while setting state, but seems to be doing so before the DerivedProperty had a chance to update? That's not making much sense to me. I would assume that the DerivedProperty would update as soon as a value change triggers it... I'll want some help on this.

Subject: [PATCH] IQR bug
---
Index: js/variability/model/VariabilitySceneModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/variability/model/VariabilitySceneModel.ts b/js/variability/model/VariabilitySceneModel.ts
--- a/js/variability/model/VariabilitySceneModel.ts	(revision 926804bd2384899d4c3349ee0c3ed66b42d6bd59)
+++ b/js/variability/model/VariabilitySceneModel.ts	(date 1697236712958)
@@ -111,6 +111,7 @@
       hasListenerOrderDependencies: true
     } );
     this.iqrValueProperty = new DerivedProperty( [ this.q1ValueProperty, this.q3ValueProperty ], ( q1, q3 ) => {
+      console.log( 'q1, q3', q1, q3 );
       if ( q1 === null || q3 === null ) {
         return null;
       }
@@ -176,6 +177,7 @@
 
         assert && assert( q1 <= q3, 'q1 must be less than q3' );
 
+        console.log( 'updating q values', q1, q3 );
         this.q1ValueProperty.value = q1;
         this.q3ValueProperty.value = q3;
 
Index: js/variability/view/IQRNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/variability/view/IQRNode.ts b/js/variability/view/IQRNode.ts
--- a/js/variability/view/IQRNode.ts	(revision 926804bd2384899d4c3349ee0c3ed66b42d6bd59)
+++ b/js/variability/view/IQRNode.ts	(date 1697237208768)
@@ -219,6 +219,7 @@
       iqrBar.centerX = iqrRectangle.centerX;
 
       // Gracefully handle transient intermediate states during phet-io set state
+      console.log( 'IQR Value: ', sceneModel.iqrValueProperty.value );
       iqrBarLabel.string = sceneModel.iqrValueProperty.value === null ? '' : sceneModel.iqrValueProperty.value;
       iqrBarLabel.centerBottom = iqrBar.centerTop;
     };
@@ -240,7 +241,10 @@
       const max = dataPointsWithoutOutliers[ dataPointsWithoutOutliers.length - 1 ];
 
       const boxLeft = this.modelViewTransform.modelToViewX( sceneModel.q1ValueProperty.value! );
+      console.log( 'updating IQR Node q1 value: ', sceneModel.q1ValueProperty.value );
       const boxRight = this.modelViewTransform.modelToViewX( sceneModel.q3ValueProperty.value! );
+      console.log( 'updating IQR Node q3 value: ', sceneModel.q3ValueProperty.value );
+
 
       const medianPositionX = this.modelViewTransform.modelToViewX( sceneModel.medianValueProperty.value! );
       medianLabelNode.x = boxWhiskerMedianLine.x1 = boxWhiskerMedianLine.x2 = medianPositionX;
@@ -297,6 +301,7 @@
     model.isIQRVisibleProperty.link( updateIQRNode );
     model.selectedVariabilityMeasureProperty.link( updateIQRNode );
     SHOW_OUTLIERS_PROPERTY.link( updateIQRNode );
+    sceneModel.iqrValueProperty.lazyLink( updateIntervalBar );
 
     // We want to ensure that label overlaps and plotNode layout are handled with dynamic text as well.
     Multilink.multilink( [ minLabelNode.boundsProperty, q1LabelNode.boundsProperty, q3LabelNode.boundsProperty, maxLabelNode.boundsProperty ],

[marlitas]

Closing this since #580 is more isolated and descriptive for the specific problem remaining here.