`brighterColor` fails if any RGB component of the target Color is 0.

[pixelzoom]

new Color( r, g, b ).brighterColor( 0 ) fails if any of the args to Color's constructor are 0.

To demonstrate in a browser console:

> new phet.scenery.Color( 255, 255, 255 ).brighterColor( 0 )
Color { ... r: 255, g:255, b:255, a:1,... }

> new phet.scenery.Color( 255, 255, 0 ).brighterColor( 0 )
assert.js:21 Assertion failed: Ensure color components are finite and not NaN
assert.js:25 Uncaught Error: Assertion failed: Ensure color components are finite and not NaN
    at window.assertions.assertFunction (assert.js:25)
    at Color.updateColor (Color.js:360)
    at Color.setRGBA (Color.js:233)
    at Color.set (Color.js:111)
    at new Color (Color.js:49)
    at Color.brighterColor (Color.js:484)
    at eval (eval at repositionFromKeys (AnimatedPanZoomListener.js:592), <anonymous>:1:37)
    at AnimatedPanZoomListener.repositionFromKeys (AnimatedPanZoomListener.js:592)
    at AnimatedPanZoomListener.windowKeydown (AnimatedPanZoomListener.js:366)
    at TinyEmitter.emit (TinyEmitter.js:86)

According to the doc and assertions in Color.js, all of the arguments are valid in the above case that is failing.

I don't see a problem with any of these similar edge cases:

> new phet.scenery.Color( 255, 255, 0 ).brighterColor( 1 )
> new phet.scenery.Color( 255, 255, 255 ).brighterColor( 0 )
> new phet.scenery.Color( 255, 255, 255 ).brighterColor( 1 )
> new phet.scenery.Color( 0, 0, 0 ).darkerColor( 0 )
> new phet.scenery.Color( 0, 0, 0 ).darkerColor( 1 )

[pixelzoom]

There's a divide-by-zero problem in brighterColor:

  brighterColor( factor ) {
    factor = this.checkFactor( factor );
    const red = Math.min( 255, Math.floor( this.r / factor ) );
    const green = Math.min( 255, Math.floor( this.g / factor ) );
    const blue = Math.min( 255, Math.floor( this.b / factor ) );
    return new Color( red, green, blue, this.a );
  }

And 0 is a valid value for factor:

  checkFactor( factor ) {
    assert && assert( factor === undefined || ( factor >= 0 && factor <= 1 ), `factor must be between 0 and 1: ${factor}` );

    return ( factor === undefined ) ? 0.7 : factor;
  }

[pixelzoom]

Ha, looks like I introduced this problem back in 2013, see 026a12fe5a5da543d886adc4923a9542df0c77ce.

Suggestions for how to reimplement while still remaining compatible with Java's implementation?

[jonathanolson]

Java only uses 0.7, from https://developer.classpath.org/doc/java/awt/Color-source.html. So adjusting the factor range for this method to not allow zero sounds best. Thoughts?

[pixelzoom]

In java.awt.Color:

/** Amount to scale a color by when brightening or darkening. */
private static final float BRIGHT_SCALE = 0.7f;

BRIGHTNESS_SCALE is used in method darker, but 0.7f is hardcoded in method brighter. Go figure...

I don't see any "darker" or "brighter" methods in Java that take a factor, so I'm not sure how we're being Java compatible. So I guess 0.7f is the factor that is applied on every call to darker or brighter.

I see 10 uses of brighterColor in PhET code. Only 2 of them are using a factor = 0.7, and 3 are relying on factor=0. So not supporting zero is going to be a pain.

I was going to suggest ditching brighterColor and darkerColor, and using colorUtilsBrighter and colorUtilsDarker. But there are 44 uses of darkerColor.

Resolving this is going to take a lot of time, first fixing the problem, then inspecting sims to see if visual changes are acceptable. I don't have time for that, so I'm going to unassign. If @jonathanolson or someone else wants to work on this, go for it.