search

phetsims / scenery-phet

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

Consolidation of light bulb styles #170

Open jessegreenberg opened 8 years ago

jessegreenberg commented 8 years ago

There are two styles of light bulbs requested across sims. One is currently used in acid-base-solutions and uses light rays to indicate brightness.

acid bulb

The other style is used in Faraday's Law and is requested in Capacitor Lab: Basics, uses halos to indicate brightness.

faraday_bulb

The ray style bulb exists in scenery-phet and we need to decide how to organize the two styles. The following are previous discussions about this issue.

[6/22/15, 10:17:03 AM] John Blanco: Thanks.On another topic: Jesse is supposed to do a light bulb in capacitor lab, but the designers want the one that's in Faraday's Law, not the one that is currently in scenery-phet. He's going to move the one in Faraday's to common code. Any opinions on how these are named in scenery-phet? BulbNode and LightBulbNode would be the result if we just moved it. Could do LightBulbNodeWithLines and LightBulbNodeWithCircles, but that seems bulky. Could do LightBulbNode and LightBulbNode2. Any input?

[6/22/15, 10:19:18 AM] Chris Malley: is there a legitimate need for 2 light bulb implementations? could 1 implementation with options (e.g., type of rays) meet sim needs?

[6/22/15, 10:20:30 AM] Chris Malley: why are rays OK in some sims but not in capacitor-lab?

[6/22/15, 10:20:39 AM] Sam Reid: I’m working now, let me know if anything above still requires my attention.

[6/22/15, 10:21:04 AM] John Blanco: I was wondering that too. Right now the appearance of the two is pretty different. I'll have Jesse ask his design team, and we'll go from there.

[6/22/15, 10:21:26 AM] Sam Reid: IMO and ITAOFI (in the absence of further information) rays on/off seems like it should be an option.

[6/22/15, 10:22:39 AM] Chris Malley: if rays are off, then how do you know when the bulb is on or how bright it is? doesn't there need to be an alternative representation of brightness?

[6/22/15, 10:23:07 AM] Chris Malley: in the case of faradays-law, the alternative representation is white circles.

[6/22/15, 10:24:25 AM] John Blanco: Exactly. The white circles is what the designers want. If that's ALL they want, and the rest of the bulb doesn't matter, we should just make that an option on the current bulb node.

[6/22/15, 10:25:01 AM] Chris Malley: I'd like to hear why they need white circles, vs why they want white circles.

[6/22/15, 10:25:20 AM] Chris Malley: I suspect that it's related to the look of the original (Flash) sim.

[6/22/15, 10:26:27 AM] Chris Malley: Or why we can't also use white circles in the sims that are currently using LightBulbNode.

On 7/13/2015, @pixelzoom brought up the following point:

[11:48:27 AM | Edited 11:48:40 AM] Chris Malley: LightBulbNode is also used in more complex UI components, e.g. ConductivityTesterNode. If we have 2 bulbs, I can imagine that someone will want to choose between bulb looks in ConductivityTesterNode. More work, more maintenance,...That may make sense if it's conscious and justified.

@ariel-phet, we need to decide why the two styles are necessary, do you know who in the design team might like to comment on this?

ariel-phet commented 8 years ago

Basically, the halo style tends to be preferred for very dynamic changes (such as in faraday's law), and the ray style tends to be preferred when you have a more static representation that can be changed by a parameter in the sim (such as with the conductivity meter). The rays give an easily "countable" comparison, we will want both styles. If that means keeping these separate "raylightbulb" and "halolightbulb" or something like that, it would be fine. But we need both styles.

ariel-phet commented 8 years ago

@jessegreenberg feel free to make a topic for dev meeting as to the best approach.

pixelzoom commented 8 years ago

If both are needed, then my recommendation is:

  1. rename scenery-phet.LightBulbNode to RaysLightBulbNode
  2. create scenery-phet.HaloLightBulbNode
  3. defer switching between bulb types in ConductivityTesterNode until someone asks for it.

After 1 & 2, someone can review to see if there's enough commonality to warrant a base type, or a single type with {string} lightType "rays"|"holo"

jessegreenberg commented 8 years ago

Thanks for the recommendation @pixelzoom. I will go ahead with this plan.

samreid commented 8 years ago

I'm getting close to a point for Circuit Construction Kit: Basics where I could use the appropriate light bulb artwork together with the rays representation. For CCK, it is supposed to look like the light bulb artwork from Faraday's Law and Capacitor Lab: Basics, not the one from Acid Base Solutions. I asked over email if Acid Base Solutions could use the same light bulb as CCK/Faraday's Law/Capacitor Lab.

@ariel-phet can you please review the status of this issue and help delegate?

ariel-phet commented 8 years ago

As noted https://github.com/phetsims/scenery-phet/issues/170#issuecomment-121023048 it is the light brightness representation that needs to be different, I am completely agnostic as to the the lightbulb artwork that is used for acid-base solutions, but that lightbulb is also used in Energy forms and changes, and the filament glowing red is an important property for that sim.

However, @pixelzoom should not be spending time in acid-base right now.

samreid commented 8 years ago

On Jul 13, 2015 @jessegreenberg said he would proceed with the plan described in https://github.com/phetsims/scenery-phet/issues/170#issuecomment-121033856, is he available to work on this? What is the status and schedule for Capacitor Lab: Basics?

ariel-phet commented 8 years ago

Not sure if @jessegreenberg is available

@jessegreenberg what say you?

jessegreenberg commented 8 years ago

Yes, I am available to work on this now. Leaving assigned to me to follow through with the recommendation in https://github.com/phetsims/scenery-phet/issues/170#issuecomment-121033856.

samreid commented 8 years ago

Thanks @jessegreenberg, let me know when it is ready to take for a test drive in Circuit Construction Kit.

samreid commented 8 years ago

@jessegreenberg asked whether we need two representations of light bulb images in scenery-phet. I said probably not, but it is really a question for designers, so I have emailed @ariel-phet and @arouinfar to ask if we can use the new bulb across sims.

image

@jessegreenberg is putting this issue on hold until we have a game plan.

samreid commented 8 years ago

@ariel-phet responded:

@samreid, please also note the use in EFAC is even slightly different

1 - the filament "glows" more red as the bulb gets brighter 2 - the rays (unlike in ABS) get brighter instead of getting longer and such 3 - these bulbs because they have different shapes would have ray images mapping to them differently (the halo buld is specifically more circular)

I am not opposed to unifying these representations, but it involves some subtleties.

Ariel

EDIT: the=>they

ariel-phet commented 8 years ago

Also, at the very least we would need to have different bases for the bulb (one of the EFAC style, and one of the two obvious connection points style).

samreid commented 8 years ago

Here's an image of the single connection point style @ariel-phet was referring to from EFAC:

image

samreid commented 8 years ago

Also of note, the filament in Acid Base Solutions is glowing red even though there is no current.

samreid commented 8 years ago

@pixelzoom said he cannot think of any reason the faraday's law light bulb could not be used in acid base solutions, but it should be double checked with the design team.

samreid commented 8 years ago

@jbphet says it is important that the filament is a straight line in Energy Forms and Changes.

arouinfar commented 8 years ago

Currently, this bulb appears in CCK: Black Box. image

In future versions, it will be necessary to show the electron flow through the wire filament. The Faraday's Law-style bulb has a zig-zag filament, which is not ideal to show electrons moving through it.

@samreid I think we should stick with the light bulb currently in CCK.

pixelzoom commented 3 years ago

Discussion for this issue stalled almost 5 years ago, and it's assigned to @jessegreenberg. I'm going to reassign to myself to read through the comments and figure out what has been done, and what's left to do.

pixelzoom commented 3 years ago

Discussion for this issue stalled almost 5 years ago, and it's assigned to @jessegreenberg.

I read through the comments, and I'm still confused about what work remains to be done. But I will note that the situation has been made worse by work in CCK that has apparently ignored this issue. The most recent comment on this issue was 4/11/2016, where @arouinfar said:

I think we should stick with the light bulb currently in CCK.

But looking at CCK:AC, I see that the design changed. And presumable to satisfy that design, @samreid created yet-another lightbulb design and implementation, CCKCLightBulbNode. Since @samreid commented extensively on this issue, he must have been aware of it. But I see nothing about this new lightbulb mentioned here. CCK also has CustomLightBulbNode, documented as "Forked from SCENERY_PHET/LightBulbNode", a polite way of saying "Copied". (Reported in phetsims/circuit-construction-kit-common#679)

So now we have:

Related classes that should be addressed:

There are also many image files to support the above classes. I have not done an inventory.

I don't have time to devote to this issue now. So I think the path forward is to label this as an epic, start with a design phase where PhET identifies what is needed (by current and future sims), followed by an implementation phase where we decide how to refactor, eliminate duplication, remain flexible for future features that have been identified, etc.

zepumph commented 3 years ago

A quarterly goal will include progressing this as it makes sense with @samreid's CCK work.

samreid commented 3 years ago

I'm tracking the CCK aspect of this issue in https://github.com/phetsims/circuit-construction-kit-common/issues/679 and will wait for this issue to be planned as a quarterly goal before going further. I confirmed this issue is listed in the spreadsheet. Self-unassigning.