Convert sims to new Region and Culture approach

[pixelzoom]

This is a sub-task of #953.

graphing-lines and graphing-slope-intercept can serve as examples.

Steps to convert:

1. Create {repo}-images.json.

2. Run grunt update to create {Repo}Images.js, a set of LocalizedImageProperty.

3. For each LocalizedImageProperty, use it as the argument a scenery Image (or sim-specific subclass of scenery Image).

4. Delete localizationOptions.portrayals option to PreferencesModel, typically in {repo}-main.ts.

5. Delete all sim-specific subclasses of RegionAndCulturePortrayal.

6. Delete or modify other sim-specific classes related to the Region and Culture feature. E.g. KickerImage.ts, KickerNode.ts, BoxPlayerImages.js, BoxPlayerCharacters.js, ...

Sims to convert:

• area-model-algebra + area-model-multiplication - see Convert to new Region and Culture approach area-model-multiplication#16
• arithmetic - see Convert to new approach for Region and Culture arithmetic#215
• balancing-act - see Convert to new Region and Culture approach balancing-act#155
• center-and-variability + mean-share-and-balance (soccer-common) - see Convert to new approach for Region and Culture center-and-variability#617
• energy-skate-park + energy-skate-park-basics - see Convert to new Region and Culture approach energy-skate-park#382
• forces-and-motion-basics - see Convert to the new Region and Culture approach forces-and-motion-basics#308
• number-line-distance + number-line-integers

I'll set up a time with @Luisav1 to do one of these conversions together. We should consider doing center-and-variablility first, because it's blocking phetsims/center-and-variability#615.

[pixelzoom]

@Luisav1 and I will be pairing at 10AM MT today.

arithmetic looks like a good place to start. For the above steps:

1. Create arithmetic-images.json

2. Run grunt update to create ArithmeticImages.ts

3. Modify LevelSelectionNode and ArithmeticView to take LocalizedImageProperty[].

4. Delete preferencesModel in arithmetic-main.js

5. Delete BoxPlayerPortrayal and its subclasses.

6. Delete BoxPlayerImages and BoxPlayerCharacters.

[pixelzoom]

@Luisav1 and I completed arithmetic, see phetsims/arithmetic@970fe8b.

I'll work on center-and-variability + mean-share-and-balance (soccer-common) to expedite a fix for phetsims/center-and-variability#615 (CAV migration rules).

@Luisav1 let me know if you have any questions, encounter problems, want review, etc.

[pixelzoom]

I'll work on center-and-variability + mean-share-and-balance (soccer-common) to expedite a fix for phetsims/center-and-variability#615 (CAV migration processors).

I was able to make progress on CAV migration processors by uninstrumenting the elements that have been deprecated, see phetsims/center-and-variability#615 (comment). Since I need to get back to other higher-priority work, I'm going to pause working on CAV for now. I created phetsims/center-and-variability#617, to capture some work and thoughts.

[Luisav1]

@pixelzoom In the above commits I converted the code in number-line-distance and number-line-integers to use the new R&C approach. The transition went smoothly but I found some points where clarification is needed.

Regarding image naming conventions, I noticed slight variations in the names of Navbar and Home icon images across all simulations.

• Should we go with NavIcon or NavbarIcon as the suffix for these images to have a consistent convention? I saw more results come up for the NavbarIcon suffix.
• The same question for the ScreenHomeIcon versus the HomeIcon suffix. I saw about equal results for both.

In both sims as well, the icons in the Explore screen are slightly offset now.

• In NLD the offset is in the y-direction with varying degrees among portrayals. For example, the USA icon appears higher now, while the Oceania icon remains almost unchanged. Previously, we used an AlignGroup. Should we still use an AlignGroup or find other alignment mechanisms to address this inconsistency?
  After:
  
  Before:
  
• In NLI there is an offset is in the x-direction across different region portrayals. I'm unsure what caused the offset since the images were previously using a node that swapped images based on the regionAndCulturePortrayalProperty.

Lastly, in NLI, there's an imageSelectionFunction in ElevationSceneView that references images based on their index number in an array. This dependency on image order existed before the R&C feature. Should there be an alternative approach that reduces this dependency?

[pixelzoom]

Should we go with NavIcon or NavbarIcon as the suffix for these images to have a consistent convention? I saw more results come up for the NavbarIcon suffix. The same question for the ScreenHomeIcon versus the HomeIcon suffix. I saw about equal results for both.

I could use HomeScreenIcon and NavigationBarIcon (or NavbarIcon would be fine) because the ScreenOptions are homeScreenIcon and navigtionBarIcon. And yes, being consistent is a good idea.

In NLD the offset is in the y-direction with varying degrees ... Should we still use an AlignGroup or find other alignment mechanisms to address this inconsistency?

The easist solution is probably to continue to use AlignGroup + AlignBox. The other (more complicated) alternatives are (1) add a boundsProperty listener to the Image that will reposition when the bounds changes, or (2) rewrite the layout using scenery GridBox.

In NLI there is an offset is in the x-direction across different region portrayals. I'm unsure what caused the offset since the images were previously using a node that swapped images based on the regionAndCulturePortrayalProperty.

Probably also solvable with AlignGroup + AlignBox. If that doesn't work, fill me in on more specifics.

Lastly, in NLI, there's an imageSelectionFunction in ElevationSceneView that references images based on their index number in an array. This dependency on image order existed before the R&C feature. Should there be an alternative approach that reduces this dependency?

I agree, that looks brittle. But that also looks like code that you probably don't want to touch. If it's working, I would leave it alone. Consider creating a separate GitHub issue, noting the dependency and brittleness, and assign to @jbphet -- it looks like he created imageSelectionFunction in the initial commit phetsims/number-line-integers@d855bd4.

[Luisav1]

In the above commits I've renamed the image suffixes. and tried aligning NLD images with the AlignGroup + AlignBox method but it doesn't exactly seem to be working.

NLD
Both icons (in the bottom-left corner and in the legend) use a function that creates an AlignBox with option yAlign: 'bottom' but it's still aligning them at the top in the legend icon and at the center in the bottom-left icon. I also tried setting preferredHeight: 40, the tallest personImage height.

NLI
I tried an AlignGroup + AlignBox the same way as was previously done in NLD on, using personInWaterRepresentation as the Image node as below:

 const legendAlignGroup = new AlignGroup();
    const legendAlignBox = legendAlignGroup.createBox(
      new Image( NumberLineIntegersImages.personInWaterImageProperty, { maxHeight: 40, center: new Vector2( 3, 5 ) } ),
      { xAlign: 'center' } );
    const personInWaterRepresentation = new Node();
    personInWaterRepresentation.addChild( legendAlignBox );

But the offset is much more noticeable than before:
.
Previously, this is how the swimming images were placed and swapped, with the explorerNodes being passed as the imageList array.

const swimmingImages = explorerSets.map( set => new Image( set.swimming,
      {
        visibleProperty: DerivedProperty.valueEqualsConstant( regionAdnCulturePortrayalProperty, set ),
        maxHeight: 40,
        center: new Vector2( 3, 5 )
      } ) );

    const flyingNode = new Node( { children: flyingImages } );
    const hikingNode = new Node( { children: hikingImages } );
    const swimmingNode = new Node( { children: swimmingImages } );

    /**
     * @public
     * @type {Node[]}
     */
    this.explorerNodes = [ flyingNode, hikingNode, swimmingNode ];

@pixelzoom might you know why the AlignGroup + AlignBox approach is not working? Should we try the other boundsProperty listener or rewriting the layout with a scenery GridBox approach?

[pixelzoom]

I confirmed that bounds for Image + LocalizedImageProperty is working properly by adding this to the end of DistanceSceneView constructor:

    const personImage = new Image( NumberLineDistanceImages.personImageProperty, { scale: 0.2 } );
    const rectangle = new Rectangle( 0, 0, 10, 80, { fill: 'black' } );
    const hBox = new HBox( {
      children: [ rectangle, personImage ],
      spacing: 5,
      left: 20,
      top: 100
    } );
    this.addChild( hBox );

While switching region, the person remains vertically centered. For example:

[pixelzoom]

NLDBaseView was such a tangled mess that it was difficult to see what was related to the NDL layout problem. So the first major thing that I've done is to factor out PointControllerLegendNode in phetsims/number-line-distance@69a7ddc, which is this this UI component:

It is now in TypeScript, and a number of other problems/oddities have been corrected. Now I'll work on identifying and fixing the layout problem.

[pixelzoom]

phetsims/number-line-distance@499b578 fixes the problem with PointControllerLegendNode. There was an unnecessary invisible Rectangle that was being added behind each icon, and that was preventing the HBox from vertically centering the icons.

[pixelzoom]

phetsims/number-line-distance@67a8757 fixes the layout of items in the toolbox.

[pixelzoom]

Re this item for NLD:

For the person on the number line, their feet need to remain on the sidewalk. Once the relevant code is identified, this can likely be accomplished with a boundsProperty listener or ManualConstraint.

I'm stuck here.

The first problem is that the toolbox is not using using the PhET creator pattern. It's just a rectangle, and something else entirely (DistancePointControllerNode) renders the images on top of the rectange.

The next problem is in DistancePointControllerNode. Both the house and person require their origin to be at their center, so that they appear to be centered in the toolbox:

    image.localBoundsProperty.link( () => {
      image.centerX = 0;
      image.centerY = 0;
      image.touchArea = image.localBounds.dilated( IMAGE_DILATION / image.getScaleVector().x );
    } );

Having the origin at the center make it impossible to keep the feet at a constant position on the sidewalk.

PointController model element handles the position, and knows nothing about the changing height of the person image. And in DistanceSceneModel, there is this awful specification of where the person's origin is relative to the sidewalk (the 3rd arg here):

      new DistancePointController(
        numberLine,
        lockingBounds,
        SIDEWALK_OFFSET_FROM_NUMBERLINE + SIDEWALK_HEIGHT / 2 - 25,
        0.5
      ),

Since I can't get the feet to be at constant y position on the sidewalk... I changed 27 to 25 in the above, to split the difference for tall vs short people. Here's what it looks like:

I'm going to stop here, and toss this back to the responsible developer. @jbphet is this good enough?

[pixelzoom]

I have two options for addressing the "feet on the sidewalk" problem in NLD. I'd like to discuss with @Luisav1 @marlitas and @jbphet after today's standup meeting.

Option 1: Person is vertically centered in the toolbox. Feet are always on the sidewalk, but not always in the same vertical position.
See https://phet-dev.colorado.edu/html/number-line-distance/1.2.0-dev.1/phet/number-line-distance_all_phet.html

Options 2: Person is not vertically centered in the toolbox. Feet are always on the sidewalk, and always in the same vertical position. This is the same behavior as published 1.1.3. This version is not checked in; I'll demonstrate.

[pixelzoom]

Mini design meeting with @amanda-phet @Luisav1 @marlitas and @jbphet, to discuss the 2 options for NLD in #958 (comment). The consensus was that Option 1 is preferred; it's more important (and noticeable) for the person to be vertically centered in the toolbox, the person is still on the sidewalk, and no one is likely to notice that their feet move up/down on the sidewalk when R&C is changed.

If anyone changes their mind and wants Option 2, here's a patch that does that:

patch

Subject: [PATCH] rename point controllers, https://github.com/phetsims/joist/issues/958
---
Index: js/explore/view/DistanceSceneView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/explore/view/DistanceSceneView.js b/js/explore/view/DistanceSceneView.js
--- a/js/explore/view/DistanceSceneView.js	(revision 1fc4a77ff697b986df5d72f462f097437b7b315f)
+++ b/js/explore/view/DistanceSceneView.js	(date 1711467227122)
@@ -15,7 +15,7 @@
 import numberLineDistance from '../../numberLineDistance.js';
 import NumberLineDistanceImages from '../../NumberLineDistanceImages.js';
 import NumberLineDistanceStrings from '../../NumberLineDistanceStrings.js';
-import DistancePointControllerNode from './DistancePointControllerNode.js';
+import { HousePointControllerNode, PersonPointControllerNode } from './DistancePointControllerNode.js';
 import NLDSceneView from './NLDSceneView.js';
 
 const eastStringProperty = NumberLineDistanceStrings.symbol.eastStringProperty;
@@ -90,8 +90,8 @@
       personPointControllerImage.mouseArea = localBounds.dilated( 5 / personPointControllerImage.getScaleVector().x );
     } );
 
-    const housePointControllerNode = new DistancePointControllerNode( model.pointControllerOne, housePointControllerImage );
-    const personPointControllerNode = new DistancePointControllerNode( model.pointControllerTwo, personPointControllerImage );
+    const housePointControllerNode = new HousePointControllerNode( model.pointControllerOne, housePointControllerImage );
+    const personPointControllerNode = new PersonPointControllerNode( model.pointControllerTwo, personPointControllerImage );
 
     // Point controllers have different parent Nodes so that the person is always in front of the house.
     const pointControllersLayer = new Node( {
Index: js/explore/model/DistanceSceneModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/explore/model/DistanceSceneModel.js b/js/explore/model/DistanceSceneModel.js
--- a/js/explore/model/DistanceSceneModel.js	(revision 1fc4a77ff697b986df5d72f462f097437b7b315f)
+++ b/js/explore/model/DistanceSceneModel.js	(date 1711466344006)
@@ -59,7 +59,7 @@
       new DistancePointController(
         numberLine,
         lockingBounds,
-        SIDEWALK_OFFSET_FROM_NUMBERLINE + SIDEWALK_HEIGHT / 2 - 25,
+        SIDEWALK_OFFSET_FROM_NUMBERLINE + SIDEWALK_HEIGHT / 2 - 28,
         0.5
       ),
       tandem
Index: js/explore/view/DistancePointControllerNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/explore/view/DistancePointControllerNode.js b/js/explore/view/DistancePointControllerNode.ts
rename from js/explore/view/DistancePointControllerNode.js
rename to js/explore/view/DistancePointControllerNode.ts
--- a/js/explore/view/DistancePointControllerNode.js	(revision 1fc4a77ff697b986df5d72f462f097437b7b315f)
+++ b/js/explore/view/DistancePointControllerNode.ts	(date 1711468148159)
@@ -1,40 +1,62 @@
 // Copyright 2021-2024, University of Colorado Boulder
 
 /**
- * A point controller node that is meant for use in the distance scene.
+ * DistancePointControllerNode is the base class for point controllers the Distance scene of the Explore screen.
+ * HousePointControllerNode is the subclass used for the house representation.
+ * PersonPointControllerNode is the subclass used for the person representation.
  *
  * @author Saurabh Totey
  */
 
 import PointControllerNode from '../../../../number-line-common/js/common/view/PointControllerNode.js';
-import { Node } from '../../../../scenery/js/imports.js';
+import { Image, Node } from '../../../../scenery/js/imports.js';
 import numberLineDistance from '../../numberLineDistance.js';
+import PointController from '../../../../number-line-common/js/common/model/PointController.js';
 
 // constants
 const IMAGE_DILATION = 20;
 
-class DistancePointControllerNode extends PointControllerNode {
+export default class DistancePointControllerNode extends PointControllerNode {
+
+  protected constructor( pointController: PointController, image: Image ) {
+    super( pointController, {
+      connectorLine: false,
+      node: new Node( { children: [ image ] } )
+    } );
+  }
 
-  /**
-   * @param {PointController} pointController
-   * @param {Image} image - is mutated
-   * @public
-   */
-  constructor( pointController, image ) {
+}
 
-    image.localBoundsProperty.link( () => {
-      image.centerX = 0;
-      image.centerY = 0;
-      image.touchArea = image.localBounds.dilated( IMAGE_DILATION / image.getScaleVector().x );
-    } );
+export class HousePointControllerNode extends DistancePointControllerNode {
+
+  public constructor( pointController: PointController, image: Image ) {
+
+    // The house is a static image, with origin at its center.
+    image.centerX = 0;
+    image.centerY = 0;
+    image.touchArea = image.localBounds.dilated( IMAGE_DILATION / image.getScaleVector().x );
+
+    super( pointController, image );
+  }
+}
+
+export class PersonPointControllerNode extends DistancePointControllerNode {
+
+  public constructor( pointController: PointController, image: Image ) {
 
-    super( pointController, {
-      connectorLine: false,
-      node: new Node( { children: [ image ] } )
+    // The person is a localized image. Its origin is at a fixed distance from the person's feet (the bottom of
+    // the image) so that feet will remain in the same position on the sidewalk when changing Region and Culture.
+    // The compromise for this is that the feet will also remain in the same position in the toolbox, and the person
+    // will therefore not be vertically centered in the toolbox.
+    image.localBoundsProperty.link( () => {
+      image.x = -image.width / 2;
+      image.y = -image.height + 44;
+      image.touchArea = image.localBounds.dilated( IMAGE_DILATION / image.getScaleVector().x );
     } );
-  }
 
+    super( pointController, image );
+  }
 }
 
-numberLineDistance.register( 'DistancePointControllerNode', DistancePointControllerNode );
-export default DistancePointControllerNode;
\ No newline at end of file
+
+numberLineDistance.register( 'DistancePointControllerNode', DistancePointControllerNode );
\ No newline at end of file

I also inspected NLI, and it still looks fine, unaffected by changes to NLD.

So we're done with conversion of NLD and NLI.

[jbphet]

As @pixelzoom mentioned above, we reviewed this in a Zoom meeting with @amanda-phet and decided which tradeoff to go with. Unassigning myself.

[pixelzoom]

Looks like we're done here, so closing. Any remaining loose ends are being tracked in sim-specific issues.