Instrument `originalCurveNode.inputEnabledProperty`

pixelzoom commented 1 year ago

2/27/2023 design meeting, @kathy-phet requested:

Instrument originalGraphNode.inputEnabledProperty

pixelzoom commented 1 year ago

Done in the above commit. @amanda-phet please review, close if OK.

kathy-phet commented 1 year ago

I took a quick look, and the inputEnabledProperty is applying to both f(x) and Predict f(x). Ideally, it would only apply to f(x), so you could still interact with the Predict f(x) curve, but could not change the original f(x) curve if you flipped back to the f(x) mode. I know that is more complex, requiring a conditional or a combo situation for that property.

pixelzoom commented 1 year ago

Self assigning to start over.

pixelzoom commented 1 year ago

There's a bug in Node that is preventing me providing a custom inputEnabledProperty, see https://github.com/phetsims/scenery/issues/1540. This issues is blocked until that is resolved.

pixelzoom commented 1 year ago

When https://github.com/phetsims/scenery/issues/1540 has been addressed, here's the patch for creating a custom originalGraphNode.inputEnabledProperty:

patch

```diff Subject: [PATCH] convert to TypeScript --- Index: js/common/view/GraphNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/GraphNode.ts b/js/common/view/GraphNode.ts --- a/js/common/view/GraphNode.ts (revision e2a3512d3977ad3622575451a4d60ada70aecca2) +++ b/js/common/view/GraphNode.ts (date 1677545206622) @@ -100,7 +100,7 @@ }; export type GraphNodeOptions = SelfOptions & - PickOptional & + PickOptional & PickRequired; export default class GraphNode extends Node { Index: js/common/view/OriginalGraphNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/OriginalGraphNode.ts b/js/common/view/OriginalGraphNode.ts --- a/js/common/view/OriginalGraphNode.ts (revision e2a3512d3977ad3622575451a4d60ada70aecca2) +++ b/js/common/view/OriginalGraphNode.ts (date 1677545635706) @@ -86,6 +86,11 @@ 'The value of this Property can be changed by toggling showOriginalCurveCheckbox.' } ); + const originalCurveInputEnabledProperty = new BooleanProperty( true, { + tandem: providedOptions.tandem.createTandem( 'originalCurveInputEnabledProperty' ), + phetioDocumentation: 'Use this Property to disable interactivity of the original curve.' + } ); + const originalCurveNodeTandem = providedOptions.tandem.createTandem( 'originalCurveNode' ); const options = optionize()( { @@ -112,7 +117,10 @@ stroke: CalculusGrapherColors.originalChartBackgroundStrokeProperty }, labelNode: labelNode, - phetioInputEnabledPropertyInstrumented: true // see https://github.com/phetsims/calculus-grapher/issues/240 + inputEnabledProperty: DerivedProperty.or( [ predictEnabledProperty, originalCurveInputEnabledProperty ], { + tandem: providedOptions.tandem.createTandem( 'inputEnabledProperty' ), + phetioValueType: BooleanIO + } ) }, providedOptions ); super( graphType, originalCurve, model.gridVisibleProperty, options ); ```

pixelzoom commented 1 year ago

So that I could proceed, I resolved https://github.com/phetsims/scenery/issues/1540 myself and assigned to @jonathanolson for review.

Ready for review by @amanda-phet and @kathy-phet. The screenshot below shows the relevant part of the Studio tree.

originalGraphNode.originalCurveInputEnabledProperty is used to enabled/disable input for f(x).
originalGraphNode.inputEnabledProperty is derived from originalGraphNode.originalCurveInputEnabledProperty and predictModeEnabledProperty. It's true if either of those Properties is true.

Note that it is NOT possible to disable input for the Predict curve. That's not what you asked for, but I wanted to point that out.

pixelzoom commented 1 year ago

Thinking about this more... The current solution doesn't change the state of the cueing arrows, should it? And perhaps originalGraphNode.originalCurveInputEnabledProperty should be relocated to originalGraphNode.originalCurveNode.inputEnabledProperty -- though that feels a little "clever", since there is no input listener associated with originalCurveNode, and will complicate/confuse the code.

I'm also not entirely sure that I have all the requirements. @kathy-phet with high-priority, could you please confirm that this is what you're expecting:

Be able to premanently disable f(x) interactivity.
Hide the cueing arrows when f(x) interactivity is disabled.
No such need for the Predict curve.

kathy-phet commented 1 year ago

I'm also not entirely sure that I have all the requirements. @kathy-phet with high-priority, could you please confirm that this is what you're expecting:

Be able to premanently disable f(x) interactivity. Hide the cueing arrows when f(x) interactivity is disabled. No such need for the Predict curve.

Yes - that is what I was thinking of as the requirements. Nice catch on the cuing arrows. I hadn't thought about that - but it makes complete sense to hide those if input is disabled.

kathy-phet commented 1 year ago

Assigning back to you Chris. Since it sounded like the above still needed to be implemented.

pixelzoom commented 1 year ago

@amanda-phet @kathy-phet ready for review.

The screenshot below shows the Studio tree for the revised implementation. The important PhET-iO elements are:

originalCurveNode.inputEnabledProperty use this to enabled/disable interactivity for f(x)
originalGraphNode.inputEnabledProperty is derived. Note that it's dependency on originalCurveNode.inputEnabledProperty is not currently shown in the Studio tree because of the scenery/PhET-iO bug, see https://github.com/phetsims/scenery/issues/1541
originalCurveNode.cueingArrowsNode.visibleProperty is derived. Note that it's dependency on originalCurveNode.inputEnabledProperty is not currently shown in the Studio tree because of the scenery/PhET-iO bug, see https://github.com/phetsims/scenery/issues/1541. When originalCurveNode.inputEnabledProperty is false, the cueing arrows will be hidden.

Note that https://github.com/phetsims/scenery/issues/1541 is a serious problem, and affects all sims. I could work around in this sim, but I don't think we want to have to apply a workaround everywhere that Node Properties are used (visibleProperty, enabledProperty, inputEnabledProperty, etc.) So if there are no additional change requests for this issue, I'll be marking this issue on-hold/blocked.

pixelzoom commented 1 year ago

Note that https://github.com/phetsims/scenery/issues/1541 is a serious problem, and affects all sims. I could work around in this sim, but I don't think we want to have to apply a workaround everywhere that Node Properties are used (visibleProperty, enabledProperty, inputEnabledProperty, etc.) So if there are no additional change requests for this issue, I'll be marking this issue on-hold/blocked.

Another option would be to uninstrument originalGraphNode.inputEnabledProperty and cueingArrowsNode.visibleProperty. They are derived (not settable), I seriously doubt that they are useful, or that anyone will ever look at them. And they could be added in a future release, after addressing https://github.com/phetsims/scenery/issues/1541. @amanda-phet @kathy-phet let me know if you're OK with uninstrumenting them.

pixelzoom commented 1 year ago

Another option would be to uninstrument originalGraphNode.inputEnabledProperty and cueingArrowsNode.visibleProperty.

3/2/2023 PhET-iO meeting: @kathy-phet and @arouinfar said to do this.

pixelzoom commented 1 year ago

The screenshot below shows the part of the Studio tree under view.graphsNode. originalCurveNode.inputEnabledProperty is the sole new Property; clients can use it to disable interactivity of f(x).

@amanda-phet if this looks OK, please close.

amanda-phet commented 1 year ago

Looks good to me!

phetsims / calculus-grapher

Instrument `originalCurveNode.inputEnabledProperty` #240