State Wrapper is setting `pointsProperty` for derived curves

[pixelzoom]

For https://github.com/phetsims/qa/issues/924 ...

While working on #325, I added this to Curve.ts:

this.pointsProperty.lazyLink( () => console.log( `${this.pointsProperty.phetioID}`  ) );

... and was dismayed to see that every curve (not just originalCurve and predictCurve) is having its pointsProperty set by the State Wrapper:

Getting the state took 265 ms.
Curve.ts:124 calculusGrapher.derivativeScreen.model.curves.originalCurve.pointsProperty
Curve.ts:124 calculusGrapher.derivativeScreen.model.curves.predictCurve.pointsProperty
Curve.ts:124 calculusGrapher.derivativeScreen.model.curves.derivative.pointsProperty
Curve.ts:124 calculusGrapher.integralScreen.model.curves.originalCurve.pointsProperty
Curve.ts:124 calculusGrapher.integralScreen.model.curves.predictCurve.pointsProperty
Curve.ts:124 calculusGrapher.integralScreen.model.curves.integral.pointsProperty
Curve.ts:124 calculusGrapher.advancedScreen.model.curves.originalCurve.pointsProperty
Curve.ts:124 calculusGrapher.advancedScreen.model.curves.predictCurve.pointsProperty
Curve.ts:124 calculusGrapher.advancedScreen.model.curves.derivative.pointsProperty
Curve.ts:124 calculusGrapher.advancedScreen.model.curves.integral.pointsProperty
Curve.ts:124 calculusGrapher.labScreen.model.curves.originalCurve.pointsProperty
Curve.ts:124 calculusGrapher.labScreen.model.curves.predictCurve.pointsProperty
Curve.ts:124 calculusGrapher.labScreen.model.curves.derivative.pointsProperty
Curve.ts:124 calculusGrapher.labScreen.model.curves.secondDerivative.pointsProperty
Curve.ts:124 calculusGrapher.labScreen.model.curves.integral.pointsProperty
Setting the state took 961 ms.

When we added thepointsProperty feature in https://github.com/phetsims/calculus-grapher/issues/90, our requirements were:

(1) Be able to set/get pointsProperty for originalCurve and predictCurve via Studio and wrappers (2) Be able to get pointsProperty for derived curves via wrappers only, and never set its value.

We are violating the 2nd requirements. The State Wrapper is setting pointsProperty for derived curves.

So in https://github.com/phetsims/calculus-grapher/issues/309, we elimintated the curveChangedEmitter that was called for derived curves, and impacting PhET-iO performance. But the value of pointsProperty is still being set for derived curves. While it's not currently causing a problem, ignoring a state change is a bad practice. And does it allow clients to set pointsProperty for derived curves?

If we added phetioState: false to pointsProperty for the derived curves, will this address the issue? And will we still be able to get (but not set) the value of pointsProperty via a wrapper?

[pixelzoom]

I'll need to discuss with @samreid or @zepumph, and will reach out to them on Slack.

@veillette FYI.

[samreid]

How can you be sure that the derived property value isn't just changing in response to other properties that change during the state set?

[pixelzoom]

Thanks for the reply @samreid . But I can tell by your question that I need to catch you up on the model and requirements. So this is an issue that will be more effective to discuss synchronously.

[pixelzoom]

I made this change to Curve.ts, to make pointsProperty have phetioState: false for derived curves:

    this.pointsProperty = new Property( initialPoints, {
      isValidValue: points => isValidPoints( initialPoints, points ),
      tandem: options.tandem.createTandem( 'pointsProperty' ),
      phetioValueType: ArrayIO( CurvePoint.CurvePointIO ),
      phetioReadOnly: options.pointsPropertyReadOnly,
+     phetioState: !options.pointsPropertyReadOnly,

In the State Wrapper, I verified that this eliminates get/set state for pointsProperty of the derived curves. The time to set state (as reported in the console) went from ~900ms to ~500ms, a huge performance boost.

In Studio, I switched Chrome dev tools to "JavaScript context: sim-frame", then typed this into the console. So I believe this means that clients can still request pointsProperty.value even if phetioState: false.

> phet.phetio.phetioEngine.phetioObjectMap[ 'calculusGrapher.derivativeScreen.model.curves.derivative.pointsProperty' ].value
< (1251) [CurvePoint, ...]

[pixelzoom]

Assuming that phetioState: false is the solution, I'm wondering about a couple of other things...

• [ ] Should CurveOptions.pointsPropertyReadOnly be replaced with something like pointsAreDerived or pointsPropertyOptions? E.g.:

    this.pointsProperty = new Property( initialPoints, {
...
      phetioReadOnly: options. pointsAreDerived,
      phetioState: !options. pointsAreDerived,

• [ ] Should a guard be added to Curve.ts, like the one below? If something changes in the future that requires setting pointsProperty, this would tell us that there's more work to be done.

    if ( !this.pointsProperty.phetioState ) {
      this.pointsProperty.lazyLink( () => {
        assert && assert( false, 'pointsProperty should not change if it is phetioState:false' );
      } );
    }

• [ ] Should this code that was added to Curve.ts for #309:

    if ( Tandem.PHET_IO_ENABLED && this.pointsProperty.isPhetioInstrumented() && !this.pointsProperty.phetioReadOnly ) {
      this.pointsProperty.lazyLink( () => {
        phet.log && phet.log( `pointsProperty changed: ${this.phetioID}` );
        this.curveChangedEmitter.emit();
      } );
    }

... be relocated to TransformedCurve.ts, like this:

    this.pointsProperty.lazyLink( () => this.curveChangedEmitter.emit() );

[pixelzoom]

@samreid said this is a better way of confirming that clients can get a value:

await phetioClient.invokeAsync( 'calculusGrapher.derivativeScreen.model.curves.derivative.pointsProperty', 'getValue' )

[samreid]

I think I understand why I was confused. calculusGrapher.derivativeScreen.model.curves.derivative.pointsProperty is actually not a DerivedProperty in the code, so that's why the value is going through. Properties that are phetioReadOnly: true still participate in state if they are phetioState: true, so you can consider whether calculusGrapher.derivativeScreen.model.curves.derivative.pointsProperty should be changed from Property to DerivedProperty.

[pixelzoom]

... you can consider whether calculusGrapher.derivativeScreen.model.curves.derivative.pointsProperty should be changed from Property to DerivedProperty.

That's not going to work with the current architecture. For derived curves, it's now a Property that can take only 1 value, is phetioReadOnly: true, and phetioState: false.

[pixelzoom]

Changes committed to master and 1.0 in the above commits.

In addition to reducing the time to set state by 44% (from ~900ms to ~500 ms, for unbuilt), the API file is now 37% smaller (from 6.12MB to 3.85MB).

@veillette would you please have a look, to check my work? If you run the State Wrapper with ?log, it will identify the pointsProperty elements that are changed when setting state.

Assign back to me when done.

[veillette]

It is a nice performance improvement for the state wrapper. I ran it with ?log and only the predictCurve and originalCurve are getting changed, as intended.

[veillette]

I also reviewed the commits. Assigning back to @pixelzoom for cherry-picking.

[zepumph]

@samreid, will you please create a separate issue in the phet-io repo if you think that we should investigate having phetioReadOnly respected by the state wrapper? I believe it will open up a can of worms, because we need to support setting implementation details.

• [ ] At the very least, let's document in code and markdown that phetioReadOnly does not apply to being set by PhET-iO State.

[samreid]

@samreid, will you please create a separate issue in the phet-io repo if you think that we should investigate having phetioReadOnly respected by the state wrapper?

I believe values with phetioState: true, phetioReadOnly: true should be restored during via setState. To remove something from the state, mark it as phetioState: false or to prevent something from being set during setState, use phetioType: DerivedPropertyIO or a similar pattern in a custom IO Type.

[pixelzoom]

Cherry-picking was done above, in https://github.com/phetsims/calculus-grapher/commit/f8179ab8c70ddd1f5ef3facf40702027cd48ee7c, https://github.com/phetsims/calculus-grapher/commit/bc73aa51c5caf8bedc49afaa3c7e19feb4d44ab7, and https://github.com/phetsims/phet-io-sim-specific/commit/e4189f9ebfa8e22ebdd4d748c04fd98d6be59197.

This is a relatively major change. But it's isolated to 2 files. So we'll continue with the current RC test https://github.com/phetsims/qa/issues/924, and verify in the next RC. I'll summarize what/how to test in a forthcoming comment.

[zepumph]

Any updates to doc needed?

[pixelzoom]

@zepumph said:

Any updates to doc needed?

Yes - what you suggested in https://github.com/phetsims/calculus-grapher/issues/327#issuecomment-1490757337 sounds great. Tracking in https://github.com/phetsims/phet-io/issues/1927.

[pixelzoom]

To verify in the next RC:

In PhET brand, verify that:

• [ ] Interacting with f(x) and "Predict" curves is as responsive as the previous RC.
• [ ] Changes to f(x) result in correct changes to derived curves (integral, derivative, second derivative).

In Studio, verify that:

• [ ] Interacting with f(x) and "Predict" curves is as responsive as the previous RC.
• [ ] Changes to f(x) result in correct changes to derived curves (integral, derivative, second derivative).
• [ ] originalCurve.pointsProperty and predictCurve.pointsProperty have "Get Value" and "Set Value" buttons, and that they work as expected.
• [ ] integral.pointsProperty, derivative.pointsProperty and secondDerivative.pointsProperty do not have "Get Value" and "Set Value" buttons.

In the State Wrapper, verify that:

• [ ] The State Wrapper is more responsive than the previous RC.
• [ ] Changes to f(x) and "Predict" curves made in the Upstream sim appear in the DownStream sim.
• [ ] Changes to f(x) in the Upstream sim result in correct changes to derived curves (integral, derivative, second derivative) in the Upstream and Downstream sims.

[Nancy-Salpepi]

Things looks good in rc.2:

In PhET brand, verify that:

• [X] Interacting with f(x) and "Predict" curves is as responsive as the previous RC.

• [x] Changes to f(x) result in correct changes to derived curves (integral, derivative, second derivative).

In Studio, verify that:

• [x] Interacting with f(x) and "Predict" curves is as responsive as the previous RC.

• [x] Changes to f(x) result in correct changes to derived curves (integral, derivative, second derivative).

• [x] originalCurve.pointsProperty and predictCurve.pointsProperty have "Get Value" and "Set Value" buttons, and that they work as expected.

• [x] integral.pointsProperty, derivative.pointsProperty and secondDerivative.pointsProperty do not have "Get Value" and "Set Value" buttons.

In the State Wrapper, verify that:

• [x] The State Wrapper is more responsive than the previous RC.

   I compared with set state rate = 0 and using the console. 

• [x] Changes to f(x) and "Predict" curves made in the Upstream sim appear in the DownStream sim.

• [x] Changes to f(x) in the Upstream sim result in correct changes to derived curves (integral, derivative, second derivative) in the Upstream and Downstream sims.