phetsims / energy-forms-and-changes

"Energy Forms And Changes" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
5 stars 5 forks source link

PhET-iO State: support UserMovableModelElement.supportingSurface #424

Open zepumph opened 1 year ago

zepumph commented 1 year ago

From https://github.com/phetsims/energy-forms-and-changes/issues/423, HorizontalSurface likely needs to support serialization in some form. We were able to bypass a CT error over there with deep comprison of Vector2s, but we will want supportingSurface to maintain the right pointer through PhET-iO state, even if a lack of this doesn't currently cause behavioral or assertion problems. I am not planning to work on this right now. Marking deferred until next iO release of this sim.

jbphet commented 1 year ago

There is a new CT error that is now popping up occasionally. The trace is below. It seems like a distinct possibility that the cause of this is that the supporting surface isn't being cleared, so things can essentially get recursively stacked on top of one another.

energy-forms-and-changes : phet-io-state-fuzz : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1694583921792/phet-io-wrappers/state/?sim=energy-forms-and-changes&phetioDebug=true&phetioWrapperDebug=true&fuzz&wrapperContinuousTest=%7B%22test%22%3A%5B%22energy-forms-and-changes%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1694583921792%22%2C%22timestamp%22%3A1694591327260%7D
Uncaught RangeError: Maximum call stack size exceeded
RangeError: Maximum call stack size exceeded
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
at (UserMovableModelElement.js:1:0)
[URL] http://128.138.93.172/continuous-testing/aqua/html/wrapper-test.html?url=..%2F..%2Fct-snapshots%2F1694583921792%2Fphet-io-wrappers%2Fstate%2F%3Fsim%3Denergy-forms-and-changes%26phetioDebug%3Dtrue%26phetioWrapperDebug%3Dtrue%26fuzz&testInfo=%7B%22test%22%3A%5B%22energy-forms-and-changes%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1694583921792%22%2C%22timestamp%22%3A1694591327260%7D
zepumph commented 1 year ago

@jbphet and I worked on this today and got to a commit point for instrumenting HorizontalSurface and supportingSurface, but it didn't solve all the state issues, so we removed the sim from PhET-iO state testing.

When we get to this next time, we will want to instrument RectangularThermalMovableModelElement, which holds some bounds entities that update as movables translation around. This is likely some of the cause of the state issues.

Also noting though that all actual assertions we hit were because the supportSurface was out of sync, so an audit on the above commit will be worthwhile too.

At one point this was helpful debug code:

```js // Debug code. To use, set phet.preloads.phetio.jb to either 'brick' or 'iron' from the console. if ( phet.preloads.phetio.queryParameters.frameTitle === 'destination' ) { if ( window.parent.phet.jb === undefined ) { window.parent.phet.jb = ''; } if ( ( window.parent.phet.jb === 'brick' && this.blockType.toString() === 'BRICK' ) || ( window.parent.phet.jb === 'iron' && this.blockType.toString() === 'IRON' ) ) { console.log( '-----------------------' ); console.log( `this.id = ${this.id}` ); console.log( `this.blockType = ${this.blockType}` ); const elementOnTopSurface = this.topSurface.elementOnSurfaceProperty.value; if ( elementOnTopSurface ) { console.log( `elementOnTopSurface.id = ${elementOnTopSurface.id}` ); console.log( `elementOnTopSurface.blockType = ${elementOnTopSurface.blockType}` ); } else { console.log( 'Nothing on top surface.' ); } window.parent.phet.jb = ''; } }
jbphet commented 1 year ago

This is being marked as deferred and we will give it more attention when we decide to upgrade this sim to the Hydrogen level of phet-io.