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

TSL: Introduce renderOutput() #28781

Merged
merged 7 commits into from
Jul 2, 2024
Merged

TSL: Introduce renderOutput() #28781

merged 7 commits into from
Jul 2, 2024

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Jul 2, 2024

Related issue: #28779 (comment)

Description

Color Space Conversion and Tone Mapping are applied in Post Processing pass, it is possible to manipulate the sequence too using renderOutput() as updated in the 3dlut example.

// post processing

postProcessing = new THREE.PostProcessing( renderer );

// ignore default output color transform ( toneMapping and outputColorSpace )
// use renderOutput() for control the sequence

postProcessing.outputColorTransform = false;

// scene pass

const scenePass = pass( scene, camera );
const outputPass = renderOutput( scenePass );

lutPass = outputPass.lut3D();
lutPass.lutNode = texture3D( lutMap[ params.lut ] );
lutPass.intensityNode = uniform( 1 );

postProcessing.outputNode = lutPass;

.toneMappingNode has been removed and the examples updated, I think it can be replaced by post-processing now.

@sunag sunag added this to the r167 milestone Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
682.2 kB (169 kB) 682.2 kB (169 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
459.4 kB (110.9 kB) 459.4 kB (110.9 kB) +0 B

@sunag
Copy link
Collaborator Author

sunag commented Jul 2, 2024

#28780 It seems to work :)

@sunag sunag marked this pull request as ready for review July 2, 2024 02:11
Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

Awesome! This change solves both issues discussed in #28779. Devs can now controls via RenderOutputNode when output tone mapping and color space conversion should happen in the pass chain. And second, the additional output pass in Renderer is now disabled when post processing is used (so there is on separate output pass anymore).

@Mugen87 Mugen87 merged commit 74d2d41 into mrdoob:dev Jul 2, 2024
12 checks passed
@sunag sunag deleted the dev-colorspace branch July 2, 2024 16:24
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 4, 2024

I've noticed an issue when experimenting with FXAANode.

Unlike Lut3DNode, some passes like FXAA require the result of RenderOutputNode as a texture. But if you call toTexture() on RenderOutputNode, the result is getting too dark. Check out the following fiddle to see the effect:

https://jsfiddle.net/3vgz4sxw/

If you remove the toTexture() call, colors are as expected. Ideally, toTexture() does not alter the final result.

@WestLangley
Copy link
Collaborator

FWIW, maybe

postProcessing.outputColorTransform = true;

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 4, 2024

I'm afraid that is not right. The idea is to set outputColorTransform to false if you use RenderOutputNode.

The texture node returned by toTexture() should hold sRGB values but it looks like it doesn't.

@donmccurdy
Copy link
Collaborator

I don't think I quite understand this code:

  // post processing

  postProcessing = new THREE.PostProcessing( renderer );
  postProcessing.outputColorTransform = false;

  // scene pass

  const scenePass = pass( scene, camera );
  const outputPass = renderOutput( scenePass );


  postProcessing.outputNode = outputPass.toTexture();

Correct me if I get this wrong, but it looks like scenePass is the initial render pass, renderOutput tone maps and converts to the display color space, and then the result is assigned to postProcessing.outputNode ... so postProcessing.outputNode is the input to the post-processing stack?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 4, 2024

so postProcessing.outputNode is the input to the post-processing stack?

It is the last node in the stack that produces the final values for display. It's like when you assign node compositions to *Node material properties (like colorNode).

With FXAANode a typical setup could look like this:

postProcessing = new THREE.PostProcessing( renderer );
postProcessing.outputColorTransform = false;

  // scene pass (ideally produced via MRT)

const scenePass = pass( scene, camera );
const scenePassColor = scenePass.getTextureNode();
const scenePassViewZ = scenePass.getViewZNode();
const scenePassNormal = scenePass.getNormalNode();

// Bloom -> Output (tone-mapping+sRGB) -> FXAA

const bloomPass = scenePassColor.bloom( scenePassViewZ, scenePassNormal );
const outputPass = bloomPass.renderOutput();
const fxaaPass = outputPass.fxaa();

postProcessing.outputNode = fxaaPass;

FXAA requires sRGB input and since it is not a per-pixel operation, the result of previous pass as a texture. The toTexture() call is done internally in FXAANode. The code in the fiddle is just a simplified setup.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 4, 2024

You can actually chain all commands or write them more explicitly:

const bloomPass = bloom( scenePassColor, scenePassViewZ, scenePassNormal );
const outputPass = renderOutput( bloomPass );
const fxaaPass = fxaa( outputPass );
postProcessing.outputNode = fxaaPass;

Or everything in one row:

postProcessing.outputNode = scenePassColor.bloom( scenePassViewZ, scenePassNormal ).renderOutput().fxaa();

@sunag
Copy link
Collaborator Author

sunag commented Jul 4, 2024

@Mugen87 Try add this for now:

const outputPass = renderOutput( scenePass, renderer.toneMapping, renderer.outputColorSpace )

.toTexture() It is not transferring the context of rendering, but this can be fixed.

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.

4 participants