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

DepthTexture: change default type to UnsignedIntType #24019

Merged
merged 1 commit into from
May 20, 2022

Conversation

wmcmurray
Copy link
Contributor

Description

I noticed depth precision issues (horizontal banding + z-fighting) when rendering my project with post-processing (using WebGLRenderTargets) starting at very short distances (10 - 15 meters, where 1 unit = 1 meter), and those issues did not happen when rendering the same scene directly to canvas !

So I investigated, and it turns out setting a DepthTexture with the UnsignedIntType type into renderTarget.depthTexture fixed my issues. Precision issues only appear at much greater distances now (as expected) and exactly where they start appearing when rendering directly to canvas.

About this PR

So I changed the default to UnsignedIntType because I believe defaults should give best results out of the box, but I don't know what I'm doing, I didn't test anything beyond my project, and I don't know the implications of this change ⚠️
(also updated the docs + bits of code that seemed relevant)

This example: https://threejs.org/examples/webgl_depth_texture.html helped me to visualize different types, just zoom out until the meshes are barely visible and you should notice banding with UnsignedShortType.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 8, 2022

I think we have decided to use UnsignedShortType in the past since it is the most performant option and the related depth precision might be sufficient for certain scenarios.

That said I've noticed on more than one occasion that users run into the same issue like you but can't find a solution by themselves. Using UnsignedIntType as the default should mitigate this situtation.

However, if we want to head for quality, using DepthStencilFormat and UnsignedInt248Type is even the better default. Only this setup guarantees at least 24 bit depth precision which is more than UnsignedIntType. And unlike floating point textures it should be supported on all platforms.

@mrdoob
Copy link
Owner

mrdoob commented May 9, 2022

@wmcmurray Could you give DepthStencilFormat and UnsignedInt248Type a try?

@wmcmurray
Copy link
Contributor Author

@mrdoob Both DepthFormat + UnsignedIntType and DepthStencilFormat + UnsignedInt248Type resulted in seemingly identical results. I haven't noticed any performance drop either, when I enable post-processing I get an increase of 0.5 - 1 ms per frame in both cases (which is exactly what I get using UnsignedShortType also).

@Mugen87
Copy link
Collaborator

Mugen87 commented May 9, 2022

The difference in depth precision varies from device to device. So yes, it is possible that you don't see a difference between UnsignedIntType and UnsignedInt248Type. However, guaranteed precision with UnsignedInt248Type is definitely higher.

@mrdoob
Copy link
Owner

mrdoob commented May 10, 2022

Lets use UnsignedInt248Type by default then?

@mrdoob mrdoob added this to the r141 milestone May 10, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented May 12, 2022

Reconsidering this issue: WebGLRenderTarget.stencilBuffer is false by default which is also an appropriate default. However, I'm not entirely sure if it's safe to use DepthStencilFormat with a render target that has no stencil attachment. I can' tell from the WEBGL_depth_texture spec if this combination is supported on all platforms.

Hence, I think the PR is fine as it is.

@mrdoob mrdoob merged commit 9a56e26 into mrdoob:dev May 20, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 20, 2022

Thanks!

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.

None yet

3 participants