-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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.
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
Color: Add 'colorSpace' argument for getters/setters #23392
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a few additional notes in comments here... I'll go into more detail in an upcoming documentation PR.
ce9eebf
to
eece875
Compare
Would you please explain why the Textures allows to define the color space per texture. Why can't we do this for The PR looks great, I'm just asking for a better understanding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really glad to see how this has come along! A few thoughts other thoughts:
-
In docs it should be very explicitly noted that the
Color.r/g/b
members are not in a reliable color space. Not only because of this legacy mode support but in the future if other wider gamut color spaces are supported then it will depend on the current working color space. -
Should functions like
Color.convertLinearToSRGB
log a deprecation warning when the legacy color path is enabled since they're inconsistent with the new color storage model? -
I assume we'll only be changing
WebGLRenderer.outputEncoding
tosRGBEncoding
by default once this legacy mode is disabled by default?
Overall I'm happy with what this has evolved in to. The API surface is consistent (aside from the Color.r/g/b fields) which was my original concern when improving the color spaces for the project.
Let me come back to this question once I've opened the documentation PR, I think my answer will not be very clear without that additional context. |
@gkjohnson I would rename the property:
|
Update — @Mugen87 to your question earlier, I believe that almost anything a user will 'do' with a Color instance should done in the working color space (Linear-sRGB, as defined in link above), and that's not the color space users will find in a color picker or their CSS stylesheets, which are usually sRGB. If we define |
As there are a few steps here, my suggested grouping of PRs would be: r138
r139
Wouldn't need to be r138 and r139 specifically of course, but basically I think these changes should be spread over at least two releases, with the breaking changes grouped together. |
The As I have said, the API to follow is: color.convertSRGBToLinearSRGB();
color.copySRGBToLinearSRGB( source_color ); You need a source color space and a destination color space. There are not that many variations, so hardwiring a separate method for each pair seems OK to me. // Alternatively, you can provide: color.convert( fromColorSpace, toColorSpace );
color.copy( source_color, fromColorSpace, toColorSpace ); |
This feels like a decision we need to make, not a foregone conclusion — I believe there are advantages to associating the RGB components of the Color class with the working color space (currently Linear-sRGB), as described above. Could you help me understand what advantages you see in the API you are suggesting, or the disadvantages in this PR? Am I correct in understanding that you'd like to have these convert/copy methods and not have these parameters on setters/getters? If we only offer methods that convert a color from A to B, it seems like we are still expecting all Color instances to be provided on materials in Linear-sRGB, but we are making it harder for users to do that — because each needs to be explicitly converted from sRGB. I would like this syntax to 'just work', without needing to be told the hex color is sRGB: const material = new MeshStandardMaterial( { color: 0x502020 } ); |
The statement is true. But, yes, the implication of my statement is it should stay that way. Just my humble opinion. :-)
Yes.
IMO, your approach results in too much magic behind the scenes. That is just my view. I know your concept is proposed with good-intention. // Good news, thanks to recent work by @gkjohnson:
|
I don't feel that enforcing color storage in a common color space is all that magic. If we're going to hope to ensure that a color is in our working color space by the type they get to a shader then we need to have some reliable way of knowing what space a color instance is in. Another option for enabling that is to store colorSpace on the Color instance itself but that requires converting the color to a common color space whenever an operation is done (lerp, add, etc) and converting before uploading to the shader. But that feels unnecessarily complicated. |
Yeah, unfortunately I think this way would get too complicated.
There'd be no magic in saying, "all three.js methods and properties using Color instances assume the color's RGB components are Linear-sRGB". It's not totally true yet — see The harder question is whether using sRGB for getters/setters (hex, CSS, HSL) and Linear-sRGB for floating-point RGB components is too much magic. Personally I think it's the right choice — A-Frame and R3F read/write sRGB hex colors, and internally convert to Linear-sRGB for THREE.Color instances. As far as I can tell Filament makes the color space a required argument when setting a color on a material. Users seem to manage well enough; it helps that it's hard to find a hex color that isn't already sRGB. But either way, it's a big change — see implications under Working with THREE.Color Instances in #23430. I'm hoping more people will weigh in on that part of the API. |
There will be confusion, IMO. Vertex colors in glTF are linear multipliers, while the base color in glTF is assumed to be encoded with the sRGB transfer function. You are trying to enforce a particular color space with |
The glTF specification's phrasing of "linear multipliers" is something I'd prefer to leave out of three.js. It's clearer to say that both three.js vertex colors,
The current state of three.js is that every time the user provides a color from virtually any other software, they must immediately do an explicit sRGB → Linear-sRGB conversion. The same is true for our own examples using |
@donmccurdy OK. I do not want to be a blocker. Your approach deserves a try. If others are happy with it, that is great. :-) |
@WestLangley thank you for being open on this. I don't want to force this through if it's divisive. Perhaps let's leave the PR up for a while and see if we can gather more feedback. The Pulling in a few people from #11337 — @bhouston and @zomboh do either of you have preferences here, or examples from experience in other tools? |
This makes sense and solves an issue I ran into with Three.js back in 2016: #8827 I think that in the future, we will likely start to support more output encodings rather than just sRGB, specifically HDR outputs, but that requires browser support first, which is coming: https://twitter.com/BenHouston3D/status/1481979159077339143 To speed up the transition to ColorManagement = true, we should likely have a warning for when it is off. I think that we should aim to actually just remove the idea of ColorManagement.enabled in the future because supporting multiple paths is annoying and this one is clearly right. If people want the old behaviour just use the ColorSpace specificers when setting/getting Color values. |
The above confuses me. I am so confused that renderer.outputEncoding is linear by default. I thought it was sRGB for the longest time? outputEncoding linear is simply wrong and will lead to screwed up color output. I guess over here at @Threekit, we would just always set outputEncoding = sRGB and forgot that Three.js was using the wrong default? |
Ah, I understand the mistake. "Encodings" and "ColorSpaces" are being mixed together. Originally we thought of these options:
Per: #11076 (comment) But I think that the above dichotomy isn't completely accurate. RGBE, RGBM, LogLUV are the same linear HDR colorspace but represented in 4 bytes (which some clipping - maybe that makes them different colorspaces?.) Where as CMYK is an LDR color space without Alpha. I think we need better clarify on ColorSpace and Encoding usage. |
There are fewer encodings left now (see #23392 (comment)), so I agree with @WestLangley's previous suggestion that sRGBEncoding should be removed and replaced with
I think we've all agreed for a while that the default should be changed to sRGB – but I'm at least partly responsible for why that hasn't happened yet, as I've been pushing to wait and change the output defaults and the input defaults (for hex and CSS colors) at the same time: #22275 (comment). |
From what I can understand it's already here to some extent: https://developer.chrome.com/blog/new-in-chrome-94/#canvas-colorspace |
After being sparked by Don's tween earlier I've (yet again) spent far too long going down the color rabbit hole.. In the UI/Design side we've had color pains in the web resulting in some amazing efforts to not only educate. but also provide solutions. Even with all that I still struggle to really get what's going on or know what the "right" action is to take.. Converting in/blending/how it'll react with lights etc. On top of all the other things a person faces when trying something new. So in regards to if you should convert with
|
Unfortunately limited to Canvas 2D contexts – no support for WebGL yet. But eventually it will happen, in WebGPU if not in WebGL.
Thanks @DennisSmolek! Improving defaults (for both correctness and ease of use) is a goal of this PR, along with the related changes to |
d001e49
to
0b1dc14
Compare
0b1dc14
to
09c1997
Compare
NOTE: I've just rewritten the first few paragraphs of this PR's description, to reflect the feedback and changes to the PR. Specifically, calling out the fact that only hexadecimal and CSS input are considered sRGB and converted. @Mugen87 after some more discussion and mulling over, I think I'd like to delay the question of adding In my opinion, this PR brings THREE.Color getter/setter APIs to where they ought to be in a linear workflow, while accommodating conventions our users expect from experience with CSS and Blender. Certain naming choices (e.g. |
Does @WestLangley approve? 😅 |
Regarding adding
I guess I'm just not sure I see the benefit. Another thing that I don't think we've really thought about, though I suppose it's longer term, is how vertex colors are handled. It's easy when we say Linear sRGB is the working color space and they're stored as such but what happens when P3 or ACES are the working color space? If vertex colors are going to be interpolated correctly in a shader or with animations then they must have either a color space associated with them or be implicitly converted to the correct color space. I think a solution for that can wait but I thought I'd mention it in the interest of being thorough. edit: I see now we're waiting on |
A few quick thoughts. If we added such a property, its default should certainly be the working color space, Linear-sRGB (not sRGB). Automatic conversions should be generally avoided — if an operation is valid on the given color space it should run, otherwise the operation should probably warn or fail — to avoid a whole mess of complexity. For instances managed by three.js (like I do think use good cases exist — e.g. working with LightProbes or LUTs that don't need to be constrained to the sRGB gamut, perhaps? But I don't feel sure about those use cases yet, and I don't think easier Linear-sRGB
I think it's fair to say that any wide-gamut working color space will require considerable care; nothing in any 3D model format will work 'out of the box'. 😢 But ultimately we'd expect vertex colors to end up in the working color space, and it's straightforward to pre-process glTF files for that purpose. |
Thanks! |
I guess to that end I'm not seeing what the real benefit is to the flag. It seems to make things more confusing. I just imagine users wondering why they can't interpolate between two colors. And if two colors must be in the same color space to be operated on together then we're not adhering very strictly to our concept of a "working color space" across three.js, then.
I want to believe there's a better solution here but I acknowledge that it's a future problem. Something like a |
Perhaps a line item should be added to #23614 to think about handling of Color BufferAttributes? |
I would say uint8 is not ideal for Linear-sRGB vertex color data, but uint16 and float32 are fine. glTF is (as far as I know?) the only format we have that supports anything more compact than float32, and it specifies Linear-sRGB vertex colors. So I feel like sRGB vertex colors in uint8 buffers don't seem to justify a new BufferAttribute API and shader permutations here. To the extend that we can avoid conversions and store data in working color space, that is a very valuable thing. Perhaps a new issue if there's more to consider on that side? I'm not ready to think about vertex colors in P3 or ACEScg quite yet I'm afraid. 😓 |
I agree -- this isn't just about file formats, though. It's about enabling users to consistently use, operate on, and render color in three.js with or without existing model formats.
I just suggest adding it to #23614 in order to keep everything consolidated understanding that it's not an immediate priority. Perhaps as the last line item or as a (2.0+) category bullet. |
Sounds good! We'll have to explore more, but added this under 2.x in #23614 for now. |
* ColorManagement: Initial commit * Color: revert to original CSS whitespace. * Constants: NonColorData → NoColorSpace
* ColorManagement: Initial commit * Color: revert to original CSS whitespace. * Constants: NonColorData → NoColorSpace
Related
Description
Certain ways of expressing colors — hexadecimal and CSS strings — can safely be assumed to be in the sRGB color space. Hexadecimal syntax implies sRGB by convention, and CSS colors are always sRGB except where otherwise specified in the (very recent) CSS Color Module Level 4.
However, colors passed to three.js shader must be Linear-sRGB. Because hexadecimal and CSS colors are not (by convention or specification) in the required color space, I believe we should take the step of converting them from sRGB to Linear-sRGB on the user's behalf. Importantly, this ensures that hexadecimal colors from a color picker, or CSS colors from a stylesheet, can be used directly in three.js APIs. They will appear correctly without manual conversion, assuming
.outputEncoding = sRGBEncoding
is also set.The changes provided by this PR are disabled by default (
ColorManagement.legacyMode = true
), but can be enabled by default (in a future release) at the same time as we change the default value ofrenderer.outputEncoding
tosRGBEncoding
, minimizing impact of the output encoding change on the remaining users who haven't yet enabled it.As discussed in #23614,
.outputEncoding
should perhaps also be renamed to.outputColorSpace
at this time.RGB and HSL components (e.g.
color.r
) continue to be Linear-sRGB ("Linear"), and can be manipulated exactly as before. I understand this may seem like an inconsistency, but it's exactly how Blender's color picker works as well, and ensures that lerping and keyframe animation continue to execute in Linear-sRGB color space.Changes
SRGBColorSpace
andLinearSRGBColorSpace
enumscolorSpace
argument to THREE.Color getters/settersColorManagement
API. Only its.legacyMode
flag is intended as a public API at this time. Other methods or properties may be added to support wide-gamut workflows in the future.Effects
renderer.outputEncoding=sRGB
, and so hard-coded colors (like background and ground colors) are implicitly Linear-sRGB... These will need to be updated (or their appearance will change) with color management enabled.basicMaterial.color.setHex( ..., sRGBColorSpace )
background.setHex( ..., sRGBColorSpace )
Next steps
An upcoming PR will add documentation explaining what is meant by "working color space", and how that relates to choices of input and output color spaces. That documentation is meant to be a significantly expanded version of the content at https://www.donmccurdy.com/2020/06/17/color-management-in-threejs/.
/cc @gkjohnson @WestLangley