Create ColorProperty

[samreid]

From phetsims/axon#221 we would like to create a custom ColorProperty type.

[jonathanolson]

Would you also have a ColorDefProperty?

[samreid]

Good point @jonathanolson. I think we only need one color property type, and it should use ColorDef values. I'm not certain whether ColorDefProperty is the best name or if we should call it ColorProperty and indicate that it takes ColorDef values. Recommendations from @pixelzoom or @jonathanolson?

[pixelzoom]

We have numerous Properties like Screen's backgroundColorProperty whose name is "Color" but whose value is technically a ColorDef. I have a slight preference for backgroundColorProperty over backgroundColorDefProperty, and therefore a slight preference for ColorProperty.

If we go with ColorDefProperty, we should probably rename instances of those Properties accordingly; e.g. Sim backgroundColorDefProperty = new ColorDefProperty(...).

The name decision should also consider how these Properties will be resented via PhET-iO. Is there any advantage for ColorProperty vs ColorDefProperty?

[pixelzoom]

More thoughts on the above...

More than once, I've encountered common code that was relying on something named "Color" having a scenery.Color value, with no support for {string}. Would ColorDefProperty discourage that practice?

[jonathanolson]

I've also run into cases where I want things to be Property.<Color>, so I can e.g. darken/blend colors. Also some cases we may want Property.<PaintDef>. Seems like we would presumably want all three (which is a general up-front cost with the "typed" Properties).

[pixelzoom]

Re Property.<ColorDef> vs Property.<Color> vs Property.<PaintDef>... I don't think we necessary want (or need) all three. The goal was to provide type-specific Properties for common occurrences, as identified in phetsims/axon#221 (comment). I see very few occurrences of values that are PaintDef, so I doubt it's worth the effort to create PaintDefProperty.

[samreid]

Some comments over here:

phetsims/axon#221 (comment)

It seems we are leaning toward using ColorDefProperty and not renaming the left hand side, so it would be like:

backgroundColorProperty = new ColorDefProperty('blue');

[samreid]

It seems it would be simpler and more useful to allow ColorDef constructor arguments, but to upconvert to a Color instance in the constructor, so the values would have a uniform interface, and you could do things like this:

backgroundColorProperty = new ColorDefProperty('blue');
backgroundColorProperty.value.darkerColor(0.5);

In that case, the value would always be a Color so it would make sense to call it ColorProperty. @jonathanolson and/or @pixelzoom does that sound reasonable to you?

[jonathanolson]

In that case, the value would always be a Color so it would make sense to call it ColorProperty. @jonathanolson and/or @pixelzoom does that sound reasonable to you?

Have we had Properties that do implicit type conversion before? That's also not desirable in some circumstances to have ColorDefProperty upconvert

[samreid]

Have we had Properties that do implicit type conversion before?

I don't think so.

[pixelzoom]

It seems it would be simpler and more useful to allow ColorDef constructor arguments, but to upconvert to a Color instance in the constructor, so the values would have a uniform interface ...

Would you do the same conversion for setValue? Or would you require setValue arg to be {Color|null}?

The downside of this conversion would be Color instantiation, something that may be undesirable in situations where a ColorProperty changes frequently.

[samreid]

Maybe we should schedule this for a PhET-iO developer discussion, because I agree that its best for our codebase to use ColorDef, but I wonder if it would be a clearer, simpler API for PhET-iO clients for it to always be a Color.

But I'd like to check in with @zepumph first. What do you recommend?

[pixelzoom]

There's been no progress on this issue for over a year. But we may want to defer until we discuss #1135 (revisit "color" and "paint").