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

Molecule is not captured in the state #151

Open
samreid opened this issue Aug 27, 2017 · 16 comments
Open

Molecule is not captured in the state #151

samreid opened this issue Aug 27, 2017 · 16 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 27, 2017

From https://github.com/phetsims/phet-io/issues/367 molecule splitting is not captured in the state:

image

@mbarlow12
Copy link
Contributor

I don't think I have access to any of the phet-io repos. Is it possible to grant that access for me?

@samreid samreid changed the title Molecule splitting is not captured in the state Molecule is not captured in the state Aug 28, 2017
@samreid
Copy link
Member Author

samreid commented Aug 28, 2017

Likewise from https://github.com/phetsims/phet-io/issues/765

Upon breaking the Ozone or Nitrogen Dioxide molecule with an ultraviolet photon, the return molecule button does not return the molecule on the second smaller sim at all.
molecules-light phetio return button not working
Resetting the main sim or pressing return molecule on the smaller sim works as usual.

For phetsims/tasks/issues/718.

@samreid
Copy link
Member Author

samreid commented Aug 28, 2017

@ariel-phet can you please check on permissions for @mbarlow12?

@ariel-phet
Copy link

@mbarlow12 I have converted you to an "owner" which means you now have access to all our repos. Behold the awesome power....

You may need to adjust some settings if you are getting notifications from repos you are not interested in, but now you should have access to anything you need.

@ariel-phet ariel-phet assigned mbarlow12 and samreid and unassigned ariel-phet Aug 28, 2017
@samreid
Copy link
Member Author

samreid commented Aug 28, 2017

Thanks @ariel-phet. @mbarlow12 let me know if I can provide any context/assistance as you are learning about PhET-iO.

@samreid samreid removed their assignment Aug 28, 2017
@mbarlow12
Copy link
Contributor

Thanks @ariel-phet & @samreid. I'll let you both know when I have questions.

@mbarlow12
Copy link
Contributor

Theory 1: the molecule's breakApart event is only emitted in BreakApartStrategy's step function, and it doesn't appear that the state calls the step function. It might be possible to instrument the emitter so that breaking apart is tracked in the state as well. It's also this event that handles updating the active molecules in the observable array.

Theory 2: Unlike photons, none of the Molecule types are instrumented for phet-io. I'm still rather fuzzy on the role of TMolecule & TObservableArray, but this might point towards the discrepancy as well.

phet-io in general is still pretty confusing to me, so a more thorough intro may be necessary. I can still try taking a crack at this, but I think it'll be really slow-going until I have a better handle on how the instrumenting works.

@mbarlow12
Copy link
Contributor

@samreid had time to discuss this issue today. It appears that the Molecules are not instrumented at all; completing that would be the first phase to address this issue. The second will be ensuring proper creation and disposal/cleanup of the instrumented types.

@samreid estimated at least an hour for the initial work.

@ariel-phet and @kathy-phet, should we make this fix a priority for another release?

@kathy-phet
Copy link

@mbarlow12 - It sounds like you are working in this sim for a11y? If its synergistic, then fixing this phet-io bug would be great. Right now I don't know of a client who is using this sim, so its not requiring a quick turnaround with special testing.

@mbarlow12
Copy link
Contributor

mbarlow12 commented Sep 21, 2017

@kathy-phet - That's correct, as of now, I was assigned to a11y. Additionally, a11y is quite separate, so I can leave it alone for now if we don't want to spend the hours on it.

If we decide to move forward with sonification for the sim, then I think it'd make sense to tackle this issue simultaneously.

Removing my assignment for now.

@zepumph
Copy link
Member

zepumph commented Oct 5, 2017

Mike Winters was delivered a phet-io version of this sim this week. He is interested in getting state data of the molecule from the data stream (see issue here). In a test was able to pass tandems through to the CO molecule and convert a few properties to Properties and instrument them. It was pretty straightforward and I think is necessary to completing instrumentation. Depending on what priorities are, I could work with @mbarlow12 on the phet-io side of this sim, or just go for it if @mbarlow12 has a11y stuff to tackle. I'm happy with anything, but I think that this state is hindering further sonification progress. Tagging @samreid so that he is aware of a bump in priority.

@zepumph zepumph self-assigned this Oct 5, 2017
@zepumph
Copy link
Member

zepumph commented Oct 5, 2017

tagging #154 in this issue as well.

@zepumph
Copy link
Member

zepumph commented Oct 5, 2017

After talking with @samreid, I am going to first pass tandems through to molecules, that will solve Mike's current issues. From there we should work on dynamic save/load.

@zepumph
Copy link
Member

zepumph commented Oct 6, 2017

I was able to instrument the Molecules enough to get the rotation, rotationClockwise, and vibrating properties onto the data stream. From here we will need to add a way to dynamically create molecules just like we are doing for photons.

@zepumph
Copy link
Member

zepumph commented Oct 6, 2017

So far I was not able to get the whole molecule state to save. I set things up well to continue though. I was clearing the molecule successfully, now I just have to find a way to add the molecule back in. One worry I have is that we need to know what the ObservableArray model.activeMolecules looks like before creating the molecules, so I think that we need to rework to setState method in phetio to allow a few loops without "changed = true" before erroring out. This is because if we try to create a molecule from a tandem, but the array has not yet been set, then we have no way of knowing what molecules to create.

I think that the ObservableArray is serializing its elements just with a phetioID, and I believe that is fine for this case, but maybe we need an option in ObservableArray to either serialize with phetioID's, or with state, and you can choose based on your situation.

Notes to self to get me back to where I am now when I have time.

Here is what the TPhotonEmissionModel looks like so far:

    clearChildInstances: function( instance ) {
        instance.clearPhotons();
        // instance.chargedParticles.clear();
        // instance.electricFieldSensors.clear();

        instance.activeMolecules.forEach( function( activeMolecule ) {
          activeMolecule.dispose();
        } );
        // Clear the old active molecules array
        instance.activeMolecules.clear();
      },

      addChildInstance: function( instance, tandem, stateObject ) {
        console.log( tandem );

        if( tandem.tail.indexOf( 'photon') === 0 ) {
          var value = TPhoton.fromStateObject( stateObject );

          var photon = new phet.moleculesAndLight.Photon( value.wavelength, tandem );
          photon.setVelocity( stateObject.vx, stateObject.vy );
          instance.photons.add( photon );
        }
        else{
          // TODO: create a molecule
        }
      }

I also stashed this all under "disposeMolecule"

In Molecule.js you just have to addInstance of TMolecule and dispose correctly to get back to where I am right now.

zepumph added a commit that referenced this issue Oct 6, 2017
@zepumph
Copy link
Member

zepumph commented Oct 9, 2017

Decreasing priority.

@zepumph zepumph removed their assignment Oct 9, 2017
jessegreenberg pushed a commit to phetsims/greenhouse-effect that referenced this issue Apr 21, 2021
jessegreenberg pushed a commit to phetsims/greenhouse-effect that referenced this issue Apr 21, 2021
jessegreenberg pushed a commit to phetsims/greenhouse-effect that referenced this issue Apr 21, 2021
jessegreenberg pushed a commit to phetsims/greenhouse-effect that referenced this issue Apr 21, 2021
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

5 participants