search

phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

Public API for `isSettingStateProperty` should be read-only. #311

Closed pixelzoom closed 3 months ago

pixelzoom commented 3 months ago

Noted while investigating https://github.com/phetsims/gas-properties/issues/240#issuecomment-2104909109, yet another problem that involves isSettingPhetioStateProperty.

I find myself having to check isSettingPhetioStateProperty frequently when instrumenting sims, to short-circuit code that should not be called when restoring state. So I was surprised to find that this rather important (global!) Property allows me to set it, with no complains from tsc. For example:

    // When the number of particles in the container changes ...
    this.particleSystem.numberOfParticlesProperty.link( numberOfParticles => {
      if ( !isSettingPhetioStateProperty.value ) {
        isSettingPhetioStateProperty.value = true;

My understanding is that the only place that isSettingPhetioStateProperty should be set (and therefore the only place this should be settable) is in PhetioStateEngine.ts, specifically:

  public setState( state: FullPhetioState, scopeTandem: Tandem ): void {
    this.isSettingStateProperty.value = true;
    ....
    this.isSettingStateProperty.value = false;
  }

Let's tighten this up so that isSettingStateProperty is read-only.

samreid commented 3 months ago

Here is a working implementation of the proposal above. I used a patch instead of branches:

```diff Subject: [PATCH] Rename "Case Index" to "Case index", see https://github.com/phetsims/projectile-data-lab/issues/251 --- Index: phet-io/js/PhetioStateEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io/js/PhetioStateEngine.ts b/phet-io/js/PhetioStateEngine.ts --- a/phet-io/js/PhetioStateEngine.ts (revision f2f620fb6f958ac3e7c504355b7c7d1dd2652795) +++ b/phet-io/js/PhetioStateEngine.ts (date 1715377244737) @@ -16,7 +16,6 @@ import Emitter from '../../axon/js/Emitter.js'; import TEmitter, { TReadOnlyEmitter } from '../../axon/js/TEmitter.js'; -import TProperty from '../../axon/js/TProperty.js'; import PropertyStateHandler from '../../axon/js/PropertyStateHandler.js'; import propertyStateHandlerSingleton from '../../axon/js/propertyStateHandlerSingleton.js'; import merge from '../../phet-core/js/merge.js'; @@ -31,7 +30,7 @@ import dataStream from './dataStream.js'; import phetio from './phetio.js'; import { TPhetioEngine } from './phetioEngine.js'; -import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js'; +import { writableIsSettingPhetioStateProperty } from '../../tandem/js/isSettingPhetioStateProperty.js'; import isClearingPhetioDynamicElementsProperty from '../../tandem/js/isClearingPhetioDynamicElementsProperty.js'; import isPhetioStateEngineManagingPropertyValuesProperty from '../../tandem/js/isPhetioStateEngineManagingPropertyValuesProperty.js'; import { TPhetioStateEngine } from '../../tandem/js/TPhetioStateEngine.js'; @@ -74,7 +73,7 @@ // to indicate whether state is being set or not. In general when using for a sim. TANDEM/isSettingPhetioStateProperty // should be preferred to this Property because it will exist for all brands (where this is only available in // phet-io brand). - public isSettingStateProperty: TProperty = isSettingPhetioStateProperty; + public isSettingStateProperty = writableIsSettingPhetioStateProperty; // a list of setStateHelpers. These are sim specific functions that help set state to avoid // "Impossible set state errors". Index: projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts --- a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts (revision b00659aac770fd068b6c23828495f3b1ec3305a0) +++ b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts (date 1715377375059) @@ -187,7 +187,7 @@ } }; - isSettingPhetioStateProperty.addListener( updateHistogram ); + isSettingPhetioStateProperty.lazyLink( updateHistogram ); // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes. fields.forEach( field => { Index: projectile-data-lab/js/common/view/HistogramNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/common/view/HistogramNode.ts b/projectile-data-lab/js/common/view/HistogramNode.ts --- a/projectile-data-lab/js/common/view/HistogramNode.ts (revision b00659aac770fd068b6c23828495f3b1ec3305a0) +++ b/projectile-data-lab/js/common/view/HistogramNode.ts (date 1715377366180) @@ -258,7 +258,7 @@ } }; - isSettingPhetioStateProperty.addListener( () => updateHistogram( true ) ); + isSettingPhetioStateProperty.lazyLink( () => updateHistogram( true ) ); // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes. fields.forEach( field => { Index: tandem/js/isSettingPhetioStateProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/isSettingPhetioStateProperty.ts b/tandem/js/isSettingPhetioStateProperty.ts --- a/tandem/js/isSettingPhetioStateProperty.ts (revision a444e3f516f4c52eea41f71d87e4051450fa1c1c) +++ b/tandem/js/isSettingPhetioStateProperty.ts (date 1715377505170) @@ -2,15 +2,22 @@ /** * Property that is set to true when the PhET-iO State Engine is setting the state of a simulation. + * * @author Sam Reid (PhET Interactive Simulations) * @author Michael Kauzmann (PhET Interactive Simulations) */ import tandemNamespace from './tandemNamespace.js'; import TinyProperty from '../../axon/js/TinyProperty.js'; +import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; -const isSettingPhetioStateProperty = new TinyProperty( false ); +// This one is for specialized usage in the PhetioStateEngine, which changes the value +const writableIsSettingPhetioStateProperty = new TinyProperty( false ); + +// Simulations can use this one to observe the value +const isSettingPhetioStateProperty: TReadOnlyProperty = writableIsSettingPhetioStateProperty; tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty ); -export default isSettingPhetioStateProperty; \ No newline at end of file +export default isSettingPhetioStateProperty; +export { writableIsSettingPhetioStateProperty }; \ No newline at end of file ```

This is also passing all precommit hooks (took 1m45s). @pixelzoom or @zepumph please review and feel free to commit if it passes review.

pixelzoom commented 3 months ago

The patch is marginally better, but still provides a public API for modifying writableIsSettingPhetioStateProperty.

Since PhetioStateEngine is the sole place where isSettingPhetioStateProperty should be set, a better implementation would be:

const isSettingPhetioStateProperty = PhetioStateEngine.isSettingPhetioStateProperty;

tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty );

export default isSettingPhetioStateProperty;
pixelzoom commented 3 months ago

Below is a patch to demonstrate the implementation I described in https://github.com/phetsims/tandem/issues/311#issuecomment-2105322229. It's equivalent to the current implementation in that isSettingPhetioStateProperty is still settable. To make it readonly, add a type specification to public static readonly isSettingPhetioStateProperty. But I'm not sure what that type specification is, since I'm not familiar with the TinyProperty hierarchy. And the readonly type needs to support addListener, which is used (exclusively) by PDL.

patch ```diff Subject: [PATCH] add isSettingPhetioStateProperty check, https://github.com/phetsims/gas-properties/issues/238 --- Index: phet-io/js/PhetioStateEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io/js/PhetioStateEngine.ts b/phet-io/js/PhetioStateEngine.ts --- a/phet-io/js/PhetioStateEngine.ts (revision f2f620fb6f958ac3e7c504355b7c7d1dd2652795) +++ b/phet-io/js/PhetioStateEngine.ts (date 1715379079440) @@ -31,10 +31,10 @@ import dataStream from './dataStream.js'; import phetio from './phetio.js'; import { TPhetioEngine } from './phetioEngine.js'; -import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js'; import isClearingPhetioDynamicElementsProperty from '../../tandem/js/isClearingPhetioDynamicElementsProperty.js'; import isPhetioStateEngineManagingPropertyValuesProperty from '../../tandem/js/isPhetioStateEngineManagingPropertyValuesProperty.js'; import { TPhetioStateEngine } from '../../tandem/js/TPhetioStateEngine.js'; +import TinyProperty from '../../axon/js/TinyProperty.js'; // constants const DELETED_VALUE = 'DELETED'; // this is copied in documentation for PhetioEngineIO.getChangedState(), and used in Studio. @@ -48,6 +48,8 @@ alreadyCalledForThisStateSet?: boolean; }; +const isSettingPhetioStateProperty = new TinyProperty( false ); + class PhetioStateEngine implements TPhetioStateEngine { // emits before state the state is set. Emits with the state object that will be set. Use the @@ -86,6 +88,8 @@ hasListenerOrderDependencies: true // We rely on Properties undeferring value first (since everything else just notifies). } ); + public static readonly isSettingPhetioStateProperty = isSettingPhetioStateProperty; + public constructor( private readonly phetioEngine: TPhetioEngine, providedOptions?: PhetioStateEngineOptions ) { const options = optionize()( { Index: tandem/js/isSettingPhetioStateProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/isSettingPhetioStateProperty.ts b/tandem/js/isSettingPhetioStateProperty.ts --- a/tandem/js/isSettingPhetioStateProperty.ts (revision a444e3f516f4c52eea41f71d87e4051450fa1c1c) +++ b/tandem/js/isSettingPhetioStateProperty.ts (date 1715379079447) @@ -7,9 +7,9 @@ */ import tandemNamespace from './tandemNamespace.js'; -import TinyProperty from '../../axon/js/TinyProperty.js'; +import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js'; -const isSettingPhetioStateProperty = new TinyProperty( false ); +const isSettingPhetioStateProperty = PhetioStateEngine.isSettingPhetioStateProperty; tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty ); ```
pixelzoom commented 3 months ago

But I'm not sure what that type specification is, since I'm not familiar with the TinyProperty hierarchy. And the readonly type needs to support addListener, which is used (exclusively) by PDL.

Below is a patch that I believe addresses the problem.

patch ```diff Subject: [PATCH] add isSettingPhetioStateProperty check, https://github.com/phetsims/gas-properties/issues/238 --- Index: phet-io/js/PhetioStateEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io/js/PhetioStateEngine.ts b/phet-io/js/PhetioStateEngine.ts --- a/phet-io/js/PhetioStateEngine.ts (revision f2f620fb6f958ac3e7c504355b7c7d1dd2652795) +++ b/phet-io/js/PhetioStateEngine.ts (date 1715379455639) @@ -31,10 +31,11 @@ import dataStream from './dataStream.js'; import phetio from './phetio.js'; import { TPhetioEngine } from './phetioEngine.js'; -import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js'; import isClearingPhetioDynamicElementsProperty from '../../tandem/js/isClearingPhetioDynamicElementsProperty.js'; import isPhetioStateEngineManagingPropertyValuesProperty from '../../tandem/js/isPhetioStateEngineManagingPropertyValuesProperty.js'; import { TPhetioStateEngine } from '../../tandem/js/TPhetioStateEngine.js'; +import TinyProperty from '../../axon/js/TinyProperty.js'; +import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js'; // constants const DELETED_VALUE = 'DELETED'; // this is copied in documentation for PhetioEngineIO.getChangedState(), and used in Studio. @@ -48,6 +49,8 @@ alreadyCalledForThisStateSet?: boolean; }; +const isSettingPhetioStateProperty = new TinyProperty( false ); + class PhetioStateEngine implements TPhetioStateEngine { // emits before state the state is set. Emits with the state object that will be set. Use the @@ -86,6 +89,8 @@ hasListenerOrderDependencies: true // We rely on Properties undeferring value first (since everything else just notifies). } ); + public static readonly isSettingPhetioStateProperty: TReadOnlyProperty = isSettingPhetioStateProperty; + public constructor( private readonly phetioEngine: TPhetioEngine, providedOptions?: PhetioStateEngineOptions ) { const options = optionize()( { Index: projectile-data-lab/js/common/view/HistogramNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/common/view/HistogramNode.ts b/projectile-data-lab/js/common/view/HistogramNode.ts --- a/projectile-data-lab/js/common/view/HistogramNode.ts (revision b00659aac770fd068b6c23828495f3b1ec3305a0) +++ b/projectile-data-lab/js/common/view/HistogramNode.ts (date 1715379466140) @@ -258,7 +258,7 @@ } }; - isSettingPhetioStateProperty.addListener( () => updateHistogram( true ) ); + isSettingPhetioStateProperty.link( () => updateHistogram( true ) ); // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes. fields.forEach( field => { Index: tandem/js/isSettingPhetioStateProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/tandem/js/isSettingPhetioStateProperty.ts b/tandem/js/isSettingPhetioStateProperty.ts --- a/tandem/js/isSettingPhetioStateProperty.ts (revision a444e3f516f4c52eea41f71d87e4051450fa1c1c) +++ b/tandem/js/isSettingPhetioStateProperty.ts (date 1715379079447) @@ -7,9 +7,9 @@ */ import tandemNamespace from './tandemNamespace.js'; -import TinyProperty from '../../axon/js/TinyProperty.js'; +import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js'; -const isSettingPhetioStateProperty = new TinyProperty( false ); +const isSettingPhetioStateProperty = PhetioStateEngine.isSettingPhetioStateProperty; tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty ); Index: projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts --- a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts (revision b00659aac770fd068b6c23828495f3b1ec3305a0) +++ b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts (date 1715379483432) @@ -187,7 +187,7 @@ } }; - isSettingPhetioStateProperty.addListener( updateHistogram ); + isSettingPhetioStateProperty.link( updateHistogram ); // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes. fields.forEach( field => { ```

In PhetioStateEngine:

const isSettingPhetioStateProperty = new TinyProperty( false );
...
  public static readonly isSettingPhetioStateProperty: TReadOnlyProperty<boolean> = isSettingPhetioStateProperty;

Note that this requires PDL to use link instead of addListener -- which seems highly desirable. isSettingPhetioStateProperty was formerly of type TinyProperty, which unfortunately inherits addListener from TinyEmitter.

@samreid @zepumph please review, commit if this seems OK.

samreid commented 3 months ago

This proposed line is incompatible with our constraints:

In tandem/js/isSettingPhetioStateProperty.ts

import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js';

phet-io cannot be imported from tandem, because phet-io is a private repo and not everyone (such as open source clients) can clone it.

pixelzoom commented 3 months ago

Does it also follow that sims cannot import PhetioStateEngine.ts to use PhetioStateEngine.isSettingPhetioStateProperty?

samreid commented 3 months ago

Correct, public code cannot statically import private code. Instead, we use a dynamic import in simLauncher like so: https://github.com/phetsims/joist/blob/main/js/simLauncher.ts#L23-L31

pixelzoom commented 3 months ago

Then I guess your patch in https://github.com/phetsims/tandem/issues/311#issuecomment-2105311277 is as good as it's going to get. I'm OK with that.

pixelzoom commented 3 months ago

@zepumph are you OK with @samreid's patch in https://github.com/phetsims/tandem/issues/311#issuecomment-2105311277?

zepumph commented 3 months ago

Yes perfect. Thanks for discussing. I committed with @samreid this morning.

samreid commented 3 months ago

Thanks @zepumph, @pixelzoom want to review or close?

pixelzoom commented 3 months ago

👍🏻