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

Renderer: Use output color space conversion only when rendering to screen. #28909

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jul 18, 2024

Fixed #28826.

Description

This PR ensures output tone mapping and color space conversion follow the same policy. They are only used when rendering to screen.

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
683.7 kB (169.3 kB) 683.7 kB (169.3 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
460.9 kB (111.2 kB) 460.9 kB (111.2 kB) +0 B

@WestLangley
Copy link
Collaborator

WestLangley commented Jul 18, 2024

@Mugen87 (edited)

Can you please summarize the policy for both WebGLRenderer and WebGPURenderer?

Which renderer properties should only apply when rendering to the screen -- in your view?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 18, 2024

At some point, I would be interested in making this configurable. Users may need to do these transforms when drawing to a render target, without the cost of an additional pass. Would something like renderTarget.needsOutputTransform = true make sense? Or renderer.setRenderTarget( target, true )?

@@ -457,7 +457,7 @@ class Renderer {
const { currentColorSpace } = this;
Copy link
Collaborator

@sunag sunag Jul 18, 2024

Choose a reason for hiding this comment

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

currentColorSpace already does that, I was thinking of renaming it to targetColorSpace, currenty this resolve #28909 (comment) issue too.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 18, 2024

When a render target is marked as RGBA8UnormSRGB (WebGPU) or SRGB8_ALPHA8 (WebGL 2), the framebuffer holds sRGB values. There is no need for a manual color space conversion. WebGLRendeder does honor that, WebGPURenderer doesn't.

If you want to force a color space conversion when rendering into a render target, it's maybe best to introduce a new property and don't use renderTarget.texture.colorSpace for that. Like with normal textures, colorSpace should just indicate what the color space of the texels is. The property alone is not a save indicator for triggering a color space conversion since that depends on the render targets format/type.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 18, 2024

Yes, I think the current behavior for color space conversion is probably enough, no need for a property to force that. But we need tone mapping to happen before the color space conversion, and that's hard-coded off for render targets.

EDIT: I suppose we could just ... respect tone mapping if the target is SRGB8_ALPHA8, no option needed? That's more or less what Blender does – applying the output transforms automatically to PNG exports but not EXR exports, for example.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 18, 2024

I'm afraid this could have side effects. Think of an app that configures tone mapping on renderer level and now loads an sRGB equirectangular texture. The renderer should automatically convert it to the cube map format. If the cube map render target is marked as RGBA8 + sRGB, there should be no tone mapping in place since the source is RGBA8 + sRGB.

I don' think it's possible to figure out an automatism that selectively applies tone mapping whenever rendering to SRGB8_ALPHA8 or RGBA8UnormSRGB .

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 18, 2024

Good example, and I agree that we can't and shouldn't try to automatically handle every case. I guess I was thinking of that suggestion as making it "less automatic" than it is now, automatically disabling tone mapping only for floating point render targets, rather than for all render targets.

The caller in the cube map scenario would need to do this...

renderer.setRenderTarget( rtt );
renderer.toneMapping = NoToneMapping;
renderer.render( scene, light.shadow.camera );
renderer.setRenderTarget( currentRenderTarget );
renderer.setRenderObjectFunction( currentRenderObjectFunction );
renderer.toneMapping = currentToneMapping;

... which I'm inclined to say should be best practice anyway, rather than assuming that render targets will always ignore the tone mapping setting.

But without that, I suppose an explicit option would be required.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 18, 2024

I'm okay with revisiting this topic and maybe define a new policy when tone mapping and color space conversion is applied for render targets. But this is something I would discuss in a separate issue. Would that be okay for everyone?

This PR is just intended to make WebGPURenderer honor the existing policy. And that means no color space conversion and tone mapping happens in the renderer with an active render target. The output color space conversion and tone mapping is done by the renderer when rendering to screen or by OutputPass/renderOutput() when doing FX.

@donmccurdy
Copy link
Collaborator

Yes, sorry for side-tracking – that's fine with me, I'll follow up elsewhere, and thank you!

@WestLangley
Copy link
Collaborator

This PR is just intended to make WebGPURenderer honor the existing policy.

@Mugen87 I do not think there is an existing policy that is being adhered to consistently by either the code or the docs. But I would like to check.

If you would answer my question above, I would be grateful, and I will continue the discussion in another thread if needed.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 18, 2024

If you would answer my question above, I would be grateful, and I will continue the discussion in another thread if needed.

I thought I did in #28909 (comment). Is there something missing from your point of view?

@WestLangley
Copy link
Collaborator

OK, I'll state what I think your policy is...

WebGLRenderer

  1. renderer.outputColorSpace is honored only when rendering to the default drawing buffer.

  2. According to the docs, when rendering to a render target, renderTarget.texture.colorSpace is honored instead. I am not sure if that is what you intend -- or even if it is true.

  3. renderer.toneMapping is honored only when rendering to the default drawing buffer.

  4. renderer.toneMapping is ignored when rendering to a render target -- although the docs do not explicitly make that clear.

WebGPURenderer

I assume you want the behavior to be the same as it is for WebGLRender.

//

Classes such as Reflector render to a render target and do not adhere to these policies. Correct?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 19, 2024

According to the docs, when rendering to a render target, renderTarget.texture.colorSpace is honored instead. I am not sure if that is what you intend -- or even if it is true.

This statement in the docs is wrong. When a render target is set, the output color space is always LinearSRGBColorSpace (except for XR which has a different concept of a default framebuffer). Respective line in WebGLPrograms.

outputColorSpace: ( currentRenderTarget === null ) ? renderer.outputColorSpace : ( currentRenderTarget.isXRRenderTarget === true ? currentRenderTarget.texture.colorSpace : LinearSRGBColorSpace ),

In other words, renderTarget.texture.colorSpace should not control the output color space. Output color space is just for rendering to screen.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 19, 2024

When a render target is set, the output color space is always LinearSRGBColorSpace

Minor: I think we'd like this to be ColorManagement.workingColorSpace, rather than hard-coded to LinearSRGBColorSpace, but of course they're the same in 99.9% of cases today. Effectively this means no output conversion is made before writing to the render target.

Classes such as Reflector render to a render target and do not adhere to these policies. Correct?

I think it's OK for particular classes to make different decisions than the renderer defaults ... but whether classes like Reflector are making the decisions we'd prefer, currently, I'm not sure.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 19, 2024

Classes such as Reflector render to a render target and do not adhere to these policies. Correct?

The internal render target of reflector is linear-srgb (so no color space conversion happens when rendering the scene with the virtual camera). However, the material that renders the reflective texture to screen uses inline color space conversion via the colorspace_fragment chunk like all other built-in materials.

In WebGPURenderer there is no inline color space conversion anymore since this happens as a separate (final) pass after tone mapping.

@Mugen87 Mugen87 added this to the r167 milestone Jul 19, 2024
@Mugen87 Mugen87 merged commit 9834113 into mrdoob:dev Jul 19, 2024
12 checks passed
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.

WebGPURenderer: CubeRenderTarget.fromEquirectangularTexture() does not work with sRGB input.
4 participants