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: Use inline tone mapping only when rendering to screen. #26371

Merged
merged 6 commits into from
Jul 5, 2023

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jul 4, 2023

Fixed #26346.

Description

This PR ensures the inline tone mapping can only be used when rendering to screen. Similar to WebGLRenderer.outputColorSpace, WebGLRenderer.toneMapping has no effect when rendering to a render target anymore.

When tone mapping is required in context of RTT, OutputPass/OutputShader has to be used. This class uses now RawShaderMaterial which solves #26346. Meaning it compiles even when WebGLRenderer.toneMapping is set to something different than THREE.NoToneMapping.

In the next step, OutputPass can extract the tone mapping settings from the renderer.

This PR has to be noted in the migration guide.

@Mugen87 Mugen87 added this to the r155 milestone Jul 4, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
643.1 kB (159.5 kB) 643.2 kB (159.5 kB) +106 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
436.4 kB (105.6 kB) 436.5 kB (105.7 kB) +106 B

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 4, 2023

This PR actually fixes webgl_shaders_ocean since previously the tone mapping was applied twice to the water (the beauty pass for the reflections was tone mapped and the water surface itself). Now it only happens once.

@Mugen87 Mugen87 marked this pull request as ready for review July 4, 2023 09:33

if ( material.toneMapped ) {

if ( _currentRenderTarget === null || ( _currentRenderTarget !== null && _currentRenderTarget.isXRRenderTarget === true ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this additional check, tone mapping would not properly work in XR environments. It's essentially the same check like for colorSpace.

src/renderers/WebGLRenderer.js Fixed Show fixed Hide fixed
src/renderers/webgl/WebGLPrograms.js Fixed Show fixed Hide fixed
@drcmda
Copy link
Contributor

drcmda commented Oct 17, 2023

we noticed that this pr breaks postprocessing, and HDR workflow. for instance we would previously set materials to toneMapped=false and then go beyond RGB 0-1, which gets then picked up by the threshold set by the bloom pass. threshold=1 means nothing gets bloom except the materials that go over threshold.

now the toneMapped property has no effect and practically everything that blooms is now broken. is there a migration path?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 17, 2023

we noticed that this pr breaks postprocessing, and HDR workflow.

To clarify, this change did not break post processing and HDR workflow in this repository.

is there a migration path?

The idea is to apply tone mapping during the post processing workflow with a separate (or combined pass). The inline tone mapping is now kept in sync with the inline color space conversion which is only intended if you render directly to screen.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 17, 2023

Considering your concrete use case you have to implement selective bloom differently now like any other selective (per object) FX technique. Meaning you render two beauty passes and apply the bloom to only one of them.

@mrdoob
Copy link
Owner

mrdoob commented Oct 18, 2023

Hmm...

for instance we would previously set materials to toneMapped=false and then go beyond RGB 0-1, which gets then picked up by the threshold set by the bloom pass.

Does this work?

material.emissive.set( 1, 0, 0 );
material.emissiveIntensity = 100;

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.

OutputPass: shader does not compile if renderer.toneMapping is set
3 participants