Convert sims to new Region and Culture approach

[pixelzoom]

This is a sub-task of https://github.com/phetsims/joist/issues/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:

• [x] area-model-algebra + area-model-multiplication - see https://github.com/phetsims/area-model-multiplication/issues/16
• [x] arithmetic - see https://github.com/phetsims/arithmetic/issues/215
• [x] balancing-act - see https://github.com/phetsims/balancing-act/issues/155
• [x] center-and-variability + mean-share-and-balance (soccer-common) - see https://github.com/phetsims/center-and-variability/issues/617
• [x] energy-skate-park + energy-skate-park-basics - see https://github.com/phetsims/energy-skate-park/issues/382
• [x] forces-and-motion-basics - see https://github.com/phetsims/forces-and-motion-basics/issues/308
• [x] 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 https://github.com/phetsims/center-and-variability/issues/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 https://github.com/phetsims/arithmetic/commit/970fe8bea109c48197d4b409747599138393ce1c.

I'll work on center-and-variability + mean-share-and-balance (soccer-common) to expedite a fix for https://github.com/phetsims/center-and-variability/issues/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 https://github.com/phetsims/center-and-variability/issues/615 (CAV migration processors).

I was able to make progress on CAV migration processors by uninstrumenting the elements that have been deprecated, see https://github.com/phetsims/center-and-variability/issues/615#issuecomment-1992267448. Since I need to get back to other higher-priority work, I'm going to pause working on CAV for now. I created https://github.com/phetsims/center-and-variability/issues/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 https://github.com/phetsims/number-line-integers/commit/d855bd4d66f2bf2a49ecafacbccce0db708c8f61.

[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]

@Luisav1 I'll have a look, but probably won't get to it until Monday 3/25.

[pixelzoom]

@Luisav1 I'm wishing that we had created a sim-specific issue for conversion of NLD and NLI. This is going to end up being a long issue, with lots of unrelated comments. For other sims, I think we should consider creating sim-specific issues for conversion. That said...

The AlignGroups in DistanceSceneView.ts were totally ineffective -- each AlignGroup was applied to only 1 Node. AlignGroup (and AlignBox) is for giving multiple Nodes the same effective dimensions and alignment within those dimensions. So I've removed those AlignGroups in the above commits.

The problem for this part of NLI:

... is in NLDBaseView.js. firstNodeHBox and secondNodeHBox are HBoxes, and they default to align: 'center'. So when a child's bounds change, the layout should update to vertically center all children. That is not happening for some reason. And it's most noticable when switching to 'usa' from one of the other regions.

That's as far as I got. I'll continue to investigate on Monday.

[pixelzoom]

So when a child's bounds change, the layout should update to vertically center all children. That is not happening for some reason. And it's most noticable when switching to 'usa' from one of the other regions.

Oh wait... The changes that I made above did fix the problem. (I didn't have my transpiler running - doh!). The person is now vertically centered in both of these situations:

@Luisav1 please review and confirm.

[pixelzoom]

In NLI, I'm not seeing the problem reported in https://github.com/phetsims/joist/issues/958#issuecomment-2013899948. What's currently in main looks fine to me (see below) and continues to look fine when I switch regions. @Luisav1 Please clarify -- is there still a problem?

[pixelzoom]

@Luisav1 FYI, I took care of forces-and-motion-basics in https://github.com/phetsims/forces-and-motion-basics/issues/308.

[Luisav1]

Thanks for looking into it @pixelzoom.

The issue in NLD is that the people were previouslly vertically aligned through their feet. Even though the person's height changed, their feet were all at the same position but now they're all vertically centered. It's not am issue to me, though I more so thought they probably should be kept the same compared to before.

In NLI, the issue is not seen since the changes were in this new code patch.

 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 );

The issue there was similar to NLD where instead the people are now aligned by their hands all being in the same position in the legend box. Whereas before, they were aligned horizontally on the center of their images.

Sorry I wasn't clear earlier on this. Thanks for finding the culprit in NLD though. I changed the 'center' alignment to 'bottom' though and that still didn't bottom-align them through their feet.

[pixelzoom]

@Luisav1 @marlitas @jbphet @pixelzoom met about remaining issues with NLD and NLI. Summary:

NLI:

• Vertical centering for the toolbox and pointControllerLegend (in NLDBaseView) would be fine. But the people seem to be top aligned for some reason.
• 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.

NLD:

• Alignment in the toolbox looks good, but should be re-tested when NLI problems are resolved.

In my court to investigate.

[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 https://github.com/phetsims/number-line-distance/commit/69a7ddcd2fe8705533ddafb5176cf15e368b5c2e, 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]

https://github.com/phetsims/number-line-distance/commit/499b5780cf0b1e5af77f2832e9d739bcb7d8fa23 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]

https://github.com/phetsims/number-line-distance/commit/67a87573206b2891d68445bc9853cc820b75ce59 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 https://github.com/phetsims/joist/issues/958#issuecomment-2020594391. 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

```diff 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.