instrumented color Property can only support values of type scenery.Color

[pixelzoom]

This:

Color.ColorIO = new IOType( 'ColorIO', {
  valueType: Color,
  documentation: 'A color, with rgba',
  toStateObject: color => color.toStateObject(),
  fromStateObject: stateObject => {
    return stateObject.hex ? new Color( stateObject.hex ) : new Color( stateObject.r, stateObject.g, stateObject.b, stateObject.a );
  }
} );

... requires the value to be a Color, and makes it impossible to use other valid color values (a CSS color {string} or null) with an instrumented color Property.

E.g. this:

new Property( 'rgb( 255, 200 ,200 )', {
  phetioType: Color.ColorIO,
  tandem: ...
} );

fails with this:

Color.js:1076 Uncaught TypeError: color.toStateObject is not a function
    at Object.toStateObject (Color.js:1076)
    at IOType.toStateObject (IOType.js:125)
    at PhetioStateEngine.getValueJSON (PhetioStateEngine.js:106)
    at PhetioStateEngine.js:167
    at Array.forEach (<anonymous>)
    at PhetioStateEngine.getState (PhetioStateEngine.js:163)
    at PhetioStateEngine.initialize (PhetioStateEngine.js:475)
    at phetioEngine.js:151
    at TinyEmitter.emit (TinyEmitter.js:71)
    at Property._notifyListeners (Property.js:280)

This seems like an undesirable restriction on instrumented color Properties. I can workaround it by replacing strings with Color instances, but not sure what I'll do if I need null.

[zepumph]

This seems intimately related to #948 (comment). I don't this it is ColorIO's responsibility to serialize ColorDefIO. We should instead just create a ColorDefIO to manage that when that is what your Property's value should be.

and makes it impossible to use other valid color values

these are ColorDef values, not Color values. ColorIO is the IOType for Color.js.

I'm happy to discuss creating a ColorDefIO to support the case that you are thinking above, but I feel like we should pick up #948 (comment) first to see what *Property classes we need (have?) first.

[pixelzoom]

and makes it impossible to use other valid color values

these are ColorDef values, not Color values. ColorIO is the IOType for Color.js.

I didn't say they were Color values, I said they were color values. Case is significant in written communication.

[pixelzoom]

ColorDefIO is not appropriate. ColorDef is {null|string|Color|Property.<null|string|Color>}. Since my Property shouldn't take another Property as a value, I don't want {Property.<ColorDef>}, I want {Property.<null|string|Color>}.

What is the current recommended method of instrumenting a {Property.<null|string|Color>} ?

[samreid]

If {Property.<null|string|Color>} is necessary, then we would support it with a custom IO type. Or we could create an Or type, then create Property<Nullable<Or(stringIO,ColorIO)>>. But it would probably be much easier if we could constrain the use case to be Property<null|Color>.

One example was given as:

new Property( 'rgb( 255, 200 ,200 )')

Could we use this instead?

new Property( new Color(255, 200 ,200 ))

[pixelzoom]

But it would probably be much easier if we could constrain the use case to be Property<null|Color>.

If CSS color strings are a valid color value, then I should be able to use them with a Property. But I'd like to hear @jonathanolson's opinion.

[jonathanolson]

I can workaround it by replacing strings with Color instances, but not sure what I'll do if I need null.

Color.TRANSPARENT

ColorDefIO is not appropriate.

ColorDefIO would presumably be able to serialize the subset of {Property.<null|string|Color>}. It wouldn't be {Property.<ColorDef>}, but instead just a {ColorDef}.

It seems like here, we should ideally have a way of handling ColorDef nicely (which would probably handle this case), but if we want to do something atypical/custom, then a custom IO type seems appropriate.

[pixelzoom]

@jonathanolson here's a valid {Property.<ColorDef>}:

new Property( new Property( new Color( 255, 230, 220 ) ) );

Do things like Node.fill support this? If not, then phetioType: PropertyIO(ColorDefIO) doesn't seem like what we want.

[jonathanolson]

I was recommending phetioType: ColorDefIO, unless you need to restrict it to JUST be a Property. Otherwise presumably we'd need something more custom? Or presumably we have a way to do union types?

[samreid]

PhET-iO does not support union types at the moment.

[pixelzoom]

I feel like we're going in circles here. Going back to the beginning... I have an instrumented Property whose value is restricted to {null|string|Color}, not {ColorDef}. How do I do that?

And yes, I know that I could restrict this to {Property.<Color>} and use phetioType: ColorIO. But I'm looking for a solution, not a workaround.

[samreid]

I tried to answer that in #1115 (comment). Should we try one of those options?

[pixelzoom]

So would it be phetioType: PropertyIO( NullableIO( Or( StringIO, ColorIO ) ) ) ? This is starting to look like Lisp.

I think I'll just stop using CSS color strings and start using Color exclusively. More objects, but whatever.

Any reason to keep this issue open, or OK to just close?

[samreid]

phetioType: PropertyIO( NullableIO( Or( StringIO, ColorIO ) ) ) would require us creating support for union types in PhET-iO, which we don't have at the moment, as mentioned in #1115 (comment).

The first sentence from #1115 (comment) is:

If {Property.<null|string|Color>} is necessary, then we would support it with a custom IO type.

That could look something like:

PropertyIO(NullColorStringIO)

// or
PropertyIO(NullableIO(ColorStringIO))

In any case PropertyIO(ColorIO) is fully supported and would be nice to use unless null|string|Color is really useful in this context.

[pixelzoom]

Thanks. I'll convert to Color and use PropertyIO(ColorIO). Closing.

[pixelzoom]

Reopening.

I converted Fourier to Color (or thought I did), and then the State wrapper started to fail,
see phetsims/fourier-making-waves#13. I was using valid color values. I've already spent time converting CSS strings to Color. Now I have to spend time debugging. Then I have to remember when to avoid using CSS strings so I don't introduce new problems. All of this to satisfy a PhET-iO limitation.

If we're going to continue to use CSS strings for Colors, then it seems like we need an instrumented "color Property" that accepts all valid "color" values. Maybe that's described in phetsims/axon#221 (comment), maybe not.

Assigning to the iO team. In the meantime, I'll continue to convert/debug.

[samreid]

The previous comment sounds reasonable to me, thanks for working on this! @pixelzoom can you please review the last 3 comments and recommend if anything remains to be done for this issue?

[pixelzoom]

I've reread the previous 3 comments multiple times, and I'm not at all clear on what has been proposed/decided. I'm still left with these questions:

• When I have a Property whose value is a "color" that needs to be instrumented, what value types are supported, and how should I specify its phetioType?

• Will there be a type-specific Property to support instrumented "color" Properties, or do I need to use new Property + some complicated phetioType expression?

• How will the Properties of ColorProfile be instrumented?

[zepumph]

When I have a Property whose value is a "color" that needs to be instrumented, what value types are supported, and how should I specify its phetioType?

In almost all cases, it will not have a valueType of ColorDefIO, because the Property doesn't support a Property VALUE of another Property. ColorDefIO will be useful when serializing a type that has a member variable of type ColorDef (like in scenery). So instead you will instrument it to the needs of your Property:

• Simplest but most rigid: PropertyIO<ColorIO>
• More flexible: PropertyIO<OrIO<ColorIO,StringIO>>
• Most flexible: PropertyIO<NullableIO<OrIO<ColorIO,StringIO>>>

Alternatively, we could use ColorDefIO as the Property value type (PropertyIO<ColorDefIO>), as it supports all the above, but it is is a mismatch and misnomer, and likely not ideal for maintainability.

Will there be a type-specific Property to support instrumented "color" Properties, or do I need to use new Property + some complicated phetioType expression?

ColorProperty could become that with this patch, but then it is more like "ColorValue" Property. Basically you are asking to typify something that doesn't currently have a type in the project. I feel totally good about the above patch, but I think we should ask @samreid before committing.

Index: js/util/ColorProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/util/ColorProperty.js b/js/util/ColorProperty.js
--- a/js/util/ColorProperty.js	(revision c0011bdb63f8386fccefb92280bbc03d48900338)
+++ b/js/util/ColorProperty.js	(date 1609357739678)
@@ -2,8 +2,12 @@
 
 import Property from '../../../axon/js/Property.js';
 import merge from '../../../phet-core/js/merge.js';
-import Color from './Color.js';
+import NullableIO from '../../../tandem/js/types/NullableIO.js';
+import OrIO from '../../../tandem/js/types/OrIO.js';
+import StringIO from '../../../tandem/js/types/StringIO.js';
 import scenery from '../scenery.js';
+import Color from './Color.js';
+import ColorDef from './ColorDef.js';
 
 /**
  * Convenience type for creating Property<Color>
@@ -21,8 +25,8 @@
     }
 
     options = merge( {
-      valueType: Color,
-      phetioType: Property.PropertyIO( Color.ColorIO )
+      isValidValue: color => ColorDef.isColorDef( color ) && !( color instanceof Property ),
+      phetioType: Property.PropertyIO( NullableIO( OrIO( [ Color.ColorIO, StringIO ] ) ) )
     }, options );
     super( color, options );
   }

How will the Properties of ColorProfile be instrumented?

Most likely as PropertyIO<OrIO<ColorIO,StringIO>>, (I'm not sure if null is allowed in ColorProfile colors), but that will all be solidified and implemented in phetsims/scenery-phet#515.

Does all this make more sense @pixelzoom? If so, please toss over to @samreid to potentially commit the above patch if he wants to.

[pixelzoom]

I'm still not OK with this. I don't believe @jonathanolson would be either.

ColorDefIO (as currently defined) does not belong in ColorDef. ColorDef.isColorDef and ColorDefIO have different valid value types. The former supports Property.<null|string|Color>, while the latter does not. And it results in having to introduce tests like this one that you've proposed in the above patch for ColorProperty:

+ isValidValue: color => ColorDef.isColorDef( color ) && !( color instanceof Property ),

So while sticking ColorDefIO in ColorDef may be convenient for addressing this issue, imo it's just plain wrong. We need a type definition that is {Color|string|null}, and a type-specific Property for that type definition. And that shouldn't be shoehorned into ColorDef.

[pixelzoom]

Here's a sketch of ColorValue, ColorValueIO, and ColorValueProperty, all of which deal with values of type {null|string|Color}. I'm not sure what supertype should be for ColorValueO.

const ColorValue = {
  ...
  isColorValue( value ) {
    return color === null || typeof color === 'string' || color instanceof Color;
  }
}

ColorValue.ColorValueO = new IOType( 'ColorValueIO', {
  isValidValue: ColorValue.isColorValue,
  supertype: ?????
} ); 

class ColorValueProperty {
    ...
    phetioType: Property.PropertyIO( ColorValue.ColorValueIO )
} 

As a bonus, duplication in ColorDef.isColorDef can be removed like this:

  isColorDef( color ) {
    return ColorValue.isColorValue( color ) || 
           ( color instanceof Property && ColorValue.isColorValue( color.value )  );
  }

It would also be nice to have something that validates that {string} is a valid CSS color string. That's currently lacking from Color and ColorDef.

EDIT: I also think that ColorProperty (as currently implemented) should be renamed to ColorDefProperty. It takes a value of type ColorDef.

[jonathanolson]

It seems like the implementation hurdle to just have a ColorDefIO that can hold reference-io'd Property values might be too much? It seems great from a usability perspective.

It would also be nice to have something that validates that {string} is a valid CSS color string. That's currently lacking from Color and ColorDef.

Added this in the above commit on the Color side.

[samreid]

Self-unassigning until we hear from @zepumph.

[pixelzoom]

We may want to defer this until we discuss #1135 (revisit "color" and "paint").

[zepumph]

Sounds good to me!