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

Labeled Points don't stay on line in launched sim #307

Closed KatieWoe closed 1 year ago

KatieWoe commented 1 year ago

Device Dell OS Win 11 Browser Chrome Problem Description For https://github.com/phetsims/qa/issues/921 If you turn on Labeled Points in studio, the point stays on your line as you create curves. However, if you launch the sim the point is on the y=0 line no matter where the curve is. Steps to Reproduce

  1. Add a point with model.tools.labeledPoints
  2. Add a curve to the line so that the point is at y /= 0
  3. Launch the sim

Visuals Calculus Grapher screenshot

veillette commented 1 year ago

Good find.

veillette commented 1 year ago

It is worth noting that after launching the sim, on the integral screen, if one moves the original curve, the point C does not update its position, but its label does. The label stays within the vicinity of the point so somehow, it knows about the derivative of the curve, but not the original function.

image

I'll note that the motion of the label is only present of the Integral Screen. In fact there is no motion of the label on the Derivative and Advanced screen. This dependency on screen is rather odd.

veillette commented 1 year ago

test

veillette commented 1 year ago

I don't know if the presence/absence of motion of the labels themselves is a red herring. In all cases, the points themselves stay on the y=0 curve.

pixelzoom commented 1 year ago

I haven't investigated yet. But I have a bad feeling about this one.

Our entire architecture is built on reusing CurvePoint instances, then having curveChangedEmitter fire when the user completes an interaction. But when PhET-iO state is restored, there is no interaction, and therefore no calls to curveChangedEmitter.

Furthermore, the order in which state is restored is not something that we control. And it's often restored in a much different order than would normally occur when using the sim. I suspect that if we look at other tools, we'll find that their Property values are also not correct. And things that do look correct on startup (like rendered curves), but rely on a call to curveChangedEmitter, we're probably just getting luck with the restore order.

pixelzoom commented 1 year ago

@veillette noted:

It is worth noting that after launching the sim, on the integral screen, if one moves the original curve, the point C does not update its position, but its label does. The label stays within the vicinity of the point so somehow, it knows about the derivative of the curve, but not the original function.

This is because LabeledPointNode handles the position of the point and its label separately. Subcomponent PlottedPoint is responsible for updating the point's position, and is not getting notified. LabeledPointNode is responsible for the label position via updateLabelPosition, which is called via a Multilink that is firing properly as the derivative changes at the point.

veillette commented 1 year ago

Here is another way to reproduce the bug with the state wrapper

I tested it with the state wrapper on master with the queryParameter &labeledPointsVisible=true

To reproduce, set a non zero curve.
image

Doing a similar with test with the additional queryParameter numberOfpoints=20, to limit the numberOfPoints, I can then more easily look at the jSon object. Even though the screen does get an update the labeledPoints are receiving updates.

"calculusGrapher.derivativeScreen.model.tools.referenceLine.yIntegralProperty": {
    "value": 1.3272163569826967,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.referenceLine.yOriginalProperty": {
    "value": 0.11852734747181787,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.referenceLine.yDerivativeProperty": {
    "value": -0.5863635192242843,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.referenceLine.ySecondDerivativeProperty": {
    "value": 1.4260335721257256,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.APoint.yIntegralProperty": {
    "value": 2.762494260170336e-7,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.APoint.yOriginalProperty": {
    "value": 0.0000010497478188647276,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.APoint.yDerivativeProperty": {
    "value": 0.00014597074908655083,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.APoint.ySecondDerivativeProperty": {
    "value": 0.0005471096672766898,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.BPoint.yIntegralProperty": {
    "value": 0.002035776409264792,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.BPoint.yOriginalProperty": {
    "value": 0.00742654401938627,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.BPoint.yDerivativeProperty": {
    "value": 0.11245500934914038,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.BPoint.ySecondDerivativeProperty": {
    "value": 0.37481876539982234,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.CPoint.yIntegralProperty": {
    "value": 0.6811989471553497,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.CPoint.yOriginalProperty": {
    "value": 1.0870362076769404,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.CPoint.ySecondDerivativeProperty": {
    "value": -3.3384190255029815,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.DPoint.yIntegralProperty": {
    "value": 1.3603621179014347,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.DPoint.yOriginalProperty": {
    "value": 0.007426544019386249,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.DPoint.yDerivativeProperty": {
    "value": -0.11245500934914027,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.DPoint.ySecondDerivativeProperty": {
    "value": 0.3748187653998221,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.EPoint.yIntegralProperty": {
    "value": 1.3623978943106994,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.EPoint.yDerivativeProperty": {
    "value": -9.972604279214877e-7,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.EPoint.ySecondDerivativeProperty": {
    "value": 0.0000037895896261016532,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.labeledPoints.FPoint.yIntegralProperty": {
    "value": 1.3623978943106994,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.tangentScrubber.yOriginalProperty": {
    "value": 0.11852734747181787,
    "validValues": null,
    "units": null
},
"calculusGrapher.derivativeScreen.model.tools.tangentScrubber.yDerivativeProperty": {
    "value": 0.5863635192242844,
    "validValues": null,
    "units": null
}

}

pixelzoom commented 1 year ago

Fixed in the above commit. As I suspected, this issue and https://github.com/phetsims/calculus-grapher/issues/310 were the same underlying problem, shared by all tools. Great find @KatieWoe! Please verify in master, close if OK.

KatieWoe commented 1 year ago

Looks good in master