phetsims / graphing-quadratics

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

Point on Parabola coordinates are not restored correctly by PhET-iO. #162

Closed Nancy-Salpepi closed 3 years ago

Nancy-Salpepi commented 3 years ago

Test device Mac11 (m1 chip)

Operating System 11.4

Browser chrome

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

In the State Wrapper: In the Focus and Directrix Screen, when "set state rate" has a value of zero and the "set state now" button is clicked, the coordinates for the Point on Parabola are sometimes in a different place in the downstream sim as compared to the upstream sim.

Steps to reproduce

  1. Open the state wrapper
  2. Set the state rate to zero
  3. In the upstream sim, select the Focus and Directrix screen
  4. Select all check boxes
  5. Change the variables so that the coordinates for the Point of Parabola overlap the equation ex. p = 1.7 h= -2.9 k= 3.7
  6. Click the "set state now" button

Visuals

https://drive.google.com/file/d/1N8xeqtmL8xzvlCm-9fvsoQRlYU7O_emY/view?usp=sharing

This was a case where the difference was drastic:

Screen Shot 2021-08-20 at 8 13 26 AM

Slight difference here:

Screen Shot 2021-08-20 at 8 05 49 AM
Nancy-Salpepi commented 3 years ago

Also seen in Studio Wrapper

Studio:

Screen Shot 2021-08-20 at 8 52 46 AM

After sim is launched:

Screen Shot 2021-08-20 at 8 53 14 AM
pixelzoom commented 3 years ago

I reproduced the State Wrapper example described in https://github.com/phetsims/graphing-quadratics/issues/162#issue-975580005, and the Studio example described in https://github.com/phetsims/graphing-quadratics/issues/162#issuecomment-902668369.

My first thought was that pointOnParabolaProperty (the coordinates of the Point On Parabola) was not instrumented. But I can see that it is in fact instrumented. In FocusAndDirectrixModel.js:

    // @public
    this.pointOnParabolaProperty = new Vector2Property( initialPoint, {
      tandem: tandem.createTandem( 'pointOnParabolaProperty' ),
      phetioDocumentation: 'the interactive point on the parabola'
    } );
pixelzoom commented 3 years ago

pointOnParabolaProperty can be changed in 2 ways:

So depending on the order in which state is restored, there may be a situation here where the state of pointOnParabolaProperty is correctly restored, and then that state is changed to something incorrect as other state is restored.

pixelzoom commented 3 years ago

What I described in https://github.com/phetsims/graphing-quadratics/issues/162#issuecomment-903306795 was indeed causing this problem. In the above 2 commits, I fixed this in master and 1.2 branches. I tested with the State Wrapper and Studio scenarios that @Nancy-Salpepi reported.

@Nancy-Salpepi please review in master. If it looks OK, please change the issue label to "status:fixed-awaiting-deploy", and unassigned. Then we'll verify in the next RC.

Nancy-Salpepi commented 3 years ago

@pixelzoom I'm still seeing the problem. Was I supposed to do something else aside from: Private window-->Master-->PhETMarks-->graphing quadratics-->state wrapper ?

pixelzoom commented 3 years ago

I re-tested with master, using the Studio and State Wrapper scenarios shown in the screenshots above. I'm not seeing the problem.

@Nancy-Salpepi If you're running a local copy of phetmarks, did you run pull-all.sh to update your local copy? If you're running from phettest.colorado.edu, did you push the "Pull All" button? Or perhaps your browser is caching?

You can also verify in 1.3.0-dev.1, which I just published from master: https://phet-dev.colorado.edu/html/graphing-quadratics/1.3.0-dev.1/phet-io/

Nancy-Salpepi commented 3 years ago

Thank you for the help @pixelzoom. The issue looks fixed.

pixelzoom commented 3 years ago

Excellent, thanks @Nancy-Salpepi. Ready for testing in the next RC.

To verify in the next RC, test the scenarios shown in https://github.com/phetsims/graphing-quadratics/issues/162#issue-975580005 and https://github.com/phetsims/graphing-quadratics/issues/162#issuecomment-902668369.

Nancy-Salpepi commented 3 years ago

Point on Parabola coordinates correctly restored in both Studio and State wrappers.