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

Use multi_draw_indirect_count where available, in preparation for two-phase occlusion culling. #17211

Merged
merged 12 commits into from
Jan 14, 2025

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Jan 7, 2025

This commit allows Bevy to use multi_draw_indirect_count for drawing meshes. The multi_draw_indirect_count feature works just like multi_draw_indirect, but it takes the number of indirect parameters from a GPU buffer rather than specifying it on the CPU.

Currently, the CPU constructs the list of indirect draw parameters with the instance count for each batch set to zero, uploads the resulting buffer to the GPU, and dispatches a compute shader that bumps the instance count for each mesh that survives culling. Unfortunately, this is inefficient when we support multi_draw_indirect_count. Draw commands corresponding to meshes for which all instances were culled will remain present in the list when calling
multi_draw_indirect_count, causing overhead. Proper use of multi_draw_indirect_count requires eliminating these empty draw commands.

To address this inefficiency, this PR makes Bevy fully construct the indirect draw commands on the GPU instead of on the CPU. Instead of writing instance counts to the draw command buffer, the mesh preprocessing shader now writes them to a separate indirect metadata buffer. A second compute dispatch known as the build indirect parameters shader runs after mesh preprocessing and converts the indirect draw metadata into actual indirect draw commands for the GPU. The build indirect parameters shader operates on a batch at a time, rather than an instance at a time, and as such each thread writes only 0 or 1 indirect draw parameters, simplifying the current logic in mesh_preprocessing, which currently has to have special cases for the first mesh in each batch. The build indirect parameters shader emits draw commands in a tightly packed manner, enabling maximally efficient use of multi_draw_indirect_count.

Along the way, this patch switches mesh preprocessing to dispatch one compute invocation per render phase per view, instead of dispatching one compute invocation per view. This is preparation for two-phase occlusion culling, in which we will have two mesh preprocessing stages. In that scenario, the first mesh preprocessing stage must only process opaque and alpha tested objects, so the work items must be separated into those that are opaque or alpha tested and those that aren't. Thus this PR splits out the work items into a separate buffer for each phase. As this patch rewrites so much of the mesh preprocessing infrastructure, it was simpler to just fold the change into this patch instead of deferring it to the forthcoming occlusion culling PR.

Finally, this patch changes mesh preprocessing so that it runs separately for indexed and non-indexed meshes. This is because draw commands for indexed and non-indexed meshes have different sizes and layouts. The existing code is actually broken for non-indexed meshes, as it attempts to overlay the indirect parameters for non-indexed meshes on top of those for indexed meshes. Consequently, right now the parameters will be read incorrectly when multiple non-indexed meshes are multi-drawn together. This is a bug fix and, as with the change to dispatch phases separately noted above, was easiest to include in this patch as opposed to separately.

Migration Guide

  • Systems that add custom phase items now need to populate the indirect drawing-related buffers. See the specialized_mesh_pipeline example for an example of how this is done.

@pcwalton pcwalton added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
@pcwalton pcwalton added this to the 0.16 milestone Jan 7, 2025
two-phase occlusion culling.

This commit allows Bevy to use `multi_draw_indirect_count` for
drawing meshes. The `multi_draw_indirect_count` feature works just like
`multi_draw_indirect`, but it takes the number of indirect parameters
from a GPU buffer rather than specifying it on the CPU.

Currently, the CPU constructs the list of indirect draw parameters with
the instance count for each batch set to zero, uploads the resulting
buffer to the GPU, and dispatches a compute shader that bumps the
instance count for each mesh that survives culling. Unfortunately, this
is inefficient when we support `multi_draw_indirect_count`. Draw
commands corresponding to meshes for which all instances were culled
will remain present in the list when calling
`multi_draw_indirect_count`, causing overhead. Proper use of
`multi_draw_indirect_count` requires eliminating these empty draw
commands.

To address this inefficiency, this PR makes Bevy fully construct the
indirect draw commands on the GPU instead of on the CPU. Instead of
writing instance counts to the draw command buffer, the mesh
preprocessing shader now writes them to a separate *indirect metadata
buffer*. A second compute dispatch known as the *build indirect
parameters* shader runs after mesh preprocessing and converts the
indirect draw metadata into actual indirect draw commands for the GPU.
The build indirect parameters shader operates on a batch at a time,
rather than an instance at a time, and as such each thread writes only 0
or 1 indirect draw parameters, simplifying the current logic in
`mesh_preprocessing`, which has to have special cases for the first mesh
in each batch. The build indirect parameters shader emits draw commands
in a tightly packed manner, enabling maximally efficient use of
`multi_draw_indirect_count`.

Along the way, this patch switches mesh preprocessing to dispatch one
compute invocation per render phase per view, instead of dispatching one
compute invocation per view. This is preparation for two-phase occlusion
culling, in which we will have two mesh preprocessing stages. In that
scenario, the first mesh preprocessing stage must only process opaque
and alpha tested objects, so the work items must be separated into those
that are opaque or alpha tested and those that aren't. Thus this PR
splits out the work items into a separate buffer for each phase. As this
patch rewrites so much of the mesh preprocessing infrastructure, it was
simpler to just fold the change into this patch instead of deferring it
to the forthcoming occlusion culling PR.

Finally, this patch changes mesh preprocessing so that it runs
separately for indexed and non-indexed meshes. This is because draw
commands for indexed and non-indexed meshes have different sizes and
layouts. *The existing code is actually broken for non-indexed meshes*,
as it attempts to overlay the indirect parameters for non-indexed meshes
on top of those for indexed meshes. Consequently, right now the
parameters will be read incorrectly when multiple non-indexed meshes are
multi-drawn together. *This is a bug fix* and, as with the change to
dispatch phases separately noted above, was easiest to include in this
patch as opposed to separately.
@pcwalton pcwalton force-pushed the multidraw-indirect-count branch from 22c95b3 to bbebd62 Compare January 7, 2025 06:25
@BenjaminBrienen BenjaminBrienen added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jan 7, 2025
@pcwalton pcwalton requested a review from JMS55 January 12, 2025 21:15
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

this is good stuff! nice!

crates/bevy_pbr/src/render/mesh_preprocess_types.wgsl Outdated Show resolved Hide resolved
crates/bevy_pbr/src/render/mesh_preprocess_types.wgsl Outdated Show resolved Hide resolved
mesh_index_slice.range.start,
mesh_index_slice.range.end - mesh_index_slice.range.start,
),
None => (false, !0, !0),
Copy link
Contributor

Choose a reason for hiding this comment

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

why !0 here but 0 above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched it to 0, 0. In an earlier version of this patch, I used an index buffer range starting with !0 to indicate that a mesh was non-indexed, but now there's no need to do that as indexed and non-indexed meshes are kept fully separated throughout the pipeline.

base_output_index,
batch_set_index: match batch_set_index {
Some(batch_set_index) => u32::from(batch_set_index),
None => !0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why !0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 79 of build_indirect_params.wgsl checks this value to see whether the batch belongs to a batch set.

mesh_index: input_index,
base_output_index,
batch_set_index: match batch_set_index {
None => !0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why !0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 79 of build_indirect_params.wgsl checks this value to see whether the batch belongs to a batch set.


// If this batch belongs to a batch set, then allocate space for the
// indirect commands in that batch set.
if (batch_set_index != 0xffffffffu) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is one of the !0s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's the batch_set_index value.

crates/bevy_pbr/src/lib.rs Outdated Show resolved Hide resolved
@IceSentry IceSentry added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 14, 2025
@pcwalton pcwalton added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 14, 2025
@pcwalton
Copy link
Contributor Author

@rparrett rparrett added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 14, 2025
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

* The retained view key from bevyengine#16942 was insufficient to uniquely
  identify a shadow cascade when multiple cameras were present. In such
  cases, the stable ID for a shadow cascade is actually (light entity,
  camera entity, cascade index), not (light entity, cascade index) as
  the PR in bevyengine#16942 assumed. This caused failures in the
  `camera_sub_view` example.

* Sorted phase items didn't push batch sets as they were supposed to. I
  updated `batch_and_prepare_sorted_render_phase` to do so. This fixes
  the examples with transparency.

* Unbatchable binned entities didn't push batch sets as they were
  supposed to. This fixes the `morph_targets` example.

* As the `GpuPreprocessNode` now runs per camera (a necessary change for
  occlusion culling), it should only run on views associated with the
  current camera (the camera itself plus the shadow maps). It was
  running again for every view, causing failures in tests with multiple
  views like `split_screen`.

* 3D meshes need to be re-extracted if their assets change, so that the
  first vertex and first index in `MeshInputUniform` are updated. I
  added a system to do so. Note that this system is somewhat inefficient
  when meshes change; once `cold-specialization` lands it can be updated
  to use the asset change infrastructure in that patch to fix the issue.
  This fixes the `query_gltf_primitives` example.

* `specialized_mesh_pipeline` wasn't allocating indirect work items. I
  changed the example to do so.
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 14, 2025
@pcwalton
Copy link
Contributor Author

pcwalton commented Jan 14, 2025

I believe all the regressions are fixed. The ones worth calling out are:

  • wireframe_2d -- I believe that my PR actually fixes the rendering from main.
  • texture_atlas -- Locally I can't reproduce the bots' rendering on main. My "before" and "after" are identical to the bot's "after". So I think it's some kind of pre-existing race.
  • 2d_gizmos -- Seems to be a bug fix from main, possibly related to the indexed/non-indexed fix.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 14, 2025
Merged via the queue into bevyengine:main with commit 35101f3 Jan 14, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants