Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PhET-iO instrumentation for ColorProfile #515

Closed
pixelzoom opened this issue Jul 16, 2019 · 71 comments
Closed

PhET-iO instrumentation for ColorProfile #515

pixelzoom opened this issue Jul 16, 2019 · 71 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 16, 2019

I discovered while adding tandems to Gas Properties in phetsims/gas-properties#30 that ColorProfile is not set up for PhET-iO use. I'm not sure how important it is, that would be a question for designers. But I can imagine clients wanting to (for example) customize the color of particles in Gas Properties.

@pixelzoom
Copy link
Contributor Author

Thoughts:

  • Instrument some colors but not others. For example in Gas Properties there are only a few colors that clients might be interested in changing
  • Provide metadata per color. For example to describe a color via phetioDocumentation, or inspect (but not set) a color via phetioReadOnly

@pixelzoom
Copy link
Contributor Author

Looks like profileNameProperty is the only bit that is currently instrumented.

@zepumph
Copy link
Member

zepumph commented Oct 3, 2019

@pixelzoom I'm trying to note the priority for this. Is there a sim currently under phet-io development that is using a ColorProfile. If not please let us know when that is the case. I think that will be the easiest way to proceed here.

@zepumph zepumph assigned pixelzoom and unassigned zepumph Oct 3, 2019
@pixelzoom
Copy link
Contributor Author

As noted in the first comment, gas-properties uses ColorProfiles, and whether they need to be instrumented is a question for designers. If (for example) allowing PhET-iO clients to change the particle colors is a desired feature, then ColorProfiles (and the Properties that they synthesize) will need to be instrumented.

@pixelzoom
Copy link
Contributor Author

This came up again in Fourier, where we want to use ColorProfile, and be able to change colors via PhET-iO. See phetsims/fourier-making-waves#5.

@zepumph
Copy link
Member

zepumph commented Nov 9, 2020

Above, I added studio support for PropertyIO<ColorIO>. It was simple enough, and felt like something we should have no matter what.

This patch could instrument every colorProperty, which could be a poor, manual approach to creating your own color profile, but it can be overridden, for example if you set some Properties, and then toggle the projector mode checkbox on and then off again, it blows away your customizations.

Index: js/ColorProfile.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/ColorProfile.js	(revision 1a81c88e54ba0f370fdce8459aafb4f1bcfea4dd)
+++ js/ColorProfile.js	(date 1604956799275)
@@ -89,6 +89,8 @@
       validValues: profileNames
     } );
 
+    const colorPropertiesTandem = options.tandem.createTandem( 'colorProperties' );
+
     Object.keys( colors ).sort().forEach( key => {
       if ( colors.hasOwnProperty( key ) ) {
 
@@ -103,9 +105,14 @@
         // Use the requested initial profile, fallback to default.
         const initialColor = colorMap[ initialProfileName ] || colorMap[ ColorProfile.DEFAULT_COLOR_PROFILE_NAME ];
 
+        const colorPropertyName = key + 'Property';
+
         // Create a Property for the color
-        const colorProperty = new Property( initialColor );
-        this[ key + 'Property' ] = colorProperty;
+        const colorProperty = new Property( initialColor, {
+          tandem: colorPropertiesTandem.createTandem( colorPropertyName ),
+          phetioType: Property.PropertyIO( Color.ColorIO )
+        } );
+        this[ colorPropertyName ] = colorProperty;
 
         // Update the Property on profile name changes
         this.profileNameProperty.lazyLink( profileName => {

I'm not really sure if this is something to look into further, or if we should start here. Likely things should be more designed. In the mean time, @samreid will you check on my commits, and recommend a step forward for this issue as you see fit.

@samreid
Copy link
Member

samreid commented Nov 11, 2020

I tested capacitorLabBasics.capacitanceScreen.model.arrowColorProperty in studio, and it looks really great, nice work @zepumph. I think for many sims, changing the color profile programmatically will be OK. For Fourier's amplitude colors, @ariel-phet said:

I do not even think these colors should be "easily edited" with iO for the same reasons. That seems like a "bell and whistle" that is potentially problematic.

I recommend we wait until we have a concrete use case before working on this further.

@zepumph
Copy link
Member

zepumph commented Nov 13, 2020

My commits from #515 (comment) have brought up discussion in phetsims/fourier-making-waves#13 (comment) and phetsims/scenery#1115.

@zepumph
Copy link
Member

zepumph commented Nov 13, 2020

@samreid said that this desire was brought up again in a design meeting yesterday, we spoke about how we may accomplish this and thought about the following:

  1. Instrument all Color Properties in ColorProfile as phetioReadOnly.
  2. Create ColorProfileIO, which has a studio HTML for customizing the Profile colors, not just the Property values. This would likely mean an IOType method, like setColorForProfile that manipulated the underlying color map (
    const colorMap = _.mapValues( colors[ key ], Color.toColor );
    ).
  3. Provide a way that ColorProfile can be phetioReadyOnly, but that won't effect the usability of colorProfileProperty (which is never read-only). In requirements for harmonic colors fourier-making-waves#5 (comment) it sounded like this was important for Fourier Making Waves.

@zepumph zepumph self-assigned this Nov 13, 2020
zepumph added a commit to phetsims/scenery that referenced this issue Nov 13, 2020
samreid added a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
samreid added a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
samreid added a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
samreid pushed a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants