phetsims/dot

Create Vector2Property

Closed this issue · 11 comments

From phetsims/axon#221 we would like to create a custom Vector2Property type.

I've created Vector2Property and revised sites with these options to use it:

  • phetioType: PropertyIO( Vector2IO )
  • valueType: Vector2

Aqua is partway through testing phet brand and phet-io brand and no systemic issues are revealed. If sim-specific issues appear on CT, I will look into them.

I'll send a note on slack, since this is a large-ish commit.

Committing 44 files across 21 repos to introduce Vector2Property, please see #88 for details.

I saw issues in Blackbody Spectrum and Charges and Fields, but the former doesn't look related and I'm not sure about the latter.

From slack:

Chris Malley [9:45 AM]
Was the goal to convert all {Property.<Vector2>}, or just the ones that specified valueType or phetioType?
I still see 127 occurrences of “{Property.}“. Our goal in phetsims/axon#196 was to proactively convert to type-specific Properties, so that things are ready for PhET-iO.
Some of those “{Property.}” are bugs introduced by dot#88, documentation was not changed.

Chris Malley [9:55 AM]
I’m fixing doc in my sims.

Sam Reid [9:59 AM]
Some of those annotations should presumably be removed as redundant?

Chris Malley [9:59 AM]
yes

Sam Reid [10:00 AM]
I have meetings starting now but can look into that afterwards

I've fixed the documentation for my sims, deleted incorrect/redundant type expressions where appropriate. I've also converted the remainder of my sims to use Vector2Property, so that they are in good shape for eventual PhET-iO instrumentation.

Clients should not be able to change the type of a type-specific Property. Setting valueType and phetioType is done incorrectly in Vector2Property. See any of the other type-specific Properties for correct implementation. For example, BooleanProperty:

      assert && assert( !options.valueType, 'valueType is set by BooleanProperty' );
      options.valueType = 'boolean';  // BooleanProperty requires values to be primitive booleans

      assert && assert( !options.hasOwnProperty( 'phetioType' ), 'phetioType is set by BooleanProperty' );
      options.phetioType = BooleanPropertyIO;

I'm reverting the buggy/gratuitous change to BooleanProperty. All of the existing type-specific Properties use the identical pattern for setting type-related options. There's no reason to change this. Recommended to use the same pattern for Vector2Property, but if you want to use a different pattern that's fine. But there's no reason to change the existing types.

I replaced more patterns that were creating new Property(Vector.ZERO) and new Property(new Vector2()). I could probably find more cases if I put a check in the Property constructor, but not sure whether that's beyond the point of diminishing returns.

Re the change to BooleanProperty in #88 (comment)...
Tracking changes to how options are handled in phetsims/axon#234.
Discussing best practices for options in phetsims/phet-info#96.

Looks like unit tests have not been implemented for Vector2Property.

I added unit tests in the previous commit.

@samreid anything else to do here other than review?

The unit tests look like they have good coverage. I can't think of anything else for this issue. @pixelzoom does Vector2Property need further review? Can this issue be closed?

I think this is OK to close.