Skip to content

Conversation

@donmccurdy
Copy link
Collaborator

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]) and colorAttribute.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:

@donmccurdy donmccurdy marked this pull request as draft August 17, 2021 06:54
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 17, 2021

Would it be an option to introduce Color.encoding instead? And treat it similar to Texture.encoding? So when the color values are evaluated in the shader, they are converted depending on their encoding settings?

@donmccurdy
Copy link
Collaborator Author

I'm not sure that configuring every THREE.Color individually is a workflow I'd want to use myself. Defining a MeshPhysicalMaterial with color management would mean:

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?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 17, 2021

We could introduce a variable like THREE.DefaultColorSpace which is used as a default value for encoding properties (similar to DefaultUp). In this way, the above code block would disappear.

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.

Is there an advantage in deferring the sRGB → linear conversion to the shader?

No it would just be the result of treating color properties similar to textures.

@looeee
Copy link
Collaborator

looeee commented Aug 17, 2021

We could introduce a varaible like THREE.DefaultColorSpace which is used as a default value for encoding properties (similar to DefaultUp). In this way, the above code block would disappear.

How would aoMap, lightMap, normalMap etc. be handled if DefaultColorSpace = sRGBEncoding?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 17, 2021

The default encoding would be global. So loaders have to ensure to set the correct encoding value according to the map type. GLTFLoader already does this for color maps:

if ( material.map ) material.map.encoding = sRGBEncoding;
if ( material.emissiveMap ) material.emissiveMap.encoding = sRGBEncoding;

If a normal map is manually loaded via TextureLoader, the app has to define a different encoding on app level if necessary.

@donmccurdy
Copy link
Collaborator Author

If texture.encoding = DefaultColorSpace (DefaultEncoding?) were the default for all textures, and its meaning was "use the inferred encoding for the current ColorManagement state and material slot," there would be no need for loaders or users to override individual textures in most cases. Changing ColorManagement.enabled = true/false would toggle the interpretation of DefaultColorSpace.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Aug 17, 2021

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:

  • All Color instances are sRGB
  • Normal maps are assumed to be in Linear space always (you select in the editor that a project texture is to be used as a normal map)
  • If the linear workflow is disabled then no sRGB -> Linear conversion happens when sampling in shaders

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;

@donmccurdy
Copy link
Collaborator Author

My only concerns with renderer.linearWorkflow = true; are technical, the user experience sounds good otherwise.

  • ColorKeyframeTrack must interpolate linear components, not sRGB components, and doesn't have access to a renderer to check the workflow. How do we avoid breaking it?
  • Where does sRGB -> Linear conversion happen? If in the shader, does this add cost and/or complexity? If before uniform upload, is that a relevant per-frame cost?

This PR does not allow mixed usage on the same page, but it does provide simple answers to the questions above. 😕

@looeee
Copy link
Collaborator

looeee commented Aug 18, 2021

multiple renderers on the same page that want to handle this type of color management differentl

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 THREE.ColorManagement = false and then applying the settings you need manually.

@gkjohnson
Copy link
Collaborator

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.

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.

In any case, it should be possible to handle this by setting THREE.ColorManagement = false and then applying the settings you need manually.

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.

ColorKeyframeTrack must interpolate linear components, not sRGB components, and doesn't have access to a renderer to check the workflow. How do we avoid breaking it?

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.

Where does sRGB -> Linear conversion happen? If in the shader, does this add cost and/or complexity? If before uniform upload, is that a relevant per-frame cost?

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.

@donmccurdy
Copy link
Collaborator Author

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.

@donmccurdy
Copy link
Collaborator Author

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.

@gkjohnson
Copy link
Collaborator

Unity can do this at compile time and we can't

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.

As a strawman API, how about:

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 true by default? Then people will use the newly recommended system and have an easy way to revert if they're not ready to make the switch.

It's a small nit but term internalEncoding isn't super clear to me -- renderEncoding might make more sense but it's not something to get hung up on.

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 pass

Shaders 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.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Aug 20, 2021

Unity can do this at compile time and we can't

How are you expecting Unity to do this at compile time?

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?

...is there not a reason to set it to true by default?

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.

@gkjohnson
Copy link
Collaborator

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)

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; // bad

So 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.

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.

Okay I agree -- I'm getting ahead of things. I'm excited to see this (hopefully) making progress :D

@sunag
Copy link
Collaborator

sunag commented Sep 6, 2021

Maybe a NodeMaterial approach for this?

material.colorNode = new ColorSpaceNode( ColorSpaceNode.SRGB_TO_LINEAR.  colorNode );

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 10, 2021

Where does sRGB -> Linear conversion happen? If in the shader, does this add cost and/or complexity? If before uniform upload, is that a relevant per-frame cost?

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:

} else if ( v.r !== undefined ) {
if ( cache[ 0 ] !== v.r || cache[ 1 ] !== v.g || cache[ 2 ] !== v.b ) {
gl.uniform3f( this.addr, v.r, v.g, v.b );
cache[ 0 ] = v.r;
cache[ 1 ] = v.g;
cache[ 2 ] = v.b;
}
} else {

So I would implement the linear conversion right before uniform3f() is executed (and cache the original color value and encoding).

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 12, 2021

@sunag I'm inclined to say that color management should not be a per-material setting (i.e. no material.colorNode), but that ColorSpaceNode can of course be used to opt-in particular colors or maps.

@Mugen87 that's great news, thanks! I think there's a path forward that addresses the issues @gkjohnson mentioned in #22346 (comment) then:

  1. We define and document that THREE.Color should be given sRGB values, and will be considered to be an sRGB color by the renderer. This is consistent with the default three.js usage today (gamma workflow) but a change from how users currently do a linear workflow (you'll no longer need to do color.convertSRGBToLinear()). Basically the Color class implementation does not change at all, but some assumptions about it do change.
  2. When the linear workflow is enabled (proposed API below) the renderer will convert colors sRGB→Linear before uniform3f() is executed and cache the result.
  3. Texture handling doesn't necessarily need to change, but it could (see Add THREE.ColorManagement #22346 (comment)).

EDIT: In retrospect this the same process as @gkjohnson described in #22346 (comment).

With that approach, we don't need the THREE.ColorManagement class defined here. Instead the API could be exposed on the renderer, similar to #22346 (comment)

// (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-processing

I'm not sure about that API though, two questions:

Is .workflow the right name? Would renderer.colorManagement = LinearColorManagement be better? The point has been made (https://twitter.com/romainguy/status/1273744026626228225) that "color management" means more than just what we're talking about here...

Is what we do by default today actually a gamma workflow? I don't think it is. If I set material.map.encoding = sRGBEncoding the renderer converts it to linear, so many users do material.map.encoding = LinearEncoding instead. The encoding of a base color texture is almost certainly sRGB, so this is a "two wrongs make a right" situation unfortunately. There's probably no concise term for "we treat colors as linear but treat color textures as sRGB", it's neither a linear nor gamma workflow. So if we intend to support the current system and linear and gamma workflow we may need a third option:

// (a)
renderer.colorManagement: LinearColorManagement | GammaColorManagament | LegacyColorManagement;

// (b)
renderer.workflow: LinearWorkflow | GammaWorkflow | LegacyWorkflow;

@sunag
Copy link
Collaborator

sunag commented Dec 12, 2021

@sunag I'm inclined to say that color management should not be a per-material setting (i.e. no material.colorNode), but that ColorSpaceNode can of course be used to opt-in particular colors or maps.

I agree, in fact material.colorNode = new ColorSpaceNode( method, someNode ) should be used to can to opt-in particular colors or maps.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 12, 2021

So I would implement the linear conversion right before uniform3f() is executed...

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 WebGLLights.js.

// before
LinearToSRGB( SRGBToLinear( 1 ) * 500 )
13.999173208840062

// after
LinearToSRGB( SRGBToLinear( 1 * 500 ) )
499.9507076006699

This applies to methods on the Color class like multiplyScalar, lerp, ... which assume linear values.

EDIT: Related: #23010

@looeee
Copy link
Collaborator

looeee commented Dec 13, 2021

If we start making the assumption that Color values are sRGB, then scaling them directly will have very different results.

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, something similar could work there:

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();

}

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 13, 2021

Is .workflow the right name?

I think colorManagement is more descriptive and easier to understand for users.

BTW: Couldn't colorManagement just be a boolean? And then name it enableColorManagement?

Eventually the default should be true but if users don't want it for whatever reasons they can just set it to false and the engine behaves like before.

If set to true, the engine behaves like described in #22346 (comment).

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 13, 2021

I like the suggested design but want to mentioned that the issues in THREE.Color could be avoided if something like Color.encoding would be in place.

Or if the color management is no API of the renderer but indeed a new global thing that can be accessed in THREE.Color.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 13, 2021

Couldn't colorManagement just be a boolean? And then name it enableColorManagement?

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 Color.lerp or ColorKeyframeTrack will need to know what the working color space is, and a renderer-level flag does not allow that.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 13, 2021

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.

Make sense! Option a) looks most promising to me.

@gkjohnson
Copy link
Collaborator

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 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 Color.Lerp( new Color( 0, 0, 0 ), new Color( 1, 1, 1 ), 0.5 ) always yields Color( 0.5, 0.5, 0.5 ) whereas I might expect it to output Color( 0.73, 0.73, 0.73 ) when using a Linear color space. Unity only seems to care about what color space a color is in once it's assigned to a property considering you can convert Unity colors to and from Linear representation (similar to the way three behaves now). It seems that keyframe animations using AnimationCurves behave the same way -- 50% along the timeline results in Color( 0.5, 0.5, 0.5 ) even with Linear color management enabled. I agree it's best to perform math operations in the correct color space in JS animations but part of me wonders if it's really necessary considering at least one big engine seems to not handle it. Does anyone know what other engines do here?

// (a) Global default with local override.
THREE.Color.defaultColorSpace = sRGBColorSpace;
const color = new THREE.Color();
color.colorSpace = AdobeRGBColorSpace;

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:

  1. Don't worry about the interpolation inaccuracies.

  2. Add a flag on the color keyframe track telling it to convert colors to linear before interpolating. Other math operations are a users responsibility.

  3. Track the colorSpace on the Color instance and only afford add / lerp operations on Colors in a common color space. Keyframe tracks will have to be in the space they will be interpolated in.

  4. Track colorSpace on Color and add THREE.Color.OperationalColorSpace to specify what color space math operations should happen in. Colors would be converted to the target color space before operations and then back afterward. In most cases this would be entirely transparent but this still feels like a bit of a trap when using colors to represent and track non-color information (particle velocity, position) using color in textures.

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.

@looeee
Copy link
Collaborator

looeee commented Dec 14, 2021

Color.Lerp( new Color( 0, 0, 0 ), new Color( 1, 1, 1 ), 0.5 ) always yields Color( 0.5, 0.5, 0.5 ) whereas I might expect it to output Color( 0.73, 0.73, 0.73 ) when using a Linear color space

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 lerpHSL . Is it even possible to have a lerp function that operates on RGB values, returns perceptual results, and is still a linear transformation?

@looeee
Copy link
Collaborator

looeee commented Dec 14, 2021

only afford add / lerp operations on Colors in a common color space.

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?

@sunag
Copy link
Collaborator

sunag commented Dec 14, 2021

It seems to be a solution to me too:

const color = Color.fromColorSpace( AdobeRGBColorSpace, 0x0099FF );

@gkjohnson
Copy link
Collaborator

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 lerpHSL.

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.

Is it even possible to have a lerp function that operates on RGB values, returns perceptual results, and is still a linear transformation?

I wouldn't think so but that's what's being proposed if we assume Color to be sRGB while math operations happen in Linear space. An interpolation at alpha 0.5 from black to white should result an sRGB color of Color( 0.73, 0.73, 0.73 ) when using Linear color operations. The numeric result is confusing in its own way which is possibly one reason Unity doesn't implicitly convert to a linear color space for these conversions. But who knows.

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?

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 color-mix() operator allows you to specify the color space to interpolate in with sRGB as an option. Photoshop gradients used to use sRGB, as well, until they recently added support for Linear gradients and a new, more perceptually linear color space "Oklab". I think it's hard to say what the "right" solution is in the general case.

@looeee
Copy link
Collaborator

looeee commented Dec 14, 2021

From what I've been able to glean CSS gradients interpolate in sRGB.

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:

doing linear gradients in way they are currently done, are incorrect usage of sRGB. sRGB is only usefull for transfering pixels colors, not performing ANY operations on them other than displaying on devices using same color space. Any operation like resampling, rescaling, generating gradients, performing blending, performing any kind interpolation based on position, time, or other parameters must be done in linear space to produce correct sRGB result.

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?

@looeee
Copy link
Collaborator

looeee commented Dec 14, 2021

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.

You're right, I was confusing HSL with HSB/HSV. In this post several people recommend using lerpHSV for best results. Unfortunately we don't currently have that method.

An interpolation at alpha 0.5 from black to white should result an sRGB color of Color( 0.73, 0.73, 0.73 ) when using Linear color operations. The numeric result is confusing in its own way which is possibly one reason Unity doesn't implicitly convert to a linear color space for these conversions. But who knows.

This post on the Unity forums suggests using Color blendedColor = Color.Lerp(color1.linear, color2.linear, 0.5f).gamma which presumably would give (0.73, 0.73, 0.73)

@donmccurdy
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants