Node Properties and phet-io undeferring

[jonathanolson]

Scenery's synchronous-update code (PDOMDisplaysInfo, RendererSummary) makes a hard assumption that it is only handling updates for one change at a time (e.g. one visibility change, all the others are the same as before). It makes the assumption that the one change happening NOW is the only change, and that the rest of the state is consistent.

It wasn't designed to handle cases where an arbitrary number of different things are now out-of-date, and could be notified later and in an arbitrary order. The graph operations are likely to fail if multiple visibleProperties undefer at the same time.

This may be causing phetsims/greenhouse-effect#46.

Possible routes:

1. Don't undefer multiple of these at a time (seems tricky)
2. Vet/update the algorithms to handle arbitrary Node-side changes that WILL be notified (sounds trickier but potentially cleaner at the end)
3. On state-setting or cases where multiple things are undeferred, instead have a path that rebuilds this information to the "correct state" - some duplication, probably easier, but more complicated to maintain.

[zepumph]

We discussed prioritization of this in PhET-iO meeting.

@jonathanolson will work on this after Density and Molecule Shapes (which will likely go pretty quickly).

[zepumph]

Geometric optics is now blocked by this as well, phetsims/geometric-optics#318

geometric-optics : phet-io-state-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/phet-io-wrappers/state/?sim=geometric-optics&phetioDebug&fuzz&wrapperContinuousTest=%7B%22test%22%3A%5B%22geometric-optics%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1645086359601%22%2C%22timestamp%22%3A1645094346332%7D
Uncaught Error: Uncaught Error: Assertion failed: there should be at least as many PDOMDisplays as Displays
Error: Assertion failed: there should be at least as many PDOMDisplays as Displays
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/assert/js/assert.js:25:13)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:243:15)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:261:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:261:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:261:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:261:35)
at PDOMDisplaysInfo.removePDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:261:35)
at PDOMDisplaysInfo.removeAllPDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:204:10)
at PDOMDisplaysInfo.onVisibilityChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:102:14)
at LensScreenView.onVisiblePropertyChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1645086359601/chipper/dist/js/scenery/js/nodes/Node.js:3380:28)
id: Bayes Chrome
Snapshot from 2/17/2022, 1:25:59 AM

[zepumph]

Discussed in PhET-iO meeting again today. @kathy-phet mentioned that this can take precedence over advancing buoyancy for @jonathanolson.

[pixelzoom]

Note that this issue (or something related) is also likely responsible for phetsims/geometric-optics#316:

geometric-optics : phet-io-state-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/phet-io-wrappers/state/?sim=geometric-optics&phetioDebug&fuzz&wrapperContinuousTest=%7B%22test%22%3A%5B%22geometric-optics%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1644979446092%22%2C%22timestamp%22%3A1644984737723%7D
Uncaught Error: Uncaught Error: Assertion failed: Should be empty before adding everything
Error: Assertion failed: Should be empty before adding everything
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/assert/js/assert.js:25:13)
at PDOMDisplaysInfo.addAllPDOMDisplays (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:173:15)
at PDOMDisplaysInfo.onVisibilityChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/scenery/js/accessibility/pdom/PDOMDisplaysInfo.js:100:14)
at GORulerNode.onVisiblePropertyChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/scenery/js/nodes/Node.js:3380:28)
at TinyForwardingProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/axon/js/TinyEmitter.js:53:48)
at TinyForwardingProperty.notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/axon/js/TinyForwardingProperty.js:139:10)
at TinyForwardingProperty.onTargetPropertyChange (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/axon/js/TinyForwardingProperty.js:97:10)
at TinyProperty.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/axon/js/TinyEmitter.js:69:9)
at BooleanProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/axon/js/Property.js:228:23)
at PhaseCallback.listener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1644979446092/chipper/dist/js/axon/js/Property.js:276:47)
id: Bayes Chrome
Snapshot from 2/15/2022, 7:44:06 PM

[jonathanolson]

Fix and simplifications to TinyForwardingProperty made above. We're treating it a bit less in a "forwarding" way, where the value of the forwarding and target Properties can diverge (generally in deferred cases, which keeps things "synchronous" for Scenery). This can also diverge during certain phases of the listener notifications, however not in a way that seems to be happening in practice currently (and is also unlikely to happen in the future).

[jonathanolson]

Thanks @zepumph for the collaboration! Removing my assignment.

[zepumph]

I pinged the individual issues about adding their sims back to the state wrapper. I will leave this open, and close in ~1 week if nothing else comes of it. Thanks @jonathanolson for the quick turnaround.

[pixelzoom]

The phet-io-state-fuzz test is not being run very often by CT. It's only run 3 times for geometric-optics since the fix for #1312 was pushed, see screenshot below. @zepumph do you feel comfortable closing with this kind of CT coverage?

[zepumph]

Looking at all testing of the state wrapper, it looks like it get's relatively good coverage. I know there are a lot of tests done for each row, so it doesn't feel horrible to me.

[zepumph]

Now I see that GO and basics has been tested at least 20 times. Given how easily it was to reproduce, I'm feeling good about this change. Closing. Thanks again @jonathanolson.