create additional type-specific Property subclasses?

[pixelzoom]

Somewhat related to #196...

Should we create any additional type-specific Property subclasses, for types that occur frequently? The primary reason I'm asking is because of the boilerplate PhET-iO instrumentation that is involved, especially for type description/validation.

Looking through the sims that I'm responsible for, I see frequent occurrences of Property.<Vector2>} and {Property.<Color>}. Every ScreenView has an instance of the latter, and they also appear in ColorProfile.

So for example this boilerplate:

this.locationProperty = new Property( options.initialLocation, {
  valueType: Vector2,
  phetioType: PropertyIO( Vector2IO ),
  tandem: options.tandem.createTandem( 'locationProperty' )
} );

... could be replaced with:

this.locationProperty = new Vector2Property( options.initialLocation, {
  tandem: options.tandem.createTandem( 'locationProperty' )
} );

[samreid]

@zepumph was investigating an approach that would eliminate one of valueType or phetioType and infer the other automatically.

On the other hand, I'm not opposed to creating Vector2Property and ColorProperty, but just wondering if something more general would be better. Like:

this.locationProperty = new TypedProperty( options.initialLocation, Vector2, {
   tandem: options.tandem.createTandem( 'locationProperty' )
});

or

this.locationProperty = new TypedProperty( options.initialLocation, {
   phetioType: PropertyIO(Vector2IO), // valueType is inferred from this
   tandem: options.tandem.createTandem( 'locationProperty' )
});

etc.

[chrisklus]

but just wondering if something more general would be better

I think I would prefer just adding more type-specific Property subclasses instead of something general, unless we switched to using only using the general type. It might be weird to be able to do the same thing three different ways:

this.activeProperty = new Property( false, {
  phetioType: PropertyIO( BooleanIO ),
  tandem: options.tandem.createTandem( 'activeProperty' )
} );

and

this.activeProperty = new BooleanProperty( false, {
  tandem: options.tandem.createTandem( 'activeProperty' )
} );

and

this.activeProperty = new Property( false, Boolean, {
  tandem: options.tandem.createTandem( 'activeProperty' )
} );

[samreid]

We should also create EnumerationProperty

[samreid]

Should we go the extra mile and prevent types like Vector2 from being passed to a plain Property if there is a subtype that should be used?

[jessegreenberg]

Could that be done with a lint rule?

[samreid]

JS is not a statically typed language and hence we cannot determine the type of a value passed into Property until runtime. So this cannot be done with a lint rule.

[jessegreenberg]

Thanks, that makes sense. I was hoping we could search for new Property( new Color(...) ) and so on, but we would never be able to write a rule that catches new Property( referenceToColorInstance ).

[samreid]

Does this still need to be labeled for developer meeting?

[zepumph]

Does this still need to be labeled for developer meeting?

I would defer to @pixelzoom's judgement on that.

---

From #221 (comment):

@zepumph was investigating an approach that would eliminate one of valueType or phetioType and infer the other automatically.

Yes indeed, see #204

When that is added, likely we are talking about one line of code difference. I agree that it is very nice to not have to repeat the type creation everywhere you need to create an instance. I would think that only a few other specific Property subtypes would be warranted though.

[pixelzoom]

Does this still need to be labeled for developer meeting?

I would defer to @pixelzoom's judgement on that.

Did we come to a consensus at last week's dev meeting? I don't see any comment with dev meeting notes.

[chrisklus]

Did we come to a consensus at last week's dev meeting? I don't see any comment with dev meeting notes.

I think we decided that we'd go ahead and add a few other type-specific Property subclasses, but I don't remember finalizing which ones.

[samreid]

I added this code to the Property constructor:

      window.top.propertyMap = window.top.propertyMap || {};
      if ( value && value.constructor && value.constructor.name ) {
        window.top.propertyMap[ value.constructor.name ] = window.top.propertyMap[ value.constructor.name ] || 0;
        window.top.propertyMap[ value.constructor.name ]++;
      }

and ran local aqua. This reports the number of instances of Property with constructor names on instantiation. Here is the result, and a script that sorts:

/* eslint-disable */
var x = {
  "String": 10215,
  "Boolean": 480006,
  "Number": 40877,
  "Bounds2": 862,
  "Vector2": 37912,
  "Color": 82536,
  "RangeWithValue": 121,
  "Object": 305,
  "PerimeterShape": 8,
  "ImmutableVector2": 21153,
  "Orientation": 65,
  "Array": 449,
  "ProportionalArea": 5,
  "Polynomial": 307,
  "TermList": 10,
  "Partition": 40,
  "NumberProperty": 43,
  "Range": 1387,
  "ModelViewTransform2": 24,
  "PartitionedArea": 44,
  "Term": 1707,
  "DynamicProperty": 5,
  "GenericLayout": 8,
  "GenericArea": 5,
  "DerivedProperty": 6,
  "GenericPartition": 12,
  "AreaChallenge": 14,
  "Entry": 38,
  "HTMLImageElement": 89,
  "SynthesisEquation": 2,
  "DecompositionEquation": 1,
  "Solute": 7,
  "BeersLawSolution": 1,
  "LaserColor": 3,
  "Medium": 6,
  "Substance": 6,
  "Polygon": 70,
  "Circle": 15,
  "SemiCircle": 7,
  "Fraction": 388,
  "FractionChallenge": 80,
  "KitCollection": 5,
  "Rectangle": 4,
  "BAAGameState": 1,
  "Bounds3": 2,
  "Vector3": 481,
  "Shape": 226,
  "Property": 13,
  "Vertex": 746,
  "ShapesScene": 2,
  "NumbersScene": 2,
  "VariablesScene": 1,
  "ConstantTerm": 5,
  "OperationsScene": 1,
  "LabScene": 1,
  "TwoVariablesScene": 1,
  "Dimension2": 310,
  "Dimension3": 203,
  "SquarePoolModel": 2,
  "MatchSpot": 31,
  "MatchingChallenge": 24,
  "PatternsScene": 2,
  "EquationsScene": 1,
  "MysteryScene": 1,
  "FBBMysteryScene": 1,
  "MotionBounds": 108,
  "GeneA": 1,
  "Line": 170,
  "GraphTheLine": 2,
  "Quadratic": 12,
  "GravityAndOrbitsMode": 2,
  "Residual": 85,
  "Level": 1,
  "Body": 10,
  "RealMolecule": 3,
  "VSEPRMolecule": 2,
  "RealMoleculeShape": 2,
  "Pendulum": 3,
  "ProjectileObjectType": 4,
  "DataPoint": 14,
  "RadialGradient": 1,
  "NecklaceLayout": 20,
  "PaintChoice": 3,
  "NecklaceScene": 2,
  "Image": 17,
  "SandwichNode": 2,
  "SandwichRecipe": 1,
  "H2Node": 58,
  "O2Node": 98,
  "H2ONode": 58,
  "N2Node": 26,
  "NH3Node": 23,
  "CH4Node": 24,
  "CO2Node": 21,
  "Reaction": 1,
  "PCl3Node": 8,
  "Cl2Node": 16,
  "PCl5Node": 4,
  "NONode": 22,
  "CONode": 12,
  "F2Node": 14,
  "OF2Node": 6,
  "HFNode": 5,
  "CNode": 12,
  "C2H5OHNode": 4,
  "SNode": 10,
  "CS2Node": 4,
  "H2SNode": 6,
  "C2H2Node": 16,
  "SO2Node": 8,
  "C2H4Node": 4,
  "C2H6Node": 16,
  "CH2ONode": 4,
  "CH3OHNode": 2,
  "NO2Node": 8,
  "N2ONode": 4,
  "P4Node": 12,
  "PF3Node": 4,
  "PH3Node": 2,
  "HClNode": 4,
  "Easing": 2,
  "FruitScene": 2,
  "VegetableScene": 2,
  "CandyScene": 2,
  "ShoppingCategory": 2,
  "WaterScene": 3
};

var array = Object.keys( x ).map( k => {return { key: k, value: x[ k ] }} );
console.log( array );
array.sort( ( a, b ) => b.value - a.value );

const result = array.map( a => a.key + ': ' + a.value ).join( '\n' );
console.log( result );

Boolean: 480006
Color: 82536
Number: 40877
Vector2: 37912
ImmutableVector2: 21153
String: 10215
Term: 1707
Range: 1387
Bounds2: 862
Vertex: 746
Vector3: 481
Array: 449
Fraction: 388
Dimension2: 310
Polynomial: 307
Object: 305
Shape: 226
Dimension3: 203
Line: 170
RangeWithValue: 121
MotionBounds: 108
O2Node: 98
HTMLImageElement: 89
Residual: 85
FractionChallenge: 80
Polygon: 70
Orientation: 65
H2Node: 58
H2ONode: 58
PartitionedArea: 44
NumberProperty: 43
Partition: 40
Entry: 38
MatchSpot: 31
N2Node: 26
ModelViewTransform2: 24
MatchingChallenge: 24
CH4Node: 24
NH3Node: 23
NONode: 22
CO2Node: 21
NecklaceLayout: 20
Image: 17
Cl2Node: 16
C2H2Node: 16
C2H6Node: 16
Circle: 15
AreaChallenge: 14
DataPoint: 14
F2Node: 14
Property: 13
GenericPartition: 12
Quadratic: 12
CONode: 12
CNode: 12
P4Node: 12
TermList: 10
Body: 10
SNode: 10
PerimeterShape: 8
GenericLayout: 8
PCl3Node: 8
SO2Node: 8
NO2Node: 8
Solute: 7
SemiCircle: 7
DerivedProperty: 6
Medium: 6
Substance: 6
OF2Node: 6
H2SNode: 6
ProportionalArea: 5
DynamicProperty: 5
GenericArea: 5
KitCollection: 5
ConstantTerm: 5
HFNode: 5
Rectangle: 4
ProjectileObjectType: 4
PCl5Node: 4
C2H5OHNode: 4
CS2Node: 4
C2H4Node: 4
CH2ONode: 4
N2ONode: 4
PF3Node: 4
HClNode: 4
LaserColor: 3
RealMolecule: 3
Pendulum: 3
PaintChoice: 3
WaterScene: 3
SynthesisEquation: 2
Bounds3: 2
ShapesScene: 2
NumbersScene: 2
SquarePoolModel: 2
PatternsScene: 2
GraphTheLine: 2
GravityAndOrbitsMode: 2
VSEPRMolecule: 2
RealMoleculeShape: 2
NecklaceScene: 2
SandwichNode: 2
CH3OHNode: 2
PH3Node: 2
Easing: 2
FruitScene: 2
VegetableScene: 2
CandyScene: 2
ShoppingCategory: 2
DecompositionEquation: 1
BeersLawSolution: 1
BAAGameState: 1
VariablesScene: 1
OperationsScene: 1
LabScene: 1
TwoVariablesScene: 1
EquationsScene: 1
MysteryScene: 1
FBBMysteryScene: 1
GeneA: 1
Level: 1
RadialGradient: 1
SandwichRecipe: 1
Reaction: 1

[samreid]

Taking a closer look with everything 1000 or over:

Boolean: 480006
Color: 82536
Number: 40877
Vector2: 37912
ImmutableVector2: 21153
String: 10215
Term: 1707
Range: 1387

We already have BooleanProperty and NumberProperty and StringProperty. That leaves ColorProperty and Vector2Property. RangeProperty seems on the lower side, but we could add it too if desired. Term seems simulation specific, so would not be implemented in axon, if at all.

Therefore, I propose the following next steps:

1. Create ColorProperty
2. Create Vector2Property
3. Add an assertion to Property that requires the proper subtype of Property is used (for Color, Vector2, Number, String, Boolean) based on the initially supplied Property value. Not exactly sure how this will work because the straightforward implementation (instanceof check for Property subtype in Property) would introduce a loop in the module loading.

@pixelzoom does this sound like a good plan to you? If so, we can open new issues for 1-3 and close this one.

[pixelzoom]

ColorProperty and Vector2Property sound good. They are the 2 that I had zeroed in on in #221 (comment).

Unless there's a straightforward implementation of (3), I would be inclined to skip it and address during code review and PhET-iO instrumentation. That seems to be working for the current Properties subclasses.

[samreid]

Some more questions before this can be split out into issues. Should ColorProperty be declared in scenery or axon? Should Vector2Property be declared in dot or axon?

[pixelzoom]

That's probably depends on what dependencies are desired (or possible), and might be a better question for @jonathanolson.

[samreid]

For scalability and to avoid odd repo-repo dependencies, it seems appropriate for the *Property types to be declared adjacent to the corresponding * types.

[samreid]

We agreed we will put the *Property next to the * types.

[jonathanolson]

@jonathanolson does that make the discussion of ColorProperty here moot?

It shouldn't, they seem to serve different purposes.

[pixelzoom]

It shouldn't, they seem to serve different purposes.

Please elaborate. And please provide input on how ColorProperty should work.

[jonathanolson]

PaintColorProperty effectively takes a PaintDef as input (with an optional lightening/darkening factor) and turns it into a Property.<Color>. It's an adapter/converter.

ColorProperty would presumably be like a normal Property.<Color> with enhancements so that specifying type information is not needed for phet-io?

[pixelzoom]

The open issue is whether this "color Property" should take a value of type {ColorDef} or {null|string|Color} and whether it should be named either ColorProperty or ColorDefProperty.

[samreid]

In #274 we discussed adding RangeProperty.

[samreid]

I'm only seeing 3 occurrences of PropertyIO( ColorIO ) at the moment. Let's bring it back to developer meeting to determine how to proceed regarding ColorProperty. Main questions:

• Should we create ColorProperty?
• What should it be named?
• Should it support ColorDef or just Color?

PhET-iO support may be more straightforward if it just uses Color and not ColorDef.

[zepumph]

Today during dev meeting we decided that it was easy and forward thinking to create the following Properties:

• ColorProperty.js
• ColorDefProperty.js
• PaintDefProperty.js

For PhET-iO instrumentation, we decided not to eagerly instrument some of these with no instrumented usages. We should instrument ColorProperty.js though. Uninstrumented color-type Properties will be:

• ColorDefProperty
• PaintDefProperty
• Properties from ColorProfile.

[samreid]

PaintColorProperty was added in phetsims/scenery@04abdf6, which takes PaintDef values.

[samreid]

I added ColorProperty. PaintColorProperty already supports PaintDef values. @jonathanolson can you comment whether ColorDefProperty should look more like ColorProperty implementation or PaintColorProperty implementation?

[jonathanolson]

can you comment whether ColorDefProperty should look more like ColorProperty implementation or PaintColorProperty implementation?

PaintColorProperty converts {PaintDef} values to {Property.<Color>}, presumably ColorDefProperty is simply a "typed" Property. I'd assume it would look more like ColorProperty.

[pixelzoom]

We may want to defer this until we discuss phetsims/scenery#1135 (revisit "color" and "paint").