location vs position

[samreid]

When do we use position vs location? Some cases seem haphazard

Jesse Greenberg 3:14 PM

Good question, ive wondered the same and seen both used throughout. Hopefully sims are consistent in which one they use.

Chris Malley 3:46 PM

Vector Addition had an interesting spin on "location" vs "position" that I had not encountered in other sims (and which I am not advocating). Rather than change it, I chose to document it in implementation-notes.md:
"for vectors, position or point refers to model coordinates, while location refers to view coordinates"
I believe that I've (consistently?) used "location" in my sims (e.g. locationProperty), regardless of whether it's model or view. My thinking is that "location" means "where something is located", while "position" can sometimes mean both location and orientation, i.e. "a particular way in which someone or something is placed or arranged." (edited)
As for consistency... Searching for Property definitions, case insensitive.... PhET code has 55 occurrences of "locationProperty =", 146 occurrences of "positionProperty ="

This came up during phetsims/gas-properties#170 when I noticed that some stopwatches used locationProperty and some used positionProperty.

[pixelzoom]

Some cases seem haphazard ...

I noticed that some stopwatches used locationProperty and some used positionProperty. ...

Please identify.

[samreid]

For example, https://github.com/phetsims/energy-skate-park/blob/d1a734a7021646104afc2af1f95e7de97625db8f/js/energy-skate-park/common/view/EnergySkateParkTimerNode.js refers to timerPositionProperty but https://github.com/phetsims/gas-properties/blob/dc8cbc064dcbf5089ecfddb758e45fa9524e834b/js/common/model/Stopwatch.js defines stopwatch.locationProperty

By the way, I'm in progress working through phetsims/gas-properties#170 and we'll have opportunities to fine tune the naming after initial commits.

[jonathanolson]

Most notably, DragListener has locationProperty right now.

[pixelzoom]

Personally, I don't care if we standardize on this. I'd be happy to leave it up to developer discretion as to whether to use 'location' or 'position'. I realize that creates a bit of inconsistency in the PhET-iO API, but I think the cost to clients will be significantly less that the cost for PhET to standardize. My 2 cents...

What I do care about is arbitrary changes to existing code based on personal preference, like this one in common code:

   this.positionProperty = new Vector2Property( options.location, {

This was originally locationProperty. Now we have both positionProperty and options.location because the option didn't get renamed. There was no need to change it, certainly not until this issue was discussed.

[pixelzoom]

12/19/19 dev meeting:

• Consensus is to use "position" uniformly.
• First priority is to change "location" to "position" as it's exposed in the PhET-iO API.
• Developers at their discretion can eagerly change sims that they are responsible for.
• @samreid will add assertion to PhET-iO to prohibit "locationproperty" (case insensitive), assertion case be removed in the future if we have a legitimate need for "location".

Assigning to @samreid to review existing cases and decide on whether we need an assertion.

[samreid]

The renaming for balloons and static was more invasive than I expected it to be, so I consulted with @jessegreenberg about a few aspects.

(1) cases like BASEDescriber where the positions are not cartesian, but more qualitative, like:

  const POSITION_DESCRIPTION_MAP = {
    AT_LEFT_EDGE: {
      UPPER_PLAY_AREA: landmarkLeftEdgeString,
      CENTER_PLAY_AREA: landmarkLeftEdgeString,
      LOWER_PLAY_AREA: landmarkLeftEdgeString
    },

And @jessegreenberg confirmed that our decision to use position instead of location did not just apply to cartesian coordinates, but should also apply to cases like this.

(2) we confirmed that even though a11y key and string placeholder names were changed, no a11y text values were changed, so this shouldn't change anything for the end user.

Fuzzing and linting was clear, so I'll commit this part.

[pixelzoom]

Hmmm... I hadn't thought about translated strings (or their keys). Searching for case-insensitive occurrences of "location", I find 233 in "*strings*.json", 75 in "*A11yString.js". What do you propose to do about these?

[jbphet]

I was going to respond to @pixelzoom and request that string keys not be changed, but I looked at this a bit and all occurrences of "location" seem to be in translations with no corresponding string in the English file. I'm not quite sure what's going on here. There's a commit - phetsims/energy-skate-park-basics@3e68965 - where the strings with "location" in their keys were removed from the strings file, but how did they end up getting translated? Rosetta is specifically designed to only present to the user strings that are used in the sim.

Perhaps the bottom line is this - if @samreid or @jessegreenberg can confirm that these strings are not used in the currently live version of the sim, nor in the master version, the string keys in question in the translated string files can be changed, left alone, or removed from the translations files, since they will have no effect on the translated sim.

EDIT by @pixelzoom: I've create phetsims/energy-skate-park#171 to track what to do (if anything) about energy-skate-park string files.

[pixelzoom]

I attempted to change "location" to "position" in Natural Selection. It's a new sim, so I figured it would be appropriate to address immediately. I ended up introducing a bug in DataProbeDragListener, reverted in the commit above.

DataProbeDragListener is a subclass of DragListener, which has option locationProperty. So until this is addressed for common code, I'm stuck with either continuing to use locationProperty in some Natural Selection code, or having something ugly like this in DataProbeDragListener:

class DataProbeDragListener extends DragListener {

  constructor( positionProperty, ..., options ) {
    ....
    options.locationProperty = positionProperty; // YUCK!!!
    super( options );
    ....
  }

I'm not wild about either of these alternatives.

Assigning to @ariel-phet and putting this back on the developer meeting schedule to discuss when common code will be converted. It's been 19 days since we decided to make this change, and nothing has been done. What's the plan?

[pixelzoom]

Based on a search for "location" (case insensitive) here's what needs to be revised, see checklists below.

Note that this change isn't limited to Property names. It also applies to method names, variable names, constant names, comments, .... Changes may also result in PhET-iO API changes, requiring rebuild of API files.

Imo, common-code should be done first and ASAP - see my DragListener experience in #126 (comment). Sims can be done "chip away".

Common Code:

• DOT/Bounds2.closestPointTo @pixelzoom
• PHETCOMMON/SphereBucket - careful, uses both position & location! @Denz1994
• SCENERY/DragListener, DragListenerTests @pixelzoom
• SCENERY/KeyboardDragListener @zepumph
• SCENERY_PHET/BracketNode @pixelzoom
• SCENERY_PHET/ConductivityTesterNode, ComponentsScreenView @pixelzoom
• SCENERY_PHET/Drawer @pixelzoom
• SCENERY_PHET/FaceWithPointsNode @pixelzoom
• SCENERY_PHET/MeasuringTapeNode @pixelzoom
• SCENERY_PHET/MovementDescriber @zepumph
• SCENERY_PHET/MovableDragHandler @pixelzoom
• SCENERY_PHET/StopwatchReadoutNode @pixelzoom
• SCENERY_PHET/BorderAlertsDescriber @zepumph
• SCENERY_PHET/PlateChargeNode @pixelzoom
• SCENERY/AnimatedPanZoomListener @jessegreenberg
• SUN/DemoScreenView @pixelzoom

Sims:

• acid-base-solutions @pixelzoom
• area-builder @jbphet
• area-model-common (area-model-*) @jonathanolson
• balancing-act @jbphet
• balancing-chemical-equations @pixelzoom
• beers-law-lab @pixelzoom
• bending-light @samreid
• blackbody-spectrum @jbphet
• build-a-molecule @jonathanolson
• capacitor-lab-basics @Denz1994
• charges-and-fields @jonathanolson
• circuit-construction-kit-common (circuit-construction-kit-*) @samreid
• collision-lab @veillette
• color-vision @chrisklus
• concentration @pixelzoom
• coulombs-law @zepumph
• curve-fitting @jonathanolson
• density-and-buoyancy-common (density, buoyancy) @jonathanolson
• energy-forms-and-changes @chrisklus
• energy-skate-park @jessegreenberg
• expression-exchange @jbphet
• equality-explorer @pixelzoom
• example-sim @pixelzoom
• faradays-law @jbphet
• fluid-pressure-and-flow @zepumph
• forces-and-motion-basics @jessegreenberg
• fractions-common (build-a-fraction, fraction-matcher, fractions-equality, fractions-intro, fractions-mixed-numbers, number-play) @jonathanolson
• friction @zepumph
• function-builder @pixelzoom
• function-builder-basics @pixelzoom
• gas-properties @pixelzoom
• gases-intro @pixelzoom
• gene-expression @jbphet
• graphing-lines @pixelzoom
• graphing-quadratics @pixelzoom
• gravity-and-orbits @jessegreenberg
• gravity-force-lab @zepumph
• gravity-force-lab-basics @zepumph
• hookes-law @pixelzoom
• inverse-square-law-common @zepumph
• isotopes-and-atomic-mass @jbphet
• least-squares-regression @jonathanolson
• make-a-ten @jonathanolson
• models-of-the-hydrogen-atom @pixelzoom
• molecule-polarity @pixelzoom
• molecule-shapes @jonathanolson
• molecules-and-light @jessegreenberg
• natural-selection @pixelzoom
• neuron @jbphet
• number-line-integers @jbphet
• number-play @chrisklus
• pendulum-lab @jonathanolson
• ph-scale @pixelzoom
• projectile-motion @jbphet
• proportion-playground @jonathanolson
• reactants-products-and-leftovers @pixelzoom
• simula-rasa @pixelzoom
• states-of-matter, states-of-matter-basics @jbphet
• unit-rates @pixelzoom
• vector-addition - uses both position & location! @pixelzoom
• wave-interference @samreid
• wave-on-a-string @jonathanolson