phetsims / hookes-law

"Hooke's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

use PhetColorScheme for ENERGY #70

Closed arouinfar closed 5 years ago

arouinfar commented 5 years ago

See https://github.com/phetsims/scenery-phet/issues/456.

Looks like HookesLawColors.ENERGY rgb( 3, 205, 255 ) should be using PhetColorScheme.ELASTIC_ENERGY.

I've always seen PhET sims use a cyan(ish) color for elastic energy, so not sure where the plum color rgb(153, 51, 102) specified in PhetColorScheme originates from. Marking as status:on-hold until https://github.com/phetsims/scenery-phet/issues/456 is sorted out.

pixelzoom commented 5 years ago

@arouinfar said:

... not sure where the plum color rgb(153, 51, 102) specified in PhetColorScheme originates from.

I believe that PhetColorScheme was ported directly from Java.

pixelzoom commented 5 years ago

Yep, PhetColorScheme was directly ported from Java (by me!) on 3/6/2015. The lastest Java version in Unfuddle is PhetColorScheme.java.

https://github.com/phetsims/scenery-phet/issues/140 describes the port, and is worth a read. In that issue I said "After porting, designers/developers can discuss how to modify the color scheme.", but I see no evidence that designers where ever involved.

Here's the Google Doc that Java's PhetColorScheme was based on: https://docs.google.com/spreadsheets/d/1mNsOWSbcoO-Ox2evxJij5Lix4HTZbXKbFgMlPe9W-u0/edit#gid=0. It specifies rgb( 0, 204, 255 ) for elastic energy.

Regarding HTML5 PhetColorScheme.ELASTIC_ENERGY... In the Java version, it was rgb( 0, 204, 255 ), which matches the Google Doc. But in the first commit of the HTML5 version it's rgb(153, 51, 102), so this is clearly a porting error.

No sims are currently using PhetColorScheme.ELASTIC_ENERGY. And rgb( 0, 204, 255 ) is imperceptibly different from HookesLawColors.ENERGY rgb( 3, 205, 255 ). So I recommend that I immediately fix the color value for PhetColorScheme.ELASTIC_ENERGY and use it in Hooke's Law.

@arouinfar OK to proceed?

pixelzoom commented 5 years ago

I went ahead and made this change. Here's the summary of what was changed:

// PhetColorScheme
ELASTIC_ENERGY: new Color( 0, 204, 255 ),

// HookesLawColors
ENERGY: PhetColorScheme.ELASTIC_ENERGY,

"Before" screenshot from 1.0.15 production version:

screenshot_959

"After" screenshot from master:

screenshot_960

@arouinfar if this looks OK, please close.

pixelzoom commented 5 years ago

@arouinfar Just out of curiosity... Could you explain why the graphs are labeled "Potential Energy" and we're using the color for elastic energy, not potential energy?

arouinfar commented 5 years ago

Thanks @pixelzoom, rgb( 0, 204, 255 ) looks like an appropriate choice for ELASTIC_ENERGY.

Potential energy is stored energy, and can take various forms -- gravitational, elastic, chemical, electric, or magnetic. Generally, PhET sims have only dealt with gravitational potential energy and elastic potential energy. When in a given context, it's not uncommon to simply refer to this stored energy as "potential energy", such as the elastic potential energy in Hooke's Law or the gravitational potential energy in Energy Skate Park: Basics or Pendulum Lab. However, in a context like Masses and Springs, which has multiple types of potential energy, the sim would include the "elastic" or "gravitational" modifier.

If anything, PhetColorScheme could better describe POTENTIAL_ENERGY as gravitational potential energy. Gravitational potential energy is, in a sense, the default kind of potential energy in most people's minds, so I suppose that's why it was named as such. If you want to be super clear, ELASTIC_ENERGY would be better named ELASTIC_POTENTIAL_ENERGY, and POTENTIAL_ENERGY would be more appropriately named GRAVITATIONAL_POTENTIAL_ENERGY. However, these names are a bit verbose, so perhaps a comment in PhetColorScheme would do the trick.

arouinfar commented 5 years ago

Reassigning to @pixelzoom, rather than closing, so he has a chance to see https://github.com/phetsims/hookes-law/issues/70#issuecomment-452797297.

If all looks good, please close.

pixelzoom commented 5 years ago

@arouinfar Thanks for the clarification.

You said:

... If you want to be super clear, ELASTIC_ENERGY would be better named ELASTIC_POTENTIAL_ENERGY, and POTENTIAL_ENERGY would be more appropriately named GRAVITATIONAL_POTENTIAL_ENERGY. However, these names are a bit verbose, so perhaps a comment in PhetColorScheme would do the trick.

I don't think verbosity is a problem in this case, and I'm in the favor of using the more verbose names rather than a comment. Since Hooke's Law is the only sim using either of these color, now would be a good time to change their names. OK to change in PhetColorScheme?

arouinfar commented 5 years ago

Sounds good @pixelzoom, please update the names in PhetColorScheme.

pixelzoom commented 5 years ago

Summary of changes:

// PhetColorScheme
ELASTIC_POTENTIAL_ENERGY: new Color( 0, 204, 255 ),
GRAVITATIONAL_POTENTIAL_ENERGY: Color.BLUE, // formerly POTENTIAL_ENERGY in Java implementation

// HookesLawColor
ENERGY: PhetColorScheme.ELASTIC_POTENTIAL_ENERGY,

Closing.