phetsims / center-and-variability

"Center and Variability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 2 forks source link

IRQ Info dialog isn't always correct in downstream sim #580

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 14.0

Browser Safari 17

Problem description For https://github.com/phetsims/qa/issues/993 and probably related to https://github.com/phetsims/center-and-variability/issues/571, the information in the IRQ info dialog doesn't always update after pressing Set State Now.

Steps to reproduce

  1. In the State wrapper, set state rate =0
  2. In the upstream sim:
    • Select the Variability Screen
    • Select the IRQ scene
    • Press the Kick 5 button
  3. Press Set State now
  4. Open the IRQ info dialogs -- they should both match. Close both dialogs
  5. In the Upstream sim, move a soccer ball
  6. Press Set State now
  7. Open the IRQ info dialogs -- the IRQ value on the bracket won't match

Visuals

Screenshot 2023-10-11 at 9 58 14 AM
marlitas commented 1 year ago

This seems to be the similar if not exact same problem plaguing #571. I am going to close #571 since this is more isolated to the IQR bar label.

marlitas commented 1 year ago

From #571 @Nancy-Salpepi said:

Sorry @marlitas, I'm going to have to reopen this. For https://github.com/phetsims/center-and-variability/issues/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
Screenshot 2023-10-11 at 1 27 41 PM
marlitas commented 1 year ago

From #571 I investigated:

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.

```diff 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 commented 1 year ago

This was fixed above with @samreid. I believe this issue is now ready for cherry-pick.

marlitas commented 1 year ago

I noticed this is also happening in the MAD node. Removing ready-for-cherry pick.

marlitas commented 1 year ago

Nevermind this was occurring because of changes I was looking at in https://github.com/phetsims/center-and-variability/issues/583

KatieWoe commented 1 year ago

Things seem fixed in rc.2