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

Reflector/Refractor: Change render target setups. #24386

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jul 26, 2022

Fixed #24384.

Description

This PR improves HDR support for Reflector, Refractor and ReflectorForSSRPass and makes the internal render target and shader setups more consistent.

  • The internal render targets are now of type half float, and in linear color space. They are not tone mapped.
  • The fragment shaders of Reflector, Refractor and ReflectorForSSRPass now properly support color space conversion and tone mapping according to the renderer settings.

Since half float textures are supported on all WebGL 2 devices, it should be a safe default. Just want to highlight that there is a small performance loss when using FP16 instead of UINT8. Not all apps are going to require FP16 but I think it's still an appropriate default.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 26, 2022

I'm not confident about how to use HDR workflows within three.js post-processing, but I believe most of these changes should be omitted from ReflectorForSSRPass.js. Use of HalfFloatType there is good, the rest I would skip. The post-processing chain should remain in Linear-sRGB (not encoded), and open-domain [0,∞] (not tone mapped), until explicitly transformed by ToneMapShader or GammaCorrectionShader passes, or an equivalent effect in pmndrs/postprocessing.

@donmccurdy
Copy link
Collaborator

/cc @vanruesc I believe this change is compatible with pmndrs/postprocessing, but just FYI.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 26, 2022

The name might be confusing but ReflectorForSSRPass is not a post-processing pass. It's derived from Mesh and works similar to Reflector. The tone mapping and encoding changes are then necessary.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 26, 2022

Hm, ReflectorForSSRPass is meant to be used together with SSRPass though, I assume? Individual surfaces should not be applying their own tone mapping and color space encoding, if we are going to use GammaCorrectionShader later:

composer.addPass( ssrPass );
composer.addPass( new ShaderPass( GammaCorrectionShader ) );

Within our current post-processing workflow, I don't think renderer.outputEncoding=sRGBEncoding means or does anything useful1 ... That could be defined and changed, but for now EffectComposer should probably just be used with .outputEncoding=LinearEncoding unless the author is quite sure of what they're doing.


1 pmndrs/postprocessing handles both sRGB and Linear output encoding automatically, readers using it should refer to pmndrs documentation instead.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 27, 2022

Hm, ReflectorForSSRPass is meant to be used together with SSRPass though, I assume?

You're right. ReflectorForSSRPass is tightly coupled to SSRPass. I'll revert the related changes.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 27, 2022

EffectComposer should probably just be used with .outputEncoding=LinearEncoding unless the author is quite sure of what they're doing.

When using post processing, the defaults of WebGLRenderer.outputEncoding and WebGLRenderer.toneMapping should not be changed since color space conversion and tone mapping should be done at the end of the pass chain.

One could consider that EffectComposer adds a (combined) tone mapping and color space pass automatically according to the renderer settings but I suggest to discuss this topic with a different issue.

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

When using post processing, the defaults of WebGLRenderer.outputEncoding and WebGLRenderer.toneMapping should not be changed ...

To be more explicit, users should select renderer.outputEncoding=LinearEncoding and renderer.toneMapping=NoToneMapping (both defaults) when using three.js's provided post-processing implementation. Users of pmndrs/postprocessing should generally use sRGBEncoding instead.

The PR looks ready to me if @WestLangley has no objection. :)

@WestLangley
Copy link
Collaborator

The PR looks ready to me if @WestLangley has no objection.

I'm happy if it implements what I said in #24384 (comment). I think there is agreement.

I am not familiar with ReflectorForSSRPass, however.

@Mugen87 Mugen87 added this to the r144 milestone Jul 29, 2022
@Mugen87 Mugen87 merged commit d5447ca into mrdoob:dev Aug 3, 2022
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
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.

Reflector Color Space Issues
3 participants