phetsims / vector-addition

“Vector Addition” is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 5 forks source link

CT: this.isOnGraphProperty must be true #273

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

This probably started happening because CT multi-touch fuzzing ( ?fuzzPointers=2) was enabled for phetsims/aqua#106. Enabling multi-touch fuzzing first caused #272 (which I fixed), and now it's probably hitting this error.

Looks like this started occurring at 2/8/21 @ 10:32am, right around the time that the fix for #272 was pushed. And it is occurring frequently, on most (but not all) CT cycles.

vector-addition : multitouch-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/vector-addition/vector-addition_en.html?continuousTest=%7B%22test%22%3A%5B%22vector-addition%22%2C%22multitouch-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1612841514933%22%2C%22timestamp%22%3A1612842869123%7D&brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000
Query: brand=phet&ea&fuzz&fuzzPointers=2&memoryLimit=1000
Uncaught Error: Assertion failed: this.isOnGraphProperty must be true
Error: Assertion failed: this.isOnGraphProperty must be true
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/assert/js/assert.js:25:13)
at Vector.setTipWithInvariants (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/vector-addition/js/common/model/Vector.js:187:15)
at Vector.moveTipToPosition (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/vector-addition/js/common/model/Vector.js:310:10)
at VectorNode.updateTipPosition (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/vector-addition/js/common/view/VectorNode.js:366:17)
at tipListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/vector-addition/js/common/view/VectorNode.js:228:14)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/axon/js/TinyEmitter.js:71:18)
at Vector2Property._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/axon/js/Property.js:289:25)
at Vector2Property.set (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/axon/js/Property.js:187:14)
at Vector2Property.set value [as value] (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/axon/js/Property.js:359:32)
at DragListener.reposition (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1612841514933/scenery/js/listeners/DragListener.js:630:36)
id: Bayes Chrome
Snapshot from 2/8/2021, 8:31:54 PM
pixelzoom commented 3 years ago

To reproduce manually:

  1. Start the sim on a touch device
  2. Go to Explore 2D screen
  3. With one finger, drag vector 'a' out of the toolbox, but don't put it on the graph.
  4. With another finger, grab the tip of vector 'a' and try to move it. This will trigger the exception:

Error: Assertion failed: this.isOnGraphProperty must be true

pixelzoom commented 3 years ago

Two changes...

For c734080, Vector setTipWithInvariants and setTailWithInvariants both had an assertion like this:

assert && assert( this.isOnGraphProperty.value === true, 'this.isOnGraphProperty must be true' );

I've removed those assertions (and related comments). There's nothing fatal that occurs if you try to do these things while the Vector is off the graph. In the case of the tip, it's a bit odd because the tip snaps to the edge of the graph. But this use case is so unlikely that I think it's fine.

For 5719f72, interactions with VectorNode needed to be cancelled when animating back to the toolbox. Otherwise you could still grab some part of the vector and trigger an assertion failure.

Tested with local unbuilt version using ?brand=phet&ea&debugger&fuzzPointers=2&fuzz&supportsPanAndZoom=false.

I'll leave this open for a few CT cycles to verify that it addresses the issue, and doesn't cause and new problems.

pixelzoom commented 3 years ago

One more change... In 8d21585 and d617010, I consolidated the code that interrupts and disables interaction when a VectorNode is animating back to the toolbox. There were previously separate listeners related to head and tail, handling this in different (and questionable) ways.

pixelzoom commented 3 years ago

CT is clear for many cycles. Closing.