search

phetsims / capacitor-lab-basics

"Capacitor Lab: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

Light possibly too bright? #275

Closed brooklynlash closed 3 years ago

brooklynlash commented 4 years ago

This is just a suggestion for https://github.com/phetsims/QA/issues/565, but I think the light when switching from the voltage to the light source is too bright. capacitor1

Denz1994 commented 4 years ago

Reducing the brightness would mean reducing the scale of each white circle radius. I'd leave for @arouinfar to determine the appropriate ratio, assuming we want to change this scale.

arouinfar commented 4 years ago

Thanks @brooklynlash. I agree.

@Denz1994 let's turn down the max brightness. I played around with the code locally, and I think changing https://github.com/phetsims/capacitor-lab-basics/blob/master/js/common/view/BulbNode.js#L56 to the following works nicely.

  const bulbBrightnessMap = new LinearFunction( 0, 5E-13, 0, 175, true );
Denz1994 commented 4 years ago

The adjustment was made on the 1.7 branch. I'll push a new version for comparison.

KatieWoe commented 3 years ago

This seems to have been implemented in rc.8. Due to https://github.com/phetsims/capacitor-lab-basics/issues/276 I'm leaving open in case further adjustments relating to this are needed.

KatieWoe commented 3 years ago

Addition: in rc.8 I noticed that, when making changes beyond the max brightness of the bulb, the fact that the light stops changing can be a bit odd and may lead to some misunderstandings. maxlightlooksodd

arouinfar commented 3 years ago

I don't think it pedagogically useful to change the capacitance while discharging, which is why I decided to keep the arrows layered behind the halo in #276. However, I do think we could afford to increase the max brightness a bit to reduce the likelihood of running into a situation like the one in https://github.com/phetsims/capacitor-lab-basics/issues/275#issuecomment-760483503. Since we'll be moving the TimeControlNode on top of the halo, we can make the halo a bit larger. @Denz1994 let's increase the max brightness to 225.

jonathanolson commented 3 years ago

Master and 1.7 had diverged, so I have NOT pushed this to 1.7.

Can you see the behavior in master and let me know if it looks good? And then explicitly let me know if I should move it to 1.7?

arouinfar commented 3 years ago

Looks good in master. Since we'll be taking new SHAs, I think we can close.