phetsims / dot

A math library with a focus on mutable and immutable linear algebra for 2D and 3D applications.
http://scenerystack.org/
MIT License
13 stars 6 forks source link

Create Vector2Property #88

Closed samreid closed 5 years ago

samreid commented 5 years ago

From https://github.com/phetsims/axon/issues/221 we would like to create a custom Vector2Property type.

samreid commented 5 years ago

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

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 https://github.com/phetsims/dot/issues/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.

samreid commented 5 years ago

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 https://github.com/phetsims/axon/issues/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

pixelzoom commented 5 years ago

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.

pixelzoom commented 5 years ago

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;
pixelzoom commented 5 years ago

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.

samreid commented 5 years ago

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.

pixelzoom commented 5 years ago

Re the change to BooleanProperty in https://github.com/phetsims/dot/issues/88#issuecomment-470692197...
Tracking changes to how options are handled in https://github.com/phetsims/axon/issues/234.
Discussing best practices for options in https://github.com/phetsims/phet-info/issues/96.

pixelzoom commented 5 years ago

Looks like unit tests have not been implemented for Vector2Property.

pixelzoom commented 5 years ago

I added unit tests in the previous commit.

@samreid anything else to do here other than review?

samreid commented 5 years ago

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?

pixelzoom commented 5 years ago

I think this is OK to close.