Add support for THREE.ColorManagement

[donmccurdy]

@georgealways does this look OK as an approach? Unfortunately it is fairly three.js-specific, and I'm not sure how to avoid that. Normally color space argument to getHex / setHex would be THREE.SRGBColorSpace or THREE.LinearSRGBColorSpace, but the actual values of those enums are intentionally matched to constants from CSS Color Module Level 4, so we can safely hard-code them here.

I'm happy to include unit tests or any other changes, if the general approach is acceptable.

• Fixes Support for predefined input/output color spaces #117

[georgealways]

Hey Don, thanks so much for this! It does feel a bit too three-specific in its current state though. Like CLASS would be better titled THREE_COLOR. Some thinking aloud here...

It seems like the THREE.Color.get/setHex methods do the linear/srgb conversion automatically. Maybe we can use those instead? Then in match, we could do a duck-type check for the two get/set methods instead of v.isColor.

It becomes a bit more agnostic, and the pattern feels more replicable for other libs: if your color object provides get/setHex methods that accept sRGB integers, then things just work out of the box.

const CLASS = {
	isPrimitive: false,
	match: v => v.getHex && v.setHex,
	fromHexString( string, target ) {
		target.setHex( INT.fromHexString( string ) );
	},
	toHexString( target ) {
		return INT.toHexString( target.getHex() );
	}
};

Devil's advocate: in that universe, people working with linear color in other libs would probably just continue to use the onChange pattern. While in three, it would just work automatically somehow.

Alternate idea, but kind of brutish: what about an "extras" file you can import to make addColor behave correctly with three?

import GUI from 'lil-gui';

const addColorOld = GUI.prototype.addColor;

GUI.prototype.addColor = function( obj, prop ) {
	const value = obj[ prop ];
	if ( !value.isColor ) return addColorOld.call( this, obj, prop );
	const temp = { [ prop ]: value.getHex() };
	return addColorOld.call( this, temp, prop ).onChange( v => value.setHex( v ) );
};

Then saying something like import 'lil-gui/extras/three-color' would essentially follow the onChange pattern for you automatically. Couple of gross things though: 1. it's a silent side-effect, and 2. where does that file live?

Anyway, apologies for the essay. Curious for your thoughts on either approach. And thanks for your help! Would be great to get this in the next release.

[donmccurdy]

I'm happy with the getHex/setHex approach. I'll update the PR to see what that looks like.

For me, a new import has a similarly high barrier to discovery as something like this...

gui.addColor(material, 'color', { sourceColorSpace: 'srgb-linear' });

... with the added disadvantages of being library-specific, and prone to challenges with packaging so that the side effects and imports will work in every bundler. So I'm nervous about that.

[donmccurdy]

I'd been thinking I needed to use rgbScale somehow for the getHex/setHex methods, but probably not -- that simplifies a bit. Updated the PR, let me know how this seems!

Devil's advocate: in that universe, people working with linear color in other libs would probably just continue to use the onChange pattern...

For better or worse... I don't know of other relevant JS libraries that make the distinction at all, but it's pretty important for what we're doing, and will become more important with Display P3 support in WebGL, WebGPU, and CSS Color Module Level 4.

Hopefully by the time other libraries are dealing with this, HTML's color picker will just support more color spaces. 🤞

[georgealways]

Hey @donmccurdy thanks for being patient on this—I sat down to start integrating this and realized I couldn't reproduce the original issue you were describing anymore.

The color picker seems to stay in sync with a MeshBasicMaterial if I do this:

new GUI().addColor( material, 'color' );

Will you take a look at that test page I just pushed and tell me if I'm missing something? I could have sworn I reproduced this when you first posted...

[donmccurdy]

Thanks @georgealways! The test page is correct, and fails if you remove the changes in this PR. The mesh and the background stay in sync but the color picker doesn't for me. Testing #4285f4.

before:

after:

[georgealways]

That would do it! I must be short on oxygen to the brain today, I somehow failed to realize I was still on the PR branch 😅. That test case was also a little flawed since it was using the material's color.getHexString() to color the div, so they would always match...

Otherwise, no real changes in this latest batch of commits. I'll put some comments inline. Mostly I'd like to get your eyes on the documentation.

[georgealways]

might want to delete this example before we merge, but here's what i was really after...

[georgealways]

this part of the Migrating section talks about how lil-gui allows you to forego the old onChange pattern from dat.gui. i'm thinking about changing this example to use a different lib with 0-1 RGB values (maybe PIXI?). this way we don't have to gloss over colorspaces.