Closed samreid closed 4 years ago
@samreid and I dove into this today. We found that there were two issues with this sim (and others) regarding PhET-iO:
First is that ObservableArrayIO
didn't clearly differentiate between serializing its values as references or values. This was remedied by adding an option to the already parametric type. Now serializing OberservaleArray can be more explicit.
In recent state related changes to phetioEngine
from https://github.com/phetsims/phet-io/issues/1420 there was a bug. When setting the state on a sim with dynamic instances (such that there were instances with the clearChildInstances
method, you could call PhetioEngineIO.setStateFromObject
(which may only set one or two items), but would still clear out all child instances. To fix this, take this line of old code:
// Clear state from any containers that already exist
for ( const key in this.phetioObjectMap ) {
const wrapperType = this.getType( key );
wrapperType.clearChildInstances && wrapperType.clearChildInstances( this.getInstance( key ) );
}
Instead of iterating through all keys of the phetioObjectMap
, I converted it to:
// Clear state from any containers that already exist, only do so based on the state we are setting, not the whole
// state, build into this iteration is the assumption that if you are setting the state on an instance that has
// `clearChildInstances`, then you are also setting the state on the appropriate children that get cleared, see https://github.com/phetsims/energy-skate-park-basics/issues/452
for ( const key in state ) {
const wrapperType = this.getType( key );
wrapperType && wrapperType.clearChildInstances && wrapperType.clearChildInstances( this.getInstance( key ) );
}
which only looks at the keys from the state
argument. Note that it is hard to do a complete and thorough regression test, since the state wrapper is broken in 3+ other sims regardless of these changes. (this is what we are trying to fix currently). No matter I feel like this is safe for master for now. I'm going to add the blocks publication label to https://github.com/phetsims/phet-io/issues/1420 so that we can sort all of this out thoroughly without bugs slipping though.
I'm going to commit the changes we currently have here, and I hope that @samreid can inspect them to see if we are in dangerous waters.
Oh I should note another issue that we solved in the above commit. Now that we are gathering initialState
, we have a better way to keep track of "out of place" properties that change after construction into their "initial state". Skater.headPositionProperty
is an example of this, so we adapted it to initialize to the correct value on creation with a bit of refactoring. I hope soon to write a unit test to make sure that the "initialState" doesn't change between construction and a few seconds afterwards even with no fuzzing.
I don't see any other work to be done in this issue. Closing
Bayes CT has been showing this issue for a while:
@zepumph can you please investigate?