search

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

Invisible graphs can be interacted with. #270

Closed veillette closed 1 year ago

veillette commented 1 year ago

On Original graph, (and predict mode), we can make the graphs invisible using the eye-toggle button. However, it is still possible to perform curve manipulations while the graphs are hidden.

veillette commented 1 year ago

I am aware that this fall under the bug category but I personally could be a feature. It allows a teacher to manipulate a curve while hidden..

pixelzoom commented 1 year ago

Good catch. No, I don't think the curve should be manipulatable when hidden. I'll have a look.

pixelzoom commented 1 year ago

Ugh. The correct way to address this is to make originalGraphNode.inputEnabledProperty a DerivedProperty, where one of the dependencies is this.curveLayerVisibleProperty. And the other dependence would be new iO-only Property to allow iO clients to disable input. That involves revisiting https://github.com/phetsims/calculus-grapher/issues/240, and making a PhET-iO API changes.

Before I do that, I'm going to wait a bit, to see if another solution occurs to me. @veillette let me know if you have thoughts.

pixelzoom commented 1 year ago

I misspoke in https://github.com/phetsims/calculus-grapher/issues/270#issuecomment-1462491114. We are not using originalGraphNode.inputEnabledProperty. We instrumented originalGraphNode.originalCurveNode.inputEnabledProperty.

After one false start in https://github.com/phetsims/calculus-grapher/commit/71c8f150d8b1dc94e237edbb9e2c9a50f466e8f2, the fix for this was this single line in https://github.com/phetsims/calculus-grapher/commit/3b440e2f4ebb0fb413e6e7f6fc5006a093957005 (the other lines are rolling back the previous commit):

+  this.chartRectangle.setInputEnabledProperty( this.curveLayerVisibleProperty ); // see https://github.com/phetsims/calculus-grapher/issues/270

Over to @veillette to review, close if OK.

veillette commented 1 year ago

Good CLosing