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

WebGPU: Logarithmic Depth Buffer Rename/Revision + GTAONode Fixes #29870

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

PoseidonEnergy
Copy link
Contributor

@PoseidonEnergy PoseidonEnergy commented Nov 13, 2024

Fixed #29797.

  1. Renamed the function perspectiveDepthToLogarithmicDepth to viewZToLogarithmicDepth and modified it to expect a negative viewZ value in order maintain consistency with viewZToOrthographicDepth and viewZToPerspectiveDepth
  2. Revised the logarithmic depth curve once more to ensure it maintains a full 0-to-1 range no matter the camera near/far values.
    See new curves here on Desmos: https://www.desmos.com/calculator/uyqk0vex1u
  3. Added function logarithmicDepthToViewZ for use in GTAONode.js
  4. Fixed AO not working when logarithmicDepthBuffer is true

Related issue: #29797

Description

The name of the function perspectiveDepthToLogarithmicDepth is misleading because it actually expects a viewZ value (i.e. positionView.z), and NOT a depth value (i.e. a number between 0 and 1). In addition, the viewZ value that it previously expected had to be a positive number, which differed from its sister functions viewZToOrthographicDepth and viewZToPerspectiveDepth, which both expect a negative viewZ. In this pull request, the function in question is now named viewZToLogarithmicDepth, and expects a negative viewZ like the rest in its family.

AO was not working properly in the webgpu_postprocessing_ao example when logarithmicDepthBuffer was true. The function logarithmicDepthToViewZ was added to ViewportDepthNode.js, so it can now be used in GTAONode.js to obtain the proper depth values required for AO.

Here is a JSFiddle I made to demonstrate how viewZToLogarithmicDepth relates to the rest of the depth functions:

https://jsfiddle.net/go6f2y4u

And here is an animation of the JSFiddle:

depthfamily

image

image

…ZToLogarithmicDepth' and modified it to expect a negative viewZ value in order maintain consistency with 'viewZToOrthographicDepth' and 'viewZToPerspectiveDepth'

2. Added function 'logarithmicDepthToViewZ'
3. Fixed AO not working when 'logarithmicDepthBuffer' is true
Copy link

github-actions bot commented Nov 13, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.37
79.06
339.37
79.06
+0 B
+0 B
WebGPU 477.82
132.5
477.92
132.53
+90 B
+27 B
WebGPU Nodes 477.29
132.39
477.38
132.41
+90 B
+23 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 464.82
112.02
464.82
112.02
+0 B
+0 B
WebGPU 546.86
148.18
546.85
148.16
-12 B
-12 B
WebGPU Nodes 502.74
137.88
502.73
137.87
-12 B
-11 B

@PoseidonEnergy PoseidonEnergy changed the title Logarithmic Depth Buffer Rename/Revision + GTAONode Fixes WebGPU: Logarithmic Depth Buffer Rename/Revision + GTAONode Fixes Nov 13, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 13, 2024

@PoseidonEnergy Tested the change locally and it works as expected! Awesome PR 👍 !

@Mugen87 Mugen87 added this to the r171 milestone Nov 13, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 13, 2024

@PoseidonEnergy Your fiddle for testing the TSL functions is super interesting!

Somewhat related: The project is actually looking for a way to unit test TSL functions (see #28708). It would be great to establish a basic procedure in our current unit tests (see https://github.com/mrdoob/three.js/tree/dev/test/unit) for testing TSL modules in isolated routines.

@Mugen87 Mugen87 merged commit 485f7f0 into mrdoob:dev Nov 13, 2024
12 checks passed
@@ -313,10 +313,10 @@ class ShadowNode extends Node {
// The normally available "cameraNear" and "cameraFar" nodes cannot be used here because they do not get
// updated to use the shadow camera. So, we have to declare our own "local" ones here.
// TODO: How do we get the cameraNear/cameraFar nodes to use the shadow camera so we don't have to declare local ones here?
const cameraNearLocal = uniform( 'float' ).onRenderUpdate( () => shadow.camera.near );
const cameraFarLocal = uniform( 'float' ).onRenderUpdate( () => shadow.camera.far );
const cameraNearLocal = reference( 'near', 'float', shadow.camera ).setGroup( renderGroup );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just one question, did the previous approach using uniform present any problems? Was there any specific reason for changing to reference?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did only switch to reference() for consistency reasons since this API is used in the rest of the file.

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.

WebGPU: GTAO is incompatible with logarithmicDepthBuffer
4 participants