Support setting a partial instead of total state

[samreid]

From phetsims/phet-io#1697: There is 1 or more call to phetioStateEngine.stateSetEmitter.addListener. There is now a 2nd argument to the listener, which is a predicate that tests Tandems and returns true if they are in scope. We should check these listeners and see if any of them need to check whether they are in scope of a partial state set.

[zepumph]

In this sim, we have the following listener in MultipleParticleModel.js:

 Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( ( state, inScope ) => {
      // make sure that we have the right number of scaled (i.e. non-normalized) atoms
      const numberOfNormalizedMolecules = this.moleculeDataSet.numberOfMolecules;
      const numberOfNonNormalizedMolecules = this.scaledAtoms.length / this.moleculeDataSet.atomsPerMolecule;
      if ( numberOfNormalizedMolecules > numberOfNonNormalizedMolecules ) {
        this.addAtomsForCurrentSubstance( numberOfNormalizedMolecules - numberOfNonNormalizedMolecules );
      }
      else if ( numberOfNonNormalizedMolecules > numberOfNormalizedMolecules ) {
        _.times( ( numberOfNonNormalizedMolecules - numberOfNormalizedMolecules ) * this.moleculeDataSet.atomsPerMolecule, () => {
          this.scaledAtoms.pop();
        } );
      }

      // clear the injection counter - all atoms and molecules should be accounted for at this point
      this.numMoleculesQueuedForInjectionProperty.reset();

      // synchronize the positions of the scaled atoms to the normalized data set
      this.syncAtomPositions();
    } );

This feels very screen dependent, and so I think it should be wrapped in the isInScope check.

[zepumph]

When I did this, it was working very well, and I am feeling good about the change. I would close this issue, but instead I am going to pass it off to @jbphet to take a look for his knowledge gain. Recently PhET-iO state took on the ability to set subsets of a sim, like a screen. Now the stateSetEmitter gives a function that allows you to check to see if this should apply to a provided tandem. I used it in MultipleParticleModel.js as seen above. Feel free to close, ask questions, or discuss.

[zepumph]

Just kidding. Please ignore, @samreid and I are changing the API as we speak!

[zepumph]

Over in phetsims/tandem#193, we altered the api for the stateSetEmitter slightly. Now instead of checking a function, you check against a "scopeTandem" that serves as the root of the tree being state set.

@jbphet everything in #338 (comment) still applies except that the api changed from checking based on a function, to checking based on a Tandem. See ba4746a. I tested in studio by customizing some stuff on the first two screens relating to the particles, and then launching. I saw that full state set correctly. Then after playing with it a bit more, I saw that reset worked as expected, setting back to the launched state.

Please note the update to the PhET-iO state API. Feel free to close.

[kathy-phet]

We want to understand this new feature.

[zepumph]

This is not particularly new, but has been expanded and formalized recently. We have been setting partial state on just a screen since the last solution implemented in https://github.com/phetsims/studio/issues/35. We recently worked at building this out so that reset works more generally for sim scenes also, and therefore arbitrary subtrees in the Tandem tree. This cleanup should have been done back then, but likely hasn't been causing too many bugs. In https://github.com/phetsims/phet-io/issues/1697 (this issue's parent), we are going through all usages of an emitter that up until this point had been used with an assumption about setting an entire state, but could actually be called also when setting a screen/scene.

[zepumph]

Oh, and perhaps you may be worried because we talked about changing the API above a lot. This is all internal phet code api. The way that sims talk to the state engine, nothing about the public PhET-iO API has changed.

[jbphet]

I reviewed the code and I understand it well enough for now. I did some reformatting to better respect the 120 character line limit.

I think this code may end up being removed when I address #333.

@zepumph - I'm assuming that the changes made for this issue do not need to be propagated into the 1.2 release branch, which was created relatively recently (about two weeks ago). If that's correct, feel free to close this. If not, please document why and assign this issue back to me.

[zepumph]

I think that you, as the primary dev should determine if it deserves to be in MR.

Here is the question to ask. Does that above, wrapped code in the setStateEmitter cause a problem if it was called on the screen 1 model when the screen 2 reset all button reset, setting only the state for screen 2?

It could be a no-op, or it could be buggy. I'm not able to answer that with my knowledge of the particle model.

Either way it doesn't seem too high stakes since the sim made it through rc testing just fine. Up to you!

[jbphet]

In #333 I ended up moving the code that was hooked to the stateSetEmitter into the applyState method so that all of the state setting code was in one place. This seems to work fine in testing.

I believe that this makes much of the discussion in this issue moot, and it obviates the need for a isInScope check. I'm not 100% certain, though, so I'd like @samreid to review and make sure that this has been handled correctly with respect to phet-io state setting.

@samreid - My attempt to boil it down to a single question - and thereby save you time - is this: If the stateSetEmitter is no longer being used, is there any reason to worry about which tandems are and are not in scope? If the answer is no, I think this can be closed. If it's more complicated than that, I'll need to work with you to understand why and what should be done about it.

[zepumph]

If the stateSetEmitter is no longer being used, is there any reason to worry about which tandems are and are not in scope? If the answer is no, I think this can be closed. If it's more complicated than that, I'll need to work with you to understand why and what should be done about it.

I was here and read through this, so I'll just answer for the phet-io context. . . .

Your above commit (b78f163) does a very good job of eliminating the problem, because applyState will only be called for that particular instance of the model (screen-specific).

The question still remains though if this patch made it into the currently released sim, and, if not, if that is an issue.

Again to reiterate:

1. I love you commit, it is clean and great because it gets rid of a usage of setStateEmitter.addListener().
2. The fact remains that the potentially buggy code is in the published sim:

   states-of-matter/js/common/model/MultipleParticleModel.js

   Lines 316 to 336 in 5ca393b

   	// perform any phet-io-specific state setting actions
   	Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => {
   	// make sure that we have the right number of scaled (i.e. non-normalized) atoms
   	const numberOfNormalizedMolecules = this.moleculeDataSet.numberOfMolecules;
   	const numberOfNonNormalizedMolecules = this.scaledAtoms.length / this.moleculeDataSet.atomsPerMolecule;
   	if ( numberOfNormalizedMolecules > numberOfNonNormalizedMolecules ) {
   	this.addAtomsForCurrentSubstance( numberOfNormalizedMolecules - numberOfNonNormalizedMolecules );
   	}
   	else if ( numberOfNonNormalizedMolecules > numberOfNormalizedMolecules ) {
   	_.times( ( numberOfNonNormalizedMolecules - numberOfNormalizedMolecules ) * this.moleculeDataSet.atomsPerMolecule, () => {
   	this.scaledAtoms.pop();
   	} );
   	}
   	// clear the injection counter - all atoms and molecules should be accounted for at this point
   	this.numMoleculesQueuedForInjectionProperty.reset();
   	// synchronize the positions of the scaled atoms to the normalized data set
   	this.syncAtomPositions();
   	} );

   . This is state at the head of the 1.2 branch. Thus my question remains from Support setting a partial instead of total state #338 (comment):

Does that above, wrapped code in the setStateEmitter cause a problem if it was called on the screen 1 model when the screen 2 reset all button reset, setting only the state for screen 2?

Up to you!

[jbphet]

Thanks @zepumph, I understand the concern you're expressing. The first portion of the code in question won't be executed in the described case, because there will be no discrepancy between the number of non-normalized and normalized particles on the non-active screen. The last part - the call to syncAtomPositions will essentially be a no-op, because it will be synchronizing particles that would have no reason at that point be be unsynchronized. So, the only part that could potentially cause a problem would be the code that clears the count of molecules that are queued for injection. I went to the State wrapper in the live version (1.2.0) to see if I could ever create a situation where there were queued particles on one screen and then perform a reset on the other. I couldn't. One would have to move really fast. And, even if the counter had something in it and it got cleared, a subsequent setting of the state would clear up the problem, since any missing particles would be added as part of the state. I also just messed around a bunch with setting up state on screens 1 & 2 and switching back and forth between them and pressing reset while running in the state wrapper, and I didn't see anything odd.

Bottom line (literally): I don't think there is any need to do a maintenance release with this patch. Closing.