Image localization and regionAndCulture

[jonathanolson]

Issue for tracking commits and review. Implementing @pixelzoom's vision for regionAndCulture.

[jonathanolson]

Initial implementation above (on main).

I've added a stub that will hook into any regionAndCulturePortrayalProperty (so that it is kept in-sync with the regionAndCultureProperty that was added). Presumably after things are ported over, we won't need that other Property.

I made the decision to only instrument the main regionAndCultureProperty if it has multiple valid values (will be defined/featured similarly to regionAndCulturePortrayalProperty).

Right now it is a fairly independent system from the current portrayal system. Demo in wilder.

[jonathanolson]

I'll review and refine soon with @pixelzoom. @marlitas I'd be curious to get your opinion on the above commits as well (and whether you have any concerns or misgivings about them).

[marlitas]

I've added a stub that will hook into any regionAndCulturePortrayalProperty (so that it is kept in-sync with the regionAndCultureProperty that was added). Presumably after things are ported over, we won't need that other Property.

I'm glad I re-read this because the bidirectional link between regionAndCulturePortrayalProperty and regionAndCultureProperty was very concerning. I am adding a TODO referencing this issue (can be moved elsewhere if needed) so that we don't forget to adjust.

I was also curious about the change set in initialize-globals:

- validValues: packageSimFeatures.supportedRegionsAndCultures || [ null ],
- defaultValue: packageSimFeatures.supportedRegionsAndCultures?.[ 0 ] ?
                    packageSimFeatures.supportedRegionsAndCultures[ 0 ] : null
+ defaultValue: 'usa'

• Is there a reason validValues was removed?
• The default value is not always 'usa'. For example, in Number Line Integers the default values is 'multi'. 'usa' should not be hard coded in.

[pixelzoom]

@jonathanolson and I paired on this for 1.75 hours this morning, commits above. I'll review joist/js/i18n/ and start transitioning graphing-lines.

[pixelzoom]

In https://github.com/phetsims/graphing-lines/issues/143, I moved graphing-lines and graphing-slope-intercept to the new common code. Here's a list of the things that I ran into.

• [x] (1) The output file from grunt modulify was not in pascal case. It should be GraphingLinesImages.ts (like GraphingLinesStrings.ts) but was instead graphingLinesImages.ts. I fixed that in https://github.com/phetsims/chipper/commit/14bb55331a3a8164386d1c3225ea1d00225e90ab.

• [x] (2) The entries in GraphingLinesImages.ts did not have "ImageProperty" suffixes on their names. For example, "level1" instead of "level1ImageProperty". This is analagous to strings, were we have slopeStringProperty etc. I fixed that in https://github.com/phetsims/chipper/commit/14bb55331a3a8164386d1c3225ea1d00225e90ab.

• [x] (3) The Localization tab does not appear in Preferences unless you run with &locales=*. The Localization tab, and Region and Culture combo box, need to appear in all version of the sim if package.json contains supportedRegionsAndCultures. I did not investigate fixing this.

• [x] (4) Image files must have unique basenames. So most of the work was renaming and reorganizing files. I think this is OK. Let's come up with a standard for naming and propose it to designers. For now, I use the RegionAndCulture values as a prefix, e.g. "africaLevel1_svg.ts". And I used "africaModest" and "latinAmerica", instead of "africa-modest" and "latin-america", to be consistent with RegionAndCulture type values.

• [x] (5) graphing-slope-intercept was expecting to get its "usa" images from graphing-lines, and that's not supported. My solution was to copy the "usa" images to graphing-slope-intercept.

• [x] (6) The term "region and culture" is really problematic in code. We're ending up long, unreadable, inconsistent code names, e.g. supportedRegionsAndCultures (plural "Regions") in package.json and availableRuntimeRegionAndCultures (singular "Region") in regionAndCultureProperty. I wish we could come up with a different name in code. Maybe we could just shorten it to "region" or "culture".

@jonathanolson let's discuss 3, 4, 5, and 6.

[pixelzoom]

(3) The Localization tab does not appear in Preferences unless you run with &locales=*. The Localization tab, and Region and Culture combo box, need to appear in all version of the sim if package.json contains supportedRegionsAndCultures. I did not investigate fixing this.

I addressed this in https://github.com/phetsims/joist/commit/fcddee0598c9c3feddfbdd266e5fd36fc1df1e9f. The Localization tab appears in Preferences if its appropriate to show either regionAndCultureComboBox or localePanel.

[pixelzoom]

@jonathanolson and I met to complete the items in https://github.com/phetsims/joist/issues/953#issuecomment-1977737055.

• [ ] (4) Image files must have unique basenames. So most of the work was renaming and reorganizing files. I think this is OK. Let's come up with a standard for naming and propose it to designers. For now, I use the RegionAndCulture values as a prefix, e.g. "africaLevel1_svg.ts". And I used "africaModest" and "latinAmerica", instead of "africa-modest" and "latin-america", to be consistent with RegionAndCulture type values.

Having unique filenames (specifically the basename of a complete file path) is a requirement of import throughout the entire PhET code base. When we're manually writing code, we have the option to manually resolve filename collisions by changing import names, and manually disabling ESLint. Adding that option for code that is generated (as is the case here) would add unnecessary cost and complexity. So unique filenames is a requirement for this feature; if there's a name collision, rename files.

As a convention for naming image files, @jonathanolson and I recommend (but the implementation does not require):

• Use region-and-culture values as a suffix, so that the case will be the same. E.g. for "africa", name the file "africaLevel1.svg", not "level1Africa.svg".
• Use region-and-culture values exactly as they would appear in package.json. E.g. for "latinAmerica", name the file "latinAmericaLevel1.svg", not "latin-americasLevel1.svg".

I'll discuss these requirements and conventions with @amanda-phet, @marlitas, and @Luisav1.

• [ ] (5) graphing-slope-intercept was expecting to get its "usa" images from graphing-lines, and that's not supported. My solution was to copy the "usa" images to graphing-slope-intercept.

My bad -- using images from another is supported. I made the appropriate changes for graphing-slope-intercept in https://github.com/phetsims/graphing-slope-intercept/commit/7d7959b65c797edb958897a0688ee9b5df7ab137, where the 'usa' images come from graphing-lines.

• [ ] (6) The term "region and culture" is really problematic in code. We're ending up long, unreadable, inconsistent code names, e.g. supportedRegionsAndCultures (plural "Regions") in package.json and availableRuntimeRegionAndCultures (singular "Region") in regionAndCultureProperty. I wish we could come up with a different name in code. Maybe we could just shorten it to "region" or "culture".

@jonathanolson and I worked on tightening up the naming, and that is reflected in https://github.com/phetsims/joist/commit/ad9daec5ff642b8aba6617d8ded552ca85dbce98. In general, we avoided pluralizing "regionAndCulture", and preferred names like supportedRegionAndCultureValues, since that's where the inconsistencies were creeping in.

We decided against trying to use a term other than "region and culture" in the code. It's too established in the PhET vernacular, appears in the UI ("Region and Culture" combo box), and appears in PhET-iO (regionAndCultureProperty).

[pixelzoom]

Remaining work, to be completed in this order:

• [x] Review requirements and conventions for naming image files with @amanda-phet @marlitas @Luisav1.

• [x] https://github.com/phetsims/joist/issues/954.

• [x] https://github.com/phetsims/joist/issues/958

• [x] Delete vestiges of the old implementation. Address remaining TODOs that reference this issue. Delete RegionAndCulturePortrayal.ts.

[pixelzoom]

• [ ] Review requirements and conventions for naming image files with @amanda-phet @marlitas @Luisav1.

To be reviewed in 3/7/2024 design meeting.

[pixelzoom]

From 3/7/2024 design meeting with @amanda-phet @kathy-phet @Luisav1 @jbphet @jonathanolson. FYI @marlitas.

We reviewed the requirements and conventions for naming image files. Everyone was onboard.

@amanda-phet also pointed out that when the numbering of things in the sim has meaning (e.g. level1, kicker1,...) we should be consistent and use the same numbering in the image filenames. soccer-common is an example where this has not been done, and it's confusing.

[pixelzoom]

@amanda-phet and I discussed an additonal convention for image files related to R&C. We will put all such files under images/localized/. Some sims have a large number of images, and it will be useful to differentiate between images that are localized and those that are not. If someone wants to create additional subdirectories, like images/localized/people/ and images/localized/object/, they are free to do so.

EDIT: I put the name of the subdirectory to a vote in Slack#dev-public, and images/localized/ was preferred over images/regionAndCulture.

Reorganize the images in these repos:

• [x] area-model-algebra
• [x] area-model-multiplication
• [x] arithmetic
• [x] balancing-act
• [x] energy-skate-park (energy-skate-park-basics)
• [x] forces-and-motion-basics
• [x] graphing-lines
• [x] graphing-slope-intercept
• [x] number-line-distance
• [x] number-line-integers
• [x] soccer-common (center-and-variability, mean-share-and-balance)
• [x] wilder

[pixelzoom]

Renaming of image files is done, with the exception of soccer-common. The 'usa' images for soccer-common have a problem (see https://github.com/phetsims/soccer-common/issues/13) so I'll need to circle back to rename those files.

I followed convoluted path to get to the final naming convention.

Putting all files in images/localization/ turned out to have some problems:

• odd sorting of 'africa' and 'africaModest' files, where they were interleaved
• lots of files in 1 directory and a huge license.json files

So I ended up leaving files in a directory per region: images/africa/, images/africaModest, images/asia, etc. These seems fine, and serves the same purpose as the proposed localized/ directory - to differentiate between localized and non-localized images.

[pixelzoom]

Here's the bash script that I used to rename image files. I fixed up license.json and RegionAndCulturePortrayal subclasses using global string replacement in WebStorm.

#!/bin/bash

# First command line argument is the prefix to be added to each file.
PREFIX=${1}

# Allow patterns that match no files, so we can iterate over both .png and .svg
shopt -s nullglob

for filename in *.{png,svg}; do

 # Convert the first character of the filename to upper case.
 UPPER=`echo ${filename} | awk '{ print toupper( substr( $0, 1, 1 ) ) substr( $0, 2 ); }'`

 # Add the prefix.
 NEW_FILENAME=${PREFIX}${UPPER}

 # Rename the file, preserving git history.
 git mv ${filename} ${NEW_FILENAME}
done

[pixelzoom]

Remaining work:

• [x] https://github.com/phetsims/joist/issues/958

• [x] Delete vestiges of the old implementation. Address remaining TODOs that reference this issue. Delete RegionAndCulturePortrayal.ts.

[pixelzoom]

All sims have been converted to the new approach. So I removed the old implementation from common code in the above commits.

There are references to regionAndCulturePortrayalProperty and preferences.regionAndCulturePortrayals in getOverridingQueryParameters.ts. The code is poorly documented, so it's unclear whether any changes need to be made. I've reopened https://github.com/phetsims/phet-io-wrappers/issues/637 and asked @zepumph for clarification (and documentation).

[pixelzoom]

The last bit of cleanup (getOverridingQueryParameters.ts) will be handled by @zepumph in https://github.com/phetsims/phet-io-wrappers/issues/637. So closing this issue.