-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
Add THREE.ColorManagement #22346
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
Add THREE.ColorManagement #22346
Conversation
|
Would it be an option to introduce |
|
I'm not sure that configuring every import { sRGBEncoding, MeshPhysicalMaterial } from 'three';
const material = new MeshPhysicalMaterial();
material.color.encoding = sRGBEncoding;
material.emissive.encoding = sRGBEncoding;
material.sheen.encoding = sRGBEncoding;
material.attenuationTint.encoding = sRGBEncoding;
material.specularTint.encoding = sRGBEncoding;Is there an advantage in deferring the sRGB → linear conversion to the shader? |
|
We could introduce a variable like I would try to treat textures and colors identically since I think it's easier to understand for the user. Besides, it's also more flexible since you can work with different color spaces if necessary.
No it would just be the result of treating color properties similar to textures. |
How would |
|
The default encoding would be global. So loaders have to ensure to set the correct encoding value according to the map type. three.js/examples/jsm/loaders/GLTFLoader.js Lines 3169 to 3170 in 7fb600b
If a normal map is manually loaded via |
|
If |
|
If at all possible I think it would be great to avoid global settings that affect all renderers because there will be cases with multiple renderers on the same page that want to handle this type of color management differently. Looking at Unity's documentation on Linear vs Gamma Workflow it seems the following assumptions are made:
Would this type of setup not work? With the way Unity works there's no need to change a default color space of loaded textures or change the color space that a texture is assigned at all for switching workflows. I think it's important that loaders explicitly label the color space of a texture as it's known to be. I feel like a three.js version of Unity's color space workflow management would look something like this: renderer.linearWorkflow = true; |
|
My only concerns with
This PR does not allow mixed usage on the same page, but it does provide simple answers to the questions above. 😕 |
This sounds like an unusual situation - I don't think we should allow it to block the PR unless we can identify it as something that happens reasonably often. In any case, it should be possible to handle this by setting |
I know this has been discussed for a long time and I'd like to see it fixed as much as everyone else. I'm just asking that a different solution that doesn't involve globals be considered and that I've made a suggestion for based on an existing solution from a large game engine -- generally globals I think are a poor pattern, anyway. My work also has multiple teams working on different embed-able viewers for 3d visualizations, maps, etc so multiple renderers on the same page doesn't seem so out there in my work.
I don't think this is the case. If the element relies on a certain setting for color management are you suggesting people go in and manipulate all of the model-viewer settings? All that aside I just feel the model Unity uses is a lot more intuitive. If a texture is known to be Linear or sRGB it should be labeled as such and the renderer should be responsible for whether or how to interpret that labeling. I also feel Colors should be consistent and not yield different values from what's passed into the constructor when you read the r, g, b values.
I think it's safe to assume that most users will keep linear workflow enabled by default (considering color space knowledge is needed to understand why you'd switch to gamma in the first place) so I think it's rare that a keyframe track might be needed to be switched to gamma color interpolation but we could add a flag for this to KeyframeTrack. Even if the user doesn't toggle it the visual differences due to linear vs srgb interpolation I expect to be subtle.
This is a good point though in the common case (Standard, Phong Material) only two conversions would be required and I'd think that cost would be dwarfed by the other calculations being done in the shader. Same for the Physical Material. MeshBasicMaterial might have a larger difference but perhaps it could be benchmarked with GPU timing. The other solutions that come to mind are to convert the colors every frame on upload or cache the linear representation of a color on the Color instance and lazily update it only if it's changed (check if the r, g, b values have changed since the last update). I'm wondering how problematic the shader approach would really be, though, considering the conversion is already done for textures. |
|
Perhaps we can support linear and gamma workflows on the same page, i.e. without requiring global state to switch between them. I'm not excited to add a lot of per-frame conversions to enable that — I don't think it's worth the cost or complexity, Unity can do this at compile time and we can't — but let's assume for the sake of argument that we find a performant way forward, it's at least a nice-to-have. Even then, with either a linear or gamma workflow, colors a user provides really ought to be assumed to be sRGB — hex codes, color textures, perhaps more. Users want that same color to appear on the display, +/- any lighting or tone mapping, as it would in CSS. There's no good reason not to do that, except backwards compatibility, and so I don't think it's useful to try to support both "assume all inputs are linear" and "assume all inputs are sRGB" on the same page. It's OK to make that a global option to help with migration; perhaps someday we can make it the default; and perhaps much later we can remove the alternative entirely. tl;dr — we can probably support both linear and gamma workflows on the same page without also supporting two different sets of assumptions about input colors simultaneously. |
|
As a strawman API, how about: // Once enabled, colors and color texture slots are assumed to be sRGB. Whether
// using a linear or gamma workflow, this should be enabled as best practice.
THREE.ColorManagement.enabled = true;
// Linear workflow.
renderer.internalEncoding = THREE.LinearEncoding;
renderer.outputEncoding = THREE.sRGBEncoding;
// Linear workflow + post-processing.
renderer.internalEncoding = THREE.LinearEncoding;
renderer.outputEncoding = THREE.LinearEncoding;
// application post-processing ...
// ... then linear -> sRGB pass
// Gamma workflow.
renderer.internalEncoding = THREE.sRGBEncoding;
renderer.outputEncoding = THREE.sRGBEncoding;From there, we have some flexibility about how to implement ... if the renderer's selected internal encoding does not match the selected input encoding, it does a conversion. Whether the Color instance internally stores sRGB values, or converts them in setters as in this PR, is an orthogonal choice. |
How are you expecting Unity to do this at compile time? With user code dealing in SRGB colors I'm not sure how compiling the code could make it so no conversion has to happen, though maybe materials with colors that don't change could optimize that away. From the above-linked docs it sounds like Unity uses built-in driver features to mark textures as SRGB so the GPU samples the textures in the appropriate color space in the shader so I would think it would use that at run time, as well, even after compilation considering API support is required for its use. WebGL2 has the same capability which might be worth looking at the performance benefits of if we're worried about unnecessary shader SRGB <-> Linear conversions.
At a high level the API looks good. I do think I'd expect the ColorManagement.enabled flag to be temporary (for however long term that winds up meaning) and an easy "escape hatch" to prior behavior while any unexpected rough edges are polished for the new path over subsequent releases. If it's the more clear and recommended way is there not a reason to set it to It's a small nit but term And last thing this wouldn't be needed I don't think: // Linear workflow + post-processing.
renderer.internalEncoding = THREE.LinearEncoding;
renderer.outputEncoding = THREE.LinearEncoding;
// application post-processing ...
// ... then linear -> sRGB passShaders render to the specified color space of a given render target meaning .outputEncoding is ignored, anyway. And the postprocessing package, for example, implicitly adds a gamma correction pass based on the outputEncoding specified by the renderer (see "Output Encoding" section in the docs). After some of these changes I'm having a hard time thinking of why someone would want to set the outputEncoding to Linear so it's possible we could remove that at some point, as well. |
I guess what I mean is, I'd be shocked to learn that Unity stores every color the user gives it as sRGB values and converts all of them to linear on every single frame (whether on CPU or in GLSL). For textures, yeah, this is fine and hardware has methods to accelerate it. But for color uniforms, surely not?
I don't think I'd want anything this big enabled by default until it has been available and tested for a few releases. If we want to enable it by default after that, we could. |
In Unity you cannot manipulate the RGB values of a color on a material or uniform directly. Instead you have to set the whole color itself: material.color = new Color(); // good
material.color.r = 1.0; // badSo when the whole color is set is likely when it does the conversion or perhaps it marks it as dirty and defers it to when uniforms are being updated.
Okay I agree -- I'm getting ahead of things. I'm excited to see this (hopefully) making progress :D |
|
Maybe a material.colorNode = new ColorSpaceNode( ColorSpaceNode.SRGB_TO_LINEAR. colorNode ); |
Uniforms are cached so the conversion would not happen per frame but only when the value changes. The relevant code section for color uniforms is: three.js/src/renderers/webgl/WebGLUniforms.js Lines 215 to 227 in 5b8b9c6
So I would implement the linear conversion right before |
|
@sunag I'm inclined to say that color management should not be a per-material setting (i.e. no @Mugen87 that's great news, thanks! I think there's a path forward that addresses the issues @gkjohnson mentioned in #22346 (comment) then:
With that approach, we don't need the // (1) Gamma workflow. (Temporary default for testing)
renderer.workflow = THREE.GammaWorkflow;
renderer.outputEncoding = THREE.LinearEncoding;
// (2) Linear workflow. (Long-term default, after testing)
renderer.workflow = THREE.LinearWorkflow;
renderer.outputEncoding = THREE.sRGBEncoding;
// (3) Linear workflow + post-processing.
renderer.workflow = THREE.LinearWorkflow;
renderer.outputEncoding = THREE.LinearEncoding; // followed by linear→sRGB post-processingI'm not sure about that API though, two questions: Is Is what we do by default today actually a gamma workflow? I don't think it is. If I set // (a)
renderer.colorManagement: LinearColorManagement | GammaColorManagament | LegacyColorManagement;
// (b)
renderer.workflow: LinearWorkflow | GammaWorkflow | LegacyWorkflow; |
I agree, in fact |
Hm I'm seeing at least one obstacle here. Sometimes we scale Color instances by an intensity — RoomEnvironment is a good example. If we start making the assumption that Color values are sRGB, then scaling them directly will have very different results. This also occurs in // before
LinearToSRGB( SRGBToLinear( 1 ) * 500 )
13.999173208840062
// after
LinearToSRGB( SRGBToLinear( 1 * 500 ) )
499.9507076006699This applies to methods on the Color class like
|
Presumably multiplying a color by a scalar is only valid for linear colors? So if the color is assumed to always be sRGB then we would have to convert to linear, multiply the scalar, then convert back: multiplyScalar( s ) {
this.r = SRGBToLinear( this.r ) * s;
this.g = SRGBToLinear( this.g ) * s;
this.b = SRGBToLinear( this.b ) * s;
return this.convertLinearToSRGB();
}As for lerp( color, alpha ) {
const xr = SRGBToLinear( this.r );
const xg = SRGBToLinear( this.g );
const xb = SRGBToLinear( this.b );
const yr = SRGBToLinear( color.r );
const yg = SRGBToLinear( color.g );
const yb = SRGBToLinear( color.b );
this.r = xr + ( yr - xr ) * alpha;
this.g = xg + ( yg - xg ) * alpha;
this.b = xb + ( yb - xb ) * alpha;
return this.convertLinearToSRGB();
} |
I think BTW: Couldn't Eventually the default should be If set to |
|
I like the suggested design but want to mentioned that the issues in Or if the color management is no API of the renderer but indeed a new global thing that can be accessed in |
It's more intuitive but less correct. And with @WestLangley's suggestion that we may want to eventually allow working color spaces that are not sRGB or Linear, a two-state boolean is probably not future-proof enough. Perhaps: // (a) Global default with local override.
THREE.Color.defaultColorSpace = sRGBColorSpace;
const color = new THREE.Color();
color.colorSpace = AdobeRGBColorSpace;
// (b) Renderer-level configuration.
renderer.inputColorSpace = sRGBColorSpace;
renderer.outputColorSpace = sRGBColorSpace;But the problems with (b) in #22346 (comment) remain unsolved – if we want to support other color spaces, then |
Make sense! Option a) looks most promising to me. |
This is a similar issue to the color keyframes where you effectively want the interpolation and math operations to happen in Linear color space rather than sRGB, right? As another data point I just tested Unity and it seems that the Lerp function does not change based on the selected project color management setting. Ie
As I've mentioned above I'm very weary of these types of global state switches for initializers unless they're considered temporary and discouraged from use in the long term. I'd really like a solution that doesn't allow the user to toggle global switches where possible -- at least those that would impact the way a three.js extension library or app would expect to interact with the class functions in order to not break. If users want a different default color space for colors it's easy enough to make an override implementation or helper utility. Here are a couple other thoughts / possible options on how to handle the keyframe / math operations case:
I think my preference would be option 2 or 3 because it affords users the ability to control what space their own color math operations happen in which option 4 is much more opinionated about. |
I think this is why it's recommended not to lerp RGB values. If you want perceptual results you should use another function such as |
Is it even valid to do mathematical operations like add/subtract/multiply/lerp on gamma corrected colors? Or are these ops only valid on linear colors? |
|
It seems to be a solution to me too: const color = Color.fromColorSpace( AdobeRGBColorSpace, 0x0099FF ); |
I just intended to point out Unity's current behavior which is to not convert to a Linear color space before interpolating or performing math operations even when keyframing is being used on material colors (which are assumed to be srgb). Also lerpHSL will give you significantly different, rainbow-like results in a lot of cases compared to what I think would usually be expected from a lerp.
I wouldn't think so but that's what's being proposed if we assume
At some point it becomes an artistic choice though I think generally performing operations in sRGB might be regarded least favorably. From what I've been able to glean CSS gradients interpolate in sRGB. And the CSS |
Right, but that is technically wrong. It seems to be a holdover from the distant past when nobody cared about color spaces and everybody was incorrectly operating on gamma corrected colors. Then since everyone was doing it that way, new software also had to do it that way, and so on until here we are now. This post perhaps overstates the matter, but it does get the point across. And from the linked w3 message:
If these posts are correct (and I'm pretty sure they are), allowing ops on sRGB colors should be considered legacy behavior, it's just spread across the entire industry instead of a single app. If we want to buy into this legacy behavior and continue to allow these operations to be done on sRGB colors that's probably ok since everyone else is also doing it (and it does make progressing this PR more likely). Maybe it could be noted in docs somewhere that color operations should technically only be done in linear and leave it up to users to decide whether it's important to them? |
You're right, I was confusing HSL with HSB/HSV. In this post several people recommend using
This post on the Unity forums suggests using |
|
Sorry, I think I've led things down the wrong path with some misunderstanding of concepts related to color spaces, and I've made several comments in this thread that are incorrect or misleading. I'd also like to avoid Unity's terminology of "linear workflows" and "gamma workflows" moving forward. Let me close this issue and restart the conversation in a new thread (coming soon) with a number of corrections. |
Related issues: #22275, #19169, #11337
If we move forward with #22275 these changes probably belong together. With
THREE.ColorManagement....enabled = true— hexadecimal, HSL, or CSS style colors are assumed to be sRGB. RGB components and vertex colors are still assumed to be linear. Conversion is done in setters, to avoid per-frame conversions in WebGLRenderer..enabled = false— existing behavior.Assuming CSS-style colors are sRGB, while .r/.g/.b components are linear, is based in part on the similar design of the Blender color picker. There’s no perfect answer here, but this provides a nice symmetry:
color.fromArray([.5, .5, .5])andcolorAttribute.setXYZ(index, .5, .5, .5)will give you the same result. Interpolation on .r/.g/.b components will happen in linear color space without any change to animation code, as it should.Because colors are always stored linear, and converted when setters are invoked, toggling the flag at runtime has no effect – it should be configured before constructing any Color instances. That could probably be solved but seems like a secondary issue.
Romain Guy has suggested that "color management" is too broad a term for this behavior, and I trust his judgement, but I don't have a better idea at the moment. R3F and A-Frame are using the same terminology (probably my fault?); if there are other changes that will eventually belong under this flag, we can consider growing into the name over time.
Context: https://discourse.threejs.org/t/three-color-problem/28920
References: