search

phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
http://scenerystack.org/
MIT License
8 stars 6 forks source link

PhET-iO instrumentation for ColorProfile #515

Closed pixelzoom closed 3 years ago

pixelzoom commented 5 years ago

I discovered while adding tandems to Gas Properties in https://github.com/phetsims/gas-properties/issues/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 commented 3 years ago

I also thought it was appropriate to ask @pixelzoom to review phetsims/chipper#1061

Done.

To review: ...

I'll put my review notes in this comment, and add to them as I go. Don't do anything with these until I assign back to @samreid, because I'll be modifying this comment.

SCENERY/colorProfileProperty.js

SCENERY/ProfileColorProperty.js

    // @protected - used elsewhere in this file but outside of this class.
    // values are mutated by the color wrapper.
    this.colorProfileMap = colorProfileMap;

A couple of sims that use these new patterns, such as GasPropertiesColorProfile.

Changes to ProjectorModeCheckbox.js

Otherwise ProjectorModeCheckbox.js seems OK for now. I think we'll eventually want to get rid of this control, and have a more general way of selecting color profile from the Preferences dialog.

Any related files you think are relevant.

package.json - I have reservations about the ordered list in phet.colorProfiles, the fact that it defaults to [ 'default' ], the fact that the sim starts up with phet.colorProfiles[0], etc. There's some non-obvious stuff here. But we'll see how it goes.

pixelzoom commented 3 years ago

Review completed, comments in https://github.com/phetsims/scenery-phet/issues/515#issuecomment-882927243. Back to @samreid.

samreid commented 3 years ago

'default' is duplicated in colorProfileProperty.js 2x, initialize-globals.js 1x, ProfileColorProperty (as this.colorProfileMap.default) and in numerous instantiations of ProfileColorProperty

Would you like to use computed property names? It could factor out those terms like so: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer

  screenBackgroundColorProperty: new ProfileColorProperty( 'screenBackgroundColor', {
    [ SceneryConstants.DEFAULT_COLOR_PROFILE_NAME ]: 'black',
    [ SceneryConstants.PROJECTOR_COLOR_PROFILE_NAME ]: 'white'
  } ),

We have never tried to factor out key names before, I wonder what would happen if we did that across our project.

pixelzoom commented 3 years ago

Would you like to use computed property names?

No, definitely not. I hadn't considered that.

pixelzoom commented 3 years ago

... I think we should shed the "ColorProfile" baggage, choose another name, and make it the PhET convention. +1 for (e.g) GasPropertiesColors.js.

In the above commits, I renamed this .js file for my sims. For example, GasPropertiesColorProfile.js is now GasPropertiesColors.js

pixelzoom commented 3 years ago

In the above commit, the .js file naming convention was applied to simula-rasa.

zepumph commented 3 years ago
samreid commented 3 years ago

Thanks, I removed color-profiles.

pixelzoom commented 3 years ago

Removing color-profiles has broken phetmarks for all release branches. I'm trying to test patches and RCs for ph-scale and natural-selection, and phetmarks crashes with:

jquery-2.1.0.min.js:4 GET http://localhost/~cmalley/GitHub/perennial/data/color-profiles 404 (Not Found)

It's super inconvenient not to have a working phetmarks for release branches.

On Slack dev-public, @zepumph proposed that someone should "MR phetmarks once and for all to be graceful if any data lists don't exist." That sounds like a great idea, but I don't have time to do that. So I am going to restore color-profiles.

pixelzoom commented 3 years ago

Something seemed a little funky when I reverted the deletion of color-profiles. @samreid please check my work.

samreid commented 3 years ago

Maybe a more robust way around this problem will be for phetmarks to depend on annual instead of perennial.

samreid commented 3 years ago

The revert commit looks good, thanks!

samreid commented 3 years ago

Since this issue is getting long, here is a link to the remaining work: https://github.com/phetsims/scenery-phet/issues/515#issuecomment-882927243

samreid commented 3 years ago

Thanks for the review recommendations, I addressed them in the commits. I also changed PROJECTOR_COLOR_PROFILE_NAME to PROJECTOR_COLOR_PROFILE. Every issue has been addressed or moved to a side issue. Back to @pixelzoom to see if there is any remaining work for this issue.

pixelzoom commented 3 years ago

I had to expand this issue multiple times to find my review comments. If I need to get to them again, direct link is https://github.com/phetsims/scenery-phet/issues/515#issuecomment-882927243

@samreid Sorry, I don't have time to look through each commit to see how you addressed the review comments. If you want to summarize and point me to relevant commit(s), I'll be happy to take a look at specific things.

I did notice that the CRC item is now:

All sims should use a color file named MyRepoColorProfile.js or, if using abbreviations, MRColorProfile.js, and use ProfileColorProperty where appropriate, even if they have a single (default) profile. ...

I don't agree with the naming convention. I'd like to shed the "ColorProfile" baggage, and call this file MyRepoColors.js. And I intend to put both ColorDef and ProfileColorProperty instances in this file.

samreid commented 3 years ago

Sorry, I don't have time to look through each commit to see how you addressed the review comments.

The changes were straightforward and the re-review step can be skipped at your discretion.

If you want to summarize and point me to relevant commit(s), I'll be happy to take a look at specific things.

There are 9 commits listed above with these commit messages:

The changes were straightforward and I didn't need specific feedback on any of them, just reassigned to you to double-check as part of review protocol.

I don't agree with the naming convention. I'd like to shed the "ColorProfile" baggage, and call this file MyRepoColors.js.

As mentioned in an UPDATE in https://github.com/phetsims/scenery-phet/issues/515#issuecomment-882927243, this will be addressed in https://github.com/phetsims/scenery-phet/issues/686

pixelzoom commented 3 years ago

I did a quick skim of the commits. Looks like good work, and I didn't see anything amiss.

Back to @samreid in case there's anything else to do here.

zepumph commented 3 years ago

The current CRC checklist item no longer makes sense, I believe we no longer want the colors file to look like "*ColorProfile.js", reopening.

zepumph commented 3 years ago

@samreid, does this feel adequate to you?

pixelzoom commented 3 years ago

Since I suggested changing the CRC item, and @samreid is on vacation, I took a look. The changes look great to me.

pixelzoom commented 3 years ago

Since this was reopened for the CRC item, I'll go ahead an re-close.