phetsims / calculus-grapher

"Calculus Grapher" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

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

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip)

Operating System 13.2.1

Browser Safari 16.3

Problem description For https://github.com/phetsims/qa/issues/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 commented 1 year ago

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

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

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

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

My recommendation is:

veillette commented 1 year ago

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

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

pixelzoom commented 1 year ago

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

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

amanda-phet commented 1 year ago

Discussed with @pixelzoom and @veillette

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

veillette commented 1 year ago

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

This looks good in master for both brands. Closing!