-
Notifications
You must be signed in to change notification settings - Fork 5
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
Comments
I don't think I have access to any of the phet-io repos. Is it possible to grant that access for me? |
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. |
@ariel-phet can you please check on permissions for @mbarlow12? |
@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. |
Thanks @ariel-phet. @mbarlow12 let me know if I can provide any context/assistance as you are learning about PhET-iO. |
Thanks @ariel-phet & @samreid. I'll let you both know when I have questions. |
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. |
@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? |
@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. |
@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. |
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. |
tagging #154 in this issue as well. |
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. |
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. |
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 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. |
Decreasing priority. |
From https://github.com/phetsims/phet-io/issues/367 molecule splitting is not captured in the state:
The text was updated successfully, but these errors were encountered: