PhET-iO is not aware of Property options that determine valid values

[pixelzoom]

Moved here from https://github.com/phetsims/phet-io/issues/1210.

In https://github.com/phetsims/beers-law-lab/issues/206, wavelengthProperty was defaulting to Range(0,1), and needed to be changed to Range(VisibleColor.MIN_WAVELENGTH, VisibleColor.MAX_WAVELENGTH).

So I tried changing this:

    this.wavelengthProperty = new Property( solutionProperty.get().molarAbsorptivityData.lambdaMax /*nm*/, {
      tandem: tandem.createTandem( 'wavelengthProperty' ),
      phetioValueType: TNumber( { units: 'nanometers' } )
    } );

to this:

    this.wavelengthProperty = new NumberProperty( solutionProperty.get().molarAbsorptivityData.lambdaMax /*nm*/, {
      range: new Range( VisibleColor.MIN_WAVELENGTH, VisibleColor.MAX_WAVELENGTH ),
      tandem: tandem.createTandem( 'wavelengthProperty' ),
      phetioValueType: TNumber( {
        units: 'nanometers'
      } )
    } );

So apparently PhET-iO is ignoring the range associated to NumberProperty, and requires you to duplicate the range like this:

    this.wavelengthProperty = new NumberProperty( solutionProperty.get().molarAbsorptivityData.lambdaMax /*nm*/, {
      range: new Range( VisibleColor.MIN_WAVELENGTH, VisibleColor.MAX_WAVELENGTH ),
      tandem: tandem.createTandem( 'wavelengthProperty' ),
      phetioValueType: TNumber( {
        units: 'nanometers',
        range: new Range( VisibleColor.MIN_WAVELENGTH, VisibleColor.MAX_WAVELENGTH )
      } )
    } );

PhET-iO needs to be aware of NumberProperty's range. And it needs to be aware of Property's validValue and isValidValue options.

[pixelzoom]

In light of #136, it's worth repeating that this is not just about NumberProperty range. This has to do with all validation-related options related to Property and its subtypes.

[samreid]

@zepumph and I discussed getting rid of TNumber options and trying to make parametric types like TProperty concrete by putting their metadata as instance properties. It may be possible to do so here: track ranges, units and metadata on Property and eliminate it from TNumber.

[samreid]

I'm seeing 180+ places with code like this:

    // @public
    this.soluteAmountProperty = new Property( soluteAmount, {
      tandem: tandem.createTandem( 'soluteAmountProperty' ),
      phetioValueType: TNumber( {
        units: 'moles',
        range: MConstants.SOLUTE_AMOUNT_RANGE
      } )
    } );

So we would want to think carefully about whether everything will work properly if we make this large refactor.

[samreid]

• DerivedProperty will need a way to support units.
• NumberProperty doesn't support null, see Ammeter.js
• It's risky to make changes to CCK while in dev testing.
• There are a few Emitters that indicate arg units.
• Some NumberProperties will want to take validValues, like this one:

    this.photonWavelengthProperty = new Property( WavelengthConstants.IR_WAVELENGTH, {
      tandem: tandem.createTandem( 'photonWavelengthProperty' ),
      units: 'meters',
      validValues: [
        WavelengthConstants.MICRO_WAVELENGTH,
        WavelengthConstants.IR_WAVELENGTH,
        WavelengthConstants.VISIBLE_WAVELENGTH,
        WavelengthConstants.UV_WAVELENGTH
      ],
      phetioValueType: TNumber
    } );

• change type:Integer to valueType:Integer

• range and units are needed in getAPI but only available on the instance

[samreid]

To check:

1. Do units come through the data stream?
2. Do states save and load?
3. Do instance proxies ranges still work?

(moved to lower comment)

[samreid]

I've ported the 180+ usages sites to use NumberProperty and eliminated type parameter from TNumber. Things are going well so far, but I need to confirm the above 3 points and fix some ranges and null issues in sim code. I'll try to get a bit further on my working copy before committing everything.

[samreid]

There are the failed tests in non-phet-io sims:

build-an-atom
Uncaught Error: Assertion failed: invalid value: undefined
Error: Assertion failed: invalid value: undefined
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083202451:138:40)
    at NumberAtom.setSubAtomicParticleCount (http://localhost/shred/js/model/NumberAtom.js?bust=1505083202451:126:33)
    at Object.fire (http://localhost/shred/js/view/PeriodicTableCell.js?bust=1505083202451:82:24)
    at ButtonListener.setButtonState (http://localhost/scenery/js/input/ButtonListener.js?bust=1505083202451:129:31)
    at Object.up (http://localhost/scenery/js/input/ButtonListener.js?bust=1505083202451:87:14)
    at ButtonListener.buttonUp (http://localhost/scenery/js/input/DownUpListener.js?bust=1505083202451:125:22)
    at Object.up (http://localhost/scenery/js/input/DownUpListener.js?bust=1505083202451:58:16)
    at Input.dispatchToListeners (http://localhost/scenery/js/input/Input.js?bust=1505083202451:790:72)
    at Input.dispatchEvent (http://localhost/scenery/js/input/Input.js?bust=1505083202451:760:12)
circuit-construction-kit-black-box-study
Uncaught TypeError: startVertex.positionProperty.get(...).distance is not a function
TypeError: startVertex.positionProperty.get(...).distance is not a function
    at new Wire (http://localhost/circuit-construction-kit-common/js/model/Wire.js?bust=1505083250290:34:63)
    at new CCKIcon (http://localhost/circuit-construction-kit-black-box-study/js/explore/view/CCKIcon.js?bust=1505083250290:49:16)
    at new ExploreScreen (http://localhost/circuit-construction-kit-black-box-study/js/explore/ExploreScreen.js?bust=1505083250290:31:23)
    at http://localhost/circuit-construction-kit-black-box-study/js/circuit-construction-kit-black-box-study-main.js?bust=1505083250290:40:9
    at window.phetLaunchSimulation (http://localhost/joist/js/SimLauncher.js?bust=1505083250290:50:11)
    at doneLoadingImages (http://localhost/joist/js/SimLauncher.js?bust=1505083250290:72:18)
    at Object.launch (http://localhost/joist/js/SimLauncher.js?bust=1505083250290:126:9)
    at http://localhost/circuit-construction-kit-black-box-study/js/circuit-construction-kit-black-box-study-main.js?bust=1505083250290:38:17
    at Object.execCb (http://localhost/sherpa/lib/require-2.1.11.js:1650:25)
    at Module.check (http://localhost/sherpa/lib/require-2.1.11.js:866:35)
circuit-construction-kit-dc
Uncaught Error: Assertion failed: invalid initial value: null
Error: Assertion failed: invalid initial value: null
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.Property [as constructor] (http://localhost/axon/js/Property.js?bust=1505083251086:75:36)
    at new NumberProperty (http://localhost/axon/js/NumberProperty.js?bust=1505083251086:107:14)
    at new Ammeter (http://localhost/circuit-construction-kit-common/js/model/Ammeter.js?bust=1505083251086:34:28)
    at IntroScreenModel.CircuitConstructionKitModel [as constructor] (http://localhost/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.js?bust=1505083251086:63:20)
    at new IntroScreenModel (http://localhost/circuit-construction-kit-dc/js/intro/model/IntroScreenModel.js?bust=1505083251086:21:33)
    at IntroScreen.createModel (http://localhost/circuit-construction-kit-dc/js/intro/IntroScreen.js?bust=1505083251086:64:27)
    at IntroScreen.initializeModel (http://localhost/joist/js/Screen.js?bust=1505083251086:210:26)
    at Array. (http://localhost/joist/js/Sim.js?bust=1505083251086:673:18)
    at http://localhost/joist/js/Sim.js?bust=1505083251086:684:27
circuit-construction-kit-dc-virtual-lab
Uncaught Error: Assertion failed: invalid initial value: null
Error: Assertion failed: invalid initial value: null
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.Property [as constructor] (http://localhost/axon/js/Property.js?bust=1505083252011:75:36)
    at new NumberProperty (http://localhost/axon/js/NumberProperty.js?bust=1505083252011:107:14)
    at new Ammeter (http://localhost/circuit-construction-kit-common/js/model/Ammeter.js?bust=1505083252011:34:28)
    at LabScreenModel.CircuitConstructionKitModel [as constructor] (http://localhost/circuit-construction-kit-common/js/model/CircuitConstructionKitModel.js?bust=1505083252011:63:20)
    at new LabScreenModel (http://localhost/circuit-construction-kit-dc/js/lab/model/LabScreenModel.js?bust=1505083252011:21:33)
    at LabScreen.createModel (http://localhost/circuit-construction-kit-dc/js/lab/LabScreen.js?bust=1505083252011:36:27)
    at LabScreen.initializeModel (http://localhost/joist/js/Screen.js?bust=1505083252011:210:26)
    at Array. (http://localhost/joist/js/Sim.js?bust=1505083252011:673:18)
    at http://localhost/joist/js/Sim.js?bust=1505083252011:684:27
energy-skate-park-basics
Uncaught Error: Assertion failed: invalid value: null
Error: Assertion failed: invalid value: null
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083303012:138:40)
    at NumberProperty.set value [as value] (http://localhost/axon/js/Property.js?bust=1505083303012:233:36)
    at Skater.released (http://localhost/energy-skate-park-basics/js/energy-skate-park-basics/model/Skater.js?bust=1505083303012:407:45)
    at end (http://localhost/energy-skate-park-basics/js/energy-skate-park-basics/view/SkaterNode.js?bust=1505083303012:178:16)
    at SimpleDragHandler.endDrag (http://localhost/scenery/js/input/SimpleDragHandler.js?bust=1505083303012:205:26)
    at Object.up (http://localhost/scenery/js/input/SimpleDragHandler.js?bust=1505083303012:93:16)
    at Input.dispatchToListeners (http://localhost/scenery/js/input/Input.js?bust=1505083303012:790:72)
    at Input.dispatchEvent (http://localhost/scenery/js/input/Input.js?bust=1505083303012:760:12)
    at Input.upEvent (http://localhost/scenery/js/input/Input.js?bust=1505083303012:629:12)
forces-and-motion-basics
Uncaught Error: Assertion failed: invalid value: 61.98848
Error: Assertion failed: invalid value: 61.98848
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083364562:138:40)
    at http://localhost/forces-and-motion-basics/js/netforce/model/NetForceModel.js?bust=1505083364562:471:61
    at Array.forEach ()
    at NetForceModel.updateCartAndPullers (http://localhost/forces-and-motion-basics/js/netforce/model/NetForceModel.js?bust=1505083364562:471:18)
    at NetForceModel.step (http://localhost/forces-and-motion-basics/js/netforce/model/NetForceModel.js?bust=1505083364562:437:14)
    at Sim.stepSimulation (http://localhost/joist/js/Sim.js?bust=1505083364562:837:22)
    at Sim.runAnimationLoop (http://localhost/joist/js/Sim.js?bust=1505083364562:777:16)
gravity-force-lab-basics
Uncaught Error: Assertion failed: invalid initial value: -2000
Error: Assertion failed: invalid initial value: -2000
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.Property [as constructor] (http://localhost/axon/js/Property.js?bust=1505083496468:75:36)
    at new NumberProperty (http://localhost/axon/js/NumberProperty.js?bust=1505083496468:107:14)
    at Mass.ISLCObject [as constructor] (http://localhost/inverse-square-law-common/js/model/ISLCObject.js?bust=1505083496468:47:29)
    at new Mass (http://localhost/inverse-square-law-common/js/model/Mass.js?bust=1505083496468:42:16)
    at new GravityForceLabBasicsModel (http://localhost/gravity-force-lab-basics/js/gravity-force-lab-basics/model/GravityForceLabBasicsModel.js?bust=1505083496468:59:17)
    at Screen.createModel (http://localhost/gravity-force-lab-basics/js/gravity-force-lab-basics-main.js?bust=1505083496468:45:20)
    at Screen.initializeModel (http://localhost/joist/js/Screen.js?bust=1505083496468:210:26)
    at Array. (http://localhost/joist/js/Sim.js?bust=1505083496468:673:18)
    at http://localhost/joist/js/Sim.js?bust=1505083496468:684:27
masses-and-springs
Uncaught Error: Assertion failed: invalid value: -2.16025641025641
Error: Assertion failed: invalid value: -2.16025641025641
    at window.assertions.assertFunction (http://localhost/assert/js/assert.js:23:13)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083567629:138:40)
    at updateViewLength (http://localhost/masses-and-springs/js/common/view/OscillatingSpringNode.js?bust=1505083567629:71:27)
    at http://localhost/masses-and-springs/js/common/view/OscillatingSpringNode.js?bust=1505083567629:77:7
    at NumberProperty.link (http://localhost/axon/js/Property.js?bust=1505083567629:245:11)
    at new OscillatingSpringNode (http://localhost/masses-and-springs/js/common/view/OscillatingSpringNode.js?bust=1505083567629:75:41)
    at http://localhost/masses-and-springs/js/common/view/SpringView.js?bust=1505083567629:60:24
    at Array.forEach ()
    at IntroScreenView.SpringView [as constructor] (http://localhost/masses-and-springs/js/common/view/SpringView.js?bust=1505083567629:59:19)
    at IntroScreenView.TwoSpringView [as constructor] (http://localhost/masses-and-springs/js/common/view/TwoSpringView.js?bust=1505083567629:36:16)
optics-lab
Uncaught TypeError: Cannot read property 'getStart' of undefined
TypeError: Cannot read property 'getStart' of undefined
    at RayPath.getRelativeShape (http://localhost/optics-lab/js/optics-lab/model/RayPath.js?bust=1505083648394:110:42)
    at SourceNode.drawRays (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:239:51)
    at Array. (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:161:12)
    at Emitter.emit2 (http://localhost/axon/js/Emitter.js?bust=1505083648394:206:49)
    at NumberProperty._notifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:204:29)
    at NumberProperty._setAndNotifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:193:14)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083648394:140:16)
    at NumberProperty.set value [as value] (http://localhost/axon/js/Property.js?bust=1505083648394:233:36)
    at OpticsLabModel.processRays (http://localhost/optics-lab/js/optics-lab/model/OpticsLabModel.js?bust=1505083648394:117:43)
    at Array. (http://localhost/optics-lab/js/optics-lab/model/SourceModel.js?bust=1505083648394:75:22)
optics-lab
Uncaught TypeError: Cannot read property 'getStart' of undefined
TypeError: Cannot read property 'getStart' of undefined
    at RayPath.getRelativeShape (http://localhost/optics-lab/js/optics-lab/model/RayPath.js?bust=1505083648394:110:42)
    at SourceNode.drawRays (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:239:51)
    at Array. (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:161:12)
    at Emitter.emit2 (http://localhost/axon/js/Emitter.js?bust=1505083648394:206:49)
    at NumberProperty._notifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:204:29)
    at NumberProperty._setAndNotifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:193:14)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083648394:140:16)
    at NumberProperty.set value [as value] (http://localhost/axon/js/Property.js?bust=1505083648394:233:36)
    at OpticsLabModel.processRays (http://localhost/optics-lab/js/optics-lab/model/OpticsLabModel.js?bust=1505083648394:117:43)
    at SourceModel.setPosition (http://localhost/optics-lab/js/optics-lab/model/SourceModel.js?bust=1505083648394:240:24)
optics-lab
Uncaught TypeError: Cannot read property 'getStart' of undefined
TypeError: Cannot read property 'getStart' of undefined
    at RayPath.getRelativeShape (http://localhost/optics-lab/js/optics-lab/model/RayPath.js?bust=1505083648394:110:42)
    at SourceNode.drawRays (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:239:51)
    at Array. (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:161:12)
    at Emitter.emit2 (http://localhost/axon/js/Emitter.js?bust=1505083648394:206:49)
    at NumberProperty._notifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:204:29)
    at NumberProperty._setAndNotifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:193:14)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083648394:140:16)
    at NumberProperty.set value [as value] (http://localhost/axon/js/Property.js?bust=1505083648394:233:36)
    at OpticsLabModel.processRays (http://localhost/optics-lab/js/optics-lab/model/OpticsLabModel.js?bust=1505083648394:117:43)
    at SourceModel.setPosition (http://localhost/optics-lab/js/optics-lab/model/SourceModel.js?bust=1505083648394:240:24)
optics-lab
Uncaught TypeError: Cannot read property 'getStart' of undefined
TypeError: Cannot read property 'getStart' of undefined
    at RayPath.getRelativeShape (http://localhost/optics-lab/js/optics-lab/model/RayPath.js?bust=1505083648394:110:42)
    at SourceNode.drawRays (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:239:51)
    at Array. (http://localhost/optics-lab/js/optics-lab/view/SourceNode.js?bust=1505083648394:161:12)
    at Emitter.emit2 (http://localhost/axon/js/Emitter.js?bust=1505083648394:206:49)
    at NumberProperty._notifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:204:29)
    at NumberProperty._setAndNotifyListeners (http://localhost/axon/js/Property.js?bust=1505083648394:193:14)
    at NumberProperty.set (http://localhost/axon/js/Property.js?bust=1505083648394:140:16)
    at NumberProperty.set value [as value] (http://localhost/axon/js/Property.js?bust=1505083648394:233:36)
    at OpticsLabModel.processRays (http://localhost/optics-lab/js/optics-lab/model/OpticsLabModel.js?bust=1505083648394:117:43)
    at SourceModel.setPosition (http://localhost/optics-lab/js/optics-lab/model/SourceModel.js?bust=1505083648394:240:24)

[samreid]

I addressed several issues in my working copy. I skipped 2 issues for sims I am unfamiliar with, and they are noted in the above 2 issues.

[samreid]

Remaining issues in aqua fuzz phet-io are not related to this problem:

[samreid]

To check:

1. Do units come through the data stream?

   No, we don't have access to the instance in phetEvents.js. We should add that, but it may not be critical. Or we could add data in TProperty's call to toEventOnEmit.

2. Do states save and load?

   Concentration looks good.

3. Do instance proxies ranges still work?

   Nope, perhaps getAPIForType can be updated.

[samreid]

iframe API tests are all passing:

I'm inclined to commit these changes and address instance proxies ranges as a separate step. If there is no way to pass this data over the instanceAdded API, it can still be done as an asynchronous callback.

[samreid]

Even better to change:

!typeAPI.dataStreamOnlyType && instanceAddedEmitter.emit2( phetioID, typeAPI );

to

!typeAPI.dataStreamOnlyType && instanceAddedEmitter.emit3( phetioID, typeAPI, instance.phetioMetadata );

But I think this should be done as a separate step. I'll commit shortly.

[samreid]

Maybe safer to commit but not push, so I can get instance proxies somewhat working before pushing.

[samreid]

• [ ] PhET-iO could leverage Property.validValues as well.

[samreid]

Instance proxies is working again with phetioInstanceMetadata. Builds with phet and phet-io brand are working. Aqua seems OK. Iframe API test is OK. I'll see if any other developer wants to review before I push these commits.

[zepumph]

@samreid and I dove into looking at a way to centralize all of the metadata needed for Properties (including NumberProperty and DerivedProperty) in TProperty. We chose to look at accomplishing this with toStateObject, but didn't get to a commit point yet, here are the static methods for TProperty. NOTE: this would be a huge api change for the data stream and state.

  /**
       * Decodes a state into a Property.
       * @param {Object} stateObject
       * @returns {Object}
       */
      fromStateObject: function( stateObject ) {
        return {
          value: phetioValueType.fromStateObject( stateObject.value )
        };
      },

      /**
       * Encodes a DerivedProperty instance to a state.
       * @param {Object} instance
       * @returns {Object} - a state object
       */
      toStateObject: function( instance ) {
        assert && assert( instance, 'instance should be defined' );
        assert && assert( phetioValueType.toStateObject, 'toStateObject doesn\'t exist for ' + phetioValueType.typeName );
        return {
          value: phetioValueType.toStateObject( instance.value ),
          units: instance.units
        };
      },

      /**
       * Used to set the value when loading a state
       * @param instance
       * @param fromStateObjectResult
       */
      setValue: function( instance, fromStateObjectResult ) {
        instance.set( fromStateObjectResult.value );
      },

[samreid]

Trying to enumerate what's left to do here:

• [ ] Use toStateObject to get a snapshot of each object on instanceAdded
• [ ] Use the snapshots throughout instance proxies
• [ ] Use the code above to add state for Property
• [ ] Possibly segregate TNumberProperty
• [ ] Find a way to get TNumberProperty features like these into TDerivedProperty
• [ ] PhET-iO could leverage Property.validValues
• [ ] Move phetioInstanceDocumentation to the toStateObject data in a uniform way.

After seeing these issues, it seems they should move to the PhET-iO repo, closing here.

[samreid]

There are still TODOs marked for this issue, discovered during https://github.com/phetsims/chipper/issues/946

[zepumph]

Range was added to xProperty in FAMB for phet-io. At first glance it didn't seem like xProperty range needed to be included to support current phet-io design patterns. Likely xProperty will be phetioReadOnly in the future. Anyways. I felt good removing the TODO without any work. Closing