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

WebGLRenderer: Stable reversed Z buffer implementation. #29579

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

eXponenta
Copy link
Contributor

@eXponenta eXponenta commented Oct 7, 2024

Fixed #29578
Related PR: #29445

Description
Fix: reset clip state when reset is called
Fix: valid depth clear value when reversed is enabled

Feat: non-persistent reversedZ state ( can be controlled via renderer.state.buffers.depth.setReversed( )))

Fix: reset clip state when reset is called
Fix: valid depth clear value when reversed is enabled

Feat:  non-persistent reversedZ state ( can be controlled via renderer.state.buffers.depth.setReversed(  )))
Copy link

github-actions bot commented Oct 7, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 692.71
171.44
692.88
171.51
+171 B
+75 B
WebGPU 817.93
220.59
817.93
220.59
+0 B
+0 B
WebGPU Nodes 817.44
220.46
817.44
220.46
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 464.93
112.2
465.09
112.28
+161 B
+76 B
WebGPU 538.86
145.56
538.86
145.56
+0 B
+0 B
WebGPU Nodes 494.86
135.37
494.86
135.37
+0 B
+0 B

fix: program parameters
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.

LGTM! Would be great if @CodyJasonBennett could also have a look 👍 .

@Mugen87 Mugen87 added this to the r170 milestone Oct 8, 2024
@Mugen87 Mugen87 changed the title fix: stable reversed Z buffer implementation WebGLRenderer: Stable reversed Z buffer implementation. Oct 8, 2024
@eXponenta
Copy link
Contributor Author

eXponenta commented Oct 11, 2024

@CodyJasonBennett can you explain how logarithmicDepth should work with reversedZ?
Looks like it works no fully stable ( maybe for me only ).
I got flipped depth when logarithmic is enabled.

Looks like needs to change math for it also:

gl_FragDepth = vIsPerspective == 0.0 ? gl_FragCoord.z : log2( vFragDepth ) * logDepthBufFC * 0.5;

p_uniforms.setValue( _gl, 'logDepthBufFC',

@CodyJasonBennett
Copy link
Contributor

You should not use those together. Reverse depth is a strict upgrade and doesn't disable early-z optimizations like logarithmic depth does. I'd expect something is not cleaning up if you see any change to logarithmic depth.

@eXponenta
Copy link
Contributor Author

You should not use those together. Reverse depth is a strict upgrade and doesn't disable early-z optimizations like logarithmic depth does. I'd expect something is not cleaning up if you see any change to logarithmic depth.

But by default we can create it together, because capabilities not disable logDepth if we can use reversedZ

@Mugen87 Mugen87 merged commit 1075735 into mrdoob:dev Oct 21, 2024
12 checks passed
@WestLangley
Copy link
Collaborator

You should not use those together.

@Mugen87 Perhaps an API change is warranted...

THREE.StandardDepthBuffer
THREE.ReversedZDepthBuffer
THREE.LogarithmicDepthBuffer
const renderer = new THREE.WebGPURenderer( { depthBuffer: THREE.LogarithmicDepthBuffer } );

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 21, 2024

This is a great suggestion! 👍

@mrdoob
Copy link
Owner

mrdoob commented Oct 22, 2024

Indeed! 👍

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Oct 22, 2024

If EXT_clip_control was widely available, I would suggest removing logarithmic depth entirely.

@CodyJasonBennett
Copy link
Contributor

Is it really worth an API change which will not be future proof when exactly reverse-z is opt-in? It is a strict improvement over logarithmic depth where EXT_clip_control is available. I'd wager it will be GA and diffuse before WebGPU, as I implemented this after sitting in the meeting for the announcement of this extension's shipping in Safari.

@prideout
Copy link
Contributor

FWIW, logarithmic depth already doesn't play nicely with some features (e.g. polygonOffset) so I think Cody's proposal (API simplification rather than API enhancement) is worth considering , especially since EXT_clip_control is now supported in Safari and Chrome.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 28, 2024

I agree it's better to just have two types of depth buffer implementation and deprecate logarithmic depth buffer in the future.

It might still be a good idea to switch to enums instead of using flags since it feels more readable. However, I'm not feeling strong about that.

@WestLangley
Copy link
Collaborator

Given the recent comments, I am OK with leaving the API as-is.

@prideout
Copy link
Contributor

prideout commented Oct 30, 2024

Hi @eXponenta, before this PR, reversed Z worked in my simple sandbox app without any changes other than passing the new param to the renderer constructor. However after this PR, my app does not render anything unless I add renderer.getContext().clearDepth(0) to my app as a workaround.

Could that be due to the removed line that called _gl.clearDepth?

Comment on lines +105 to +107
const oldDepth = currentDepthClear;
currentDepthClear = null;
this.setClear( oldDepth );
Copy link
Contributor

Choose a reason for hiding this comment

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

@eXponenta Additionally, maybe we should also do:

const oldFunc = currentDepthFunc;
currentDepthFunc = null;
this.setFunc( oldFunc );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Yep, this one.
Because we also should upgrade depth func to inverter version.

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.

Reverse-Z state problem
6 participants