Skip to content

Conversation

@donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Apr 6, 2023

Changes in this PR:

  • WebGLRenderer: Default output to sRGB
  • WebGPURenderer: Default output to sRGB
  • Examples: Update all as needed

With r152, the default behavior in three.js will be to perform rendering in Linear-sRGB, with output to the canvas converted automatically to sRGB. WebGL and WebGPU APIs already assume that colors written to the canvas are sRGB. Many three.js examples already opt-in to this behavior, and the remainder will be updated over time.

There are limited cases where output to Linear-sRGB may be preferable, such as saving output to an HDR image, or some situations involving post-processing. For the large majority of users however, we recommend keeping this new default output (sRGB) and assigning input colors accordingly. Use of THREE.ColorManagement.enabled = true can be very helpful in making that switch.

Statistics for all WebGL and WebGPU examples:

outputColorSpace # examples
"srgb" 124
"srgb-linear" 64
unspecified 181
Scripts for statistics
# count all examples
ls web*.html | wc -l

# count examples using sRGB vs. Linear-sRGB vs. unspecified output
grep -l "outputColorSpace = THREE.SRGBColorSpace" web*.html | wc -l
grep -l "outputColorSpace = THREE.LinearSRGBColorSpace" web*.html | wc -l
grep -L "outputColorSpace" web*.html | wc -l

# count examples with THREE.ColorManagement enabled
grep -l "ColorManagement.enabled = true" web*.html | wc -l

All unspecified examples now use sRGB implicitly. 71 examples use ColorManagement.enabled = true. Please refer to the color management documentation for background and best practices.

In many cases, adding THREE.ColorManagement.enabled = true to an example and assigning texture.colorSpace = THREE.SRGBColorSpace appropriately was enough to complete the migration. Remaining cases will require some manual conversion of colors, and/or adjustments to scene lighting. This change tends to make shading appear a bit softer, and a few examples lacked contrast after the change. I've included the easier migrations in this PR, and opted-out on the rest.

Remaining changes for r152, in next PRs:

  • Update documentation

Related issue:

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize Gzipped Diff from dev
633.2 kB 156.9 kB +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize Gzipped Diff from dev
424.8 kB 103 kB +25 B

@donmccurdy donmccurdy added this to the r152 milestone Apr 6, 2023
@donmccurdy
Copy link
Collaborator Author

Most are more subtle, but a couple of notable screenshot diffs:

r151 r152
webgl_clipping webgl_clipping-1
webgl_decals webgl_decals-1

We could refine the examples' lighting more in r152 after these changes, but the "fake plastic" look on the left side is a hallmark of an incorrect color workflow, fixed by this change.

@mrdoob
Copy link
Owner

mrdoob commented Apr 7, 2023

Could we also set THREE.ColorManagement.enabled to true by default? 🤔

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 7, 2023

Related #23614 (comment)

@donmccurdy
Copy link
Collaborator Author

@mrdoob @Mugen87 Let's update THREE.ColorManagement.enabled in an additional PR if that's OK.

@Mugen87 Mugen87 merged commit 13397a8 into mrdoob:dev Apr 7, 2023
@donmccurdy donmccurdy deleted the feat/webglrenderer-outputcolorspace-default-v2 branch April 7, 2023 12:51
this.autoClearStencil = true;

this.outputColorSpace = LinearSRGBColorSpace;
this.outputColorSpace = SRGBColorSpace;
Copy link
Owner

@mrdoob mrdoob Apr 13, 2023

Choose a reason for hiding this comment

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

Oh! I almost forgot...

I think this should be:

this.outputColorSpace = null;

This default will let the renderer use SRGBColorSpace or DisplayP3ColorSpace automatically based on the user's display (eventually).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it already clear how the renderer will determine the user's display color space? I suppose there will be a special Web API for this?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we can do this check: #23614 (comment)

Copy link
Owner

Choose a reason for hiding this comment

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

By setting it to null it will also leave the door open for HDRColorSpace or whatever we get in the future.

Copy link
Collaborator

@Mugen87 Mugen87 Apr 13, 2023

Choose a reason for hiding this comment

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

I think we can do this check: #23614 (comment)

Interesting! Couldn't we implement this check in the ctor call of the renderers? I suppose the user's display color space is not something that is going to change during app usage.

I just wonder how and when the renderer determines the output color space. If the default value is null and the user is not going to define the color space (which will be true most of the time), the renderer has to set the value eventually.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright. Lets do that 👍

Copy link
Collaborator

@Mugen87 Mugen87 Apr 13, 2023

Choose a reason for hiding this comment

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

BTW: DCI-P3 related features are tracked by the task 2.2 in #23614.

Looking at the spec, I wonder if defining DisplayP3ColorSpace is sufficient. Is it required to change the WebGLRenderingContext.drawingBufferColorSpace, too? It accepts srgb and display-p3 with srgb as the default.

When I change this property in any three.js example to display-p3, I can actually see a hue shift. I wonder now which setup (values of output color space and drawing buffer color space) is actually correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's enable color management by default first before going for the DisplayP3ColorSpace change. We need the color space conversion in any event to get the colors in DisplayP3ColorSpace right.

#23614 (comment)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, as far as I understand we do not have the shaders chunks needed for supporting DisplayP3ColorSpace as output yet.

Copy link
Collaborator Author

@donmccurdy donmccurdy Apr 14, 2023

Choose a reason for hiding this comment

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

Even if encodings_fragment were implemented for Display P3, I'm not sure yet when it should be turned on. The difference when enabling it for existing content is very subtle — not an automatic improvement.

There are more impactful results when bringing in wide-gamut colors and textures in scenes designed for Display P3, particularly for unlit scenes. Or perhaps for rendering lit scenes in a wide-gamut working color space (not just wide-gamut output color space). But either of those will require more investigation.

For now I think we should not auto-detect — SRGBColorSpace will be the right default for a while yet.

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.

3 participants