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

Add: "Extended" range HDR support for supported displays #29656

Closed

Conversation

hybridherbst
Copy link
Contributor

@hybridherbst hybridherbst commented Oct 14, 2024

Related issue: #26479

Description

After seeing the PlayCanvas demo (https://engine-hlvhm84x4-playcanvas.vercel.app/#/graphics/clustered-lighting) in HDR mode, I thought I'd give it a spin on three.js as well. It looks good! (on supported screens, like all MacBooks)

I'm marking this PR as draft for a number of reasons:

  1. Because I'm not super sure what the right way to integrate is
  2. It will only be supported for WebGPUBackend at the moment.
  3. It simply disables Tonemapping instead of adapting it for HDR

Chrome 129 rolled out support for "extended" range widely, but only for WebGPU at the moment. While there is a WebGL proposal, it doesn't seem to be implemented so far.

Live Demo

image

Image does not blown out in the live link below on a HDR monitor

https://raw.githack.com/needle-tools/three.js/feature/extended-range-hdr-support/examples/webgpu_loader_gltf_anisotropy.html

This contribution is funded by Needle

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 689.92
170.93
689.92
170.93
+0 B
+0 B
WebGPU 814.88
219.41
815.14
219.51
+257 B
+103 B
WebGPU Nodes 814.39
219.28
814.65
219.38
+257 B
+102 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 462.94
111.81
462.94
111.81
+0 B
+0 B
WebGPU 537.7
145.08
537.95
145.19
+257 B
+113 B
WebGPU Nodes 493.81
134.81
494.07
134.93
+257 B
+116 B

Comment on lines -82 to +83
renderer.toneMapping = THREE.ACESFilmicToneMapping;
// renderer.toneMapping = THREE.ACESFilmicToneMapping;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Important note – omission of tone mapping is an error in the PlayCanvas implementation, and others I've seen so far. We do still need tone mapping, for SDR as well as HDR, as we are still forming an image for a display. The display is of course brighter than before, and will require adaptations to the tone mappers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware, good point! – added to the DRAFT reasons above.

I did some testing on this as well: Mac displays are supposed to be ~3x brighter in HDR mode (1600 nits instead of 500 for SDR content), from my limited testing the practical range ("how far can I crank emissiveIntensity or environmentIntensity") seems to be ~20-30x brighter though. So there might already be tonemapping on the hardware side as well.

This image also behaves kind of suspicious: https://www.instagram.com/p/C_Byx6jRfuX/
When adjusting screen brightness while it's open, there are noticeable "steps" where the display goes into different modes (for a lack of better term: "tonemapping modes") with somewhat different relative feel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, ouch! I was aware that adjusting screen brightness changed the available “HDR” headroom, and that this would be a problem with the current WebGPU HDR implementation, but I hadn't seen a clear demo of the issue like that. Really helpful illustration — thanks.

@donmccurdy
Copy link
Collaborator

Related PR:

@hybridherbst
Copy link
Contributor Author

Ah, thanks for referencing that PR, didn't see that – should I close this one then?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 16, 2024

Yes, let's focus on #29573.

@Mugen87 Mugen87 closed this Oct 16, 2024
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.

3 participants