phetsims / charges-and-fields

"Charges And Fields" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
8 stars 7 forks source link

Impossible setState because of Nodes #142

Closed zepumph closed 6 years ago

zepumph commented 6 years ago

@samreid commented in https://github.com/phetsims/charges-and-fields/issues/141#issuecomment-338531366:

If I run Charges and Fields in the state wrapper and drag out a red particle, in the next or 2 frames later I get:

SimIFrameAPI.js?bust=1508725383889:126 SimIFrameAPI Invocation Error: Error: Assertion failed: Impossible set state:
chargesAndFields.chargesAndFieldsScreen.view.chargedParticleNode_0
    at window.assertions.assertFunction (assert.js:22)
    at iterate (phetio.js?bust=1508725383889:238)
    at Object.setState (phetio.js?bust=1508725383889:255)
    at TPhETIO.implementation [as setState] (TPhETIO.js?bust=1508725383889:177)
    at Object.invoke (SimIFrameAPI.js?bust=1508725383889:262)
    at Object.handleSingleInvoke (SimIFrameAPI.js?bust=1508725383889:123)
    at Object.handleRequest (SimIFrameAPI.js?bust=1508725383889:165)
    at SimIFrameAPI.js?bust=1508725383889:282 Error: Assertion failed: Impossible set state:
chargesAndFields.chargesAndFieldsScreen.view.chargedParticleNode_0
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:22:13)
    at iterate (http://localhost/phet-io/js/phetio.js?bust=1508725383889:238:17)
    at Object.setState (http://localhost/phet-io/js/phetio.js?bust=1508725383889:255:14)
    at TPhETIO.implementation [as setState] (http://localhost/phet-io/js/types/TPhETIO.js?bust=1508725383889:177:16)
    at Object.invoke (http://localhost/phet-io/js/SimIFrameAPI.js?bust=1508725383889:262:29)
    at Object.handleSingleInvoke (http://localhost/phet-io/js/SimIFrameAPI.js?bust=1508725383889:123:28)
    at Object.handleRequest (http://localhost/phet-io/js/SimIFrameAPI.js?bust=1508725383889:165:35)
    at http://localhost/phet-io/js/SimIFrameAPI.js?bust=1508725383889:282:14

I poked around and realized the problem is that we are now instrumenting the Node's of the ModelElements as well as the model side. This isn't how the save and load was set up for CAF, so it is breaking it. We only added toStateObject on TNode recently, and I think that it is best to remove it from the state by default and opt into including certain Node instances if deemed necessary. I applied this strategy for CAF and it worked. @samreid please review.

zepumph commented 6 years ago

By the looks of it TNode only got a toStateObject a month ago, so it wouldn't be surprising to me to hear CAF has been broken for that long.

samreid commented 6 years ago

Is TNode.toStateObject destined to be the foundation for customizing visibility/pickability of nodes? If so, we may need to leave phetioState on by default.

zepumph commented 6 years ago

That's true, should we instead investigate why the Node is not able to find itself in the downstream sim, similar to https://github.com/phetsims/charges-and-fields/issues/141 and the ESPB work we did this morning?

samreid commented 6 years ago

Yes, we should look for an explanation like that.

zepumph commented 6 years ago

Sounds good.

samreid commented 6 years ago

Proposed fix is above, @zepumph please review.

zepumph commented 6 years ago

That looks good to me. I like that we are linking the tandems from the view and the model. We will need to do the same thing for the sensors.

samreid commented 6 years ago

I applied the same strategy for the sensors, @zepumph please review.

zepumph commented 6 years ago

Looks good, I just found this problem again in https://github.com/phetsims/energy-skate-park-basics/issues/395, but looks like it could be as simple of a fix. Closing this one though!