phetsims / forces-and-motion-basics

"Forces and Motion: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/forces-and-motion-basics
GNU General Public License v3.0
7 stars 10 forks source link

Uncaught Error: Assertion failed: invalid value: -404.1852750000006 #251

Closed zepumph closed 6 years ago

zepumph commented 6 years ago

Steps to reproduce:

Property.js?bust=1511308425011:119 Uncaught Error: Assertion failed: invalid value: -404.1852750000006
    at window.assertions.assertFunction (/assert/js/assert.js:22)
    at NumberProperty.set (Property.js?bust=1511308425011:119)
    at NetForceModel.updateCartAndPullers (NetForceModel.js?bust=1511308425011:469)
    at NetForceModel.step (NetForceModel.js?bust=1511308425011:438)
    at Sim.stepSimulation (Sim.js?bust=1511308425011:812)
    at Sim.stepOneFrame (Sim.js?bust=1511308425011:759)
    at Sim.runAnimationLoop (Sim.js?bust=1511308425011:742)
zepumph commented 6 years ago

@jessegreenberg is this in the rc going out right now? Or is this from axon shas later than that?

jessegreenberg commented 6 years ago

Thanks @zepumph, I reproduced with master but it is not showing up in the release branches.

jessegreenberg commented 6 years ago

I took a look at recent changes to forces-and-motion-basics and axon and didn't see anything obvious that would have introduced this.

We are hitting this after trying to set Cart.xProperty, which has valid range

range: new Range( -403, 403 )

in NetForceModel, it looks like the xProperty is set before the check for 'end-of-game' is made, then if the new value is beyond the game limits we stop the game and reset the cart. Maybe we could change this so that the xProperty value is only set if it is within limits.

That doesn't explain why it is only appearing now though.

zepumph commented 6 years ago

Ahh, it makes sense to me! It is a NumberProperty, we just added validation to ranges in NumberProperty.

jessegreenberg commented 6 years ago

Hmm, even though the assertion we are hitting is in Property.js? I thought that check was older.

jessegreenberg commented 6 years ago

I see, it looks like isValidValue is passed in through NumberProperty, just an option in Property.

Ill proceed with looking into

Maybe we could change this so that the xProperty value is only set if it is within limits.

Will involve flipping logic in MotionModel.step.

zepumph commented 6 years ago

Yes but in NumberProperty we are updating the isValidValue option to be passed into Property:

    if ( options.range ) {

      // Add a validation function that includes the range check.
      options.isValidValue = function( value ) {
        return isValidForValueType( value, options.valueType ) && ( value >= options.range.min ) && ( value <= options.range.max );
      };
    }

This is new.

jessegreenberg commented 6 years ago

I see, thanks!

jessegreenberg commented 6 years ago

Should be fixed in master with the above commit.