phetsims / capacitor-lab-basics

"Capacitor Lab: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

PhET-iO should support a Vector3 or Vector2 IOType #215

Closed zepumph closed 6 years ago

zepumph commented 6 years ago

This assertion is coming from https://github.com/phetsims/beers-law-lab/issues/215, where many assertions were added to phet-io stateObject methods. Here it looks like the MovableDragHandlers in CLB are passing in Vector3 positions, but the MovableDragHandler is expecting it to be Vector2.

@samreid I'm worried about a proliferation of these multi-type types if we made Vector2or3IO. Whatchya think?

Uncaught Error: Assertion failed: Incorrect type in phet-io wrapper. Expected Vector3, received Vector2
Error: Assertion failed: Incorrect type in phet-io wrapper. Expected Vector3, received Vector2
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at assertInstanceOf (http://localhost/phet-io/js/assertInstanceOf.js?bust=1510950042110:58:17)
    at Function.toStateObject (http://localhost/dot/js/Vector3IO.js?bust=1510950042110:47:17)
    at Property._notifyListeners (http://localhost/axon/js/Property.js?bust=1510950042110:182:49)
    at Property._setAndNotifyListeners (http://localhost/axon/js/Property.js?bust=1510950042110:174:14)
    at Property.set (http://localhost/axon/js/Property.js?bust=1510950042110:121:16)
    at MovableDragHandler.movableDragHandlerDrag (http://localhost/scenery-phet/js/input/MovableDragHandler.js?bust=1510950042110:81:24)
    at Object.move (http://localhost/scenery/js/input/SimpleDragHandler.js?bust=1510950042110:143:29)
    at Input.dispatchToListeners (http://localhost/scenery/js/input/Input.js?bust=1510950042110:800:72)
    at Input.dispatchEvent (http://localhost/scenery/js/input/Input.js?bust=1510950042110:770:12)
samreid commented 6 years ago

The appropriate solution to this problem is to use Vector3 for this case (and sometimes ignore the z attribute).

samreid commented 6 years ago

@jonathanolson you are listed as the responsible dev, can you please work on this?

jonathanolson commented 6 years ago

Looking into it. Seems like MovableDragHandler should ideally only be handling Vector2s, and CLB should be working around that.

zepumph commented 6 years ago

This is consistently failing for phet-io capacitor lab on bayes (not surprisingly), I just wanted to let people know.

samreid commented 6 years ago

The easiest way to reproduce this is screens=2 then drag the voltmeter across the bottom left wire.

samreid commented 6 years ago

I added a workaround in the preceding commit, the JSDoc says:

/**
 * Capacitor Lab Basics uses Vector3 for the model, but MovableDragHandler only supports Vector2 and PhET-iO
 * types Vector2IO and CLBVector3IO cannot be mixed.  This file is a workaround for https://github.com/phetsims/capacitor-lab-basics/issues/215
 * and supports Vector2 and Vector3
 *
 * @author Sam Reid (PhET Interactive Simulations)
 * @author Andrew Adare (PhET Interactive Simulations)
 */

This is clearly noted as a workaround, this is intended to get fuzz testing passing again. This runs the risk of giving us a false sense of hope, but will prevent any other errors from getting masked by this one.

So I recommend to (a) run this workaround past @zepumph to make sure I've got it right then (b) leave this issue open for investigation by @jonathanolson for a better long term solution.

zepumph commented 6 years ago

Looks like a good workaround to me.

pixelzoom commented 6 years ago

In https://github.com/phetsims/capacitor-lab-basics/issues/215#issuecomment-345374585, @samreid said:

The appropriate solution to this problem is to use Vector3 for this case (and sometimes ignore the z attribute).

Absolutely not. The documentation for MovableDragHandler very clearly indicates that the locationProperty is of type {Property.<Vector2>}. CL:B's use of Vector3 is therefore incorrect and inappropriate.

pixelzoom commented 6 years ago

The workaround is incomplete - it only addresses CLBVector3IO, not the improper use of MovableDragHandler. If I add this assertion to MovableDragHandler's constructor, CL:B fails:

assert && assert( locationProperty.value instanceof Vector2 );

Please address the root problem.

jonathanolson commented 6 years ago

Working on addressing the root problem now.

jonathanolson commented 6 years ago

Should be fixed in master.

Somewhat comically, it wasn't even passing in a ModelViewTransform2 as modelViewTransform! Maybe we should add type checks there?

@zepumph, can you verify?

zepumph commented 6 years ago

Looks good, thanks. Assigning back to you to decide if you want to add type checks for the drag handler.

samreid commented 6 years ago

+1 for type checks

samreid commented 6 years ago

Type checking every single option in every file seems like a lot of work and we would want a way to standardize and simplify and avoid repeating types. Let's rely on automated and manual testing to catch this error for now, closing.