Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for predefined input/output color spaces #117

Open
donmccurdy opened this issue Oct 6, 2023 · 4 comments · May be fixed by #123
Open

Support for predefined input/output color spaces #117

donmccurdy opened this issue Oct 6, 2023 · 4 comments · May be fixed by #123

Comments

@donmccurdy
Copy link

In three.js, the following code ...

const material = new THREE.MeshBasicMaterial();

gui.addColor(material, 'color');

...will display a different color in the picker than the material starts with, and will transfer the color from the picker to the material incorrectly. To fix that, we need to write:

const params = {
  color: material.color.getHex(THREE.SRGBColorSpace),
};

gui.addColor(params, 'color', (value) => {
  material.setHex(value, THREE.SRGBColorSpace);
});

The THREE.SRGBColorSpace argument is optional – it's implied for hexadecimal and CSS string inputs — but included above for clarity. The browser's color picker uses sRGB, and three.js stores RGB components as Linear-sRGB.

Would you be interested in a PR adding a shorthand for conversion to/from predefined color spaces? I'm aware that the HTML color picker is always sRGB, but thought it could be nice to support input/output to a different color space as:

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

See https://discourse.threejs.org/t/updates-to-color-management-in-three-js-r152/50791 for a bit more background on color management in recent three.js versions. No worries if this feels out of scope for lil-gui!
@georgealways
Copy link
Owner

Oh that is a bummer! Basically back where we were at with dat.gui ...

It does feel a little out of scope for colorspace conversion to live in lil-gui, especially since we got rid of HSL etc. It would be nice to provide a shorthand for this somehow though. Maybe there could be a way for you to provide the conversions back and forth from hex?

@donmccurdy
Copy link
Author

donmccurdy commented Oct 8, 2023 •
edited
Loading

Maybe there could be a way for you to provide the conversions back and forth from hex?

Interesting! Something like this maybe?

gui.addColor(material, color).transform(
  (hex) => linearToSRGB(hex), 
  (hex) => SRGBToLinear(hex)
);

Definitely that would work. I think it's more intuitive than my example above, though perhaps not more concise.

Another idea: If lil-gui were able to detect getHex/setHex methods on objects and prefer those to setting .r .g .b components directly, that would avoid the issue. three.js assumes hex colors are sRGB unless otherwise specified (as does Blender).

@georgealways
Copy link
Owner

Hey apologies for the delay, just getting back from a trip.

I had a similar idea re: getHex/setHex ... maybe lil-gui could treat objects with the shape { getHex(), setHex() } as a "custom" color format. It's not too far off from the way colors are already handled in https://github.com/georgealways/lil-gui/blob/master/src/utils/getColorFormat.js.

The one thing is that lil-gui uses hex strings, not integers, as the "interchange" format. I think I did it that way just to make it easy to pass to the color input. Internally, the conversion methods are called fromHexString and toHexString.

I'm not against changing any of that though, none of it is user-facing. I'd like for lil-gui + THREE.Color to be a one-liner.

@donmccurdy donmccurdy linked a pull request Nov 8, 2023 that will close this issue
Open
@donmccurdy
Copy link
Author

donmccurdy commented Nov 8, 2023 •
edited
Loading

Filed #123 -- let me know what you think!

I used getRGB / setRGB instead (a little simpler to code) but I think getHex / setHex would work just as well if you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for THREE.ColorManagement donmccurdy/lil-gui
2 participants
@georgealways @donmccurdy