Where should ProfileColorProperties be defined? #1257
Comments
I'm still a fan of putting them in one .js file. That makes it easy for non-developers to find the relevant code, if needed. One could argue that it makes it a tad easier for developers too. My sims will have a *Colors.js file. |
|
To understand this better, I moved a color that is only used once from GasPropertiesColors.js to its usage site. I also added a Tandem, so we could see how that pattern could work. This is not working code (since it uses Tandem.COLORS which won't exist until #1251 is further along), but it is an exploratory idea to understand the pros and cons of a pattern: Index: js/common/view/LidNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/LidNode.js b/js/common/view/LidNode.js
--- a/js/common/view/LidNode.js (revision d6584afc1f68c2ff62e8465bfe6cfc03e753481c)
+++ b/js/common/view/LidNode.js (date 1627452184280)
@@ -12,6 +12,8 @@
import HandleNode from '../../../../scenery-phet/js/HandleNode.js';
import Node from '../../../../scenery/js/nodes/Node.js';
import Rectangle from '../../../../scenery/js/nodes/Rectangle.js';
+import ProfileColorProperty from '../../../../scenery/js/util/ProfileColorProperty.js';
+import Tandem from '../../../../tandem/js/Tandem.js';
import gasProperties from '../../gasProperties.js';
import GasPropertiesColors from '../GasPropertiesColors.js';
import HoldConstant from '../model/HoldConstant.js';
@@ -20,6 +22,14 @@
const HANDLE_ATTACHMENT_LINE_WIDTH = 1;
const HANDLE_RIGHT_INSET = 3;
+// base of the lid, the part that the handle attaches to
+const lidBaseFillProperty = new ProfileColorProperty( 'lidBaseFill', {
+ default: 'rgb( 180, 180, 180 )',
+ projector: 'rgb( 128, 128, 128 )'
+}, {
+ tandem: Tandem.COLORS.createTandem( 'lidBaseFillProperty' )
+} );
+
class LidNode extends Node {
/**
@@ -37,7 +47,7 @@
}, options );
const baseNode = new Rectangle( 0, 0, options.baseWidth, options.baseHeight, {
- fill: GasPropertiesColors.lidBaseFillProperty,
+ fill: lidBaseFillProperty,
left: 0,
bottom: 0
} );
Index: js/common/GasPropertiesColors.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/GasPropertiesColors.js b/js/common/GasPropertiesColors.js
--- a/js/common/GasPropertiesColors.js (revision d6584afc1f68c2ff62e8465bfe6cfc03e753481c)
+++ b/js/common/GasPropertiesColors.js (date 1627452045551)
@@ -75,12 +75,6 @@
projector: 'rgb( 220, 220, 220 )'
} ),
- // base of the lid, the part that the handle attaches to
- lidBaseFillProperty: new ProfileColorProperty( 'lidBaseFill', {
- default: 'rgb( 180, 180, 180 )',
- projector: 'rgb( 128, 128, 128 )'
- } ),
-
// dimensional arrow that appears below the container
sizeArrowColorProperty: new ProfileColorProperty( 'sizeArrowColor', {
default: 'white',
Note that by using
I'd like to understand this appeal better. Imagine if I argued to keep all of the pointer areas in SimPointerAreas.js and all of the stroke widths in SimStrokeWidths.js etc. This is non-scalable and I fail to see the appeal. If there is a stroke width used twice, then by all means factor it out, but that doesn't mean we should strive to have one file with all the stroke widths. Why is that appealing for colors?
I don't think ease of discovery by non-developers should be a deciding factor here, since we provide Color Editor tooling for that purpose. We can also instruct non-developers how to find a ProfileColorProperty by searching for its name.
If I was going to change the strokeWidth of an electron, I would check in ElectronNode.js. If I was going to change the font of the electron's label, that's probably also in ElectronNode.js. Why would we declare everything about how electrons look in ElectronNode.js except its color (assuming that color is used only once, in ElectronNode)?
It makes sense for every sim to have a Colors.js file for shared, factored out or general colors. The main question for this issue is whether we should strive to put all ProfileColorProperties and ColorDefs there or not. I agree all sims should have a *Colors.js file. |
|
Can we please keep the PhET-iO-related conversation to #1251, I'm getting confused.
Yes, I agree with this, but I also understand that there is precedent about how colors have been set up thus far. To mandate that they are integrated for all existing usages may be a cost we aren't interested in doing, for not much gain. From PhET-iO designers, colors haven't seemed that important for the product, so likely using either pattern, on a per-sim basis, will be just fine. |
Agreed, and this was also decided on in a meeting of some sort. |
Being able to pass appropriate Tandems into ProfileColorProperties was used as an argument against putting ProfileColorProperties in files where they are used, but it has been demonstrated how to do that without a problem as long as (a) the colors remain static and (b) the tandems are kept together.
Good point, I am not excited about incurring the cost of moving all colors for existing sims. But I wouldn't want to use precedent as a reason to say we use an otherwise preferable pattern going forward. Likewise, I'm not excited about having two ways of doing something, or a precedent that half of our sims use one pattern and the other half use another pattern. Also, I realized that identifying usages of ProfileColorProperties got a lot easier now that they are declared the conventional way (not the ColorProfile/PropertySet dynamic way). Previously, it required a fuzzy text search of a dynamically constructed Property name. Now it is possible to use Alt-F7 "Find Usages" to identify whether there are multiple vs a single file uses. For instance, skimming through the 61 colors in |
especially "pass appropriate Tandems" It seems like we have a different definition of this. Where is the paper trail that says we prefer I think 6 hours is a low-ball estimate, many colors will be used by multiple things, and it may not be obvious to non-responsible devs where the best spot is. What if we consider |
|
Imo, there's close to zero value in moving colors from (e.g) MySimColor.js to their usage sites. Putting colors where they are used makes a lot of sense in common code. E.g. it would be preferrable to have static colors in BicyclePumpNode.js, rather than in SceneryPhetColors.js. For the reasons that @samreid has mentioned, colors at usage sites should be static to the file, not instantiated per instance of their client class. I predict that this will be an on-going programming error, and something that will cause on-going PhET-iO problems. Maybe we need a CRC item related to ProfileColorProperty instantiation. Finally, for sim-specific colors... Where they are defined should be up to developer discretion. If you'd like to put your colors at their usage sites, I support your decision. But I don't agree that's always the best decision, so please don't institute a policy that requires me to do that. It has proven in practice (for dozens of sims) that it's much easier for me (cognitively, procedurally, etc.) to go to one place to tweak and maintain sim colors. |
|
@zepumph and I discussed this:
@zepumph asked that @pixelzoom and @samreid take next steps to determine how to proceed. |
Again, I see no reason to require that a color be used in multiple files. If that's a hard requirement, I'm not willing to accept this proposal. |
I'm sure that @jonathanolson has good reasons for putting all of those colors in areaModelCommonColorProfile. Your "more palatable" may not be more palatable to him. There are legitimate reasons other than "reused" for putting sim-specific colors in one place. |
Sounds good to me.
ProfileColorProperty names must be unique, so declaring one in a constructor would throw a runtime assertion. For instance, I test drove this with this patch: Index: js/common/view/AreaCalculationRadioButtonGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/AreaCalculationRadioButtonGroup.js b/js/common/view/AreaCalculationRadioButtonGroup.js
--- a/js/common/view/AreaCalculationRadioButtonGroup.js (revision ddd14016500acffe7579a604c3dfbb377d5c6624)
+++ b/js/common/view/AreaCalculationRadioButtonGroup.js (date 1627482687102)
@@ -12,6 +12,8 @@
import Path from '../../../../scenery/js/nodes/Path.js';
import Rectangle from '../../../../scenery/js/nodes/Rectangle.js';
import VBox from '../../../../scenery/js/nodes/VBox.js';
+import Color from '../../../../scenery/js/util/Color.js';
+import ProfileColorProperty from '../../../../scenery/js/util/ProfileColorProperty.js';
import eyeSlashSolidShape from '../../../../sherpa/js/fontawesome-5/eyeSlashSolidShape.js';
import areaModelCommon from '../../areaModelCommon.js';
import AreaCalculationChoice from '../model/AreaCalculationChoice.js';
@@ -26,7 +28,7 @@
*/
constructor( areaCalculationChoiceProperty, selectionButtonAlignGroup ) {
- const darkColorProperty = areaModelCommonColorProfile.calculationIconDarkProperty;
+ const darkColorProperty = new ProfileColorProperty( 'calculationIconDark', { default: Color.BLACK } );
const lightColorProperty = areaModelCommonColorProfile.calculationIconLightProperty;
super( areaCalculationChoiceProperty, [ {
And the assertion message was:
Is that good enough, or do you recommend something further in the CRC?
Thanks, I don't want to institute a decision that will hamper your workflow, that's why I'm trying to understand why you prefer it as a pattern.
Maybe they are there because it was previously the only way to get them to show in the color editor? Can you comment on this point from my discussion with @zepumph?
Since (a) we are in agreement that sometimes it is best to put a color as a static in the related file, but (b) this proposal is only acceptable if you can also factor out a color used in only one file, then it seems the best way forward is to allow both styles, and leave it up to sim developers discretion where particular colors should be defined. Sound good? |
If there's 1 instance of a class, that assertion has zero chance of detecting this problem. The lucky developer who creates the 2nd instance will have to deal with it.
Reasons why I prefer putting sim-specific colors in RepoColors.js:
Yes. |
|
@pixelzoom and I discussed this further, another aspect not mentioned above is that the Constants.js and Colors.js files are a nice place to "fine tune" the simulation. For instance FMWConstants has: // Number of points awarded for each correct answer in the Wave Game
POINTS_PER_CHALLENGE: 1,this is only used in one place, but it's still nice to have it in the constants file since it parameterizes the overall behavior of the simulation. We also agreed that there are numerous colors, strokes, fills that we would never want to move to the Colors.js file. It's just the ones we want to tinker with, or things that need to be used in more than one place. Like a panel fill or stroke that is broadly used. So it is a balance of convenience, design-orientation, ease of use for designers, etc. Having them together makes it easy to share with designers and ask "which of these should be phet-io instrumented"? If they are spread out, it is more difficult to share that list with phet-io designers. So there may be a reduced PhET-iO instrumentation cost to keeping these co-located. For a common code file, like BicyclePumpNode.js it is better to include them in the specific file rather than in the Colors.js file. @pixelzoom suggested that when a sim is published, would be a good time to review Colors.js and make sure it has the appropriate colors. See if any should be moved closer to where they are used. @pixelzoom also described that having different styles for this is not a significant maintenance problem. @pixelzoom suggested we could raise this for discussion at dev meeting before closing the issue. |
|
We went over the topics in the above during dev meeting today, as a PSA. Ready to close. |
In phetsims/scenery-phet#515 (comment) @jessegreenberg said:
I later learned this cannot be done on the instance level, and I commented in phetsims/scenery-phet#515 (comment)
In phetsims/scenery-phet#515 (comment) @zepumph commented:
In the review, @pixelzoom commented phetsims/scenery-phet#515 (comment)
Once we have an easy-to-use global color tandem, it may make it easy to define static constants in each file where appropriate. Self-assigning to investigate that option.
The text was updated successfully, but these errors were encountered: