Rendering order is incorrect for snapshot restored via PhET-iO.

[Nancy-Salpepi]

Test device MacBook Air

Operating System 11.4

Browser chrome and safari

Problem description https://github.com/phetsims/qa/issues/692

Studio Wrapper:

In the Vertex Form and Focus and Directrix screens, the parabola (black) turns gray in the area that overlaps the equation of the snapshot parabola.

In the Explore and Standard Form screens, The snapshot equation and snapshot parabola are in front of the main parabola (orange). The parabola remains orange here (unlike the above scenario).

Steps to reproduce

1. Check Equations option
2. Click the snapshot tool.
3. Then adjust the parabola so that the lines/equations overlap. Ex: In Focus and Directrix Screen: p = 2.0 h= -0.5 and k = -1.6 Ex: In Standard Form Screen: a = 1, b=5, c=0
4. Launch sim

Visuals Focus and Directrix Screens--Studio on top and Launched Sim underneath:

Standard Form Screen--Studio on left and Launched Sim on right:

[Nancy-Salpepi]

Also seen in State wrapper

[Nancy-Salpepi]

Screenshot parabola also moves forward when other equations aside from the main are involved:

[pixelzoom]

Thanks @Nancy-Salpepi. I reproduced in master with Studio and State Wrapper. The rendering order is different - in the restored sim, saved curves and equations are in front, when they should be behind. This is probably due to the order of restoring elements. I'll investigate.

[pixelzoom]

Here's the relevant code, in GQGraphNode.js:

    // When the saved quadratic changes...
    model.savedQuadraticProperty.link( savedQuadratic => {
      if ( savedQuadratic ) {
        ...
        savedLineNode = new QuadraticNode( ... );

        // Add it in the foreground, so the user can see it.
        // See https://github.com/phetsims/graphing-quadratics/issues/36
        allLinesParent.addChild( savedLineNode );
      }
    } );

    // When quadraticProperty changes, move saved line to background.
    // See https://github.com/phetsims/graphing-quadratics/issues/36
    model.quadraticProperty.link( quadratic => {
      savedLineNode && savedLineNode.moveToBack();
    } );

So when the user took a snapshot (savedQuadraticProperty), we want the snapshot to be in the foreground "so the user can see it". For example: In the Explore screen, immediately after pressing the snapshot button, the sim (correctly) looks like this in Studio and in the Preview Sim:

As soon as the user makes a change to quadraticProperty (the primary quadratic), the snapshot then moves to the background.

To fix this problem, I'll need to know (a) whether state is being restored, and (b) whether the snapshot is the same curve as the primary quadratic.

[pixelzoom]

In the above commits, this was fixed in master and 1.2 branches.

@Nancy-Salpepi please verify in master, or in this dev version that was built from master: https://phet-dev.colorado.edu/html/graphing-quadratics/1.3.0-dev.2/phet-io/

You should test these 2 situations to confirm that rendering order is correct:

(1) Immediately after pressing the snapshot button, but before making any other changes, the snapshot should be IN FRONT OF the primary quadratic.

(2) After pressing the snapshot button, then changing the primary quadratic, the snapshot should be BEHIND the primary quadratic.

If this looks OK, please unassign yourself, and change the label to "status:fixed-awaiting-deploy".

[Nancy-Salpepi]

No issues.

[pixelzoom]

To verify in the next RC:

1. Run the sim in Studio
2. Go to Vertex Form screen
3. Check Equations checkbox
4. Press the snapshot button
5. Press "Preview Sim" button in Studio.
6. In the preview sim, verify that the snapshot curve and equation are IN FRONT OF the primary curve and equation.
7. Back in Studio, make some changes so that the primary curve and snapshot overlap.
8. Press "Preview Sim" button in Studio.
9. Verify that the snapshot curve and equation are BEHIND the primary curve and equation.

Repeat the above steps in the State Wrapper.

[Nancy-Salpepi]

Rendering order correct in Studio and State wrappers.

[KatieWoe]

Step 9 is not working. Reopening. For https://github.com/phetsims/qa/issues/700

[Nancy-Salpepi]

It is behind for me:

[KatieWoe]

I had interpreted 7 as moving the curve back over the screenshot. That is where I'm seeing the issue.

[pixelzoom]

I had interpreted 7 as moving the curve back over the screenshot.

Sorry, that's not what a meant. When you've just taken a snapshot, it will always be IN FRONT of the primary quadratic, because otherwise you wouldn't see the snapshot. As soon as you change the primary quadratic, it moves to the front, so the snapshot is BEHIND the primary curve. You can tell what's in front by looking at where the curves overlap.

So is there actually a problem here?

[KatieWoe]

If you take the screen shot, then move the curve, then move the curve in front of the snapshot so it is behind, then preview the sim, the snapshot is in front of the curve instead. This is what I show in the gif.

[Nancy-Salpepi]

@KatieWoe after you relaunch the sim, if you move the curve, is the gray overlapping in front of the black? It is especially evident when the equation and graph overlap. The issue I had posted, the gray parabola was in front ALWAYS, instead of in the back when moving the black curve. Now you can see from my snapshots above, the black equation is in front of the gray curve and the gray equation should be behind the black curve.

[KatieWoe]

What I'm seeing:

1. In studio go to the vertex screen and take a snapshot
2. Snapshot is in front of curve
3. Move curve away from, then fully in front of snapshot
4. Snapshot is behind curve
5. Preview sim
6. When sim is launched, snapshot is in front of curve, instead of behind.

[pixelzoom]

Thanks for clarifying @KatieWoe, I see what you're saying.

The problem is that there is no way to know whether you moved the curve, then moved it back; when restoring the state, the snapshot is in front if it's the same as the primary quadratic, and that can happen if you (a) take a snapshot and don't move the curve, or (b) take a snapshot, move the primary curve, then move it back so that it covers the primary curve (your 6 steps above). I'm also not sure why anyone would want to do (b) to set up an initial state.

So here are the options, and I'll let @amanda-phet decide how to proceed:

(1) Do nothing, because (b) is an unlikely scenario.

(2) Add an additional Property, used only by PhET-iO when restoring state, that keeps track of whether the snapshot should be in the front or back.

[pixelzoom]

For option (2) above, that new Property would need to be instrumented (because its state needs to be saved and restored), and will therefore appear in Studio and in the PhET-iO API. It will need to be phetioReadOnly: true, and of course would not be featured.

[pixelzoom]

I spent ~45 min investigating option (2). It's doable, but it's going to make the code mighty complicated. I'm not convinced it's worth it, because I don't see the point in creating an initial state where the snapshot is completely covered by the primary quadratic.

That said, here's a "documentation patch" that I'd like to apply, regardless of which option we decide to go with:

Patch

```diff Index: js/common/view/GQGraphNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/GQGraphNode.js b/js/common/view/GQGraphNode.js --- a/js/common/view/GQGraphNode.js (revision 080cea8912e256c9d7e20a4c097979bc45cadd5f) +++ b/js/common/view/GQGraphNode.js (date 1630272230890) @@ -115,19 +115,17 @@ // See https://github.com/phetsims/graphing-quadratics/issues/36 allLinesParent.addChild( savedQuadraticNode ); - // When restoring state, move savedQuadraticNode to the background if savedQuadratic is NOT identical to - // quadraticProperty, because that means that the user changed quadraticProperty after taking the snapshot, - // and the primary quadratic should be in front. If savedQuadratic IS identical to quadraticProperty, we want - // to leave savedQuadraticNode in the foreground, and let the quadraticProperty listener below handle moving - // it to the back, when the user makes changes to quadraticProperty. - // See https://github.com/phetsims/graphing-quadratics/issues/165 + // When restoring state, if savedQuadratic is identical to quadraticProperty, then leave it in front, so + // the user can see it. Otherwise move it to the back. + // See https://github.com/phetsims/graphing-quadratics/issues/165. if ( phet.joist.sim.isSettingPhetioStateProperty.value && !savedQuadratic.hasSameCoefficients( model.quadraticProperty.value ) ) { savedQuadraticNode.moveToBack(); } } } ); - // When quadraticProperty is changed BY THE USER, move the saved quadratic to the background. + // When a quadratic is saved, it is initially in front of quadraticProperty. Otherwise it wouldn't be visible + // because they are identical. When quadraticProperty is changed BY THE USER, move the saved quadratic to the background. // See https://github.com/phetsims/graphing-quadratics/issues/36 and https://github.com/phetsims/graphing-quadratics/issues/165. model.quadraticProperty.link( quadratic => { if ( !phet.joist.sim.isSettingPhetioStateProperty.value ) { ```

[pixelzoom]

@amanda-phet and I discussed this issue via Zoom. She said supporting the case identified by @KatieWoe in https://github.com/phetsims/graphing-quadratics/issues/165#issuecomment-907465826 isn't a pedagocial concern, and we could publish 1.2 without addressing that case. And even longterm, we could probably not address that case and it would never present a problem in practice. So I've removed the "blocks-sim-publiction" label.

We also agreed that I'll spend a little more time on this, to see if I've overlooked a more straightforward way to address this. If not, then we'll likely close this issue as "won't fix".

[pixelzoom]

I spent another 30 minutes on this, then discovered that adding the additional Property will require not only instrumenting that one Property, but also instrumenting the graphs, so that they have a tandem that can be used to instrument that new Property. But in GQGraphNode.js (where this happens), there is a very clear/explicit decision NOT to instrument graphs:

    assert && assert( !options.tandem, 'GQGraphNode should not be instrumented' );

So continuing with this would also require revisiting the decision not to instrument graphs, deciding what the metadata should be for graph Properties, etc. This is just not worth it, and we should live with this one case where restored state is a little "off".

I'm going to clean up some code comments (to note that we're not addressing this) and we'll verify this again in the next RC.

[pixelzoom]

Code documentations pushed to master and 1.2 branches in the above commits.

Ready for verification in the next RC. To verify:

1. Follow the steps in https://github.com/phetsims/graphing-quadratics/issues/165#issuecomment-906716303
2. Ignore the case in https://github.com/phetsims/graphing-quadratics/issues/165#issuecomment-907465826. It's a case that we have chosen not to support.