phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
10 stars 8 forks source link

Can we eliminate some PhET-iO State order dependency code now that Properties self-guard? #413

Open samreid opened 1 year ago

samreid commented 1 year ago

https://github.com/phetsims/axon/issues/409 seems like another solution to the order dependency and inconsistent states problems. Can we relax/remove any of the order dependency code now that https://github.com/phetsims/axon/issues/409 is in place?

@zepumph and I discussed it briefly.

samreid commented 1 year ago

I tried commenting out large parts of setDeferred, propertyStateHandlerSingleton and phetioDependencies and in testing several sims, it seems like things still worked. Gas properties state did not work (box width not correct in the state), but I reverted and saw that it also did not work. Therefore, I think we should pursue this more. Since the changes span many repos and any observation of bugs may want us to restore the functionality, maybe we should implement this as a "shutoff valve" that we can easily commit and revert if we see trouble. I'll look for an appropriate shutoff valve.

samreid commented 1 year ago

I tried these shutoff valves, and gravity and orbits worked ok:

```diff Index: js/DerivedProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/DerivedProperty.ts b/js/DerivedProperty.ts --- a/js/DerivedProperty.ts (revision c36835648bbc0897b01fd1bcf1677c6ec601b766) +++ b/js/DerivedProperty.ts (date 1662173812995) @@ -184,8 +184,10 @@ * notifications. This way we don't have intermediate derivation calls during PhET-iO state setting. */ public override setDeferred( isDeferred: boolean ): ( () => void ) | null { - if ( this.isDeferred && !isDeferred ) { - this.deferredValue = getDerivedValue( this.derivation, this.definedDependencies ); + if ( !ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES ) { + if ( this.isDeferred && !isDeferred ) { + this.deferredValue = getDerivedValue( this.derivation, this.definedDependencies ); + } } return super.setDeferred( isDeferred ); } Index: js/propertyStateHandlerSingleton.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/propertyStateHandlerSingleton.js b/js/propertyStateHandlerSingleton.js --- a/js/propertyStateHandlerSingleton.js (revision c36835648bbc0897b01fd1bcf1677c6ec601b766) +++ b/js/propertyStateHandlerSingleton.js (date 1662173969649) @@ -10,6 +10,10 @@ import axon from './axon.js'; import PropertyStateHandler from './PropertyStateHandler.js'; -const propertyStateHandlerSingleton = new PropertyStateHandler(); +const propertyStateHandlerSingleton = { + registerPhetioOrderDependency() {}, + initialize() {}, + unregisterOrderDependenciesForProperty() {} +}; axon.register( 'propertyStateHandlerSingleton', propertyStateHandlerSingleton ); export default propertyStateHandlerSingleton; \ No newline at end of file Index: js/ReadOnlyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ReadOnlyProperty.ts b/js/ReadOnlyProperty.ts --- a/js/ReadOnlyProperty.ts (revision c36835648bbc0897b01fd1bcf1677c6ec601b766) +++ b/js/ReadOnlyProperty.ts (date 1662174004172) @@ -109,6 +109,8 @@ protected readonly valueValidator: Validator; public static readonly TANDEM_NAME_SUFFIX: string = 'Property'; + public static readonly ALLOW_PHET_IO_ORDER_DEPENDENCIES = false; + /** * This is protected to indicate to clients that subclasses should be used instead. * @param value - the initial value of the property @@ -328,33 +330,42 @@ * - null if isDeferred is true, or if the value is unchanged since calling setDeferred(true) */ public setDeferred( isDeferred: boolean ): ( () => void ) | null { - assert && assert( !this.isDisposed, 'cannot defer Property if already disposed.' ); - if ( isDeferred ) { - assert && assert( !this.isDeferred, 'Property already deferred' ); - this.isDeferred = true; - } - else { - assert && assert( this.isDeferred, 'Property wasn\'t deferred' ); - this.isDeferred = false; + + if ( ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES ) { + assert && assert( !this.isDisposed, 'cannot defer Property if already disposed.' ); + if ( isDeferred ) { + assert && assert( !this.isDeferred, 'Property already deferred' ); + this.isDeferred = true; + } + else { + assert && assert( this.isDeferred, 'Property wasn\'t deferred' ); + this.isDeferred = false; - const oldValue = this.get(); + const oldValue = this.get(); - // Take the new value - if ( this.hasDeferredValue ) { - this.setPropertyValue( this.deferredValue! ); - this.hasDeferredValue = false; - this.deferredValue = null; - } + // Take the new value + if ( this.hasDeferredValue ) { + this.setPropertyValue( this.deferredValue! ); + this.hasDeferredValue = false; + this.deferredValue = null; + } - // If the value has changed, prepare to send out notifications (after all other Properties in this transaction - // have their final values) - if ( !this.equalsValue( oldValue ) ) { - return () => !this.isDisposed && this._notifyListeners( oldValue ); - } - } + // If the value has changed, prepare to send out notifications (after all other Properties in this transaction + // have their final values) + if ( !this.equalsValue( oldValue ) ) { + return () => !this.isDisposed && this._notifyListeners( oldValue ); + } + } - // no action to signify change - return null; + // no action to signify change + return null; + } + else { + return () => { + // ??? + }; + } + } public get value(): T { @@ -372,14 +383,16 @@ */ public addPhetioStateDependencies( dependencies: Array> ): void { assert && assert( Array.isArray( dependencies ), 'Array expected' ); - for ( let i = 0; i < dependencies.length; i++ ) { - const dependency = dependencies[ i ]; + if ( ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES ) { + for ( let i = 0; i < dependencies.length; i++ ) { + const dependency = dependencies[ i ]; - // only if running in PhET-iO brand and both Properties are instrumenting - if ( dependency instanceof ReadOnlyProperty && dependency.isPhetioInstrumented() && this.isPhetioInstrumented() ) { + // only if running in PhET-iO brand and both Properties are instrumenting + if ( dependency instanceof ReadOnlyProperty && dependency.isPhetioInstrumented() && this.isPhetioInstrumented() ) { - // The dependency should undefer (taking deferred value) before this Property notifies. - propertyStateHandlerSingleton.registerPhetioOrderDependency( dependency, PropertyStatePhase.UNDEFER, this, PropertyStatePhase.NOTIFY ); + // The dependency should undefer (taking deferred value) before this Property notifies. + propertyStateHandlerSingleton.registerPhetioOrderDependency( dependency, PropertyStatePhase.UNDEFER, this, PropertyStatePhase.NOTIFY ); + } } } } @@ -392,7 +405,7 @@ * @param [options] */ public link( listener: PropertyLinkListener, options?: LinkOptions ): void { - if ( options && options.phetioDependencies ) { + if ( options && options.phetioDependencies && ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES) { this.addPhetioStateDependencies( options.phetioDependencies ); } @@ -405,7 +418,7 @@ * listener without an immediate callback. */ public lazyLink( listener: PropertyLazyLinkListener, options?: LinkOptions ): void { - if ( options && options.phetioDependencies ) { + if ( options && options.phetioDependencies && ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES) { this.addPhetioStateDependencies( options.phetioDependencies ); } this.tinyProperty.lazyLink( listener ); Index: js/PropertyStateHandler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/PropertyStateHandler.js b/js/PropertyStateHandler.js --- a/js/PropertyStateHandler.js (revision c36835648bbc0897b01fd1bcf1677c6ec601b766) +++ b/js/PropertyStateHandler.js (date 1662173600313) @@ -129,7 +129,7 @@ * @param {PropertyStatePhase} afterPhase */ registerPhetioOrderDependency( beforeProperty, beforePhase, afterProperty, afterPhase ) { - if ( Tandem.PHET_IO_ENABLED ) { + if ( Tandem.PHET_IO_ENABLED && ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES ) { this.validatePropertyPhasePair( beforeProperty, beforePhase ); this.validatePropertyPhasePair( afterProperty, afterPhase ); @@ -157,7 +157,7 @@ * @public */ unregisterOrderDependenciesForProperty( property ) { - if ( Tandem.PHET_IO_ENABLED ) { + if ( Tandem.PHET_IO_ENABLED && ReadOnlyProperty.ALLOW_PHET_IO_ORDER_DEPENDENCIES ) { this.validateInstrumentedProperty( property ); // Be graceful if given a Property that is not registered in an order dependency. ```

However, natural selection failed with:

naturalSelection.introScreen.model.bunnyCollection.bunnyGroup.countProperty listener fired and array length differs, count=1, arrayLength=0