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

Do we need a enableHDR flag? #26071

Closed
Mugen87 opened this issue May 17, 2023 · 11 comments
Closed

Do we need a enableHDR flag? #26071

Mugen87 opened this issue May 17, 2023 · 11 comments

Comments

@Mugen87
Copy link
Collaborator

Mugen87 commented May 17, 2023

Description

I want to add better support for RTT in HDR setups.

Post-processing and other RTT example code currently use RGBA8 render targets in most cases. However, this configuration does not support HDR and can introduce banding artifacts.

The banding artifacts can be fixed by using SRGBA8 render targets (meaning UnsignedByteType + SRGBColorSpace) or by using half float render targets (via HalfFloatType). The latter one also supports HDR.

The problem is without knowing whether the application uses HDR or not, it's not possible to distinct between SRGBA8 and FP16.

Solution

Introduce a new flag enableHDR on renderer or composer level so it's possible to setup correct render targets.

Alternatives

  • Always use half float. There is a performance impact when doing this though because using half float is a bit more costly than SRGB8. However, this should be measured since when the difference in performance is only small, I would suggest to always use half float (e.g. as the default in EffectComposer and built-in passes)

  • Maybe we can evaluate which type of inline tone mapping is configured via WebGLRenderer.toneMapping? In a proper HDR post-processing scenario right now, the property should be NoToneMapping though (and tone mapping applied via a post-processing pass).

Additional context

There is a bug with SRGB8 render targets and M1 chips, see https://bugs.chromium.org/p/chromium/issues/detail?id=1329199&q=&can=2. The flickering also happens with M2 chips since I see it with Chrome on a M2 Pro mac Mini. Because of this we can't safely use SRGB8 render targets at the moment.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 17, 2023

HDR can mean a number of things. I would try to avoid the term in property names, if we can find a more specific way to say what we mean. In particular, output to canvas in PQ or HLG color spaces would probably be a more traditional meaning of HDR, and these are under consideration. In this case, we're focused on preserving open domain [0, ∞] data in the working color space throughout the post-processing effect stack.

Alternatives:

A: Consider an option to change the default settings for render targets constructed by EffectComposer and provided passes. The pmndrs/postprocessing library takes this approach.

composer.frameBufferType = HalfFloatType

B: Consider an API that identifies the type of data required for each pass. Choice of color space does not necessarily imply the choice of open [0, ∞] or closed [0,1] domains.

composer.addPass( bloomPass, { colorSpace: LinearSRGBColorSpace, clamped: false } ); 
composer.addPass( toneMappingPass, { colorSpace: LinearSRGBColorSpace, clamped: false } ); 
composer.addPass( gammaPass, { colorSpace: LinearSRGBColorSpace, clamped: true } );
composer.addPass( otherPass, { colorSpace: SRGBColorSpace, clamped: true } ); 

Some passes cannot work properly without open domain [0, ∞] input. Others may not work properly without closed domain [0,1] input. A per-pass API might make it easier to warn when these choices are incorrect, but I am probably still leaning toward (A) here.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 17, 2023

HDR can mean a number of things. I would try to avoid the term in property names, if we kind find a more specific way to say what we mean.

I'm okay with that. I would also like to keep things simple and just focus on the configurability of the internal render targets for now.

The thing is that it's already possible to configure the default render targets by passing a configured render target to the constructor of EffectComposer. However, I'm not so happy with this approach since I would prefer to not confront the basic user with render targets. I wonder if we can make things more simple and replace the second parameter with a composer mode value. Additionally, I would add EffectComposer.setRenderTarget() for everyone who needs full control over the render target creation.

So I wonder if we could introduce two new constants representing the possible composer modes (HalfFloatComposerMode (default) and SRGBComposerMode). You could then do this:

const composer = new EffectComposer( renderer, mode );

This will setup the render targets in the combination HalfFloatType + LinearSRGBColorSpace OR UnsignedByteType + SRGBColorSpace. Users can overwrite this via setRenderTarget() if necessary.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 17, 2023

The thing is that it's already possible to configure the default render targets by passing a configured render target to the constructor of EffectComposer.

Does this setting get inherited by all passes? I've never been sure of that, and passes like UnrealBloomPass seem to construct render targets without checking. If not, that might be necessary to fix first.

I appreciate wanting to simplify this for users, though I am not sure what preset modes we should define. Modes named after specific framebuffer types or color spaces don't feel quite right, but perhaps something like that.

The pmndrs/postprocessing project is headed in a somewhat different direction here, with a larger pipeline redesign coming: pmndrs/postprocessing#419.

@WestLangley
Copy link
Collaborator

Here is the pattern I have used:

const parameters = {
	minFilter: THREE.LinearFilter,
	magFilter: THREE.LinearFilter,
	generateMipMaps: false,
	format: THREE.RGBAFormat,
	type: THREE.HalfFloatType
};

const size = renderer.getDrawingBufferSize( new THREE.Vector2() );

const renderTarget = new THREE.WebGLRenderTarget( size.width, size.height, parameters );

renderTarget.texture.name = 'EffectComposer.rt1';

composer = new EffectComposer( renderer, renderTarget );

So my suggestion would be to use the same defaults if the renderTarget is undefined in the EffectComposer constructor. (The parameters can be debated.)

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 17, 2023

@WestLangley This is exactly the way I've wanted to setup the half float render target inside EffectComposer ^^.

Does this setting get inherited by all passes?

Not yet. But when EffectComposer is updated with the new render target configuration, I've planned to go through every pass class and update the render target configuration as well.

I'm aware of pmndrs/postprocessing but with this issue I just want to find a solution for the HDR and banding problems. I think we can fix this without a larger redesign of the FX pipeline. This is something that can be done in context of WebGPU/node material.

Modes named after specific framebuffer types or color spaces don't feel quite right, but perhaps something like that.

I'm open for any suggestions here. I just want to avoid that users pass render targets to the ctor (I find this too technical, too many things can go wrong at this point). Ideally, they can pick between composer modes or something similar.

@donmccurdy
Copy link
Collaborator

Would passing parameters to the composer, rather than a render target, be easier?

const composer = new EffectComposer( renderer, {
  colorSpace: LinearSRGBColorSpace,
  type: HalfFloatType
} );

We could provide some presets, as well:

import { EffectComposer, PHYSICAL_EFFECT_CONFIG } from 'three/addons/<...>';

console.log( PHYSICAL_EFFECT_CONFIG ); // → { type: HalfFloatType, ... }

const composer = new EffectComposer( renderer, PHYSICAL_EFFECT_CONFIG );

@CodyJasonBennett
Copy link
Contributor

Would passing parameters to the composer, rather than a render target, be easier?

FWIW that's what pmndrs/postprocessing does to let you choose between UnsignedByteType + SRGBColorSpace (default) or HalfFloatType like below:

import { HalfFloatType } from 'three';
import { EffectComposer } from 'postprocessing';

const composer = new EffectComposer( renderer, { frameBufferType: HalfFloatType } );

Not yet. But when EffectComposer is updated with the new render target configuration, I've planned to go through every pass class and update the render target configuration as well.

I'm aware of pmndrs/postprocessing but with this issue I just want to find a solution for the HDR and banding problems. I think we can fix this without a larger redesign of the FX pipeline. This is something that can be done in context of WebGPU/node material.

Is there any reason why HalfFloatType isn't the default anywhere? I've opted for this in my own work because of the issue with Apple Mx or specifically when using bloom or unclamped effects, but I understand this is a broader consideration in the library.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 18, 2023

Is there any reason why HalfFloatType isn't the default anywhere?

We did not use it as a default so far since WebGL 1 does not necessarily support half float render targets on all hardware. But with its deprecation of the renderer (see #25959) we can now move into this direction.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 22, 2023

While making the latest changes I've noticed that the performance penalty of using half float on desktops is almost negligible. Demos which run at 60 FPS before also run a the same frame rate with FP16 render targets. The only demo that showed a drop of 2 frames (from 43 to 41) was the demanding SAO example. Because of this and the general benefits of FP16, I feel confident that FP16 is an appropriate default. Still it would be good regarding mobile environments to have the possibility for configuring SRGBA8.

We could provide some presets, as well:

I like the idea of having presents since it's similar to constants. Still trying to figure out good names for FP16/HDR and SRGB8/LDR. PHYSICAL_EFFECT_CONFIG sounds already good and would be the default preset. What about SRGBA8_EFFECT_CONFIG for the SRGB8 case?

I think the docs (the post-processing guide) could explain both presets and their intended usage in more detail.

@donmccurdy
Copy link
Collaborator

It seems like changing to the float16 default has not been a problem for users. Perhaps we can keep the settings as they are, without the need for presets?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 19, 2023

Yes, that sounds good! FP16 works more reliable and with acceptable performance than originally expected. Even on mobile devices. Closing until there is a concrete user request for SRGB8.

@Mugen87 Mugen87 closed this as completed Sep 19, 2023
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

4 participants