phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

z-ordering of curves and point tools is not stateful #202

Closed KatieWoe closed 1 year ago

KatieWoe commented 1 year ago

Device Samsung OS Win 11 Browser Chrome Problem Description For https://github.com/phetsims/qa/issues/962 When you first take a "picture" with the picture button a grey line sits on top of the orange line. If you move the orange line, it passes over the gray line where they overlap. The orange line is on top and covers the grey line if you move it back to it's original position, the same as the grey line. However, this last situation is not stateful. If you move the orange line fully over the grey line, and look at the bottom sim in the state wrapper, the grey line will instead sit on top of the orange line, as if just created.

Steps to Reproduce

  1. Open the sim in the state wrapper
  2. Go to the second screen (works on any with the camera button, but easy to see here)
  3. Press the camera button
  4. Move the main orange line, then move it back to its original position

Visuals zorderstate

pixelzoom commented 1 year ago

This is another one of those cases where we need to use IndexedNodeIO. Similar to https://github.com/phetsims/molecule-polarity/issues/157.

KatieWoe commented 1 year ago

I also just noticed that what the point tool is locking onto is different.

pixelzoom commented 1 year ago

@KatieWoe said:

I also just noticed that what the point tool is locking onto is different.

I don't understand. Can you elaborate? Steps to reproduce?

KatieWoe commented 1 year ago

In the image above, the top sim has the point tool as grey, and the bottom sim has it as orange. Same steps as before, but put a point tool on the grey line before moving the orange line back into position.

Nancy-Salpepi commented 1 year ago

The point tool color in the downstream sim will be incorrect any time it is on a line that intersects the orange curve when the state is set.

Steps:

  1. In State, set State Rate = 0
  2. On the Explore Screen, check the Constant Term Checkbox
  3. Place a Point Tool on the Pink line and drag it to point (0,0) so that it overlaps the orange--The Point Tool is pink.
  4. Press Set State Now--The Point Tool is orange in the downstream sim. Screenshot 2023-07-12 at 8 56 26 AM
pixelzoom commented 1 year ago

There's a similar problem with rendering order of the point tools, see screenshot below. When you grab a point tool, it moves to the front. That does not occur in the downstream sim.

The take-away here is that if the developer uses moveToFront and moveToBack to change the rendering order of Nodes, then those Nodes need to have these options in order to make that rendering order stateful:

      phetioType: IndexedNodeIO,
      phetioState: true
screenshot_2671
pixelzoom commented 1 year ago

In the above commits, I fixed the rendering order issue for curves and point tools, in master and 1.3 branches.

The problem demonstrated by @Nancy-Salpepi in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1632498402 is not fixed yet.

pixelzoom commented 1 year ago

The problem with point-tool color reported in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1632498402 is due to the implementation of getQuadraticNear in PointTool, see relevant doc below. It prefers the curve that it's already on. And that presents an ordering problem when restoring state. So we may need to remove that feature, which was requested by @kathy-phet in https://github.com/phetsims/graphing-quadratics/issues/47.

  /**
   * Gets the quadratic that is close to a specified position, within a specified distance.
   * This algorithm prefers to return the quadratic that the point tool is already on.
...
  public getQuadraticNear( position: Vector2, offDistance: number, onDistance: number ): Quadratic | null {
pixelzoom commented 1 year ago

The problem with point-tool color reported in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1632498402 was fixed in the above commits, for master and 1.3 branches. And I was able to keep the "prefer the quadratic that the point tool is already on" behavior.

@KatieWoe and/or @Nancy-Salpepi please verify in master. If it looks OK, please change the label to "fixed-awaiting-deploy".

kathy-phet commented 1 year ago

I was catching up - but its already fixed. Thanks @pixelzoom.

KatieWoe commented 1 year ago

The grey line being on top of the orange line as described in the first part of the issue still occurs on master.

KatieWoe commented 1 year ago

This mismatch occurs if:

  1. On the top sim take an image to create a grey line
  2. Put a point tool on the orange line while the grey is under it
  3. Move the orange line away.
  4. The point tool will lock onto the grey line in the top sim but still look orange on the bottom. thismismatch
KatieWoe commented 1 year ago

Due to https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1634518466 and https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1634523189 this doesn't seem fixed on master

Nancy-Salpepi commented 1 year ago

@pixelzoom what I originally reported in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1632498402 doesn't seem to be fixed either.

pixelzoom commented 1 year ago

Reproduced - thanks QA.

Very weird, I could have sworn this was fixed... Part of what threw me is that https://github.com/phetsims/graphing-quadratics/issues/202#issue-1799724568 says "Go to the second screen" but shows visuals for the first screen.

Also noting that @KatieWoe's report in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1634523189 is a different use case than what was previously reported.

pixelzoom commented 1 year ago

I found some other PhET-iO hackery that is probably interacting badly with the changes that I made above in https://github.com/phetsims/graphing-quadratics/commit/801b2e1929c36d2a3f72c0222ce515587f384ee0 and https://github.com/phetsims/graphing-quadratics/commit/1cce96283d52a577c104bfc1223947bc6d79a70c. It references #165 and #36, so this is a problem that has come up before.

In GQGraphNode.ts:

        // 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. Note that this does not address the corner case where
        // the instructional designer has changed quadraticProperty, then changed it back to match savedQuadratic.
        // We decided not to address that case, see https://github.com/phetsims/graphing-quadratics/issues/165.
        if ( isSettingPhetioStateProperty.value && !savedQuadratic.hasSameCoefficients( model.quadraticProperty.value ) ) {
          savedQuadraticNode.moveToBack();
        }
      }
    } );

    // 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 back.
    // If it's changed by restoring PhET-iO state, do nothing, because savedQuadraticProperty listener above will
    // handle it. See https://github.com/phetsims/graphing-quadratics/issues/36 and
    // https://github.com/phetsims/graphing-quadratics/issues/165.
    model.quadraticProperty.link( quadratic => {
      if ( !isSettingPhetioStateProperty.value ) {
        savedQuadraticNode && savedQuadraticNode.moveToBack();
      }
KatieWoe commented 1 year ago

It occurs on any screen with the camera button. I found it easiest to reproduce on the second screen, but took a picture on the first.

pixelzoom commented 1 year ago

This approach to making z-order stateful:

      phetioType: IndexedNodeIO,
      phetioState: true

... only works if the Nodes are instrumented. Which they were not. So I instrumented them.

But in order to instrument them, I had to instrument graphNode, their parent Node. That changed the structure of the scenegraph, changed the API, and required several new migration rules. Changes were pushed to master and 1.3 branches in the above commits.

@arouinfar please verify that the introduction of graphNode is OK, as described below. Leave this assigned to me, because I still need to resolve the problems reported in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1632498402 and https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1634523189.

These were the old phetioIDs:

These are the new phetioIDs. "view" was changed to "view.graphNode":

These new elements were added to make z-order stateful. There is a set of these for each screen. None of them are featured, and their visibleProperty is not instrumented.

pixelzoom commented 1 year ago

I noticed that it was possible to change the indices for the "new element" Nodes listed in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1635059920, by using the "Move Up" and "Move Down" buttons in Studio. So I made them read-only in the above commits to master and 1.3 branches. For example:

screenshot_2677
pixelzoom commented 1 year ago

The remaining problems with the point tool color are due to this bit of code in GQModel.ts:

   const pointToolQuadraticsProperty = new DerivedProperty(
      [ this.quadraticProperty, this.quadraticTermsProperty, this.savedQuadraticProperty ],
      ( quadratic, quadraticTerms, savedQuadratic ) => {
        // order is important! compact to remove nulls
        return _.compact( [ quadratic, ...quadraticTerms, savedQuadratic ] );
      } );

When the saveQuadraticNode is in the foreground, the savedQuadratic model element is still at the end of the list, and therefore chosen last by the point tool. Getting the model and view orders to match is going to require big changes.

I tried this workaround. But it's too clever and has ordering problems when state is restored, so does not resolve the problems.

workaround ```ts // {DerivedProperty.} Quadratics that are visible to the point tools, // in the order that they will be considered by point tools (foreground to background). // ObservableArrayDef is not used here because we need to change the entire array contents atomically. let previousSavedQuadratic = this.savedQuadraticProperty.value; const pointToolQuadraticsProperty = new DerivedProperty( [ this.quadraticProperty, this.quadraticTermsProperty, this.savedQuadraticProperty ], ( quadratic, quadraticTerms, savedQuadratic ) => { // order is important! let quadratics: ( Quadratic | null )[] = []; if ( previousSavedQuadratic !== savedQuadratic ) { previousSavedQuadratic = savedQuadratic; // a quadratic was saved, so put it at the front of the list quadratics = [ savedQuadratic, quadratic, ...quadraticTerms ]; } else { quadratics = [ quadratic, ...quadraticTerms, savedQuadratic ]; } // Use _.compact to remove nulls. return _.compact( quadratics ); } ); ```
arouinfar commented 1 year ago

The changes to the tree all look good @pixelzoom. However, I had to change a few phetioIDs in examples.md to include graphNode in the path. Commit is referenced above. This is another example of why https://github.com/phetsims/phet-io-sim-specific/issues/21 would be a really nice feature to have.

pixelzoom commented 1 year ago

I discussed this issue with @arouinfar. Summary:

Note to self... The relevant files are:

pixelzoom commented 1 year ago

Another failed attempt... The patch below worked correctly in the standalone sim, but the point tool color is incorrect in the State Wrapper due to the order that GQModel pointToolQuadraticsProperty dependencies are restored.

patch ```diff Subject: [PATCH] add normalPercentageStringProperty and mutantPercentageStringProperty, https://github.com/phetsims/natural-selection/issues/339 --- Index: js/common/model/GQModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/GQModel.ts b/js/common/model/GQModel.ts --- a/js/common/model/GQModel.ts (revision 88ec2be4048b60a29ecde6a6036a3e263e813c03) +++ b/js/common/model/GQModel.ts (date 1690150137955) @@ -70,11 +70,27 @@ // {DerivedProperty.} Quadratics that are visible to the point tools, // in the order that they will be considered by point tools (foreground to background). // ObservableArrayDef is not used here because we need to change the entire array contents atomically. + let previousSavedQuadratic: Quadratic | null = null; const pointToolQuadraticsProperty = new DerivedProperty( [ this.quadraticProperty, this.quadraticTermsProperty, this.savedQuadraticProperty ], ( quadratic, quadraticTerms, savedQuadratic ) => { - // order is important! compact to remove nulls - return _.compact( [ quadratic, ...quadraticTerms, savedQuadratic ] ); + + // order is important! + let quadratics; + if ( previousSavedQuadratic !== savedQuadratic ) { + + // When the save quadratic changes, move it to the front. + quadratics = [ savedQuadratic, quadratic, ...quadraticTerms ]; + previousSavedQuadratic = savedQuadratic; + } + else { + + // Otherwise, the interactive quadratic is in front, and the saved quadratic is at the very back. + quadratics = [ quadratic, ...quadraticTerms, savedQuadratic ]; + } + + // compact to remove nulls + return _.compact( quadratics ); } ); this.leftPointTool = new PointTool( pointToolQuadraticsProperty, this.graph, { Index: js/common/view/PointToolNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/PointToolNode.ts b/js/common/view/PointToolNode.ts --- a/js/common/view/PointToolNode.ts (revision 88ec2be4048b60a29ecde6a6036a3e263e813c03) +++ b/js/common/view/PointToolNode.ts (date 1690149587591) @@ -198,6 +198,7 @@ // note where the drag started start: ( event, listener ) => { + pointTool.isDragging = true; // Note the mouse-click offset when dragging starts. const position = modelViewTransform.modelToViewPosition( pointTool.positionProperty.value ); @@ -220,7 +221,7 @@ if ( graph.contains( position ) && graphContentsVisibleProperty.value ) { // If we're close enough to a quadratic, snap to that quadratic. - const snapQuadratic = pointTool.getQuadraticNear( position, GQQueryParameters.snapOffDistance, GQQueryParameters.snapOnDistance ); + const snapQuadratic = pointTool.getQuadraticNear( position, GQQueryParameters.snapOffDistance, GQQueryParameters.snapOnDistance, true ); if ( snapQuadratic ) { // Get the closest point that is on the quadratic. @@ -244,6 +245,10 @@ // move the point tool pointTool.positionProperty.value = position; + }, + + end: () => { + pointTool.isDragging = false; } }, providedOptions ); Index: js/common/model/PointTool.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/PointTool.ts b/js/common/model/PointTool.ts --- a/js/common/model/PointTool.ts (revision 88ec2be4048b60a29ecde6a6036a3e263e813c03) +++ b/js/common/model/PointTool.ts (date 1690149587584) @@ -39,6 +39,7 @@ public readonly probeSide: ProbeSide; public readonly dragBounds: Bounds2; + public isDragging: boolean; private readonly quadraticsProperty: TReadOnlyProperty; public readonly positionProperty: Property; public readonly quadraticProperty: TReadOnlyProperty; @@ -66,6 +67,7 @@ this.probeSide = options.probeSide; this.dragBounds = options.dragBounds; + this.isDragging = false; this.quadraticsProperty = quadraticsProperty; this.positionProperty = new Vector2Property( options.position, { @@ -78,7 +80,7 @@ [ this.positionProperty, quadraticsProperty ], ( position, quadratics ) => { if ( graph.contains( position ) ) { - return this.getQuadraticNear( position, GQQueryParameters.pointToolThreshold, GQQueryParameters.pointToolThreshold ); + return this.getQuadraticNear( position, GQQueryParameters.pointToolThreshold, GQQueryParameters.pointToolThreshold, this.isDragging ); } else { return null; @@ -94,16 +96,18 @@ /** * Gets the quadratic that is close to a specified position, within a specified distance. - * This algorithm prefers to return the quadratic that the point tool is already on. - * If that quadratic is too far away, then examine all quadratics, in foreground-to-background order. + * When the user is dragging the point tool, this algorithm prefers to return the quadratic that the point tool + * is already on. If that quadratic is too far away, then examine all quadratics, in foreground-to-background order. * See https://github.com/phetsims/graphing-quadratics/issues/47. * @param position - the point tool's position * @param offDistance - if <= to this distance, snaps ON to a curve * @param onDistance - if > this distance, snaps OFF of a curve + * @param isDragging - whether the user is dragging the point tool * @returns null if no quadratic is close enough */ - public getQuadraticNear( position: Vector2, offDistance: number, onDistance: number ): Quadratic | null { - let quadraticNear = this.quadraticProperty.value; + public getQuadraticNear( position: Vector2, offDistance: number, onDistance: number, isDragging: boolean ): Quadratic | null { + console.log( `getQuadraticNear isDragging=${isDragging}` );//XXX + let quadraticNear = isDragging ? this.quadraticProperty.value : null; const quadratics = this.quadraticsProperty.value; if ( !quadraticNear || !quadratics.includes( quadraticNear ) || @@ -121,6 +125,7 @@ public reset(): void { this.positionProperty.reset(); + this.isDragging = false; } } ```
pixelzoom commented 1 year ago

I finally worked out a resonable solution in the above commit, which resolves the order dependences involved in restoring state. The API changed, but no migration rules were needed.

The important changes are:

(1) GQModel isSavedQuadraticInFrontProperty is a new instrument Property that determines whether the saved quadratic is in front of the interactive quadratic. isSavedQuadraticInFrontProperty is not featured, and it is read-only. GQGraphNode observes isSavedQuadraticInFrontProperty, and handles moving the saved quadratic to the front/back of the scenegraph.

  // Whether the saved quadratic should be displayed in front of the other interactive quadratic. This is true
  // immediately after saving a quadratic because it is identical to the interactive quadratic, and would therefore
  // be hidden behind the interactive quadratic.  This Property was needed to properly restore z-order of curves
  // and association of point tools to curves. See https://github.com/phetsims/graphing-quadratics/issues/202
  public readonly isSavedQuadraticInFrontProperty: Property<boolean>;

(2) PointTool's quadraticProperty (the quadratic that the point tool is on) is now a Property<Quadratic|null> instead of a DerivedProperty<Quadratic|null>. It is phetioReadOnly: true, and it is still featured. It is updated (based on its dependencies, via a Multilink) only when we are NOT restoring state. Otherwise its statefulness results in the correct value being restored.

(3) Point tools now prefers to stay associated with a curve only while they are being dragged. This was the request in https://github.com/phetsims/graphing-quadratics/issues/47, and we went a little too far by applying that behavior to all situations. So the behavior is now less confusing when you are not dragging a point tool -- the curve the point tool appears to be "on" will determine its color. For example, if you have a point tool on the interactive curve, then save a curve (via the camera button), the point tool will now change to the color (gray) of the saved curve.

@KatieWoe and/or @Nancy-Salpepi please review in master. If it looks OK, change the label to "fixed-awaiting-deploy" for verification in the next RC.

KatieWoe commented 1 year ago

Things are looking ok in the state wrapper on master from what I can tell.

pixelzoom commented 1 year ago

@Nancy-Salpepi would you like to have a look too, or shall I proceed with the next RC?

Nancy-Salpepi commented 1 year ago

I was going to take a look during the spot check, unless you rather I looked now?

pixelzoom commented 1 year ago

Given the number of times that I've unsuccessfully tried to address this issue 🙄, I think it would be better if @Nancy-Salpepi had a look before I publish the next RC.

Nancy-Salpepi commented 1 year ago

looking now

Nancy-Salpepi commented 1 year ago

I am seeing one odd thing with the pointer color in state and in the standalone. Steps:

  1. On the explore screen, check all of the quadratic terms.
  2. Change values so that the pink graph intersects any of the other graphs.
  3. Place a point tool on the pink curve and move it to a place where the pink curve intersects any other curve.
  4. Press the save button--the pointer color will change.

https://github.com/phetsims/graphing-quadratics/assets/87318828/a5e05974-7c95-4a5b-9727-ad7522e9f25f

pixelzoom commented 1 year ago

I discussed the behavor in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1648460255 with @Nancy-Salpepi . When you stop dragging the point tool, its color remains the same as whatever curve it had been attached to -- in this case, the pink curve. When any curve changes (in this case, the gray curve was saved), all curves must be considered as possible candidates, because the point tool doesn't know what changed, how close the curves are to it, etc. The candidates are considered in foreground-to-background order. So in this case, it finds the green curve, which is in front of the pink curve, and the color changes to green. And yes, I agree that it feels a little weird and unexpected.

I'll ruminate some more, and discuss with @arouinfar. But my first reaction is that this is a corner case that we should probably just live with.

pixelzoom commented 1 year ago

I met with @arouinfar.

The first thing we agreed on was... nice job @Nancy-Salpepi at finding this edge case! 😬

The second thing that we agreed on is that this edge case is not worth addressing. If we revert to the previous (published) behavior, that is significantly worse, especially when the interactive and saved curves overlap. Trying to address this edge case would definitely be a complicated/convoluted implementation. The average user is unlikely to have the attention to detail that QA has, so probably won't even notice. And while the behavior is a bit unexpected, it's actually logical when considering the implementation (which admittedly benefits the developer more than user, but c'est la vie).

So... We're willing to live with the behavior reported in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1648460255, and will label this as ready for RC testing.

pixelzoom commented 1 year ago

Ready for testing in 1.3.0-rc.2. Please close if this issue verifies OK.

Test these scenarios reported above:

... and any other scenarios that occur to you.

Nancy-Salpepi commented 1 year ago

All of scenarios in https://github.com/phetsims/graphing-quadratics/issues/202#issuecomment-1648577744 look good to me in rc.2.

Closing (and hopefully never reopening!).