three.js icon indicating copy to clipboard operation
three.js copied to clipboard

WebGLRenderer: Rename outputEncoding to outputColorSpace.

Open Mugen87 opened this issue 2 years ago β€’ 24 comments

Related issue: #23614

Description

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

Mugen87 avatar Apr 23 '22 14:04 Mugen87

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.

donmccurdy avatar Apr 23 '22 14:04 donmccurdy

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.

Mugen87 avatar Apr 23 '22 15:04 Mugen87

(From https://github.com/mrdoob/three.js/pull/23937#discussion_r856938059) 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).

LeviPesin avatar Apr 23 '22 16:04 LeviPesin

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.

Mugen87 avatar Apr 24 '22 08:04 Mugen87

@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. πŸ‘

donmccurdy avatar Apr 24 '22 15:04 donmccurdy

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

gkjohnson avatar Apr 24 '22 17:04 gkjohnson

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 https://github.com/mrdoob/three.js/pull/23936#issuecomment-1107513416 in a single release. Ideally r141.

Mugen87 avatar Apr 25 '22 08:04 Mugen87

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:

  • The current implemented WebGLRenderer.outputColorSpace (the original suggestion of #23614)
  • The simplified version WebGLRenderer.colorSpace. However, it was highlighted in https://github.com/mrdoob/three.js/issues/23614#issuecomment-1107467806 that this version is confusing since the name does not distinct between working and output color space.
  • @gkjohnson suggested WebGLRenderer.canvasColorSpace. This name would highlight WebGLRenderer.canvasColorSpace only affects the rendering to the canvas/default framebuffer.

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 https://github.com/mrdoob/three.js/issues/23614#issuecomment-1107638945).

Mugen87 avatar Apr 25 '22 08:04 Mugen87

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.

donmccurdy avatar Apr 25 '22 13:04 donmccurdy

Could then it be named displayColorSpace?

LeviPesin avatar Apr 25 '22 13:04 LeviPesin

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 https://github.com/mrdoob/three.js/issues/23019#issuecomment-1011221811.

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 https://github.com/mrdoob/three.js/issues/23614#issuecomment-1107467806. If we decide against canvasColorSpace, outputColorSpace seems the best choice.

Mugen87 avatar Apr 25 '22 13:04 Mugen87

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.

donmccurdy avatar Apr 25 '22 13:04 donmccurdy

Okay, then I change my vote back to outputColorSpace πŸ˜‡ .

Mugen87 avatar Apr 25 '22 13:04 Mugen87

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. πŸ‘πŸ»

donmccurdy avatar Apr 25 '22 13:04 donmccurdy

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 ? πŸ™

sciecode avatar May 20 '22 18:05 sciecode

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.

LeviPesin avatar May 20 '22 18:05 LeviPesin

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?

sciecode avatar May 20 '22 18:05 sciecode

Thanks for the comments @sciecode!

The summary in https://github.com/mrdoob/three.js/issues/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...

  • https://github.com/w3c/ColorWeb-CG/blob/master/hdr_html_canvas_element.md
  • https://github.com/WICG/canvas-color-space
  • https://www.w3.org/TR/css-color-4/#predefined

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.

donmccurdy avatar May 20 '22 18:05 donmccurdy

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.

sciecode avatar May 20 '22 19:05 sciecode

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.

donmccurdy avatar May 20 '22 19:05 donmccurdy

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.

sciecode avatar May 20 '22 19:05 sciecode

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.

donmccurdy avatar May 20 '22 19:05 donmccurdy

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 πŸ˜‚

sciecode avatar May 20 '22 19:05 sciecode

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.

romainguy avatar May 21 '22 00:05 romainguy

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

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

LeviPesin avatar Nov 09 '22 06:11 LeviPesin

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 https://github.com/mrdoob/three.js/pull/23936#issuecomment-1133259680. @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.

donmccurdy avatar Nov 12 '22 16:11 donmccurdy

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

WestLangley avatar Nov 12 '22 19:11 WestLangley

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.

donmccurdy avatar Nov 12 '22 21:11 donmccurdy

Closing, see https://github.com/mrdoob/three.js/issues/23614#issuecomment-1467889921.

Mugen87 avatar Mar 14 '23 11:03 Mugen87