WebGLRenderer: Move ReversedCoordinateSystem to camera.reversedDepth

[sunag]

Related issue: #31370

Description

// Use `camera.revesedDepth = true` instead of `camera.coordinateSystem = THREE.ReversedCoordinateSystem`
camera.reversedDepth = true;

camera.coordinateSystem and renderer.coordinateSystem are properties that are intended to guide other components of three.js like addons and TSL to work on different backends, which is preferable in order to reduce the number of possible checks.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

	Before	After	Diff
WebGL	338.37 78.95	338.27 78.93	-103 B -26 B
WebGPU	557.67 154.41	557.65 154.4	-25 B -8 B
WebGPU Nodes	556.59 154.19	556.57 154.18	-25 B -8 B

🌳 Bundle size after tree-shaking

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

	Before	After	Diff
WebGL	469.6 113.66	469.62 113.66	+11 B -3 B
WebGPU	633.34 171.44	633.39 171.46	+56 B +25 B
WebGPU Nodes	588.47 160.79	588.53 160.8	+56 B +11 B

[WestLangley]

Good. I support this change. The depth range or depth mapping is reversed, not the coordinate system.

My question is, why is it not sufficient to simply set reverseDepthBuffer: true in the renderer's constructor? Why is a per-camera property required?

[sunag]

My question is, why is it not sufficient to simply set reverseDepthBuffer: true in the renderer's constructor? Why is a per-camera property required?

The renderer could even set the property internally and update the projection Matrix if it isn't configured according to the renderer's needs, similar to what we did in the WebGPURenderer. We can leave this for other PR, when it arrives on WebGPURenderer I would also like to consider this detail. But the property will still be important for generating the projection matrices since in the flow of the call to the updateProjectionMatrix() function there is no external reference to the renderer.

[WestLangley]

@sunag We have a nomenclature inconsistency...

camera.reversedDepth vs. camera.reverseDepth

reversedDepthBuffer vs. reverseDepthBuffer

For consistency, I'd select camera.reversedDepth and reversedDepthBuffer.

[CodyJasonBennett]

camera.reversedDepth is also misleading because reversed depth has the WebGL backend use the [0, 1] clipping range. A reversed [-1, 1] depth buffer is not useful because of how precision is distributed in floating point, which is why this feature relies on EXT_clip_control. This was the motivation for adding yet another coordinate system enum, as it maps to the perspective/orthographic matrix methods. It is therefore not useful to allow a combination of WebGLCoordinateSystem and camera.reversedDepth.

Good. I #31370 (comment) this change. The depth range or depth mapping is reversed, not the coordinate system.

This is misinformed. Read the above; it does more than reverse the depth range. Further, the behavior is similar to WebGLCoordinateSystem and WebGPUCoordinateSystem when applied to a camera. It is equally effective to a renderer; otherwise, you display incorrect results. In other words, they must be synchronized all the same. Even if not aptly named, I think it is appropriate to make these exclusive to each other as an enum or ignore the effect when a [0, 1] clipping range is not used. You can bikeshed the details if you must, provided it not lead to similar confusion.

I would suggest reverting my PR or removing the WebGL implementation entirely if I keep finding this feature broken because people are confused or APIs are simply incompatible. It was my hope that EXT_clip_control could align backends and API, especially as this is a strict improvement, but I just see endless pedantry and conflict in the API itself.

[sunag]

I never thought that the term coordinate systems should only cover the clipping range as I referred to in the PR about paths #26140, its use is more comprehensive and involves the framebuffer coordinates, the threejs coordinate system enumeration is more related to gpuweb/gpuweb#416, this is seen in the code relationship with other components including TSL.

[CodyJasonBennett]

More simply, I really don't like the API options here or the invalid configurations that are now allowed. I think my fixes should be reverted (meaning #31370 and, of course, #31391), and we lean on TSL to hide the tension in the future. I have explained everything topically in excruciating detail, hoping that it would provide some reference. Please read it if this is to be continued.

[Mugen87]

I agree that coordinateSystem should encompass all differences of 3D backends, not just the z-range in NDC.

However, I also understand the confusion that the property reversedDepth does actually not just reverse the depth. It also modifies the z-range.

But isn't that something that we could clarify in the documentation? Meaning using a reverse depth buffer only makes sense in the [0,1] range because floating point has more precision near 0 than 1? In WebGPU, the z-range is already in the expected range so the conversion from [-1,1] to [0,1] is just required in WebGL to make the feature actually work.

We could also argue as follows: If EXT_clip_control would be always available in WebGL 2, we could use it to always force a [0,1] z-range for better compatibility between WebGL and WebGPU. However, since the extension is optional, we don't do that. Only if the user requests a reverse depth buffer.

Besides, it seems at least on the Chrome side having WebGPU compatibility mode seems more preferable than supporting both WebGPU and WebGL in the long term. When the WebGL fallback in WebGPURenderer is indeed a temporary thing, we can focus on a solution that is tailored for WebGPU. Even if that means some inconsistencies in the WebGL space.

[CodyJasonBennett]

Can you explain to me how you came to the conclusion that this is any different from WebGL or WebGPU coordinate systems from the camera or renderer? Is it literally just viewport Y? That is not strictly a coordinate system, but why don't we flip that also for completeness sake? Is that worth having a divergent and quite fragile API to entertain a useless, if not outright wrong, attempt at pedantics at the cost of sweeping breaking changes?

Had anyone read any of the literature myself and others have created on this topic? I just can't continue this alone anymore. We need people who actually use three.js and have empathy for users and the foresight to anticipate the tension it is putting on critical core API. This has been the epitome of actively harmful bike shedding, and I have a moral obligation to reduce that where I can.

I can't be the only one alarmed by the growing divergence three is creating here, to where I am motivated to remove this feature completely from three until 'optionally' is no longer there. I ask that this is stripped down to where it was before as a renderer parameter, and for anything more complex, rely on TSL to bridge the gap. Otherwise, remove it. I will not be supporting this feature with the resistance I've seen all around. It's not worth the time or effort.

[CodyJasonBennett]

For posterity, my changes were merged prematurely as a WIP. That should be reverted immediately and continued for correctness. I left reviewer notes and comments, and this was one of the outstanding issues. Variance depth being more problematic for the visualizer, perhaps fixed by proper clearing.

I don't think a single word of mine has been read, just seeing the regressions and complete lack of regard in all threads. I leave this for someone else to champion and will invest time elsewhere should I continue with three.

[mrdoob]

For posterity, my changes were merged prematurely as a WIP.

Uh oh! Was your PR marked as Draft PR? We use that as signal from the developer.

Edit: @CodyJasonBennett I've used Gemini to format (add paragraphs) to your posts. They're really hard to read for me otherwise.

[mrdoob]

I feel like this should just be a property of the renderer.

And... Instead of modifying makePerspective() and makeOrthographic(), can we have a internal camera in the renderer that has the inverted matrices and use that one instead when the renderer uses reversedDepthBuffer?

Can an already created projectionMatrix be converted? Or do we have to create it from scratch?

[mrdoob]

I guess #31410 goes in those lines already.

[mrdoob]

@CodyJasonBennett Is this perhaps what you were going for...?

THREE.ReversedWebGLCoordinateSystem and THREE.ReversedWebGPUCoordinateSystem

Edit: I just saw #31370 (comment) (which I hard to format too...)

[mrdoob]

@CodyJasonBennett Trying to read all the threads on this issue...

Any chance you can summarize here what you think should be changed in the current code in dev?