phetsims / vegas

Reusable game components for PhET simulations.
MIT License
1 stars 4 forks source link

CT: Do not move the RewardNode in the scene graph after calling initialize or the transformation may break. #111

Closed KatieWoe closed 2 years ago

KatieWoe commented 2 years ago
vegas : fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1657759719324/vegas/vegas_en.html?continuousTest=%7B%22test%22%3A%5B%22vegas%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1657759719324%22%2C%22timestamp%22%3A1657774506368%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.
Error: Assertion failed: Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1657759719324/assert/js/assert.js:28:13)
at assert (RewardNode.ts:236:18)
at (TransformTracker.ts:116:6)
at notifyListeners (TransformTracker.ts:127:9)
at onTransformChange (TransformTracker.ts:61:13)
at listener (TinyEmitter.ts:93:8)
at emit (Node.ts:2740:26)
at listener (TinyEmitter.ts:93:8)
at emit (Transform3.js:102:23)
at invalidate (Transform3.js:74:9)
id: Bayes Puppeteer
Snapshot from 7/13/2022, 6:48:39 PM

----------------------------------

vegas : fuzz : unbuilt : assertSlow
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1657759719324/vegas/vegas_en.html?continuousTest=%7B%22test%22%3A%5B%22vegas%22%2C%22fuzz%22%2C%22unbuilt%22%2C%22assertSlow%22%5D%2C%22snapshotName%22%3A%22snapshot-1657759719324%22%2C%22timestamp%22%3A1657799344555%7D&brand=phet&eall&fuzz&memoryLimit=1000
Query: brand=phet&eall&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.
Error: Assertion failed: Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1657759719324/assert/js/assert.js:28:13)
at assert (RewardNode.ts:236:18)
at (TransformTracker.ts:116:6)
at notifyListeners (TransformTracker.ts:127:9)
at onTransformChange (TransformTracker.ts:61:13)
at listener (TinyEmitter.ts:93:8)
at emit (Node.ts:2740:26)
at listener (TinyEmitter.ts:93:8)
at emit (Transform3.js:102:23)
at invalidate (Transform3.js:74:9)
id: Bayes Puppeteer
Snapshot from 7/13/2022, 6:48:39 PM

----------------------------------

vegas : pan-and-zoom-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1657759719324/vegas/vegas_en.html?continuousTest=%7B%22test%22%3A%5B%22vegas%22%2C%22pan-and-zoom-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1657759719324%22%2C%22timestamp%22%3A1657787855602%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=true
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=true
Uncaught Error: Assertion failed: reentry detected, value=3.722795411031844 0 13.99999999999983
0 3.722795411031844 -208.34280735830856
0 0 1, oldValue=3.722795411031844 0 13.99999999999983
0 3.722795411031844 -208.34280735830868
0 0 1
Error: Assertion failed: reentry detected, value=3.722795411031844 0 13.99999999999983
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1657759719324/assert/js/assert.js:28:13)
at assert (ReadOnlyProperty.ts:258:14)
at _notifyListeners (ReadOnlyProperty.ts:212:13)
at set (Property.ts:68:10)
at set (MultiListener.js:497:24)
at reposition (PanZoomListener.js:154:10)
at reposition (MultiListener.js:389:11)
at addPress (PanZoomListener.js:140:10)
at addPress (AnimatedPanZoomListener.js:781:10)
at addPress (MultiListener.js:483:11)
id: Bayes Puppeteer
Snapshot from 7/13/2022, 6:48:39 PM
pixelzoom commented 2 years ago

This is a problem with pan and zoom, so reassigning to @jessegreenberg. This blocks publication of all sims that use RewardNode (which is probably most games).

To reproduce locally:

  1. Run the vegas demo
  2. Go to the Components screen
  3. Select "RewardNode" from the combo box
  4. Zoom-in in the browser. On macOS + Chrome, that's View > Zoom In or ⌘+.
  5. The demo will immediately crash with this stack trace:
Assertion failed:  Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.
assert.js:28 Uncaught Error: Assertion failed: Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.
    at window.assertions.assertFunction (assert.js:28:13)
    at Array.updateBounds (RewardNode.ts:236:19)
    at TransformTracker.notifyListeners (TransformTracker.ts:116:7)
    at TransformTracker.onTransformChange (TransformTracker.ts:127:10)
    at TransformTracker.ts:61:14
    at TinyEmitter.emit (TinyEmitter.ts:93:9)
    at Node.onTransformChange (Node.ts:2762:27)
    at TinyEmitter.emit (TinyEmitter.ts:93:9)
    at Transform3.invalidate (Transform3.js? [sm]:102:24)
    at Transform3.setMatrix (Transform3.js? [sm]:74:10)

The relevant code is in RewardNode.ts:

      // Listen to the bounds of the scene, so the canvas can be resized if the window is reshaped.
      const updateBounds = () => {
        assert && assert( this.getUniqueTrail().equals( uniqueTrail ),
          'Do not move the RewardNode in the scene graph after calling initialize or the transformation may break.' );

So it looks like pan-and-zoom is changing the structure of the scenegraph, which is apparently a problem for RewardNode.

pixelzoom commented 2 years ago

The failing assertion was added by @jessegreenberg recently, on 7/7/2022 in https://github.com/phetsims/vegas/commit/efe7863e2835c3adfe5c40193e9fb22b2bc9eca2, to address https://github.com/phetsims/vegas/issues/97 (Zoom feature results in incorrect behavior of RewardNode). So apparently there are still problems with zoom and RewardNode. @jessegreenberg I'll leave it up to you where to investigate here, or reopen https://github.com/phetsims/vegas/issues/97.

jessegreenberg commented 2 years ago

I just pulled all, restarted the transpiler, and followed the steps in https://github.com/phetsims/vegas/issues/111#issuecomment-1187720023 but I do not see this issue.

But I did see it after doing this: 1) Run the vegas demo 2) Go to the Components screen 3) Select "RewardNode" from the combo box 4) Select any other component from the combo box 5) Zoom in

I am guessing something is wrong with dispose of RewardNode or the demo is actually reusing the component and moving it in the scene graph.

jessegreenberg commented 2 years ago

RewardNode is a child of oldDemoNode so the oldDemoNode.children also need to be disposed here: https://github.com/phetsims/sun/blob/e80aa83be039a1064310288d7819abfa03d631cd/js/demo/DemosScreenView.ts#L143-L146

Or maybe the RewardNode (and other components) can be disposed directly if there is a direct reference to it.

KatieWoe commented 2 years ago

Is this related, or seperate?

vegas : pan-and-zoom-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1658161781235/vegas/vegas_en.html?continuousTest=%7B%22test%22%3A%5B%22vegas%22%2C%22pan-and-zoom-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1658161781235%22%2C%22timestamp%22%3A1658167803968%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=true
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000&supportsPanAndZoom=true
Uncaught Error: Assertion failed: reentry detected, value=4 0 2.2737367544323206e-13
0 4 -5.12128234552921
0 0 1, oldValue=4 0 52.00000000000023
0 4 -168.1212823455292
0 0 1
Error: Assertion failed: reentry detected, value=4 0 2.2737367544323206e-13
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1658161781235/assert/js/assert.js:28:13)
at assert (ReadOnlyProperty.ts:258:14)
at _notifyListeners (ReadOnlyProperty.ts:212:13)
at set (Property.ts:68:10)
at set (MultiListener.js:497:24)
at reposition (PanZoomListener.js:154:10)
at reposition (MultiListener.js:425:9)
at removePress (AnimatedPanZoomListener.js:794:10)
at removePress (MultiListener.js:156:13)
at inputEvent (Input.ts:1826:91)
id: Bayes Puppeteer
Snapshot from 7/18/2022, 10:29:41 AM
jessegreenberg commented 2 years ago

Thanks, that is different. Ill create a new issue for that.

pixelzoom commented 2 years ago

I am guessing something is wrong with dispose of RewardNode or the demo is actually reusing the component and moving it in the scene graph.

Yep, that's the problem - thanks! Fixed in the above commit. I'll leave this open until CT is happy.

pixelzoom commented 2 years ago

CT is happy. Closing.