Impedance mismatch between RangeWithValue and NumberProperty?

[samreid]

From https://github.com/phetsims/dot/issues/78

I think there's a problem with some (many?) usages of RangeWithValue. A typical usage is like this:

// 1. Create range with value
// 2. Create a NumberProperty that has range: rangeWithValue and initialValue: rangeWithValue.defaultValue

Why not create the NumberProperty directly in the first place? Declaring it as a RangeWithValue in one place then picking values out for the NumberProperty redundant? For example, CLBConstants.js declares:

BATTERY_VOLTAGE_RANGE: new RangeWithValue( -1.5, 1.5, 0 ), // Volts

Then the Property is declared like so:

    // @public {Property.<number>}
    this.voltageProperty = new NumberProperty( voltage, {
      tandem: tandem.createTandem( 'voltageProperty' ),
      units: 'volts',
      range: CLBConstants.BATTERY_VOLTAGE_RANGE
    } );

And the voltage variable came from:

CLBConstants.BATTERY_VOLTAGE_RANGE.defaultValue,

Or is this expected because we sometimes have one range creating many Property instances (say, one per screen?)

Here's the same thing happening in BeersLawSolution:

    this.concentrationProperty = new NumberProperty( concentrationRange.defaultValue, {
      units: 'moles/liter',
      range: concentrationRange,
      tandem: options.tandem.createTandem( 'concentrationProperty' )
    } );

By that token, why aren't the units associated with the range, so we could do something like this?

    this.concentrationProperty = new NumberProperty( concentrationRange.defaultValue, {
      units: concentrationRange.units,
      range: concentrationRange,
      tandem: options.tandem.createTandem( 'concentrationProperty' )
    } );

Or is there a better way to pass RangeWithValue to a NumberProperty?

Leaving self-assigned for now to think it over before discussing with other members of the team.

[pixelzoom]

@samreid said:

I think there's a problem with some (many?) usages of RangeWithValue. A typical usage is like this: ...

Until 10/17/18 (which is actually before this issue was created), the type of NumberProperty's range option was {Range|{min:number, max:number}|null}. The API changed, so keep that in mind when you're evaluating the usages shown above. And for at least one of the examples above (BLL), NumberProperty and its range option didn't exist when the code was written.

Or is this expected because we sometimes have one range creating many Property instances (say, one per screen?)

I don't know if it's "expected". But it's certainly possible with this implementation, and I've used that frequently. If 2 screens have models with the same NumberProperty (e.g. voltageProperty), they need 2 instances of that NumberProperty, but can share one instance of its Range or RangeWithValue.

Ranges are also things that tend to evolve during sim design/implementation, so I typically put them in a Constants.js file, along with other things that I may need to touch frequently. I don't have a need to put a NumberProperty instance in Constant.js, because Property instances are typically not shared across screens.

Or is there a better way to pass RangeWithValue to a NumberProperty?

The range option to NumberProperty is optional, and it's a {Range}, not a {RangeWithValue}. The range is used for validation, not initialization. If it is a {RangeWithValue}, there's no requirement that the initial value comes from range.defaultValue. I see nothing wrong with the current approach - it's flexible and straightforward.

[samreid]

One other possibility would be to create a new type RangeWithValueNumberProperty which knows how to take the default value and range, like so:

this.concentrationProperty = new RangeWithValueNumberProperty( concentrationRange, {
  units: 'moles/liter',
  tandem: options.tandem.createTandem( 'concentrationProperty' )
} );

Skimming through occurrences of .defaultProperty, I saw >20 cases that could use this pattern, and it will free us up from specifying the same thing twice in all of these cases (and prevent the values from getting out of sync during maintenance).

@pixelzoom can you please review and advise?

[pixelzoom]

I still don't think any change is necessary. Combining Range + NumberProperty is more complicated and less flexible, and the "savings" is minimal/questionable. If you proceed with RangeWithValueNumberProperty, please don't change in sims that I'm responsible for.

[pixelzoom]

If you don't mind, let's defer work on this issue until after the holiday. I'd like to discuss before you proceed.

[samreid]

I'm somewhat on the fence at the moment, so I'm happy to avoid implementing RangeWithValueNumberProperty for now. However, if we had, say 50+ occurrences that matched this pattern, I would probably change my tune. So how about marking deferred for now and re-evaluating in a year or so?

[pixelzoom]

As noted above, this has nothing to do with RangeWithValue. NumberProperty takes option {Range|null} range, not {RangeWithValue} range. So this would presumably be something like class NumberWithRangeProperty extends NumberProperty.

And yes, OK to defer.

[pixelzoom]

Deferred, to be revisited no later than 11/21/2019.