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

Fix web depth/projection regression, causing incorrect rendering on all 3D scenes #2170

Merged
merged 4 commits into from
May 22, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented May 17, 2023

Reverts

#2123 didn't make sense: The assumption was that depth range is -1 to 1 on the Web, but if that was true all our clipping would have been wrong. In actuality, wgpu emulates 0 to 1 range for us but this causes precision issues.

Fixes #2166

This also improves the z offset on all platforms

Checklist

PR Build Summary: https://build.rerun.io/pr/2170

@Wumpf Wumpf added 🔺 re_renderer affects re_renderer itself 🦟 regression A thing that used to work in an earlier release labels May 17, 2023
@Wumpf Wumpf changed the title Fix web depth regression Fix web depth/projection regression, causing incorrect rendering on all 3D scenes May 17, 2023
@Wumpf Wumpf added the 🕸️ web regarding running the viewer in a browser label May 17, 2023
@Wumpf Wumpf requested a review from emilk May 17, 2023 17:28
@Wumpf Wumpf force-pushed the andreas/web-depth-regression branch from fd6e25c to c08c123 Compare May 17, 2023 17:28
@Wumpf Wumpf removed the request for review from emilk May 17, 2023 17:38
Comment on lines 1 to 3
// True if we're running on Gles/WebGL.
// (patched accordingly if necessary)
const GLES = false;
Copy link
Member

Choose a reason for hiding this comment

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

I think something like this would be better:

Suggested change
// True if we're running on Gles/WebGL.
// (patched accordingly if necessary)
const GLES = false;
// True if we're running on Gles/WebGL.
// Will be set by `re_renderer` during startup
const GLES = ${GLES};

That way we cannot accidentally break the patch-code without us noticing it.

…or just pass it in the global uniform buffer

Comment on lines 23 to 24
// On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped.
// wgpu/Naga counteracts this by patching all vertex shader with:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 and y is flipped.
// wgpu/Naga counteracts this by patching all vertex shader with:
// On GLES/WebGL, the NDC clipspace range for depth is from -1 to 1 (and y is flipped).
// wgpu/Naga counteracts this by patching all vertex shaders with:

// This is great, since it means that we can pretend depth is 0 to 1 as specified by WebGPU.
// But it completely messes up depth precision, in particular since we use
// an inverse depth projection that tries to make use of the high float precision closer to zero.
eps *= 1000.0;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like an ideal fix. It seems like flipping this around (like the previous PR did) is better for depth precision, just that we also need to flip the Z test?

@emilk emilk force-pushed the andreas/web-depth-regression branch from 79983a0 to 7fdca02 Compare May 22, 2023 17:39
@Wumpf Wumpf merged commit 7856111 into main May 22, 2023
@Wumpf Wumpf deleted the andreas/web-depth-regression branch May 22, 2023 18:51
emilk added a commit that referenced this pull request May 25, 2023
…ll 3D scenes (#2170)

* Revert "Fix depth precision issues on WebGL due to different NDC space (#2123)"

This reverts commit 4f60fd7.

* fudge depth offsets on Webgl

* hardware_tier global constant, better depth handling for gles

* Improve the depth offset for all platforms

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔺 re_renderer affects re_renderer itself 🦟 regression A thing that used to work in an earlier release 🕸️ web regarding running the viewer in a browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Depth is broken on the web due to inverted projection matrix
2 participants