User Feedback: Double check code for generating photons for white bulb

[kathy-phet]

The photon view for the white bulb should pull from the rainbow spectrum of bulb color. That is there should be no "brown" or "pink" type colors that you don't find in sun spectrum.

Can a developer take a look and see what is being used to generate the photons? Some look suspect as non-pure light colors:

[kathy-phet]

Assigning to CK as responsible dev to take a look at the code. @chrisklus - the photons should all be colors from this spectrum:

The ones that look brown, white, grey, for instance, shouldn't be there.

[arouinfar]

This was previously discussed in https://github.com/phetsims/color-vision/issues/96 and was elevated to a (now quite stale) issue in scenery-phet https://github.com/phetsims/scenery-phet/issues/340.

[arouinfar]

@kathy-phet what's the priority of this issue? Did a client bring it to your attention?

If you pause and inspect the photon stream, it does look like there are some non-spectrum colored photons. However, it's something you have to be on the lookout for.

It doesn't look like we've received any phethelp feedback about this specifically, though many years ago there were some comments about purple photons in the context of monochromatic light (#96 #88). I suspect the solution to this issue will be somewhat complicated and will end up touching other sims too, so it would be good to know more before @chrisklus spends much time on this.

[kathy-phet]

It was a physics faculty member, training high school teachers, who brought it to my attention.

Let's schedule a bit of time at a design meeting, after the current batch of sims that are working their way out the door are out the door. It's not particularly time-sensitive.

[pixelzoom]

This looks like a relatively easy problem to fix, and took me ~5 minutes to track down.

The problem is in SingleBulbPhotonBeam.js, where the photons are created.

With white light, a random color is chosen by calling randomColor:

  // @public
  createPhoton( timeElapsed ) {

    // if the flashlight is on, create a new photon this animation frame
    if ( this.model.flashlightOnProperty.value ) {
      const newColor = ( this.model.lightTypeProperty.value === 'white' ) ?
                       randomColor() : VisibleColor.wavelengthToColor( this.model.flashlightWavelengthProperty.value );

randomColor is unfortunately defined like this, which generates random colors that are not visible colors:

function randomColor() {
  const r = Math.floor( dotRandom.nextDouble() * 256 );
  const g = Math.floor( dotRandom.nextDouble() * 256 );
  const b = Math.floor( dotRandom.nextDouble() * 256 );
  return new Color( r, g, b, 1 );
}

What randomColor should be doing is generating a random visible wavelength, then converting it to a color. So something like this (untested):

function randomColor() {
  const randomWavelength = dotRandom.nextIntBetween( VisibleColor.MIN_WAVELENGTH, VisibleColor.MAX_WAVELENGTH );
  return VisibleColor.waveLengthToColor( wavelength );
}

And I would recommend renaming randomColor to randomVisibleColor.

Let me know if you want me to implement and test this fix.

[kathy-phet]

@arouinfar, @pixelzoom - This looks good to me. If Amy agrees, lets go ahead. It would be nice to maintenance release into the current version, if easy.

[arouinfar]

Yes, let's go for it. Sounds like @pixelzoom will be taking care of this, so reassigning.

[pixelzoom]

We should also check with @chrisklus, since he's responsible dev for this sim. @chrisklus would you like me to make this change, or do you want to handle it?

[pixelzoom]

Fixed in the above commit. I considered moving randomVisibleColor to VisibleColor.js, but that would be a common-code change, and would make maintenance release more complicated/expensive. And there is currently no other sim that appears to need this functionality. So randomVisibleColor remains in SingleBulbPhotonBeam.js, and the cherry-pick involves only that file.

@arouinfar ready for verification in master or 1.3.0-dev.3. When verified, leave this issue open, and I'll proceed with maintenance release.

Screenshot comparisons...

1.1.23 (published at phet.colorado.edu):

1.3.0-dev.3 / GitHub master:

[pixelzoom]

Looks like there's also a PhET-iO branch for this sim.

% grunt release-branch-list --repo=color-vision
...
color-vision 1.1 phet
color-vision 1.2-phetio phet-io

So I'll need to patch and deploy the 1.1 and 1.2 branches, following the Manual maintenance patching process, and cherry-picking https://github.com/phetsims/color-vision/commit/79eb1c80567ca130c46ea1db8b512f99ddc899de.

[arouinfar]

@pixelzoom looks good in master. I no longer see non-spectrum colored photons in the photon view of white light.

[pixelzoom]

Cherry-picking was not possible because the color-vision 1.1 and 1.2-phetio branches are so ancient. So I had to manually create and apply fixes for both of those branches.

The 1.1 branch has no dotRandom or phet.joist.random for random-number generation, so (after discussion on Slack) I went with Math.random. The commit is 20f7692.

Steps for the 1.1.24 maintence release were:

% cd perennial % grunt checkout-target --repo=color-vision --target=1.1 // manually edit SingleBulbPhotonBeam.js and apply the change in 20f7692 % cd color-vision % grunt // manually test local build % cd perennial % grunt rc --repo=color-vision --brands=phet --branch=1.1 // wait for completion email from build server // manually test 1.1.24-rc.1 at https://phet-dev.colorado.edu/html/color-vision/1.1.24-rc.1 % grunt production --repo=color-vision --brands=phet --branch=1.1 // wait for completion email from build server // manually test 1.1.24 for English and a few other locales at https://phet.colorado.edu/en/simulation/color-vision

Steps for maintenance release 1.2.16-phetio

% cd perennial % grunt checkout-target --repo=color-vision --target=1.2-phetio % cd color-vision // manually edit SingleBulbPhotonBeam.js and apply the change % grunt --brand=phet-io // manually test local build in Instance Proxies wrapper % cd perennial % grunt rc --repo=color-vision --brands=phet-io --branch=1.2-phetio // manually test 1.2.16-rc.1 in Events Colorized wrapper at https://phet-dev.colorado.edu/html/color-vision/1.2.16-phetiorc.1/wrappers/ (password required) % grunt production --repo=color-vision --brands=phet-io --branch=1.2-phetio // manually test all wrappers at https://phet-io.colorado.edu/sims/color-vision/1.2/wrappers/index/

[pixelzoom]

Cherry-picking was not possible because the color-vision 1.1 and 1.2-phetio branches are so ancient. So I had to manually create and apply fixes for both of those branches. The 1.1 and 1.2-phetio branches have no dotRandom or phet.joist.random for random-number generation, so (after discussion on Slack) I went with Math.random.

Navigting the maintenance release process was very time consuming (over 2 hours), and I couldn't have figured out the 1.2-phetio branch without @samreid's assistance. So I'm capturing the steps that I followed here.

Steps for the 1.1.24 maintence release were:

% cd perennial % grunt checkout-target --repo=color-vision --target=1.1 // test requirejs version in phetmarks before making changes % cd color-vision % grunt // test local build to confirm that this branch worked before making changes // manually edit SingleBulbPhotonBeam.js and apply the change in 20f7692 % grunt // manually test local build with changes // push changes % cd perennial % grunt rc --repo=color-vision --brands=phet --branch=1.1 // wait for completion email from build server // manually test 1.1.24-rc.1 at https://phet-dev.colorado.edu/html/color-vision/1.1.24-rc.1 % grunt production --repo=color-vision --brands=phet --branch=1.1 // wait for completion email from build server // manually test 1.1.24 for English and a few other locales at https://phet.colorado.edu/en/simulation/color-vision

Steps for the 1.2.16-phetio maintenance release were:

% cd perennial % grunt checkout-target --repo=color-vision --target=1.2-phetio // before making changes, tried to test with PhET-iO link in phetmarks -- it didn't work, URL seems to be bogus, moved on % cd color-vision % grunt // manually test local build to confirm that this branch worked before making changes // manually edit SingleBulbPhotonBeam.js and apply the change in 20f7692 % grunt // manually test local build with changes // push changes % cd perennial % grunt rc --repo=color-vision --brands=phet-io --branch=1.2-phetio // manually test 1.2.16-rc.1 in Events Colorized wrapper at https://phet-dev.colorado.edu/html/color-vision/1.2.16-phetiorc.1/wrappers/ (password required) % grunt production --repo=color-vision --brands=phet-io --branch=1.2-phetio // manually test all wrappers at https://phet-io.colorado.edu/sims/color-vision/1.2/wrappers/index/

[pixelzoom]

This work is done. The maintenance releases are:

1.1.24 at https://phet.colorado.edu/en/simulation/color-vision 1.2.16-phetio at https://phet-io.colorado.edu/sims/color-vision/1.2/wrappers/index/

@arouinfar @kathy-phet would you like to spot check?

@kathy-phet please reply to the user who reported this, then close this issue.

[kathy-phet]

I checked it on phet.colorado.edu and phet-io.colorado.edu and it looks great. Thanks, @pixelzoom for jumping through those hoops for an older sim! Here is the new photon colors:

[kathy-phet]

I'll write the faculty member. @arouinfar - please close once you have done your spot check.

[arouinfar]

Looks great, thanks @pixelzoom!