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

WebGPURenderer: Fix filterable depth textures #30023

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Dec 3, 2024

Related issue: #28784 (comment)

Description

The problem seems a bit more complicated than it seems. We needed a Texture.renderTarget property to bind RenderTarget to get RenderTarget.samples and thus handle it in WebGPUBackend.

I tried to make Texture.isRenderTargetTexture become a get if .renderTarget !== null but this seems to break the tests in WebGLRenderer so I kept the property.

Now webgpu_backdrop_water work with antialias.

@sunag sunag added this to the r172 milestone Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.11
78.98
339.11
78.98
+0 B
+0 B
WebGPU 484.6
134.48
484.8
134.59
+205 B
+110 B
WebGPU Nodes 484.06
134.38
484.27
134.49
+205 B
+110 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 464.62
111.98
465
112.04
+385 B
+58 B
WebGPU 553.55
149.82
554.14
150.01
+590 B
+188 B
WebGPU Nodes 509.43
139.53
510.02
139.72
+590 B
+191 B

@Spiri0
Copy link
Contributor

Spiri0 commented Dec 3, 2024

That sounds interesting, because since I think r165 I have to use the depthTexture with f32 in the shader, which I find very nice but the sampling has suffered a bit compared to the way with texture_depth_2d and its sampler. Sometimes I see very fine seems with the scalar depth value. It's nothing that really bothers me but with texture_depth_2d and sampler the quality was perfect.
Will this have an impact on this application?

sceneDepthPass = depthPass( scene, camera );
sceneDepthPassColor = sceneDepthPass.getTextureNode( 'depth' );

shaderParams = {
   viewPortDepthTexture: sceneDepthPassColor,
}

//in the wgsl shader header
viewPortDepthTexture: f32,

Sounds almost like texture_depth_2d is usable again with sampler.

@sunag
Copy link
Collaborator Author

sunag commented Dec 3, 2024

I remember you had commented, in theory it should solve, I just haven't tested it in wgslFn, it will add multisampled in the type like texture_depth_multisampled_2d.

@sunag sunag marked this pull request as ready for review December 3, 2024 15:02
@Spiri0
Copy link
Contributor

Spiri0 commented Dec 3, 2024

I'm curious and will add your extension locally to test it. I now need the depth texture for occlusion culling anyway. I'm confident. Everything else is going like clockwork too.

@sunag sunag merged commit ae2791a into mrdoob:dev Dec 3, 2024
12 checks passed
@sunag sunag deleted the dev-depth-2 branch December 3, 2024 21:25
@Spiri0
Copy link
Contributor

Spiri0 commented Dec 4, 2024

Ah, so the type will extended.
In the fragment shader it works like this again:

fn main_fragment(
   vUv: vec2<f32>,
   depthTexture: texture_depth_2d,
   depthSampler: sampler,
) -> vec4<f32> {

   var depth = textureSample( depthTexture, depthSampler, vUv );

   //...

But now I can also use texture_depth_2d in the compute shader. Just samplers don't work. Since samplers and textureSampleLevel can be used in compute shaders, I added them in 3 places in the WGSLNodeBuilder and it works.
Your PR has good timing now that I'm dealing with occlusion culling in compute shaders. I'll make a PR right away where I've added the samplers for textureSampleLevel for compute shaders, it's pretty small.

@Spiri0
Copy link
Contributor

Spiri0 commented Dec 11, 2024

I noticed a small conflict. The line:

( this.isSampleCompare( texture ) === false && texture.minFilter === NearestFilter && texture.magFilter === NearestFilter ) ||

in this function in the WGSLNodeBuilder.js corrupt #30033.
If I comment out the line, the sampler for depth textures works again. The line specifically excludes depth textures.

isSampleCompare( texture ) {
	return texture.isDepthTexture === true && texture.compareFunction !== null;
}
isUnfilterable( texture ) {
	return this.getComponentTypeFromTexture( texture ) !== 'float' ||
		( ! this.isAvailable( 'float32Filterable' ) && texture.isDataTexture === true && texture.type === FloatType ) ||
		( this.isSampleCompare( texture ) === false && texture.minFilter === NearestFilter && texture.magFilter === NearestFilter ) ||
		texture.isMultisampleRenderTargetTexture === true;
		this.renderer.backend.utils.getTextureSampleData( texture ).primarySamples > 1;

}

@sunag is the line essential?
Even with nearest filters for depth textures, samplers are important in compute shaders.
This looks like a harmless thing to me :)

@RenaudRohlinger
Copy link
Collaborator

Adding antialias: true caused the webgpu_backdrop_water to run at less than 50fps on my M1 Max, compared to a smooth 120fps previously. This is the first time I've experienced such low performance. Removing antialias fixes the issue. @sunag

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.

3 participants