phetsims / color-vision

"Color Vision" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/color-vision
GNU General Public License v3.0
1 stars 7 forks source link

invalid argument to SingleBulbPhoton #129

Closed pixelzoom closed 5 months ago

pixelzoom commented 5 years ago

Added to SingleBulbPhotonBeam (line 168) by @samreid in https://github.com/phetsims/color-vision/commit/b5006267aa102107b35f348f37e203eacb672881

        this.photons.push( new SingleBulbPhoton(
           new Vector2( this.beamLength, ColorVisionConstants.BEAM_HEIGHT / 2 ),
           new Vector2( ColorVisionConstants.X_VELOCITY, 0 ),
           1,
           BLACK_ALPHA_0,
           false,
           undefined,// TODO: quoi?
           self.photonGroupTandem.createNextTandem()
         ) );

... which eventually became:

        this.photons.push( new SingleBulbPhoton(
          new Vector2( this.beamLength, ColorVisionConstants.BEAM_HEIGHT / 2 ),
          new Vector2( ColorVisionConstants.X_VELOCITY, 0 ),
          1,
          BLACK_ALPHA_0,
          false,
          undefined, {// TODO: quoi?
            tandem: self.photonGroupTandem.createNextTandem()
          }
        ) );

Setting something to undefined (in this case, a constructor argument) is an anti-pattern. And the corresponding parameter in SingleBulbPhoton is documented as @param {number} wavelength, so the value is technically invalid. Since "quo?" means "what?" (see the TODO comment), I'm assuming @samreid had a question about this, even though he created the code.

@samreid do you recall why you did this? Or why you needed to do this?

pixelzoom commented 5 years ago

The same anti-pattern occurs on line 152 of SingleBulbPhotonBeam:

        var blackPhoton = new SingleBulbPhoton(
          new Vector2( this.filterOffset, ColorVisionConstants.BEAM_HEIGHT / 2 ),
          new Vector2( ColorVisionConstants.X_VELOCITY, 0 ),
          1,
          BLACK_ALPHA_0,
          false,
          undefined, {
            tandem: self.photonGroupTandem.createNextTandem()
          }
        );
samreid commented 5 years ago

It looks like in December 2016 the SingleBulbPhoton constructor signature changed from:

function SingleBulbPhoton( location, velocity, intensity, color, isWhite, wavelength ) {

to this, in order to accommodate a tandem in the options:

function SingleBulbPhoton( location, velocity, intensity, color, isWhite, wavelength, options ) {

However, the so-called black photons were were being constructed with a 5-arg call (example taken from ccb870576504940d769ca000276bec49bc57bb1d )

      // emit a black photon for resetting the perceived color to black if the flashlight is off
      if ( !this.model.flashlightOn ) {
        this.photons.push( new SingleBulbPhoton( new Vector2( this.beamLength, ColorVisionConstants.BEAM_HEIGHT / 2 ),
          new Vector2( ColorVisionConstants.X_VELOCITY, 0 ), 1, BLACK_ALPHA_0, false ) );
      }

So it appears that the wavelength was already coming through the constructor args as undefined since it wasn't passed at all. When I added the options argument, I needed to jump over the wavelength argument, so I specified undefined. I'm sure we can think of a clearer and more elegant way to do this. Let me know if you want to take it from here, or if you would like some ideas or assistance.

Luisav1 commented 5 months ago

I fixed this by passing the wavelength argument through the options. Closing.