search

phetsims / forces-and-motion-basics

"Forces and Motion: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/forces-and-motion-basics
GNU General Public License v3.0
7 stars 10 forks source link

Does lastLocationProperty have the wrong phetioValueType? #237

Closed phet-steele closed 6 years ago

phet-steele commented 7 years ago

lastLocationProperty looks to be instrumented as a TVector2, but in NetForceModel.js it is only being set with a string value. This could explain why in Instance Proxies and Events wrappers, the value never changes. It doesn't change when moving pullers to their knots or to their home; it should. It's probably expecting x & y values but receiving a string instead...so it has no support to display that value. This is just an assumption further supported by the documentation saying this accepts a string:

  // @public {string} - a classified location in the play area
    // TODO: What are the valid values for this Property?
    this.lastLocationProperty = new Property( 'home', {
      tandem: tandem.createTandem( 'lastLocationProperty' ),
      phetioValueType: TVector2
    } );

Seen on macOS 10.12.6 Chrome. For phetsims/QA/issues/42.

jessegreenberg commented 7 years ago

Yes, it should be TString, thanks @phet-steele. It has been TVector2 for a while, probably because the Property name makes this sound like a Vector2.

jessegreenberg commented 7 years ago

Looks like valid values are 'knot' and 'home'. So maybe should we rename to lastPlacementProperty?

jessegreenberg commented 7 years ago

If I rename the Property, clients will need to update their wrappers when upgrading to the new version. Is this acceptable? Renaming the Property would presumably make it easier to understand what this Property is doing for both PhET-iO clients and sim maintainers.

jessegreenberg commented 7 years ago

Leaning toward not renaming until we have documentation that clients can use to be notified of breaking changes.

jessegreenberg commented 7 years ago

Would the change from TVector2 to TString potentially break wrappers anyway? @samreid what would you recommend in this case?

samreid commented 7 years ago

We do not need to maintain a stable API yet, we should focus on naming and typing things as they should be.

jessegreenberg commented 7 years ago

Thanks @samreid, sounds good.

jessegreenberg commented 6 years ago

Changes cherry-picked into release branches.

phet-steele commented 6 years ago

Nice! Fixed.