Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
samreid opened this issue Aug 31, 2022 · 2 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 31, 2022

#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 #409 is in place?

@zepumph and I discussed it briefly.

@samreid samreid self-assigned this Aug 31, 2022
@samreid samreid changed the title Can we eliminate some order dependency code now that Properties self-guard? Can we eliminate some PhET-iO State order dependency code now that Properties self-guard? Sep 1, 2022
@samreid
Copy link
Member Author

samreid commented Sep 3, 2022

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
Copy link
Member Author

samreid commented Sep 3, 2022

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

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<T>;
   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<TReadOnlyProperty<IntentionalAny>> ): 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<T>, 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<T>, 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

@samreid samreid removed their assignment Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant