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.

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

WebGLRenderer: Output color space impacts final render color even when ColorManagement is disabled #29549

Closed
gkjohnson opened this issue Oct 4, 2024 · 7 comments

Comments

@gkjohnson
Copy link
Collaborator

gkjohnson commented Oct 4, 2024

Description

When color management is disabled it should mean that colors are read from textures and take from use input without assuming any associated color space meaning no transformations should be happening anywhere by the time it gets to the screen. However currently changing the "outputColorSpace" does impact the resulting color.

Related to #29429, this discourse post.

cc @donmccurdy

Reproduction steps

  1. Render a basic scene with color management disabled
  2. Toggle between renderer.outputColorSpace being set to LinearSRGBColorSpace and SRGBColorSpace
  3. See that colors change unexpectedly

Code

THREE.ColorManagement.enabled = false;

// ...

renderer.outputColorSpace = THREE.LinearSRGBColorSpace;

Live example

https://jsfiddle.net/t7pLhr1c/1/

Screenshots

outputColorSpace = SRGBColorSpace

image

outputColorSpace = LinearSRGBColorSpace

image

Version

latest

Device

No response

Browser

No response

OS

No response

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 4, 2024

When color management is disabled it should mean that colors are read from textures and take from use input without assuming any associated color space meaning no transformations should be happening anywhere by the time it gets to the screen.

Hm, I would have to think about this proposed definition for a bit. When THREE.ColorManagement was turned on by default in r152+, the idea I had in mind was that enabling/disabling it should affect implicit conversions (e.g. converting sRGB hex inputs to working color space), and not that explicit properties like texture.colorSpace or renderer.outputColorSpace should be ignored. I had intended that it should be possible to set ColorManagement.enabled = false while still using a linear workflow, it would just take more manual work, as it had before we offered the option.

Perhaps mine was not the right assumption, and either way... there are still some fuzzy areas. But if we change the behavior to ignore output color space and texture color space, then users who had "opted out" according to the r152 migration guide are going to lose that workflow.

I'm not sure this is desirable. Did you have other use cases in mind, other than the "alpha blending in sRGB space" question raised at #29429 (comment)?

@gkjohnson
Copy link
Collaborator Author

then users who had "opted out" according to the r152 migration guide are going to lose that workflow.

I haven't touch these color settings in awhile so I forgot about both needing to be set - but I suppose this is just showing how confusing I feel it is to have to set the renderer output color to "Linear" in order to display the output in sRGB color space 😅. When I first started working with different color space it was in Unity which has a specific toggle for switching between a "linear" and a "gamma" workflow - see this document here. When the "gamma" workflow is enabled:

all shader calculations treat their input as if it was in linear space, and additionally, when writing the shader outputs to memory, no gamma correction is applied to the final pixel

This happens regardless of texture settings so the user doesn't have to worry about anything and all colors will be passed through the rendering pipeline unperturbed - effectively resulting in all lighting operations in gamma color space. To me this is a much more intuitive way of looking at the flag.

When THREE.ColorManagement was turned on by default in r152+, the idea I had in mind was that enabling/disabling it should affect implicit conversions (e.g. converting sRGB hex inputs to working color space), and not that explicit properties like texture.colorSpace or renderer.outputColorSpace should be ignored.

It may have been the case in r152 that texture settings were explicit but this is no longer the case. All the loaders now implicitly set texture color spaces to sRGB if it's assumed to be a color with no way to opt out. Users otherwise have to set these textures back to LinearSRGBColorSpace to get them to display correctly, which won't be obvious to anyone other than people very familiar with the behavior of the library. Bottom line is it feels like the user base who is most likely to want to "opt out" of color management will be those who are unfamiliar with the concepts generally and don't want to think about it - but ironically I think some of this behavior will be the least understandable to that user group. One solution is to have the loaders not assign texture color spaces if ColorManagement is disabled but that means any library and addon also needs to do the same otherwise colors will not come out looking as expected.

I'm not sure this is desirable. Did you have other use cases in mind, other than the "alpha blending in sRGB space" question raised at #29429 (comment)?

I think I just find the current behavior difficult to explain and understand and therefore correctly recommend as a solution to a problem. I think it's a nice feature to have but as-it-is "opting out" seems like it can add more confusion than it solves. But if there's a strong desire to not change anything here then I won't push it.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 4, 2024

I do completely agree that for a user who wants to stay with the legacy workflow (i.e. rendering in sRGB space) setting renderer output to Linear-sRGB space is conceptually very unclear. My preferred approach to that use case would actually not be disabling ColorManagement, though. I think that I would want to set:

THREE.ColorManagement.workingColorSpace = THREE.SRGBColorSpace;

We don't allow that today... but I think it would be the correct way to express what is happening in a so-called “gamma workflow,” should we choose to. This has an advantage over disabling the color conversions, because it will correctly handle glTF files where the baseColorFactor values are explicitly Linear-sRGB, not sRGB. And then textures annotated with .colorSpace = SRGBColorSpace should continue to look correct.

@gkjohnson
Copy link
Collaborator Author

THREE.ColorManagement.workingColorSpace = THREE.SRGBColorSpace;

I've considered this, as well, but aside from the rare case when colors are provided as linear (as in GLTF) this may be more complicated than just ignoring the color spaces on textures (effictively using the two-wrongs-make-a-right model for LinearSRGB rendering). Either that or more new concepts will have to be introduced to three. Users will still have to know about color spaces which isn't great if the goal is to let users not care.

Currently all textures are imported as LinearSRGBColorSpace so they will still have to correctly assign all the texture color spaces correctly. Likewise normal, metalness, etc textures currently look "correct" if they are imported without changing the color space from Linear - but if the working color space is sRGB then these will all need to be set to "NoColorSpace". This is all more complicated for end users than ignoring color spaces and having GLTFLoader convert the couple linear light values to sRGB when color management is disabled. I still think affording an "sRGB" working color space would be nice - it will enable a "retro" style light rendering and afford control for these odd cases like blending. It just doesn't help the "basic" user it seems like disabling color management was really designed for.

This is a complicated issue and admittedly it will probably become less and less possible for users to ignore the existence of color spaces as more and more non sRGB and non SDR content is produced.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Oct 7, 2024

It's not so complicated as that, fortunately! The default color space for THREE.Texture is not LinearSRGBColorSpace, but NoColorSpace. If users have been careful to follow the recommendations and assigned color textures as sRGB, and normal/metal/rough as NoColorSpace,1 then switching the working color space to sRGB requires no changes. If users have not been so careful, and left everything at the default NoColorSpace ... this also requires no changes. :)

The exception is textures like .exr or .hdr environment maps. These default to, and almost always are, LinearSRGBColorSpace. Not sure there's any really correct way to handle those in a “gamma workflow” at all, and we don't have a way to do the conversion efficiently, so probably this would result in no conversion and a warning logged for now.

Footnotes

  1. I've tried to be careful to recommend NoColorSpace and not LinearSRGBColorSpace for normal/metal/rough/etc because that's important for the wide gamut workflows. Switching to a wide gamut workflow should not mean that your normal maps get "converted" from Linear-sRGB to Linear-P3, that wouldn't make sense.

@gkjohnson
Copy link
Collaborator Author

It's not so complicated as that, fortunately! The default color space for THREE.Texture is not LinearSRGBColorSpace, but NoColorSpace.

Thanks that does seem like a better default - I seem to be mixing up different eras of three.js color space defaults in my head.

I'll close this for now since this seems like a reasonable path forward and I don't think there's an urgency to adjust the ColorManagement behavior - though the current behavior still feels a bit inconsistent to me and I question how much value this flag will have going forward. The only case may be if there are performance implications to some of the color conversions done by the gpu (see #29429 (comment)).

@donmccurdy
Copy link
Collaborator

I don't think there's an urgency to adjust the ColorManagement behavior - though the current behavior still feels a bit inconsistent to me and I question how much value this flag will have going forward

Agreed that the behavior of the ColorManagement.enabled=false flag is not very clear, and perhaps also not very useful. Maybe that's something we could even deprecate eventually? While neither ColorManagement.enabled=false nor ColorManagement.workingColorSpace=SRGBColorSpace are correct for lit rendering, the latter feels easier to explain and to understand for other use cases (unlit scenes, 2D graphics, ...) .

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

No branches or pull requests

3 participants