Can't clear an entire container with state

[zepumph]

Discovered while working on #190. This bug is that when you clear all the state from a container, then it will have no entries in the state, and PhetioDynamicElementContainer won't clear its current state in the downstream simulation because it thinks that the state being set doesn't apply to it (because it checks and sees no sub tandems from the state-to-set).

This likely came about from the work done over in https://github.com/phetsims/phet-io/issues/1668, and in a more recent issue about setting partial vs full states in https://github.com/phetsims/phet-io/issues/1678.

To reproduce the problem, run phet-io-test sim in the state wrapper, create a red circle in the top left, let it get set to the downstream sim, and then press reset all. The circle will clear in the top, but not in the bottom because there is nothing in the state that applies to the circleNodeCapsule, so it doesn't think it should clear itself.

[samreid]

To solve this, do you recommend that when a full state is set, all containers clear themselves?

[zepumph]

Yes I think that is reasonable, though I'm not sure exactly how to do that at this time.

[zepumph]

I thought that a direction like this may work, but it doesn't handle the fact that we still have partial state setting cases where we wnt to clear the entire container, like for screen reset:

Index: tandem/js/PhetioDynamicElementContainer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- tandem/js/PhetioDynamicElementContainer.js	(revision 9810b19e139279066337077d4d23f23c61144adb)
+++ tandem/js/PhetioDynamicElementContainer.js	(date 1595546136751)
@@ -122,15 +122,23 @@
       const phetioStateEngine = phet.phetio.phetioEngine.phetioStateEngine;
 
       // On state start, clear out the container and set to defer notifications.
-      phetioStateEngine.onBeforeStateSetEmitter.addListener( phetioIDsToSet => {
-        for ( let i = 0; i < phetioIDsToSet.length; i++ ) {
-          const phetioID = phetioIDsToSet[ i ];
-          if ( phetioID.startsWith( this.tandem.phetioID ) ) {
+      phetioStateEngine.onBeforeStateSetEmitter.addListener( ( settingFullState, phetioIDsToSet ) => {
+
+        // always clear when setting full state
+        if ( settingFullState ) {
+          this.clear( { fromStateSetting: true } );
+          this.setNotificationsDeferred( true );
+        }
+        else {
+          for ( let i = 0; i < phetioIDsToSet.length; i++ ) {
+            const phetioID = phetioIDsToSet[ i ];
+            if ( phetioID.startsWith( this.tandem.phetioID ) ) {
 
-            // specify that this is from state setting
-            this.clear( { fromStateSetting: true } );
-            this.setNotificationsDeferred( true );
-            return;
+              // specify that this is from state setting
+              this.clear( { fromStateSetting: true } );
+              this.setNotificationsDeferred( true );
+              return;
+            }
           }
         }
       } );
Index: phet-io/js/PhetioStateEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- phet-io/js/PhetioStateEngine.js	(revision 51d34dca055999b78faeaf83351031f3de404bc2)
+++ phet-io/js/PhetioStateEngine.js	(date 1595546136757)
@@ -215,6 +215,9 @@
 
     options = merge( {
 
+      // When true, this means that the entire sim's state is being set
+      settingFullState: false,
+
       // {string} if provided, state will only be set on phetioID subtrees of this prefix.
       phetioIDPrefix: ''
     }, options );
@@ -236,7 +239,7 @@
     const phetioIDsToSet = _.keys( stateToSet );
 
     // called with the list of elements phetioIDs that are about to get set
-    this.onBeforeStateSetEmitter.emit( Object.keys( stateToSet ) );
+    this.onBeforeStateSetEmitter.emit( options.settingFullState, Object.keys( stateToSet ) );
 
     // reset currentPhetioIDs, only use the elements from the state that apply to the prefix.
     let currentPhetioIDs = phetioIDsToSet;

@samreid, I feel stuck, and I don't know how to proceed on this as it pertains to the work done over in https://github.com/phetsims/phet-io/issues/1678

[samreid]

Here's an untested patch that uses the tandem as a guard to know when a PhetioDynamicElementContainer should be reset--if it is in the subtree of the Tandem being reset. It also takes a step toward using tandems instead of phetioIDs but that could be improved.

Index: main/phet-io/js/PhetioStateEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/phet-io/js/PhetioStateEngine.js	(revision 51d34dca055999b78faeaf83351031f3de404bc2)
+++ main/phet-io/js/PhetioStateEngine.js	(date 1595607746300)
@@ -58,7 +58,7 @@
     this.onBeforeSetValueEmitter = new Emitter( { parameters: [ { valueType: PhetioObject } ] } );
 
     // @public {Emitter} - emits before state the state is set. Emits with the state object that will be set.
-    this.onBeforeStateSetEmitter = new Emitter( { parameters: [ { valueType: Array } ] } );
+    this.onBeforeStateSetEmitter = new Emitter( { parameters: [ { valueType: Tandem }, { valueType: Array } ] } );
 
     // @private {Object|null} - keep track of the initial state of the simulation to save space while emitting and
     // setting states. Should never be null after initialize() has been called.
@@ -197,7 +197,7 @@
 
     // No-op if there is no customized state.  Avoid re-entrance, in case this is triggered during another state set.
     if ( this.mostRecentSavedState && !this.isSettingStateProperty.value ) {
-      this.setState( this.mostRecentSavedState, { phetioIDPrefix: phetioObject.tandem.phetioID } );
+      this.setState( this.mostRecentSavedState, phetioObject.tandem );
     }
   }
 
@@ -205,38 +205,30 @@
    * Sets the state from an object that represents the state.
    *
    * @param {PhetioState} state - a json object (not a string) that specifies the values to set
-   * @param {Object} [options]
+   * @param {Tandem} tandem - root of the subtree to set
    * @private
    */
-  setState( state, options ) {
+  setState( state, tandem ) {
     this.isSettingStateProperty.value = true;
     assert && assert( state !== undefined );
     assert && assert( ( typeof state ) !== 'string' );
 
-    options = merge( {
-
-      // {string} if provided, state will only be set on phetioID subtrees of this prefix.
-      phetioIDPrefix: ''
-    }, options );
-
     // {PhetioState}
-    let stateToSet = state;
+    const stateToSet = {};
 
     // only look at and set the state that applies to the subtree based on phetioIDPrefix
-    if ( options.phetioIDPrefix !== '' ) {
-      stateToSet = {};
-      const allPhetioIDs = _.keys( state );
-      for ( let i = 0; i < allPhetioIDs.length; i++ ) {
-        const phetioID = allPhetioIDs[ i ];
-        if ( phetioID.indexOf( options.phetioIDPrefix ) === 0 ) {
-          stateToSet[ phetioID ] = state[ phetioID ];
-        }
+    const allPhetioIDs = _.keys( state );
+    for ( let i = 0; i < allPhetioIDs.length; i++ ) {
+      const phetioID = allPhetioIDs[ i ];
+      if ( phetioID.indexOf( tandem.phetioID ) === 0 ) {
+        stateToSet[ phetioID ] = state[ phetioID ];
       }
     }
+
     const phetioIDsToSet = _.keys( stateToSet );
 
     // called with the list of elements phetioIDs that are about to get set
-    this.onBeforeStateSetEmitter.emit( Object.keys( stateToSet ) );
+    this.onBeforeStateSetEmitter.emit( tandem, Object.keys( stateToSet ) );
 
     // reset currentPhetioIDs, only use the elements from the state that apply to the prefix.
     let currentPhetioIDs = phetioIDsToSet;
@@ -269,7 +261,7 @@
    * @public
    */
   setFullState( state ) {
-    this.setState( state );
+    this.setState( state, Tandem.ROOT );
     this.mostRecentSavedState = state;
   }
 
Index: main/tandem/js/PhetioDynamicElementContainer.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/tandem/js/PhetioDynamicElementContainer.js	(revision 9810b19e139279066337077d4d23f23c61144adb)
+++ main/tandem/js/PhetioDynamicElementContainer.js	(date 1595607746295)
@@ -122,16 +122,12 @@
       const phetioStateEngine = phet.phetio.phetioEngine.phetioStateEngine;
 
       // On state start, clear out the container and set to defer notifications.
-      phetioStateEngine.onBeforeStateSetEmitter.addListener( phetioIDsToSet => {
-        for ( let i = 0; i < phetioIDsToSet.length; i++ ) {
-          const phetioID = phetioIDsToSet[ i ];
-          if ( phetioID.startsWith( this.tandem.phetioID ) ) {
+      phetioStateEngine.onBeforeStateSetEmitter.addListener( tandem => {
 
-            // specify that this is from state setting
-            this.clear( { fromStateSetting: true } );
-            this.setNotificationsDeferred( true );
-            return;
-          }
+        // Only clear if this PhetioDynamicElementContainer is in scope of the state to be set
+        if ( this.tandem.phetioID.startsWith( tandem.phetioID ) ) {
+          this.clear( { fromStateSetting: true } );
+          this.setNotificationsDeferred( true );
         }
       } );
 

[zepumph]

This problem is easy to reproduce with PhetioCapsule in phet-io-test-sim, but harder to do with PhetioGroup, since it creates a countProperty that is always in the state, I can reproduce this on master in CAF by dragging out a particle, letting it set state, and then resetting the sim (particle won't clear in the downstream sim) with this temp patch:

Index: js/PhetioGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetioGroup.js	(revision 9810b19e139279066337077d4d23f23c61144adb)
+++ js/PhetioGroup.js	(date 1595615505049)
@@ -54,11 +54,13 @@
       tandem: options.tandem.createTandem( 'countProperty' ),
       phetioDocumentation: 'the number of elements in the group',
       phetioReadOnly: true,
+      rangePropertyOptions: { phetioState: false },
       phetioFeatured: true,
-      numberType: 'Integer'
+      numberType: 'Integer',
+      phetioState: false
     } );
 
-    assert && this.countProperty.link( count => assert( count === this._array.length, 'countProperty should match array length.' ) );
+    // assert && this.countProperty.link( count => assert( count === this._array.length, 'countProperty should match array length.' ) );
 
     // countProperty can be overwritten during state set, see PhetioGroup.createIndexedELement(), and so this assertion
     // makes sure that the final length of the elements array matches the expected count from the state.
@@ -66,7 +68,7 @@
 
       // This supports cases when only partial state is being set
       if ( state[ this.countProperty.tandem.phetioID ] ) {
-        assert( state[ this.countProperty.tandem.phetioID ].value === this._array.length, 'countProperty should match array length.' );
+        // assert( state[ this.countProperty.tandem.phetioID ].value === this._array.length, 'countProperty should match array length.' );
       }
     } );
   }

[zepumph]

@samreid and I discussed this today, and we came up with a solution that fixes the problem. It uses that Tandem hierarchy, when setting partial state (like we already do) to convey if a dynamic element container is a descendant of the "root" Tandem of what is being state set. That could be a screen tandem, a random PhetioObject, or Tandem.ROOT for setting full state. For encapsulation, we took @samreid's patch in #193 (which by the way worked perfectly right out of the box) and instead of passing Tandem, passed a predicate that takes a Tandem and returns that Tandem applies to the current state setting. I was able to test in CAF (PhetioGroup) and phet-io-test-sim (PhetioCapsule), and both are working as expected in the state wrapper. Summary of work:

• In PhetioDynamicElementContainer when deciding if it is time to clear, instead of looking through elements of the state for a child of the container, we ask if the dynamic element container is a descendant of the root of the state setting hierarchy (encapsulated in a closure for client-side-simplicity).
• We factored "prefix-specifc" logic out of setState, and into restoreStateForPhetioObject, since that was the only method calling setState that used a prefix. This further simplifies the setState method.
• We added an assertion to the top of setState to make sure that it is never called while state setting is already occurring.

We think that this is ready for unblock for publication, even though there is more cleanup to be done below.

To Cherry-pick for RC branches
b383824
https://github.com/phetsims/phet-io/commit/4346edaeb093caddcbd8f5c24e141316935ee4fe

Assigning to @pixelzoom to take these SHAs for ph-scale(-basics), feel free to unassign yourself once that is done.

• For completeness, I wonder if we should be be passing this predicate to the stateSetEmitter too, so that we don't have the same issue with a general emitter broadcast when only a partial state is being set.
• Could using stubTrue for setting fullState be confusing? You could pass in Tandem.OPTIONAL and it would still be true. Should it check on if the Tandem is supplied at least? It also feels like ideally the predicate would return false if a PhetioObject was either uninstrumented or not phetioState:true, but I understand that we are just dealing with Tandem here, and not PhetioObjects. This bullet really only applies if we decide to provide this to the stateSetEmitter, since that will be broadly available on the state api, and not phet-io-internal like it is now.
• Instead of comparing based on strings, it could be nice to build out Tandem functionality to do this ancestor/descendant check in isInStateScope. Here is a patch of two untested Tandem functions:

Index: js/Tandem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Tandem.js	(revision 9810b19e139279066337077d4d23f23c61144adb)
+++ js/Tandem.js	(date 1595615524132)
@@ -168,6 +168,25 @@
     }
   }
 
+  /**
+   * @public
+   * @param otherTandem
+   * @returns {boolean|null|*}
+   */
+  isInSubTree( otherTandem ) {
+    return this === otherTandem || this.isDescendant( otherTandem );
+  }
+
+  /**
+   *
+   * @param otherTandem
+   * @returns {null|boolean|*}
+   * @public
+   */
+  isDescendant( otherTandem ) {
+    return this.parentTandem && ( this.parentTandem === otherTandem || this.parentTandem.isDescendant( otherTandem ) );
+  }
+
   /**
    * Removes a PhetioObject and signifies to listeners that it has been removed.
    * @param {PhetioObject} phetioObject

@samreid let's keep discussing these final points together.

[pixelzoom]

Assigning to @pixelzoom to take these SHAs for ph-scale(-basics), feel free to unassign yourself once that is done.

@zepumph please clarify why this is relevant to ph-scale(-basics), which does not use PhetioGroup or PhetioCapsule. If it is relevant, please explain how it should be regression tested by QA.

Slack discussion:

Chris Malley 10:46 AM
Question... I see that #193 needs to be cherry-picked for ph-scale and ph-scale-basics RC. How will QA know to test this in the next RC, or how to test it? I have no familiarity with this issue, and don't understand it after reading through the comments. (edited)

Sam Reid:house_with_garden: 10:48 AM
Does ph-scale or ph-scale-basics use PhetioGroup or PhetioCapsule (other than the PhetioCapsules in joist)?

Chris Malley 10:48 AM
There is no sim-specific code that uses PhetioGroup or PhetioCapsule.
Is the cherry-pick needed to correct a joist problem?

Sam Reid:house_with_garden: 10:49 AM
OK. It seems best to check in with @zepumph about whether that issue will impact the PhetioCapsules in joist.

Chris Malley 10:50 AM
And I guess my question is a general one. How will QA know to test something that has been cherry-picked and has no sim-specific issue? Should the policy be to create a sim-specific issue if you're going to cherry-pick anything from common code?

[zepumph]

In #193 (comment) I told @pixelzoom to cherry pick certain shas to fix this bug in PH scale (since it uses joist PhetioCapsules. While this bug is in the code in that shas, I now don't think he needs to cherry pick. This is because I tried to reproduce the bug in the AboutDialog and found the same thing happening here as with PhetioGroup.countProperty as described in #193 (comment). Sub-tandems like phScale.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.termsPrivacyAndLicensingLinkText.pickableProperty are always in the state. With that in mind, we could still cherry-pick, with no way to confirm the bug fixed, or we could leave it, and not cherry-pick for ph-scale. The odds of this being a problem for long-term maintenance for the AboutDialog in ph-scale is small.

I'm inclined to tell @pixelzoom to ignore this issue altogether for his sim.

[zepumph]

I think it would be best to have @samreid confirm before moving forward with ignoring this. issue for ph-scale.

[samreid]

That sounds reasonable to me, but the main part I don't understand is:

the same thing happening here as with PhetioGroup.countProperty

Can you please clarify how this is happening for PhetioCapsule?

[pixelzoom]

I don't understand this issue at all. So I will defer to @zepumph and @samreid to decide whether this should be cherry-picked for ph-scale. If it does need to be cherry-picked for ph-scale, please

• create a ph-scale issue, linked to this issue
• note the shas to be cherry picked
• describe how to regression test for the next RC

[zepumph]

@samreid and I discussed, and found that the actual reason that the AboutDialog's capsules doesn't demonstrate this bug is because the AboutDialog is never disposed, it is created lazily and then only get's show/hide calls to it. Thus, it is always in the state after lazy creation, and so never needs clearing (and never has the bug that is about clearing). We also noted that other joist PhetioCapsules aren't in this sim (keyboard help and options Dialogs)

From this @samreid and see anything to be cherry-picked to ph-scale, and see nothing to be testing for that sim. Thanks @pixelzoom for your patience.

The rest of this issue involves discussing the bottom three bullets in #193 (comment) for cleanup with @samreid:

• For completeness, I wonder if we should be be passing this predicate to the stateSetEmitter too, so that we don't have the same issue with a general emitter broadcast when only a partial state is being set.
• Could using stubTrue for setting fullState be confusing? You could pass in Tandem.OPTIONAL and it would still be true. Should it check on if the Tandem is supplied at least? It also feels like ideally the predicate would return false if a PhetioObject was either uninstrumented or not phetioState:true, but I understand that we are just dealing with Tandem here, and not PhetioObjects. This bullet really only applies if we decide to provide this to the stateSetEmitter, since that will be broadly available on the state api, and not phet-io-internal like it is now.
• Instead of comparing based on strings, it could be nice to build out Tandem functionality to do this ancestor/descendant check in isInStateScope. Here is a patch of two untested Tandem functions:

Index: js/Tandem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Tandem.js	(revision 9810b19e139279066337077d4d23f23c61144adb)
+++ js/Tandem.js	(date 1595615524132)
@@ -168,6 +168,25 @@
     }
   }
 
+  /**
+   * @public
+   * @param otherTandem
+   * @returns {boolean|null|*}
+   */

[samreid]

For completeness, I wonder if we should be be passing this predicate to the stateSetEmitter too, so that we don't have the same issue with a general emitter broadcast when only a partial state is being set.

Yes, but I'm not sure whether we should eagerly visit each stateSetEmitter.addListener call site to respect the scope. Chip away? Or address as we re-visit simulations?

Could using stubTrue for setting fullState be confusing? You could pass in Tandem.OPTIONAL and it would still be true. Should it check on if the Tandem is supplied at least? It also feels like ideally the predicate would return false if a PhetioObject was either uninstrumented or not phetioState:true, but I understand that we are just dealing with Tandem here, and not PhetioObjects. This bullet really only applies if we decide to provide this to the stateSetEmitter, since that will be broadly available on the state api, and not phet-io-internal like it is now.

I think this should just be a scope check. For example: If the scope of state set is "everything under screen2" then it seems like mySim.screen2.something.myProperty should be in the scope, even if it is not instrumented or doesn't support phet-io state, or decides not to do something during the state set operation.

Instead of comparing based on strings, it could be nice to build out Tandem functionality to do this ancestor/descendant check in isInStateScope. Here is a patch of two untested Tandem functions:

I'll work on that shortly.

[samreid]

I'm concerned that this ancestor check could fail in a corner case:

  /**
   * @public
   * @param {Tandem} ancestor
   * @returns {boolean}
   */
  isInSubTree( ancestor ) {
    return this === ancestor || this.isDescendant( ancestor );
  }

  /**
   * @param {Tandem} ancestor
   * @returns {boolean}
   * @public
   */
  isDescendant( ancestor ) {
    return this.parentTandem && ( this.parentTandem === ancestor || this.parentTandem.isDescendant( ancestor ) );
  }

For instance:

const a1 = tandem.createTandem('a');
a1.dispose();
const a2 = tandem.createTandem('a');
a1.isInSubTree(tandem); // false
a2.isInSubTree(tandem); // true

Is this the expected behavior? Should there be errors when calling methods on disposed Tandems? Should we just use a string check instead so that both of these would "match"? Let's discuss further.

[samreid]

@zepumph @chrisklus and I discussed and we recommend passing the isInStateScope predicate to this.stateSetEmitter.emit as well as onBeforeApplyStateEmitter so they can properly guard when partial state doesn't apply to them. Cases like:

  Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( function() {
    self.invalidatePaint();
  } );

may not need to be changed, if they are safe to happen even on unrelated partial state.

We briefly discussed how to move the guards to PhetioStateEngine, instead of always getting a callback for every client, then the client needing to check if the change is within its scope. We don't think this is necessary at the moment.

    this.addBeforeStateEmitter.addListener({
      me: phetioObject,
      function:()=>{}
    });
    
    this.fullStateSetEmitter.addListener();
    
    this.addPartialStateEmitter = (predicate,listener)=>{
      
    };

We sketched out a predicate that is more refined:

    this.setState( state, tandem => {
      if ( tandem ) {
        assert && assert( tandem instanceof Tandem );
      }
      const phetioObject = phetioEngine.getPhetioObject( tandem.phetioID );
      assert && assert( phetioObject, 'object not found' + tandem.phetioID );
      return phetioObject.phetioState;
    } );

My main concern would be to make sure it doesn't have any false negatives--avoiding an update when one should be applied. But if we can avoid that, it seems reasonable to proceed.

For the preceding comment, we would like to move that function into Tandem. For now it could be a string check, but we would like to experiment with the proposal in the comment, but throwing an error if you ask about a disposed tandem. If that happens rarely, we would like to clean that up and proceed with it. If it happens too much, we may need to stick with string prefix checking.

[samreid]

My working patch for later:

Index: main/phet-io/js/PhetioStateEngine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/phet-io/js/PhetioStateEngine.js	(revision 6f83cc6ed9b00d6264d5a5397afd40e86389f4c6)
+++ main/phet-io/js/PhetioStateEngine.js	(date 1597691237590)
@@ -209,7 +209,7 @@
         }
       }
 
-      this.setState( stateToSet, tandem => tandem.phetioID.startsWith( phetioObject.tandem.phetioID ) );
+      this.setState( stateToSet, tandem => tandem === phetioObject.tandem || phetioObject.tandem.hasAncestor( tandem ) );
     }
   }
 
@@ -217,19 +217,26 @@
    * Sets the state from an object that represents the state.
    *
    * @param {PhetioState} state - a json object (not a string) that specifies the values to set
-   * @param {function(Tandem):boolean} isInStateScope - if a given Tandem applies to this state setting
+   * @param {function(Tandem):boolean} scopePredicate - if a given Tandem applies to this state setting
    * @private
    */
-  setState( state, isInStateScope ) {
+  setState( state, scopePredicate ) {
     assert && assert( !this.isSettingStateProperty.value, 'cannot set state while setting state' );
     assert && assert( state !== undefined );
     assert && assert( ( typeof state ) !== 'string' );
-    assert && assert( typeof isInStateScope === 'function', 'isInStateScope is a required arg' );
+    assert && assert( typeof scopePredicate === 'function', 'isInStateScope is a required arg' );
 
     this.isSettingStateProperty.value = true;
 
+    const fullPredicate = tandem => {
+      assert && assert( tandem instanceof Tandem );
+      const phetioObject = this.phetioEngine.getPhetioObject( tandem.phetioID );
+      assert && assert( phetioObject, 'object not found' + tandem.phetioID );
+      return phetioObject.phetioState && scopePredicate( tandem );
+    };
+
     // called with root-most Tandem that encapsulates all phetioIDs that are being set.
-    this.onBeforeStateSetEmitter.emit( isInStateScope );
+    this.onBeforeStateSetEmitter.emit( fullPredicate );
 
     // reset currentPhetioIDs, only use the elements from the state that apply to the prefix.
     let currentPhetioIDs = Object.keys( state );
Index: main/tandem/js/Tandem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/tandem/js/Tandem.js	(revision a8574eb10cae861b69376e2e88a1f2b7cd73f815)
+++ main/tandem/js/Tandem.js	(date 1597691394270)
@@ -164,6 +164,16 @@
     }
   }
 
+  /**
+   * Returns true if this Tandem has the specified ancestor Tandem.
+   * @param ancestor
+   * @returns {boolean}
+   * @public
+   */
+  hasAncestor( ancestor ) {
+    return this.parentTandem === ancestor || ( this.parentTandem && this.parentTandem.hasAncestor( ancestor ) );
+  }
+
   /**
    * Removes a PhetioObject and signifies to listeners that it has been removed.
    * @param {PhetioObject} phetioObject

[samreid]

I updated the emitters and added a new issue to iterate through listeners and see if they need to change when a sub state is being set. This issue is ready for review.

[samreid]

Several sims are failing CT after the "improve scope check" tests above. Self-reassigning for investigation.

[samreid]

@chrisklus and I collaborated on this, and we discovered this test in PhetioDynamicElementContainer:

      // On state start, clear out the container and set to defer notifications.
      phetioStateEngine.onBeforeStateSetEmitter.addListener( ( state, isInStateScope ) => {

        // Only clear if this PhetioDynamicElementContainer is in scope of the state to be set
        if ( isInStateScope( this.tandem ) ) {
          this.clear( { fromStateSetting: true } );
          this.setNotificationsDeferred( true );
        }
      } );

The bunnyGroup is phetioState:false because it is a PhetioDynamicElementContainer, so when it checks if its tandem is in scope, the new rule that has:

const phetioObject = this.phetioEngine.getPhetioObject( tandem.phetioID );
return phetioObject.phetioState // ...

is returning false, i.e., indicating that the bunnyGroup is not in the scope of the reset. Therefore @chrisklus and I recommend to make the predicate based purely on tandem trees, and not checking whether items are instrumented or phetioState: true.

@zepumph can you see a better way to proceed?

[zepumph]

I found a bug in the screen-related scope check, the parent was checking if the child was an ancestor when it should have been the other way around.

[zepumph]

• Assert that tandem methods aren't called when it is disposed. UPDATE: or even better, disposed Tandems will never be ancestors to anyone, so they would always return "false" from an isAncestor check.

[zepumph]

Assert that tandem methods aren't called when it is disposed. UPDATE: or even better, disposed Tandems will never be ancestors to anyone, so they would always return "false" from an isAncestor check.

dispose already removes itself from being the parent's child:

  /**
   * @private
   */
  dispose() {
    this.parentTandem.removeChild( this.name );
  }

So I don't think there is anything else to do here.

[zepumph]

@samreid and I discussed the work done regarding isInScope functions above (commits around #193 (comment)), and decided that it made for a cleaner api and code to assume that the primary use case is to check Tandem subtrees. This is most (all?) of the cases we have in a our project. So instead of having an opaque function that takes a Tandem, we decided to provide a "scopeTandem" that was to root of the subtree being set.

I think this will handle the majority of cases, and can be guarded on in the client listener with an isAncestor call. An example of this is phetsims/states-of-matter@ba4746a.

This puts more burden, and potential for duplication on the client, but it also allows for more flexibility, and also clarity as to what logic is actually being performed. We didn't like the idea that the isInScope function hid what was actually being checked. This felt harder to maintain long term.

Now if a client usage needs to check isPhetioInstrumented() or phetioState, they can for that usage, and we don't need to worry about creating a perfect, general, and inflexible solution.

@chrisklus will you please review the above commits, and let us know if you recommend anything else for this issue.

UPDATE: I wanted to add that I tested this in NS state wrapper, and in studio with SOM and GAO. For studio I would customize something that I knew applied to a full and partial state (like part of a screen/scene that I could trigger a reset for). Then I launched the sim, confirmed the whole state was set on startup, and then, after playing with the sim a bit, confirmed that reset all/scene went back to the launched state for the partial set.

[zepumph]

@samreid will take this on.

[samreid]

I reviewed this issue and everything seems complete and ready to close.