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: Rename outputEncoding to outputColorSpace. #23936

Closed
wants to merge 3 commits into from

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 23, 2022

Related issue: #23614

Description

Renames outputEncoding to outputColorSpace as suggested in #23614 (section 1.3).

@donmccurdy
Copy link
Collaborator

I'm certainly in favor of the change — my only reservation is about which of these three changes can be done independently and which should be batched into a single release:

  • renderer.outputEncoding -> outputColorSpace
  • renderer.outputXXX default -> sRGB
  • texture.encoding -> colorSpace

Changing the name might be as good a time as any to change the default.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 23, 2022

How about using r141 as the target release? A soon as this PR is merged at the end of next week, I'll make the next one and rename Texture.encoding to Texture.colorSpace. After that, we change the default value of Texture.colorSpace and WebGLRenderer.outputColorSpace and update the examples/docs/manual accordingly.

@LeviPesin
Copy link
Contributor

LeviPesin commented Apr 23, 2022

(From #23937 (comment))
I think that not only this property should be renamed, but its value should be changed to LinearSRGBColorSpace/SRGBColorSpace instead of the LinearEncoding/sRGBEncoding (and the encodings constants can be set equal to the color spaces for the backwards compatibility).

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 24, 2022

Yes, the constants should be changed too. But it's sufficient to note this at #23614. Please let's keep the discussion focused on what the PR is about. Renaming WebGLRenderer.outputEncoding.

@donmccurdy
Copy link
Collaborator

@Mugen87 I can't tell if you're saying we should wait to replace the constants until a different release or a different PR. I think a different PR is fine but not a different release. Note that the SRGBColorSpace and LinearSRGBColorSpace constants already exist. If you still prefer to wait that's fine with me. 👍

@gkjohnson
Copy link
Collaborator

I'll just not here as I have in other issues that naming this field something like canvasColorSpace could be more clear.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 25, 2022

I can't tell if you're saying we should wait to replace the constants until a different release or a different PR.

Different PR. Otherwise the PRs are going to be too complex. I'd like to implement the things you have mentioned in #23936 (comment) in a single release. Ideally r141.

@Mugen87 Mugen87 added this to the r141 milestone Apr 25, 2022
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 25, 2022

I'll just not here as I have in other issues that naming this field something like canvasColorSpace could be more clear.

I think it's best if @mrdoob makes a final decision here 😇 . The options are:

After some more thinking I guess I vote for WebGLRenderer.canvasColorSpace. It seems indeed the most accurate/descriptive name. I'm making this vote under the assumption that we never evaluate WebGLRenderer.canvasColorSpace when rendering to render targets (see #23614 (comment)).

@donmccurdy
Copy link
Collaborator

Either outputColorSpace or canvasColorSpace are OK with me. The thing that confused me about the original name was that it is ignored if writing to canvas comes at the end of a post-processing chain. I wish we could fix that but I suppose it's out of scope here, and it's the behavior not the name that should ideally be changed for that issue.

I would consider parallelism among renderers though. Does canvasColorSpace still make sense for WebGPURenderer? With WebXR APIs? I've been hoping to add an outputColorSpace option to SVGRenderer at some point — the only renderer that could actually support a wider-gamut Display P3 output to display today — and obviously canvasColorSpace does not make sense there.

@LeviPesin
Copy link
Contributor

Could then it be named displayColorSpace?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 25, 2022

that it is ignored if writing to canvas comes at the end of a post-processing chain

WebGLRenderer.outputEncoding triggers the color space conversion in GLSL shaders. That only works for built-in materials but not for custom shader passes. That's the reasons why you need an additional post processing pass for color space conversion.

I've tried to eliminate this inconsistency in #23019 because it also introduces a transparency issue. The idea was to remove the inline tone mapping and color space conversion and always apply them with an additional post processing pass. However, this approach introduces a noticeable performance degradation. So at least with WebGLRenderer, we have to stick to the current approach.

Does canvasColorSpace still make sense for WebGPURenderer?

Yes. But ideally it is not necessary anymore to perform the final color space conversion inline, see #23019 (comment).

With WebXR APIs?

WebXRManager evaluates WebGLRenderer.outputEncoding for defining the right color space for its projection layers/framebuffers. But I think the name canvasColorSpace is okay in this context.

I've been hoping to add an outputColorSpace option to SVGRenderer at some point

Well yes, in this case outputColorSpace would be better since it's the more generic term.

Could then it be named displayColorSpace?

See #23614 (comment). If we decide against canvasColorSpace, outputColorSpace seems the best choice.

@donmccurdy
Copy link
Collaborator

The only part of "outputEncoding" I would change is "Encoding". Output is a common enough term for what it does, I think we can clear up any confusion around render targets in other ways.

Display would probably be too specific, as we also use the renderer with OffscreenCanvas, or to bake IBL, or to save a scene screenshot to disk.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 25, 2022

Okay, then I change my vote back to outputColorSpace 😇 .

@donmccurdy
Copy link
Collaborator

So at least with WebGLRenderer, we have to stick to the current approach.

I think the design in pmndrs/postprocessing is perhaps a good alternative. But we can keep that discussion in #23937 or elsewhere.

This PR looks good to me for r141. 👍🏻

@sciecode
Copy link
Contributor

sciecode commented May 20, 2022

Well, I'm gonna be pesky about naming stuff, hope you folks don't mind it.

Turns out, color-space is a loaded term. However it does have an established meaning, which is a combination of both a standard gamut and a set of transfer functions. ( and a white-point of calibration, if you wanna be really pesky about it )

But that's not what we are really changing when we set an output encode for the final render, be it in built-in shaders or post-processing step. If I'm not mistaken, THREE.js hasn't adopted any support in regards to operating in anything other than standard sRGB gamut. ( can't use Adobe RGB, DCI-P3 ... )

So, if we are not changing gamuts. The only other thing we can be doing is changing if the data is encoded as linear or perceptual data. This is sometimes referred to as EOTF ( Electro-Optical Transfer Function ). Which is indeed sometimes referred to as gamma or simply sRGB, but in practice is just a single set of linear <-> perceptual functions possible.

I don't expect we to name it outputEOTF as there is no value in naming it something the large comunity won't recognize. But the word 'encode' does better represent what we are doing. Either we are rendering in linear ( energy intensity ) which can be later operated on by other various post-processing steps or perhaps exported to a file ( HDR, EXR ), or perceptually encoded which is meant to be the encoded in the way our eyes interpret light. or as I like to say "human encoded light".

Perhaps we can keep outputEncoding ? 🙏

@LeviPesin
Copy link
Contributor

If I'm not mistaken, THREE.js hasn't adopted any support in regards to operating in anything other than standard sRGB gamut.

Has not yet, but AFAIK aim to in the future.

@sciecode
Copy link
Contributor

sciecode commented May 20, 2022

If I'm not mistaken, THREE.js hasn't adopted any support in regards to operating in anything other than standard sRGB gamut.

Has not yet, but AFAIK aim to in the future.

My point exactly, when we do. Is gonna be easier to both adopt and understand, if we name things more carefully now. Even if we are operating in different color-space, we are still gonna need to ask if we want the render in linear or perceptual data. So it's better if we don't confuse people into thinking those and color spaces are the same thing.

@romainguy have I learned anything from your talks?

@donmccurdy
Copy link
Collaborator

Thanks for the comments @sciecode!

The summary in #23614 covers where we are (tentatively) headed in the longer term, which is meant to be self-consistent in the end. We can see where WebGL, WebGPU, and other Web APIs are going in various in-progress specifications...

In all of these, the API provides pre-defined color spaces like "srgb", "srgb-linear", and "display-p3", but the web does not expose the transfer function as a distinct user-configurable option. I believe users understand "sRGBEncoding" as synonymous with "the sRGB color space" in three.js today. I wish three.js terminology were more precise — whether we mean color space or transfer function — because "encoding" is ambiguous.

When Display P3 (supported on many mobile displays now) lands, it will have the same transfer function as sRGB. I am proposing...

// this
renderer.outputColorSpace = THREE.DisplayP3ColorSpace;

// not this
renderer.outputEncoding = THREE.sRGBEncoding;
renderer.outputPrimaries = THREE.DisplayP3Primaries;

I've also tried to document these terms better in https://threejs.org/docs/#manual/en/introduction/Color-management, but we're in a weird middle-state at the moment where both "LinearSRGBColorSpace" and "LinearEncoding" enums are in use at the same time.

@sciecode
Copy link
Contributor

sciecode commented May 20, 2022

So it's better if we don't confuse people into thinking those and color spaces are the same thing

In all of these, the API provides pre-defined color spaces like "srgb", "srgb-linear", and "display-p3"

Well, spec makers definitely aren't making it any easier for us, are they?

I'll take a look at the proprosed plan and comment there if it makes sense.

But theoretically, just defining just a color space would not be suficient. That's why they need to create things like "srgb" and "srgb-linear", because the official API also doesn't understand nomenclature. Thanks for letting me know @donmccurdy, will reply after I'm done reading the proposed plan.

@donmccurdy
Copy link
Collaborator

It may not be too late to influence the designs for WebGL and WebGPU, if we feel the need. But personally — knowing that a color space is a choice of {primaries, white point, transfer function} — I am OK with selecting color space as a unit, rather than exposing the individual properties. If I look at an OpenColorIO config those are mostly defined in terms of named color spaces and transforms to/from a reference space — and this is sufficient for professional VFX software, Blender, Maya, and Unreal Engine.

@sciecode
Copy link
Contributor

It may not be too late to influence the designs for WebGL and WebGPU

I might give it a try perhaps, as I don't see a problem in having a single "setter" for defining the combination for a color-space and the encoding, but making it perhaps clearer of both the color-space and encoding in every option seems more prudent to me.

As I do believe this API is fundamental for shaping the future of color management in the web and guiding the first principles of color management for many developers.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 20, 2022

We could also make our (new) enums more specific, while keeping the .colorSpace terminology. For example:

// Transfer Function, Primaries, White Point
renderer.outputColorSpace = THREE.SRGB_Rec709_D65;    // sRGB
renderer.outputColorSpace = THREE.Linear_Rec709_D65;  // Linear-sRGB
renderer.outputColorSpace = THREE.SRGB_DisplayP3_D65; // Display-P3

I worry it will scare people, but it's certainly precise. I'd also considered proposing that we just use the color space identifier strings defined by CSS Color Module Level 4...

renderer.outputColorSpace = 'srgb';
renderer.outputColorSpace = 'srgb-linear';

The current THREE.SRGBColorSpace and THREE.LinearSRGBColorSpace enums do use those as values. There is some benefit to fewer parameters than more, as these go into getters/setters like color.setHex. But those APIs are new enough (and gated behind THREE.ColorManagement.legacyMode = false) that they can still be changed.

@sciecode
Copy link
Contributor

sciecode commented May 20, 2022

I agree, specifying every thing is counter productive in terms of usability. I think a standard name_id-encode would be sufficient in any case. I would just love if both three and official API used:

renderer.outputColorSpaceEncoding = 'p3-perceptual' // 'p3-linear', 'srgb-perceptual'

or

renderer.outputColorSpace = 'displayP3' // 'sRGB', 'AdobeRGB'
renderer.outputEncoding = 'linear' // 'perceptual'

Just for clarity sake, in the end you are still setting a single property as a tuple or two properties as singles.

Apparently they chose to omit the encoding in perceptual case, which is not horrible. Just is what it is.

Given what you just mentioned, I dont oppose the naming. Will still review the plan in its entirety when I have the time 👍

edit: maybe just renderer.colorSpaceEncoding, I don't know 😂

@romainguy
Copy link

We could also make our (new) enums more specific, while keeping the .colorSpace terminology. For example:

// Transfer Function, Primaries, White Point
renderer.outputColorSpace = THREE.SRGB_Rec709_D65;    // sRGB
renderer.outputColorSpace = THREE.Linear_Rec709_D65;  // Linear-sRGB
renderer.outputColorSpace = THREE.SRGB_DisplayP3_D65; // Display-P3

I very much like this proposal :) I think this is better than "perceptual" since perceptual says nothing about which encoding is being used. And while I understand it might scare off people, it's also how we may avoid perpetuating the ongoing confusion about sRGB, Linear, etc.

@mrdoob mrdoob added this to the r142 milestone May 26, 2022
@mrdoob mrdoob modified the milestones: r142, r143 Jun 29, 2022
@mrdoob mrdoob modified the milestones: r143, r144 Jul 28, 2022
@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@LeviPesin
Copy link
Contributor

I worry it will scare people, but it's certainly precise.

Maybe we can use aliases? Like THREE.SRGB = THREE.SRGB_Rec709_D65?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 12, 2022

I think this is better than "perceptual" since perceptual says nothing about which encoding is being used. And while I understand it might scare off people, it's also how we may avoid perpetuating the ongoing confusion about sRGB, Linear, etc.

Agreed, I would like to avoid the term "perceptual" in the enums for that reason. We'll likely need to be able to distinguish between multiple different perceptual transfer functions, when HDR canvas APIs become available. For example: sRGB, PQ, and HLG.

I'm satisfied with either of the alternatives in #23936 (comment). @WestLangley do you have a preference between these, or other ideas?

Maybe we can use aliases? Like THREE.SRGB = THREE.SRGB_Rec709_D65?

I'd probably rather not ship these aliases "out of the box", unless we need them temporarily for backward-compatibility... part of the point is to get used to seeing a fully-specified color space, so that introducing color spaces for upcoming web APIs will be less disruptive. In that world there is more than one color space using the "sRGB" and "linear" transfer functions, and their differences are important.

I could be OK with exposing an API on THREE.ColorManagement to register new color spaces, at some point. In that case the user could register an alias themselves, if they prefer. There are other use cases for this, but it doesn't feel like a priority quite yet.

@WestLangley
Copy link
Collaborator

WestLangley commented Nov 12, 2022

@WestLangley do you have a preference between these, or other ideas?

Color spaces are well-defined, so I do not see the benefit of listing the transfer function, primaries, and white point in the constant name.

My suggestion would be to continue with the following pattern:

THREE.SRGBColorSpace
THREE.LinearSRGBColorSpace // sRGB color space with a linear transfer function

// possible future
THREE.Rec2100PQColorSpace // for HDR monitor support
THREE.ACEScgColorSpace // a wider-gamut working color space...

@@ -134,7 +134,7 @@ <h2>纹理</h2>
颜色空间并显示在屏幕上。除非你需要使用线性颜色空间进行后期处理,否则请在使用glTF的时候将[page:WebGLRenderer]进行如下配置:</p>

<code>
renderer.outputEncoding = THREE.sRGBEncoding;
renderer.outputColorSpace = THREE.sRGBEncoding;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK with me to keep this for another PR but I do think it's important that we make sure .outputColorSpace accepts the color space enum values, within the same release as this PR.

Optionally — we could change the values of the encoding enums, such that THREE.sRGBEncoding and THREE.LinearEncoding are interchangeable with their color space equivalents, during the transition.

@donmccurdy
Copy link
Collaborator

Color spaces are well-defined, so I do not see the benefit of listing the transfer function, primaries, and white point in the constant name.

@WestLangley with Rec2100PQColorSpace and Rec2100HLGColorSpace we're already 2/3rds of the way there... 😅 But yes, there's also a real advantage to using names that closely parallel the names found in the web's own (upcoming) color APIs, if not using the web's string enum values directly. These would be good pairings with "rec2100-hlg" and "rec2100-pq" in the web APIs. This works for me.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 14, 2023

Closing, see #23614 (comment).

@Mugen87 Mugen87 closed this Mar 14, 2023
@Mugen87 Mugen87 removed this from the r151 milestone Mar 14, 2023
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.

8 participants