Studio Molecules do not have same start positions when launched/reset

[KatieWoe]

For https://github.com/phetsims/QA/issues/501. Win 10 Firefox. I noticed that, when comparing molecules in the state or mirror wrappers, the molecules are in the same position each time. However, when launching from studio with a certain molecule positions, the launched position and the position in the sim upon reset do not match. To see this, put the molecules in an easy to recognize position in the second screen and pause the sim (it must be a non-neon gas). When you launch the sim, look at the position of the atoms/molecules. Hit the reset button to see the change.

[KatieWoe]

Same on first screen.

[jbphet]

There is a specific type for the state of the particle information called MultipleParticleModelIO which @zepumph and I worked on together. This problem has something to do with that class and how it is used in the launch-interact-reset scenario. @zepumph is pretty knowledgeable in this, so I'm adding him as an assignee.

[zepumph]

My first thought was to look at MoleculeForceAndMotionDataSetIO and MultipleParticleModelIO to make sure that they were saving all of the data from the core type that would effect the position/rotation of the particles. Nothing looked suspect at first glance (though I don't know the sim at all).

Next I wonder if there is any unseeded randomness that is causing this to be problematic. It was unlikely since mirror-inputs looks correct. There the downstream sim is actually getting seeded, but that isn't happening with State.

From this I looked through usages of phet.joist.random, and I'm becoming a skeptic that this is a problem. For example looking at MultipleParticleModel.injectMoleculeInternal, there are random values that are not stored in the state there. Should they be? I'm not sure why the positions of the particles need to be stored exactly, but if they do, then likely we need to save the random elements of the particle model too, to be able to load those back on the downstream sim.

@samreid can you speak to the importance of having state restore the perfect positions of these elements?

@jbphet can you?

If it were me, I think I would leave it alone.

[jbphet]

@zepumph said:

@jbphet can you [speak to the importance of having state restore the perfect positions of these elements]?

It doesn't strike me as super important, but we did go to a lot of effort to support accurate particle positioning, and it works in the basic launch case, so it is odd that it doesn't work in this case. It seems to me that we should endeavor to fully understand why it doesn't work so that we can be sure that there isn't some deeper issue with the way PhET-iO state is restored.

I guess I'll leave the issue assigned to me and will dig into and figure out what's going on.

[samreid]

Is it also failing to save/restore their velocity and angular momentum?

[KatieWoe]

@samreid it seems it is. I heated up water molecules so that their temperature was in the thousand Kelvin range, and it launched at the default 400 range. That seems like a much larger deal.

[zepumph]

https://www.facebook.com/ThePocketLab/videos/1092633144442595/

Agreed! @jbphet, would you like to pair on this either today or tomorrow to investigate what isn't getting into the state?

[jbphet]

@arouinfar and I reviewed this on 5/29/2020 and we decided that it should be fixed for the upcoming early customer dev release.

[zepumph]

Sorry it fell off my radar.

[jbphet]

I did some investigation of this issue, and the problematic behavior is due to the order in which state is being restored. The velocities, positions, etc, of the particles are being set prior to the setting of the substance type (e.g. argon, oxygen, neon), and there is an elaborate handler for the setting of the substance that initializes the particles information, thus overwriting the previously restored state.

I met with @zepumph and we discussed this, and it sounds like the phet-io state engine at this point in time wouldn't be able to support the sort of dependency ordering that we need here. It does have some support for order dependence, but not quite what we need in this case. @zepumph offered to add some additional capability, but I'd like to try making the state restoration process very explicitly ordered using MultiParticleModelIO first, so that's the plan for now. I'll implement it, get it working, and then have @zepumph review.

[jbphet]

I've made significant progress on this with help from @zepumph by taking more explicit control of the serialization for MultipleParticleModel. As of the previous commit, the original problem described in this issue, where the particles for non-neon substances aren't correctly positioned on a launch from studio, now appears to be resolved. However, we're not quite out of the woods yet. In my testing, I noticed that for oxygen and water the particles evolve quite differently after a launch-unpause-reset-unpause test, and it is very noticeable for oxygen. Through setting breakpoints and logging output to the console, I could see that the rotational state information is not being restored to the same values on reset. The next time I work on this I'll see if I can figure out why.

[jbphet]

Okay, I think I've tracked down the problem with reset. MoleculeForceAndMotionDataSetIO was using references to the arrays of scalar values in the data set rather than copying the values, so when those values got updated as the simulation ran, the saved state was also getting changed. I've modified MoleculeForceAndMotionDataSetIO to copy the data instead, and it also no longer allocates any new arrays, which should improve performance.

@zepumph - please review my changes and the resultant behavior and, if all looks good, pass this on to @KatieWoe to test on master.

@KatieWoe - Assuming this looks good on master, please leave it open for verification on the upcoming customer dev release version. It was a great find, and thus a good test to repeat.

[zepumph]

Really nice stuff here. Good work!

• Instead of manually looping and copying, perhaps you look into using Float64ArrayIO. For example see WOASModelIO. Would those array.set called copy the array as you desire?
• For numbers, please use NumberIO to serialize and deserialize these values. Just like our conversation about BooleanIO and its redundant nature, I still recommend this for consistency. If you have more thoughts about that, perhaps we should discuss during dev/phet-io meeting sometime.
• please add @override to to/fromStateObject methods in IO types.
• I want to just make sure that I understand the direction we ended up going. (basically just following up from the last time we pairing on this to make sure we both understand fully). I don't see an explicit handleSubstanceChanged call in setValue because that is a listener to substanceProperty, and so we are just making sure that the substance changes first, and then the model sets everything else. I also want to confirm that every single field on the model is not in the state, and that this is ok because handleSubstanceChanged will update the unstateful fields. I am saying all of this in part because I am not reviewing this by going through each field and making sure that it is handled appropriately. If you would like me to do that more thorough review please let me know.

I tested in the state wrapper and with Studio launch and see that state and state on reset are looking really nice. Good work @jbphet.

[jbphet]

I've addressed the first three comments above, and @samreid and @zepumph and I met to discuss and refine parts of it.

The fourth comments was:

I want to just make sure that I understand the direction we ended up going...

Your understanding is correct - much of the state gets reset when the substance is changed, so MultipleParticleModel.setValue sets the substance property first, then sets any other state that may have been overwritten. There are other Property instances that are set through the built-in phet-io system.

Back to you @zepumph for review.

[zepumph]

This is looking really good! Thanks for taking care of things. I didn't test or run anything, I just looked through https://github.com/phetsims/states-of-matter/commit/10e18d62c13de246ff4c701c0371487f4a72a6ec and everything is really quite nice.

[jbphet]

Okay, over to @KatieWoe and team for testing then. If this is fixed, please leave it open and unassigned and we can verify on next release. If not, please assign back to me.

[KatieWoe]

Looks fixed on master

[KatieWoe]

This does look like it might be fixed, but https://github.com/phetsims/states-of-matter/issues/311#issuecomment-670605633 makes it a bit hard to tell for sure. Leaving open until a fix for that is made.

[jbphet]

311 is fixed (I think), I'll leave this open and will put it on the list to verify during the next RC test.

[liammulh]

While testing https://github.com/phetsims/QA/issues/525, I noticed that in the mirror inputs wrapper, the top sim's particles don't match the bottom sim's particles.

@jbphet, do you think this warrants a separate issue? If so, please assign me and I'll make one.

[jbphet]

@muedli - two things: First, yes, what you're describing should be logged as a separate issue. The mirror wrapper works by replaying input events, and doesn't convey other state information between the two instances of the sim, so the mechanisms are fundamentally different from what we've been working to address this this issue. Second, I just tried the mirror wrapper for a few minutes and I didn't see discrepancies in particle positions, so you'll need to be clear in the description about the conditions under which you're seeing divergence.

[KatieWoe]

looks ok in rc.2. Will reopen if something comes up.