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

Add support for occlusion queries #3402

Merged
merged 26 commits into from
Aug 2, 2023
Merged

Conversation

valaphee
Copy link
Contributor

@valaphee valaphee commented Jan 19, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

Description
At the moment its possible to create an occlusion query set, but it can't be used as there is no way to start and end a occlusion
query.

This pr implements the occlusion query set and adds the occlusion_query_set to the render pass descriptor which will be used by subsequent beginOcclusionQuery and endOcclusionQuery commands.

For finding purposes, here is a good overview of the differences for queries for different backends gpuweb/gpuweb#614

Testing
I modified the mipmap example, and replaced pipeline statistics with occlusion query, as the used value (FRAGMENT_SHADER_INVOCATIONS) should be the same as the value returned by occlusion query in this case (might differ on DirectX as binary mode is used, either 0 if occluded or 1, but can be changed)

@valaphee
Copy link
Contributor Author

I also want to add, as this is my first contribution, and a rather large, to check if I did the Features Flag implementation correctly,

as for my understanding OCCLUSION_QUERY, is not a feature flag in WebGPU, and is always supported there, but atm. there is no support for this in DirectX11 FL 9.1, and Metal (Queries are generally not implemented yet)

@valaphee valaphee force-pushed the occlusion-query branch 3 times, most recently from 0382a52 to f8ab890 Compare January 19, 2023 01:51
@teoxoy
Copy link
Member

teoxoy commented Jan 19, 2023

I also want to add, as this is my first contribution, and a rather large, to check if I did the Features Flag implementation correctly, as for my understanding OCCLUSION_QUERY, is not a feature flag in WebGPU, and is always supported there, but atm. there is no support for this in DirectX11 FL 9.1, and Metal (Queries are generally not implemented yet)

It should be a new flag in the DownlevelFlags (not a flag in Features).

(Downlevel flags are features that WebGPU must support but not all backends/adapters do.)

If we want to also add query support for Metal.

Ideally, we should add core features to all primary backends (Vulkan, DX12 and Metal); not a blocker though.

@valaphee valaphee force-pushed the occlusion-query branch 2 times, most recently from 5661701 to 25f0d10 Compare February 8, 2023 18:43
@valaphee
Copy link
Contributor Author

valaphee commented Feb 8, 2023

Changed OCCLUSION_QUERY to a down-level feature, and made everything spec conform again,
but occlusion queries will be a breaking change, as it requires to add occlusion query set to the render pass descriptor

Still has to be implement in web backend; auxil, gles, and metal

deno_webgpu/src/01_webgpu.js Outdated Show resolved Hide resolved
deno_webgpu/src/01_webgpu.js Outdated Show resolved Hide resolved
deno_webgpu/src/01_webgpu.js Outdated Show resolved Hide resolved
@valaphee valaphee changed the title Add proper support for occlusion queries Add support for occlusion queries Feb 8, 2023
@valaphee valaphee force-pushed the occlusion-query branch 2 times, most recently from 5586b1b to 7f71a5c Compare February 9, 2023 22:48
@cwfitzgerald
Copy link
Member

This is really good stuff! The code looks good - though CI and merging is mad.

Would it be possible to write a simple test (a new file under wgpu/tests/root.rs) that draws two fullscreen triangles with a depth buffer and makes sure the occlusion queries return the correct values on both?

@valaphee
Copy link
Contributor Author

valaphee commented Feb 23, 2023

Added a simple test for occlusion queries, and added occlusion queries to deno (never worked with deno before)

Theoretically there is still a check missing if the provided query set is an occlusion query set, but the check is also done when starting an query

There are also no details of how the result should look like, Vulkan/Metal specifies it as non-zero and zero, in DirectX 12 there is also the possibility to choose between binary, and normal (frag. shader invocations),
Therefore I would also go with zero, non-zero (not precise, but also not binary).

@crowlKats crowlKats self-requested a review February 23, 2023 23:26
@cwfitzgerald
Copy link
Member

@crowlKats poke from triage

Copy link
Collaborator

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Deno part LGTM. not sure what i was thinking when i said something didnt look right 😓

…port it and ancient OpenGL (ES) versions, remove fragment stage in occlusion query test
# Conflicts:
#	deno_webgpu/src/01_webgpu.js
#	deno_webgpu/src/02_idl_types.js
#	wgpu-core/src/command/bundle.rs
#	wgpu-hal/src/dx12/device.rs
@valaphee
Copy link
Contributor Author

valaphee commented Apr 25, 2023

Updated the PR, but I noticed that the #3489 PR broke the mipmap example, it runs because query_set is None for some reason when running the example test, but running the example main results in an error:

Caused by:
    In Device::create_buffer
    `MAP` usage can only be combined with the opposite `COPY`, requested BufferUsages(MAP_READ | QUERY_RESOLVE)

The examples use a staging buffer now (Don't know if this is mandatory, but seems to be intended according to WebGPU spec)

And WebGL doesn't work because queries are not immediately available.

@cwfitzgerald
Copy link
Member

@valaphee Sorry this got caught up in the backlog. Could you merge in main and we'll get this reviewed and in

# Conflicts:
#	deno_webgpu/lib.rs
#	examples/mipmap/src/main.rs
#	tests/tests/shader_primitive_index/mod.rs
@valaphee
Copy link
Contributor Author

Merged with upstream

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Absolutely phenomenal work!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) August 2, 2023 18:54
@cwfitzgerald cwfitzgerald merged commit 494ae1a into gfx-rs:trunk Aug 2, 2023
19 of 20 checks passed
@cwfitzgerald cwfitzgerald mentioned this pull request Aug 2, 2023
3 tasks
@valaphee valaphee deleted the occlusion-query branch August 3, 2023 15:47
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.

4 participants