phetsims / scenery-phet

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

ColorProfile: convert from PropertySet to Property? #278

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

This was supposed to be covered in https://github.com/phetsims/scenery-phet/issues/276, but it's a bit of a mess. ColorProfile was apparently written for molecule-shapes (MoleculeShapesColors), but is no longer used by molecule-shapes. It was generalized (somewhat) by AD and moved to scenery-phet.ColorProfiles, presumably for use in gravity-and-orbits.

Three problems:

(1) ColorProfile is using a "clever" approach to create color Properties. Converting that to individual Properties is going to be a project. And I'm not really interested in doing that because (a) I have no experience with ColorProfile, and (b) the other problems below suggest that ColorProfile has not been vetted as a general approach.

(2) ColorProfile is currently used only in GravityAndOrbitsColorProfile (gravity-and-orbits) and StatesOfMatterColorProfile (states-of-matter, state-of-matter-basics, atomic-interactions) to support "projector mode". There are other sims (charges-and-fields, molecule-shapes, rutherford-scattering) that provide the project mode feature - no idea how they are implementing it, but they're not using ColorProfile. So there appears to be no agreement on how to support "project mode". Is it worth the effort to convert ColorProfile, or would one of the other approaches work?

(3) Projector mode is a subset of a general "color scheme" issue. In joist we have https://github.com/phetsims/joist/issues/342 (provide support for "Projector Mode") and https://github.com/phetsims/joist/issues/222 (color inversion feature). So the more general issue of how to describe and apply color schemes has not been addressed. And using ColorProfile seems to be a way of kicking that can down the road.

Assigning to @ariel-phet to prioritize and assign.

ariel-phet commented 7 years ago

Marking for developer meeting, seems like a good topic to discuss with parties that have used ColorProfile

pixelzoom commented 7 years ago

Above I said:

There are other sims (charges-and-fields, molecule-shapes, rutherford-scattering) that provide the project mode feature - no idea how they are implementing it, but they're not using ColorProfile.

Looking at these sims, they have their own implementations that are nearly identical to ColorProfile, with the same clever/problematic code that converts a hash of colors to a PropertySet. Looks like people have been copy-pasting code. (See ChargesAndFieldsColors, MoleculeShapesColors, RSColors.)

jonathanolson commented 7 years ago

Looks like people have been copy-pasting code.

molecule-shapes was the original code that ColorProfile was based on, not sure why it wasn't updated to use the common-code solution with that work.

As a side-note, is there an easy way to analyze our code base for duplicated (copy-pasted) code?

pixelzoom commented 7 years ago

We should also decide how to refer to these things - color scheme, color profile, theme, ...?

samreid commented 7 years ago

@jonathanolson will convert ColorProfile to use Property instead of PropertySet.

jonathanolson commented 7 years ago

Updated ColorProfile, refactored any remaining usages, and added more in-depth documentation.

@pixelzoom, does this look sufficient?

pixelzoom commented 7 years ago

👍

Best assertion:

assert && assert( key !== 'profileName', 'Unlikely, but would have hilarious consequences since we would overwrite profileNameProperty' );

Closing.