Closed arouinfar closed 1 year ago
Yes, I recall that the choice of "Africa - Conservative" is to accommodate the preferences of religiously devout countries or cultures.
I would take off one of the girls to maintain gender balance, either skater A or E since they share a lot of similarities. Skater E is preferred for her skirt.
The dog and goat will be the top 2 on my list.
We can keep the same order in the sim, interleaving female and male. Skater B will be my default.
@arouinfar - Yes, as I recall, we would have an "Africa" and an "Africa-Conservative" set.
Thanks for the feedback @solaolateju @kathy-phet.
@solaolateju we have the infrastructure to switch between any number of skater sets. A set includes 8 skaters, and the USA set has 6 humans and 2 animals. @solaolateju can you work with @mariahmaephet to adjust the skaters to create a conservative set?
Design meeting 11/17/22
Artwork TODOs:
@jessegreenberg @AgustinVallejo here are the latest assets: skaters.zip
Some notes
Please let me know if you have any questions!
@AgustinVallejo and @marlitas and I met today to review this issue and work required for publication.
@AgustinVallejo is going to work on putting the images in https://github.com/phetsims/energy-skate-park/issues/347#issuecomment-1320607871 into the sim in SkaterImages, SkaterNode, and the usage here: https://github.com/phetsims/energy-skate-park/blob/56636742762922a4061dc8a3c6a9e2e616bea7a0/js/common/view/EnergySkateParkControlPanel.js#L102-L108
@marlitas is going to work on a potentially improved way to pass a configuration for localization to PreferencesModel. Instead of regionAndCultureProperty
being a NumberProperty, it could be an EnumerationProperty that better models the supported region/culture values of the sim. An new option to PreferencesModel.localizationOptions could provide the the values of the Enumeration.
When these are done we can check in with @arouinfar before creating a dev test.
New character set is in place, locally... can't push til we decide how to handle the licensing of each image, as the current one reads like this:
"skater2_set2_left.png": {
"text": [
"Copyright 2002-2020 University of Colorado Boulder"
],
"projectURL": "https://phet.colorado.edu",
"license": "contact phethelp@colorado.edu",
"notes": "Created by Megan Lai"
},
Copyright is expired (?) and I'm not sure if it's the same designer still. Will probably meet with @jessegreenberg this week to fix that, along with the image alignment for the animals.
@AgustinVallejo the new skater artwork was created by Mariah Hermsmeyer.
Pushed! Ready for review
Thanks @AgustinVallejo @marlitas! The artwork itself looks good, but the animal alignment is off.
The red dot below the skateboard represents the model coordinate used for energy calculations. It should be centered below the skateboard, and the assets were formatted so that the skateboard is horizontally centered. With the current implementation, there is a mismatch between the visual and model positions of the animal skaters.
Oh yes! I wasn't clear on last comment. We still have that pending, should be done by tomorrow.
I switched regionAndCultureManager to take a CharacterSet Property instead of a NumberProperty. I wasn't able to figure out a way to initialize the value without null, which led to some type finnagling down the line. Not positive this is the best approach yet, so happy to hear other suggestions too. @jessegreenberg can you review?
@arouinfar All skater images should now be properly centered!
The artwork looks great, thanks @marlitas @AgustinVallejo!
Thanks @marlitas and @AgustinVallejo, these changes look great!
@marlitas here are some review suggestions:
regionAndCultureProperty
is no longer a NumberProperty?CharacterSet.imageSets
may be unused now in ESP (and everywhere), can that be removed?@ts-ignore
, what do you think?
Index: js/preferences/PreferencesModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/PreferencesModel.ts b/js/preferences/PreferencesModel.ts
--- a/js/preferences/PreferencesModel.ts (revision 31cbcb67dd4659201f5f5f6a8672518d252429ae)
+++ b/js/preferences/PreferencesModel.ts (date 1669826496608)
@@ -176,7 +176,7 @@
export type LocalizationModel = BaseModelType & {
// The selected character artwork to use when the sim supports culture and region switching.
- regionAndCultureProperty?: Property<CharacterSet>;
+ regionAndCultureProperty: Property<CharacterSet | null>;
localeProperty: Property<string>;
} & Required<LocalizationPreferencesOptions>;
@@ -301,7 +301,8 @@
}, options.inputOptions );
this.localizationModel = merge( {
- localeProperty: localeProperty
+ localeProperty: localeProperty,
+ regionAndCultureProperty: regionAndCultureManager.regionAndCultureProperty
}, options.localizationOptions );
if ( options.localizationOptions.regionAndCultureDescriptors.length > 0 ) {
@@ -310,8 +311,6 @@
regionAndCultureManager.regionAndCultureProperty.value = options.localizationOptions.regionAndCultureDescriptors[ 0 ].characterSet;
assert && assert( regionAndCultureManager.regionAndCultureProperty.value !== null, 'We have at least one descriptor, so regionAndCultureProperty should not be null.' );
-
- // @ts-ignore, regionAndCultureProperty.value cannot be null since there is at least one descriptor provided.
this.localizationModel.regionAndCultureProperty = regionAndCultureManager.regionAndCultureProperty;
}
Index: js/preferences/LocalizationPreferencesPanel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/LocalizationPreferencesPanel.ts b/js/preferences/LocalizationPreferencesPanel.ts
--- a/js/preferences/LocalizationPreferencesPanel.ts (revision 31cbcb67dd4659201f5f5f6a8672518d252429ae)
+++ b/js/preferences/LocalizationPreferencesPanel.ts (date 1669826803894)
@@ -44,8 +44,8 @@
this.disposeEmitter.addListener( () => localePanel.dispose() );
}
- // regionAndCultureProperty only gets set in PreferencesModel if there is at least one descriptor.
- if ( localizationModel.regionAndCultureProperty ) {
+ // regionAndCultureProperty value only gets set in PreferencesModel if there is at least one descriptor.
+ if ( localizationModel.regionAndCultureProperty.value ) {
const comboBox = new RegionAndCultureComboBox( localizationModel.regionAndCultureProperty, localizationModel.regionAndCultureDescriptors );
contentNode.addChild( comboBox );
this.disposeEmitter.addListener( () => comboBox.dispose() );
Index: js/preferences/RegionAndCultureComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/RegionAndCultureComboBox.ts b/js/preferences/RegionAndCultureComboBox.ts
--- a/js/preferences/RegionAndCultureComboBox.ts (revision 31cbcb67dd4659201f5f5f6a8672518d252429ae)
+++ b/js/preferences/RegionAndCultureComboBox.ts (date 1669826831115)
@@ -24,7 +24,7 @@
type SelfOptions = EmptySelfOptions;
type RegionAndCultureComboBoxOptions = SelfOptions & StrictOmit<ComboBoxOptions, 'labelNode' | 'tandem'>;
-class RegionAndCultureComboBox extends ComboBox<CharacterSet> {
+class RegionAndCultureComboBox extends ComboBox<CharacterSet | null> {
/**
* @param regionAndCultureProperty - Number indicating a selected region/culture. Map the value to particular set of
@@ -32,7 +32,7 @@
* @param regionAndCultureDescriptors - Collection of data used to create ComboBoxItems for each supported character set.
* @param [providedOptions?]
*/
- public constructor( regionAndCultureProperty: Property<CharacterSet>, regionAndCultureDescriptors: RegionAndCultureDescriptor[], providedOptions?: RegionAndCultureComboBoxOptions ) {
+ public constructor( regionAndCultureProperty: Property<CharacterSet | null>, regionAndCultureDescriptors: RegionAndCultureDescriptor[], providedOptions?: RegionAndCultureComboBoxOptions ) {
const options = optionize<RegionAndCultureComboBoxOptions, SelfOptions, ComboBoxOptions>()( {
Index: js/preferences/regionAndCultureManager.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/regionAndCultureManager.ts b/js/preferences/regionAndCultureManager.ts
--- a/js/preferences/regionAndCultureManager.ts (revision 31cbcb67dd4659201f5f5f6a8672518d252429ae)
+++ b/js/preferences/regionAndCultureManager.ts (date 1669826185382)
@@ -29,10 +29,10 @@
// An index describing the selected artwork for the simulation to display a particular region and culture. From this
// value the simulation can implement different artwork to match the selected region and culture.
- public readonly regionAndCultureProperty: Property<CharacterSet> | Property<null>;
+ public readonly regionAndCultureProperty: Property<CharacterSet | null>;
public constructor() {
- this.regionAndCultureProperty = new Property( null );
+ this.regionAndCultureProperty = new Property<CharacterSet | null>( null );
}
}
That patch was the typescript knowledge I needed. Thanks so much!
CharacterSet.imageSets may be unused now in ESP (and everywhere), can that be removed
@jessegreenberg I removed imageSets from the CharacterSet base class, but wasn't a fan that the base class didn't really have any typing to support subclasses or any of the type checking being used in preferences. I played around with these two options as well, but I'm not really a fan of either... what are your thoughts? I think the second option is my favorite so far, but there may be a typescript trick or convention that I'm not aware of that can help here.
arguments are type checked to be CharacterImageSets, and at least one is required, but unused array
public constructor( ....imageSets: [ CharacterImageSet, ...CharacterImageSet[] ] ) {
this.imageSets = imageSets;
}
ensures an empty class can't be instantiated, but makes subclass implementation a little awkward...
public constructor( ...imageSets: [ CharacterImageSet, ...CharacterImageSet[] ] ) {
// A CharacterSet subclass must contain at least one image set
this.imageSet1 = imageSets[ 0 ];
}
@marlitas yes I see what you mean. Another potential problem is that entries of CharacterSet may be specific to energy-skate-park. Here is another thought, what if we get rid of the "descriptor" idea and all of that info into CharacterSet. Then you can just pass all the character sets into PreferencesModel without needing a descriptor array.
Here is a patch. I think this simplifies the API. What do you think?
Yes that feels much cleaner! Thanks for helping out with that. I committed the changes and they are pushed up. Let me know if there's anything else, if not is this ready to close?
Great, thanks @marlitas I think this is in a much better place for publication. Ill reassign back to @arouinfar for the original issue which was more design focused. Or to close if the remaining needs for artwork are already documented in other issues.
Thanks everyone! There are still a few TODOs, so let's leave this issue open.
@AgustinVallejo @jessegreenberg @marlitas here are the assets for the updated headshots: updated-headshots.zip
I cropped the elephant's trunk a bit so that its head filled the space like the other animal headshots. Keeping the entire trunk in view requires shrinking the elephant down quite a bit, and the headshots are already fairly small.
@jessegreenberg and @AgustinVallejo, let me know if you need any help with this for preferences!
Headshots have been updated! @arouinfar
Looks great, thanks @AgustinVallejo!
@jessegreenberg has already created the infrastructure to switch between skater sets in Energy Skate Park. The current implementation in master uses stand-in artwork and region names. I've created an updated asset file with the USA skater set (including the cat and dog), and I'm working on getting the African set ready to pass off to @jessegreenberg.
Here's what the assets currently look like:
Questions: