three.js icon indicating copy to clipboard operation
three.js copied to clipboard

WebGLRenderer: Stable reversed Z buffer implementation.

Open eXponenta opened this issue 1 year ago • 4 comments

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( )))

eXponenta avatar Oct 07 '24 16:10 eXponenta

📦 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

github-actions[bot] avatar Oct 07 '24 16:10 github-actions[bot]

@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: https://github.com/mrdoob/three.js/blob/36bac08e87b0a016ce920b59b3375ed682a03997/src/renderers/shaders/ShaderChunk/logdepthbuf_fragment.glsl.js#L6

https://github.com/mrdoob/three.js/blob/36bac08e87b0a016ce920b59b3375ed682a03997/src/renderers/WebGLRenderer.js#L2008

eXponenta avatar Oct 11 '24 14:10 eXponenta

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.

CodyJasonBennett avatar Oct 11 '24 14:10 CodyJasonBennett

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

eXponenta avatar Oct 11 '24 14:10 eXponenta

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 } );

WestLangley avatar Oct 21 '24 18:10 WestLangley

This is a great suggestion! 👍

Mugen87 avatar Oct 21 '24 19:10 Mugen87

Indeed! 👍

mrdoob avatar Oct 22 '24 08:10 mrdoob

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

CodyJasonBennett avatar Oct 22 '24 08:10 CodyJasonBennett

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.

CodyJasonBennett avatar Oct 22 '24 15:10 CodyJasonBennett

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.

prideout avatar Oct 28 '24 17:10 prideout

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.

Mugen87 avatar Oct 28 '24 17:10 Mugen87

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

WestLangley avatar Oct 28 '24 18:10 WestLangley

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?

prideout avatar Oct 30 '24 02:10 prideout

Also finding nothing rendering when reverseDepthBuffer true

See this minimal example

https://jsfiddle.net/pgkz5vto/

Works in r169 but not r170

Issue: #30808

Fixed by renderer.getContext().clearDepth(0)

haxiomic avatar Mar 26 '25 20:03 haxiomic

Ah... the implementation of WebGLState.setReversed is incorrect,

setReversed: function ( value ) {

	if ( reversed !== value ) {

		const ext = extensions.get( 'EXT_clip_control' );

		if ( reversed ) {

			ext.clipControlEXT( ext.LOWER_LEFT_EXT, ext.ZERO_TO_ONE_EXT );

		} else {

			ext.clipControlEXT( ext.LOWER_LEFT_EXT, ext.NEGATIVE_ONE_TO_ONE_EXT );

		}

		const oldDepth = currentDepthClear;
		currentDepthClear = null;
		this.setClear( oldDepth );

	}

	reversed = value;

}
  • if ( reversed ) assumes reversed has been set, it hasn't yet
  • this.setClear( oldDepth ); also assumes reversed has been set

haxiomic avatar Mar 26 '25 20:03 haxiomic