-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PhET-iO is not aware of Property options that determine valid values #137
Comments
In light of #136, it's worth repeating that this is not just about NumberProperty |
@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. |
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. |
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
} );
|
To check:
(moved to lower comment) |
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. |
There are the failed tests in non-phet-io sims:
|
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. |
To check:
|
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. |
Maybe safer to commit but not push, so I can get instance proxies somewhat working before pushing. |
|
Instance proxies is working again with |
@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 );
}, |
Trying to enumerate what's left to do here:
After seeing these issues, it seems they should move to the PhET-iO repo, closing here. |
There are still TODOs marked for this issue, discovered during phetsims/chipper#946 |
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 |
Moved here from https://github.com/phetsims/phet-io/issues/1210.
In phetsims/beers-law-lab#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:
to this:
So apparently PhET-iO is ignoring the range associated to NumberProperty, and requires you to duplicate the range like this:
PhET-iO needs to be aware of NumberProperty's
range
. And it needs to be aware of Property'svalidValue
andisValidValue
options.The text was updated successfully, but these errors were encountered: