search

phetsims / hookes-law

"Hooke's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

CT error: Assertion failed: Cannot register two different instances to the same id #62

Closed pixelzoom closed 6 years ago

pixelzoom commented 6 years ago 
hookes-law : phet-io-fuzz : require.js : run
Uncaught Error: Assertion failed: Cannot register two different instances to the same id: hookesLaw.introScreen.view.system1CenterYProperty
Error: Assertion failed: Cannot register two different instances to the same id: hookesLaw.introScreen.view.system1CenterYProperty
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/assert/js/assert.js:22:13)
    at Phetio.addInstance (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/phet-io/js/phetio.js?bust=1535050001644:634:19)
    at Object.addInstance (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/phet-io/js/phetio.js?bust=1535050001644:727:14)
    at Tandem.addInstance (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/tandem/js/Tandem.js?bust=1535050001644:134:36)
    at NumberProperty.initializePhetioObject (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/tandem/js/PhetioObject.js?bust=1535050001644:151:19)
    at NumberProperty.PhetioObject [as constructor] (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/tandem/js/PhetioObject.js?bust=1535050001644:80:12)
    at NumberProperty.Property [as constructor] (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/axon/js/Property.js?bust=1535050001644:84:18)
    at new NumberProperty (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/axon/js/NumberProperty.js?bust=1535050001644:61:14)
    at new IntroAnimation (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/hookes-law/js/intro/view/IntroAnimation.js?bust=1535050001644:46:34)
    at Array.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1535048978248/hookes-law/js/intro/view/IntroScreenView.js?bust=1535050001644:103:24)
Approximately 8/23/2018, 12:29:38 PM

This popped up after I converted from TWEEN to TWIXT in https://github.com/phetsims/hookes-law/issues/46. There are now 2 Properties inside IntroAnimation that are created dynamically, each time the number of systems is changed. Those Properties are system1CenterYProperty and system2OpacityProperty. They are encapsulated inside IntroAnimation by design, and they are instrumented to support record/playback.

@samreid @zepumph How do I handle creating the tandem for a dynamically created Property?

samreid commented 6 years ago

Often the solution to that kind of problem is to make sure the tandems are unregistered by calling dispose on them. When I tried that for Hooke's Law, it solved the problem for the "Events: colorized" wrapper. However, with that solution, the "State" wrapper doesn't work properly, because the downstream simulation doesn't get the message about when it should be creating or disposing those new Propertes.

There are now 2 Properties inside IntroAnimation that are created dynamically, each time the number of systems is changed. Those Properties are system1CenterYProperty and system2OpacityProperty. They are encapsulated inside IntroAnimation by design

Even though it contradicts your "encapsulated by design" remark, I think the simplest solution will be to create system1CenterYProperty and system2OpacityProperty only once -- in IntroScreenView.js -- and pass them as arguments to IntroAnimation. Then they don't need to be disposed or recreated, and the "State" wrapper will work properly.

pixelzoom commented 6 years ago

Even though it contradicts your "encapsulated by design" remark, I think the simplest solution will be to create system1CenterYProperty and system2OpacityProperty only once ....

That's an expedient workaround. But if there's not a way to do this that works, that seems like a problem.

pixelzoom commented 6 years ago

I've worked around this as suggested in https://github.com/phetsims/hookes-law/issues/62#issuecomment-415596068. I'm not really happy with the result -- IntroAnimation shouldn't need to know about anything other than the Nodes to animate and the layoutBounds. I'm going to consider rewriting IntroAnimation so that a single instance (and its Animations) can be reused/re-run.

pixelzoom commented 6 years ago

I rewrote things so that one persistent instance of IntroAnimator handles all animation of transitions between 1 and 2 systems. And that instance is responsible for creating the problematic Properties.

@samreid A question... I did not assign a tandem id to the instance of IntroAnimator, so the 2 Properties that it creates currently have ids:

hookesLaw.introScreen.view.system1CenterYProperty hookesLaw.introScreen.view.system2OpacityProperty

I did this mainly so that I wouldn't break the API, because system1CenterYProperty existed prior to the work that I did in #46. Is this fine, or should the instance of IntroAnimation have a tandem id? I.e., should I do this in IntroScreenView?:

98 this.animator = new IntroAnimator( ..., tandem.createTandem( 'animator' ) );

... which would result in Property ids:

hookesLaw.introScreen.view.animator.system1CenterYProperty hookesLaw.introScreen.view.animator.system2OpacityProperty

(EDIT: I fixed the ids in the above.)

samreid commented 6 years ago

From a PhET-iO perspective, the animator seems like an implementation detail, not a feature that should appear in the PhET-iO API. Therefore it seems best to use:

hookesLaw.introScreen.view.system1CenterYProperty
hookesLaw.introScreen.view.system2OpacityProperty