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

Consolidate state-setting code #333

Closed
jbphet opened this issue Aug 12, 2020 · 1 comment
Closed

Consolidate state-setting code #333

jbphet opened this issue Aug 12, 2020 · 1 comment
Assignees

Comments

@jbphet
Copy link
Contributor

jbphet commented Aug 12, 2020

While working on other issues, I found a section of code in MultipleParticleModel that does some state setting based on an emitter provided by the phetio state engine. It looks like this:

    // 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();
    } );

I think that this was part of an early effort to set state, and existed before the explicit toStateObject and applyState methods existed. It may no longer be necessary, but I'm not sure, and I don't have time to investigate at the moment. This code should either be removed or should be moved into the applyState method.

@jbphet
Copy link
Contributor Author

jbphet commented Oct 20, 2020

I did some experimentation and convinced myself that the code above is needed and can't be deleted. Specifically, thinks would get off in the state wrapper if additional atoms/molecules were injected. So, I moved the code into the applyState method, and it appears to be working well. I tested it through several launches from studio, some focused testing in the state wrapper, and then fuzz testing in the state wrapper.

This should be good to go. Closing.

@jbphet jbphet closed this as completed Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant