Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the cueing arrows reappear after pressing Erase button? #299

Closed
Nancy-Salpepi opened this issue Mar 20, 2023 · 12 comments
Closed

Should the cueing arrows reappear after pressing Erase button? #299

Nancy-Salpepi opened this issue Mar 20, 2023 · 12 comments

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air (m1 chip)

Operating System
13.2.1

Browser
Safari 16.3

Problem description
For phetsims/qa#921, I noticed that on all of the screens, pressing the Erase button results in the yellow hint arrows reappearing. I wasn't sure if that was the intended design or if the hint arrows should only reappear on Reset All (like in Center and Variability)?

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Calculus Grapher‬ URL: https://phet-dev.colorado.edu/html/calculus-grapher/1.0.0-dev.25/phet/calculus-grapher_all_phet.html Version: 1.0.0-dev.25 2023-03-20 03:37:03 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.3 Safari/605.1.15 Language: en-US Window: 1279x710 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
@veillette
Copy link
Contributor

We can discuss it as a group but the reason for this behavior lies in TransformedCurve. The erase button calls the reset from TransformedCurve

The reset in TransformedCurve resets the point to their initial state AND reset the wasManipulatedProperty.

  public reset(): void {

    this.wasManipulatedProperty.reset();

    // Reset every CurvePoint to its initial state.
    this.points.forEach( point => point.reset() );
    this.curveChangedEmitter.emit();
  }

In some ways we are overloading the term 'reset in TransformedCurve. We should have a separate resetPoints function that only reset the initial state of the curvePoints, and a true reset for the reset all button.

  public reset(): void {

    this.wasManipulatedProperty.reset();
    this.resetPoints()
}

  public resetPoints(): void{

    // Reset every CurvePoint to its initial state.
    this.points.forEach( point => point.reset() );
    this.curveChangedEmitter.emit();
  }

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 20, 2023

I don't feel strongly either way. But when the curve is reset to y=0, it tends to disappear (vusually) into the x-axis, so the cueing arrows seem like a useful reminder.

But if we do decide not to show the cueing arrows after Eraser... Isn't it also odd that the cueing arrows reappear after pressing the "Reset All" button ? Why do the cueing arrows ever appear after the user has interacted with the curve?

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 20, 2023

@veillette said:

In some ways we are overloading the term 'reset in TransformedCurve. We should have a separate resetPoints function that only reset the initial state of the curvePoints, and a true reset for the reset all button.

Agreed. But rather than resetPoints, let's call it erase, because it's being called when the EraserButton is pressed.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 20, 2023

One other thought... Does the EraserButton currently behave correctly for saved PhET-iO state? I don't think so. My feeling is that it should always set the curve to y=0. I believe that it currently sets the curve to whatever initial state was saved via Studio, and that seems odd to me. Reset All should return to the sim to its initial state -- it is resetting. Erase is different -- the "erased" state should be the same, regardless of the what initial state was save for PhET-iO.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 20, 2023

My recommendation is:

  • Cueing arrows return when pressing EraserButton or ResetAllButton. (I'm not concerned about consistency with Center and Variablility.)
  • In TransformedCurve, have separate reset and erase methods, where:
    • reset resets the TransformedCurve to its initial state, whether that's the default initial state, or an initial state set by a PhET-iO wrapper
    • erase sets the TransformedCurve to y=0, regardless of the initial state. Because "erase" serves a different purpose than "reset". And note that we never actually erase the curve, because some curve is always shown.

@veillette veillette changed the title Should the yellow hint arrows reappear after pressing Erase button? Should the cueing arrows reappear after pressing Erase button? Mar 20, 2023
veillette added a commit that referenced this issue Mar 21, 2023
Signed-off-by: Martin Veillette <martin.veillette@gmail.com>
@veillette
Copy link
Contributor

veillette commented Mar 21, 2023

The commit above introduces a new method that "erases" the curve by setting its point values to zero and its point type to smooth. The method is invoked when pressing the eraser button.

I have not done any changes to the Reset All button and reset functionality of transformed curve so their functionality is preserved.

I tested the functionality of it in studio, by loading an initial curve onto the sim. The erase button sets the curve values to zero, and the reset button sets it to its initial value.

@veillette
Copy link
Contributor

@Nancy-Salpepi please verify in master, close if OK.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 21, 2023

The implementation and behavior looks nice to me. But we haven't decided whether the cueing arrows should be restored or not (they currently are not). Should we wait to have @Nancy-Salpepi review until we've discussed with @amanda-phet, so that @Nancy-Salpepi is only reviewing once?

@veillette
Copy link
Contributor

That's right, lets discuss with @amanda-phet first. You are off the hook @Nancy-Salpepi :)

@amanda-phet
Copy link
Contributor

Discussed with @pixelzoom and @veillette

We'd like the erase button to reset the curve to y=0 and not show cueing arrows.

@veillette
Copy link
Contributor

We'd like the erase button to reset the curve to y=0 and not show cueing arrows.

We are good to go then. The previous commit sets the curve to zero for phet brand and phet-io when erasing . The cueing arrows will not be present afterwards if the user had previously interacting with the curve.

I'll note that pressing the erase button before interacting with the curve will not remove the cueing arrows.

@Nancy-Salpepi please verify in master, close if OK.

@Nancy-Salpepi
Copy link
Author

This looks good in master for both brands. Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants