search

phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

Studio "Preview Sim" fails with "Cannot read property 'restTime' of undefined" #280

Closed samreid closed 3 years ago

samreid commented 3 years ago

From https://github.com/phetsims/energy-forms-and-changes/issues/410#issuecomment-852406524, I noticed that Studio "Preview Sim" is failing for Natural Selection.

?sim=natural-selection&phetioDebug&phetioElementsDisplay=all:121 Uncaught TypeError: Cannot read property 'restTime' of undefined
    at Bunny.applyState (Bunny.js:432)
    at Object.options.applyState (IOType.js:438)
    at IOType.applyState (IOType.js:186)
    at PhetioStateEngine.setStateForPhetioObject (PhetioStateEngine.js:428)
    at PhetioStateEngine.js:328
    at Array.forEach (<anonymous>)
    at PhetioStateEngine.iterate (PhetioStateEngine.js:313)
    at PhetioStateEngine.setState (PhetioStateEngine.js:247)
    at PhetioStateEngine.setFullState (PhetioStateEngine.js:266)
    at PhetioEngine.implementation (phetioEngine.js:1060)
pixelzoom commented 3 years ago

restTime is the first field that is read from Bunny's privateState. In Bunny.js line 431:

    // private fields
    this.restTime = required( NumberIO.fromStateObject( stateObject.private.restTime ) );
    this.hopTime = required( NumberIO.fromStateObject( stateObject.private.hopTime ) );
    this.cumulativeRestTime = required( NumberIO.fromStateObject( stateObject.private.cumulativeRestTime ) );
    this.cumulativeHopTime = required( NumberIO.fromStateObject( stateObject.private.cumulativeHopTime ) );
    this.hopDelta = required( Vector3.Vector3IO.fromStateObject( stateObject.private.hopDelta ) );
    this.hopStartPosition = required( Vector3.Vector3IO.fromStateObject( stateObject.private.hopStartPosition ) );

In Slack dev-public this morning, @samreid said:

After https://github.com/phetsims/phet-io/issues/1785 private state data and schema is now nested under “_private” instead of “private”.

Related?

samreid commented 3 years ago

Yes, I have a proposed fix and am testing currently.

samreid commented 3 years ago

Fixed and tested in Natural Selection, closing.

pixelzoom commented 3 years ago

Reopening.

First, I'm also not convinced that this is a good change. Typically, fields with underscores are private, and should be used ONLY inside the implementation of an object. That's not what's going on here. This field is the collection of private data that must be packed/unpacked by the IO Type implementation, so its use is not limited to the internal implementation. Therefore I think this is an improper use of the underscore prefix.

Second, if you do keep this change, https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md needs to be updated:

by convention, they should be nested under the private key in the state object.

pixelzoom commented 3 years ago

Inspecting the sites where _.private is now used (exposed) in the commits above, I like this rename even less. If we don't like private, how about privateState?

samreid commented 3 years ago

@pixelzoom and I reviewed some cases, and determined that: