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

WebGPURenderer: MeshNormalMaterial should not be tone mapped #28831

Closed
WestLangley opened this issue Jul 7, 2024 · 14 comments
Closed

WebGPURenderer: MeshNormalMaterial should not be tone mapped #28831

WestLangley opened this issue Jul 7, 2024 · 14 comments
Milestone

Comments

@WestLangley
Copy link
Collaborator

WestLangley commented Jul 7, 2024

Description

WebGLRenderer:

MeshNormalMaterial inherits the .toneMapped property as true, but the material omits the tone mapping shader chunks, so the material is never tone mapped.

This is the correct behavior. We never want the material to be tone mapped.

WebGPURenderer:

MeshNormalMaterial responds to tone mapping. It should not. This needs to be fixed.

Note: There are other problems with MeshNormalMaterial that cause the material to be washed out when rendered with WebGPURenderer. That will be addressed in a separate PR after this issue is resolved.

Reproduction steps

Code

// code goes here

Live example

Screenshots

No response

Version

r167dev

Device

No response

Browser

No response

OS

No response

@WestLangley WestLangley added this to the r167 milestone Jul 7, 2024
@sunag
Copy link
Collaborator

sunag commented Jul 8, 2024

This is not a bug, .material.toneMapped=false and material.colorSpaced=false are not supported by WebGPURenderer. #27880 #28779 (comment) #27238 (comment)

It is still possible to use MeshNormalMaterial in RenderTarget without toneMapping, where it would have more useful.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 8, 2024

Maybe it's important to highlight once more that WebGPURenderer does not support inline tone mapping and color space conversion. Hence, it is not possible to control which materials should be tone mapped or not. I think this is a good change since it solves #23019 and makes the tone mapping and color space step more consistent.

So WebGPURenderer always handles tone mapping and color space conversion as a separate render pass and thus processing the entire image.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 8, 2024

The main purpose for MeshNormalMaterial is to save normals in a render target for further processing. The fact that it can be used as a "normal" material for rendering to screen is a nice side effect but not its main purpose. So retaining the visual look of MeshNormalMaterial is negligible, imo.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Jul 8, 2024

WebGLRenderer on the left; WebGPURenderer on the right. No tone mapping applied.

Screenshot 2024-07-08 at 11 39 38 AM

IMHO, I do not think this is acceptable. By convention, the packed normal, when interpreted as a color, is assumed to be in sRGB color space. WebGPURenderer should not be applying the sRGB transfer function to an sRGB color.

@WestLangley
Copy link
Collaborator Author

Maybe it's important to highlight once more that WebGPURenderer does not support inline tone mapping and color space conversion. Hence, it is not possible to control which materials should be tone mapped or not.

Hmm... I am inclined to think that is too restrictive. If all materials are tone mapped, I think we have to have a way to disable it.

Think of overlay text or a heads-up display. That would not normally be tone mapped.

Nor should something like AxesHelper -- and MeshLineMaterial. Also MeshBasicMaterial in certain cases.

Tone mapping can cause significant hue shifts. So can exposure control.

@sunag
Copy link
Collaborator

sunag commented Jul 8, 2024

Think of overlay text or a heads-up display. That would not normally be tone mapped.

HUD is normally used in another render-pass, so it won't be affected.

@WestLangley
Copy link
Collaborator Author

What about the other issues I mentioned?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 8, 2024

The toneMapped property was coupled to inline tone mapping. There is no way to support this property when tone mapping is applied as a pass.

@WestLangley I don't think we can achieve what you are asking for without changing some fundamentals in the renderer. I do not vote for doing this since tone mapping and color space conversion is now more correctly implemented than in WebGLRenderer.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 8, 2024

There was always a difference when tone mapping was applied inline and when used in FX workflows via OutputPass. This difference is now finally resolved in WebGPURenderer.

@sunag
Copy link
Collaborator

sunag commented Jul 8, 2024

Since AxesHelper overlaps all other objects, this could be solved with a render-pass as well.

image

I believe that other cases could be solved with MRT properties applied to the material and PostProcessing, such as:

material.mrtNode = mrt( { overlay: directionToColor( normalWorld ) } );

This is being developed initially here #28833

if we look at the projects from the three.js home page, I think 99% will make better use of the new system, normals for visual purposes is more related to our example.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Jul 8, 2024

WebGPURenderer removes the ability for users to set material.toneMapped to false, a feature which has been used by WebGLRenderer internally -- particularly for objects that do not respond to lights.

Another example use case would be text labels.

Historical context: a related post from @bhouston.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 9, 2024

Sorry, but I think this goes in the wrong direction. The toneMapped property is controversial right from the beginning since you can solve the mentioned issues with other approaches as well. Separate, non-tone mapped render passes or optimized MRT passes should address stuff like helper objects, text labels or HUD.

If tone mapping is configured for a render output, it should affect the entire image and not specific objects. That is also true for color space conversion. Issues like #24362, #23019 or clear color related tone mapping and sRGB issues stem from the fact the engine did not implement this step correctly.

I think it's best if Material.toneMapped is deprecated as soon as WebGLRenderer is deprecated in the future.

@Mugen87 Mugen87 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2024
@bhouston
Copy link
Contributor

bhouston commented Jul 9, 2024

Normal, depth and velocity materials (and similar utility materials) are intended to be rendered into linear space only - never sRGB and also never tone mapped. This is because they need to be encoded consistently so that they can be decoded consistently. Thus toneMapping should never be supported on them.

@mrdoob
Copy link
Owner

mrdoob commented Jul 11, 2024

Should we add more logic in the renderers to automatically render objects that shouldn't be tone mapped in a different pass?

That way we won't need to deprecate material.toneMapped... 🤔

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

5 participants