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

use PhetColorScheme for ENERGY #70

Closed
arouinfar opened this issue Jan 9, 2019 · 9 comments
Closed

use PhetColorScheme for ENERGY #70

arouinfar opened this issue Jan 9, 2019 · 9 comments
Assignees

Comments

@arouinfar
Copy link
Contributor

arouinfar commented Jan 9, 2019

See phetsims/scenery-phet#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 phetsims/scenery-phet#456 is sorted out.

@pixelzoom
Copy link
Contributor

@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
Copy link
Contributor

pixelzoom commented Jan 9, 2019

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

phetsims/scenery-phet#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 added a commit to phetsims/scenery-phet that referenced this issue Jan 9, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit that referenced this issue Jan 9, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor

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
Copy link
Contributor

pixelzoom commented Jan 9, 2019

@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?

@pixelzoom pixelzoom removed their assignment Jan 9, 2019
@arouinfar
Copy link
Contributor Author

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
Copy link
Contributor Author

Reassigning to @pixelzoom, rather than closing, so he has a chance to see #70 (comment).

If all looks good, please close.

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Jan 9, 2019
@pixelzoom
Copy link
Contributor

@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
Copy link
Contributor Author

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

@arouinfar arouinfar removed their assignment Jan 9, 2019
pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Jan 9, 2019
pixelzoom added a commit that referenced this issue Jan 9, 2019
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants