Variable `MeshPipeline` View Bind Group Layout by coreh · Pull Request #10156 · bevyengine/bevy

coreh · 2023-10-17T02:54:01Z

Objective

This PR aims to make it so that we don't accidentally go over MAX_TEXTURE_IMAGE_UNITS (in WebGL) or maxSampledTexturesPerShaderStage (in WebGPU), giving us some extra leeway to add more view bind group textures.

(This PR is extracted from—and unblocks—#8015)

Solution

We replace the existing view_layout and view_layout_multisampled pair with an array of 32 bind group layouts, generated ahead of time;
For now, these layouts cover all the possible combinations of: multisampled, depth_prepass, normal_prepass, motion_vector_prepass and deferred_prepass:
- In the future, as @JMS55 pointed out, we can likely take out motion_vector_prepass and deferred_prepass, as these are not really needed for the mesh pipeline and can use separate pipelines. This would bring the possible combinations down to 8;
- We can also add more "optional" textures as they become needed, allowing the engine to scale to a wider variety of use cases in lower end/web environments (e.g. some apps might just want normal and depth prepasses, others might only want light probes), while still keeping a high ceiling for high end native environments where more textures are supported.
- While preallocating bind group layouts is relatively cheap, the number of combinations grows exponentially, so we should likely limit ourselves to something like at most 256–1024 total layouts until we find a better solution (like generating them lazily)
To make this mechanism a little bit more explicit/discoverable, so that compatibility with WebGPU/WebGL is not broken by accident, we add a MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES const and warn whenever the number of textures in the layout crosses it.
- The warning is gated by #[cfg(debug_assertions)] and not issued in release builds;
- We're counting the actual textures in the bind group layout instead of using some roundabout metric so it should be accurate;
- Right now MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES is set to 10 in order to leave 6 textures free for other groups;
- Currently there's no combination that would cause us to go over the limit, but that will change once StandardMaterial Light Transmission #8015 lands.

Changelog

MeshPipeline view bind group layouts now vary based on the current multisampling and prepass states, saving a couple of texture binding entries when prepasses are not in use.

Migration Guide

MeshPipeline::view_layout and MeshPipeline::view_layout_multisampled have been replaced with a private array to accomodate for variable view bind group layouts. To obtain a view bind group layout for the current pipeline state, use the new MeshPipeline::get_view_layout() or MeshPipeline::get_view_layout_from_key() methods.

superdump

Initial quick pass.

crates/bevy_pbr/src/render/mesh.rs

nicopap · 2023-10-17T07:55:09Z

Hey FYI, this looks very similar to what I did in mesh_bindings.rs in #9902. Have you considered taking a look at it for inspiration? Here is the link to the file without the diff cruft https://github.com/bevyengine/bevy/blob/9269e98052a88e3c48b8d3fe7f3dde41ad6e51ad/crates/bevy_pbr/src/render/mesh_bindings.rs

crates/bevy_pbr/src/render/mesh.rs

superdump · 2023-10-17T12:21:34Z

Hey FYI, this looks very similar to what I did in mesh_bindings.rs in #9902. Have you considered taking a look at it for inspiration? Here is the link to the file without the diff cruft https://github.com/bevyengine/bevy/blob/9269e98052a88e3c48b8d3fe7f3dde41ad6e51ad/crates/bevy_pbr/src/render/mesh_bindings.rs

The code in that felt a bit off there is all the bools for combinations. Generating combinations from bit flags can be done procedurally. Bit flags take a lot less space, and are arguably clearer when using named constants like the bitflag crate offers.

JMS55 · 2023-10-17T19:56:35Z

crates/bevy_pbr/src/prepass/mod.rs

+    motion_vector_prepass: bool,
+    deferred_prepass: bool,
+) -> Vec<BindGroupLayoutEntry> {
+    let mut result = vec![];


Not a big deal considering this is run only once, but we should use an ArrayVec here.

Looks like we don't depend on that crate, probably not worth it to add it just for this?

I'm pretty sure we have a small array crate somewhere in tree, maybe not in bevy_pbr though but if it's already in tree in another crate it's probably fine to add it.

Although, if it's only for this place it's probably not necessary

I use SmallVec in #9902, should be fine to use it here. It's pretty much identical to ArrayVec when we don't go over the set size.

JMS55 · 2023-10-17T19:59:00Z

crates/bevy_pbr/src/render/mesh.rs

+///
+/// See: <https://gpuweb.github.io/gpuweb/#limits>
+#[cfg(debug_assertions)]
+pub const MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES: usize = 10;


I won't block this PR on it, but I don't love this mechanism. Ideally we have CI tests (or a manual cargo test) that test our rendering setup end-to-end for lack of errors against the lowest spec WebGL2/GLES3/WebGPU/Vulkan/Metal/DirectX12 limits and extensions, as this is a rather common occurrence with many other rendering things.

Hmm I wonder what is the ideal fallback behavior for in this type of situation. Should we disable the prepasses? Have them have no effect? Any code that assumes they will work and reads from them will fail, so there might not be a relatively straightforward way to gracefully degrade the end result...

Maybe we could make this warning a crash by default, and have a flag to enable the "extended behaviors" that require a non-baseline spec?

I think this is a larger topic for discussion. I've wondered whether/how we will handle resource limitations long-term when we actually get to exhausting them, or having to pick and choose features to get the most out of the available resources. Previously I was thinking that at least initially, naga/wgpu will complain loudly when limits are exceeded. But now it has become a reality, and I realise the possibly obvious point that users (as in those using bevy applications/games) may want to pick and choose features at runtime, then we will probably have to have some management/allocation of resources and have features requesting those allocations so that they can fail gracefully rather than just panic somewhere. I think that is too big a topic and needs to be a separate discussion from this PR.

This PR makes the situation less problematic for those not using all the features. :)

i agree this is a wider problem. i definitely don't think this should be a crash by default, not everybody is targetting limited platforms. i don't even think we should warn by default, not everybody is targetting constrained platforms and even if they are, sometimes it won't actually be a problem.

while naga's error may be cryptic it is at least only triggered in actual error conditions. i think i'd prefer not trying to proactively warn in cases where it might be a problem. in future we can add a texture count to bevy's BindGroup and then keep actual track of textures in the TrackedRenderPass, then issue the warning there if the current limit is exceeded.

also - can 10 ever be hit? as far as i can see the view layout can only be a max of 8 textures?

crates/bevy_pbr/src/render/mesh_view_bindings.wgsl

coreh · 2023-10-18T00:20:22Z

Hey FYI, this looks very similar to what I did in mesh_bindings.rs in #9902. Have you considered taking a look at it for inspiration? Here is the link to the file without the diff cruft https://github.com/bevyengine/bevy/blob/9269e98052a88e3c48b8d3fe7f3dde41ad6e51ad/crates/bevy_pbr/src/render/mesh_bindings.rs

@nicopap Hadn't seen that PR yet, this is indeed very similar. The amount of combinations here (32) makes this a little bit more daunting to fully enumerate like that (6) which is why I added the code to generate it in an array. Other than that they're mostly the same in principle.

pcwalton

This looks fine to me, but I'm not comfortable reviewing code yet.

nicopap

I'm really not happy with the code quality. IMO, just moving the code in different files would be enough for me. Merge conflicts are super annoying, and this would increase chances of merge conflicts.

I think long term this should be refactored, there is already a lot of repeat code, and this introduces even more. But that's not in scope for this PR.

Otherwise LGTM

nicopap · 2023-10-18T11:25:36Z

crates/bevy_pbr/src/prepass/mod.rs

@@ -636,127 +635,151 @@ where

 pub fn get_bind_group_layout_entries(


Could the bind group creation code be moved to its own file? I think we should avoid large files like render/mesh.rs. And it would neatly reflect mesh_bindings.rs.

Extracted it. Also moved the mesh.rs view binding creation code to the newly created mesh_view_bindings.rs.

nicopap · 2023-10-18T11:27:32Z

crates/bevy_pbr/src/render/mesh.rs

    commands.insert_or_spawn_batch(entities);
 }

+bitflags::bitflags! {


I think we should move this to its own file. Those large files are really difficult to navigate and have a tendency to result in more merge conflicts.

I created a new mesh_view_bindings.rs and moved all the new stuff there, as well as the layout_entries() and a newly extracted generate_view_layouts() function.

nicopap · 2023-10-18T11:34:58Z

crates/bevy_pbr/src/render/mesh.rs

+        format!(
+            "mesh_view_layout{}{}{}{}{}",
+            if self.contains(MeshPipelineViewLayoutKey::MULTISAMPLED) {
+                "_multisampled"
+            } else {
+                ""
+            },
+            if self.contains(MeshPipelineViewLayoutKey::DEPTH_PREPASS) {
+                "_depth"
+            } else {
+                ""
+            },
+            if self.contains(MeshPipelineViewLayoutKey::NORMAL_PREPASS) {
+                "_normal"
+            } else {
+                ""
+            },
+            if self.contains(MeshPipelineViewLayoutKey::MOTION_VECTOR_PREPASS) {
+                "_motion"
+            } else {
+                ""
+            },
+            if self.contains(MeshPipelineViewLayoutKey::DEFERRED_PREPASS) {
+                "_deferred"
+            } else {
+                ""
+            },
+        )
+    }


Would people accept if we used this shortcut?

use MeshPipelineViewLayoutKey as Key; self.contains(Key::DEFERRED_PREPASS).then_some("_deferred").unwrap_or_default()

Then this whole function would look as follow:

use MeshPipelineViewLayoutKey as Key; format!( "mesh_view_layout{}{}{}{}{}", self.contains(Key::MULTISAMPLED).then_some("_multisampled").unwrap_or_default(), self.contains(Key::DEPTH_PREPASS).then_some("_depth").unwrap_or_default(), self.contains(Key::NORMAL_PREPASS).then_some("_normal").unwrap_or_default(), self.contains(Key::MOTION_VECTOR_PREPASS).then_some("_motion").unwrap_or_default(), self.contains(Key::DEFERRED_PREPASS).then_some("_deferred").unwrap_or_default(), )

Honestly I feel like long-term a macro would be good here because having to generate these strings at runtime introduces overhead. But a lot of stuff is waiting on this PR so I won't block it.

These should only be generated once at startup, so realistically the impact will be minimal. For 32 strings of this length, it should also be < 1kb of extra memory consumption, so not a big deal IMO either.

nicopap · 2023-10-19T10:21:04Z

crates/bevy_pbr/src/render/mesh.rs

+        let index = layout_key.bits() as usize;
+        let layout = &self.view_layouts[index];
+
+        #[cfg(debug_assertions)]


iirc @mockersf have been raising concerns wrt using cfg(debug_assertions).

Hmm, I couldn't find it on Slack, is there a writeup on that? Is it because of the conflation between "debug builds", "builds with debug symbols" and "builds without assertions?" Do we have a feature flag or some other static way to disable this check if needed?

I can't find the conversation, I might have imagined it.

crates/bevy_pbr/src/render/mesh.rs

nicopap

excellent

pcwalton · 2023-10-21T02:19:57Z

LGTM

robtfm

as a temporary solution this is fine. we should definitely create layouts on demand in future to avoid copying around the whole array (lots of places use clones of the mesh pipeline).

i don't think we should warn on high texture usage, but i don't think it will ever warn currently(?) so it's all good.

robtfm · 2023-10-21T08:54:07Z

crates/bevy_pbr/src/render/mesh.rs

+///
+/// See: <https://gpuweb.github.io/gpuweb/#limits>
+#[cfg(debug_assertions)]
+pub const MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES: usize = 10;


i agree this is a wider problem. i definitely don't think this should be a crash by default, not everybody is targetting limited platforms. i don't even think we should warn by default, not everybody is targetting constrained platforms and even if they are, sometimes it won't actually be a problem.

while naga's error may be cryptic it is at least only triggered in actual error conditions. i think i'd prefer not trying to proactively warn in cases where it might be a problem. in future we can add a texture count to bevy's BindGroup and then keep actual track of textures in the TrackedRenderPass, then issue the warning there if the current limit is exceeded.

also - can 10 ever be hit? as far as i can see the view layout can only be a max of 8 textures?

superdump · 2023-10-21T11:12:38Z

crates/bevy_pbr/src/render/mesh.rs

+                .store(true, Ordering::SeqCst);
+
+            // Issue our own warning here because Naga's error message is a bit cryptic in this situation
+            warn!("Too many textures in mesh pipeline view layout, this might cause us to hit `wgpu::Limits::max_sampled_textures_per_shader_stage` in some environments.");


@JMS55

# Objective This PR aims to make it so that we don't accidentally go over `MAX_TEXTURE_IMAGE_UNITS` (in WebGL) or `maxSampledTexturesPerShaderStage` (in WebGPU), giving us some extra leeway to add more view bind group textures. (This PR is extracted from—and unblocks—bevyengine#8015) ## Solution - We replace the existing `view_layout` and `view_layout_multisampled` pair with an array of 32 bind group layouts, generated ahead of time; - For now, these layouts cover all the possible combinations of: `multisampled`, `depth_prepass`, `normal_prepass`, `motion_vector_prepass` and `deferred_prepass`: - In the future, as @JMS55 pointed out, we can likely take out `motion_vector_prepass` and `deferred_prepass`, as these are not really needed for the mesh pipeline and can use separate pipelines. This would bring the possible combinations down to 8; - We can also add more "optional" textures as they become needed, allowing the engine to scale to a wider variety of use cases in lower end/web environments (e.g. some apps might just want normal and depth prepasses, others might only want light probes), while still keeping a high ceiling for high end native environments where more textures are supported. - While preallocating bind group layouts is relatively cheap, the number of combinations grows exponentially, so we should likely limit ourselves to something like at most 256–1024 total layouts until we find a better solution (like generating them lazily) - To make this mechanism a little bit more explicit/discoverable, so that compatibility with WebGPU/WebGL is not broken by accident, we add a `MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES` const and warn whenever the number of textures in the layout crosses it. - The warning is gated by `#[cfg(debug_assertions)]` and not issued in release builds; - We're counting the actual textures in the bind group layout instead of using some roundabout metric so it should be accurate; - Right now `MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES` is set to 10 in order to leave 6 textures free for other groups; - Currently there's no combination that would cause us to go over the limit, but that will change once bevyengine#8015 lands. --- ## Changelog - `MeshPipeline` view bind group layouts now vary based on the current multisampling and prepass states, saving a couple of texture binding entries when prepasses are not in use. ## Migration Guide - `MeshPipeline::view_layout` and `MeshPipeline::view_layout_multisampled` have been replaced with a private array to accomodate for variable view bind group layouts. To obtain a view bind group layout for the current pipeline state, use the new `MeshPipeline::get_view_layout()` or `MeshPipeline::get_view_layout_from_key()` methods.

@JMS55

# Objective This PR aims to make it so that we don't accidentally go over `MAX_TEXTURE_IMAGE_UNITS` (in WebGL) or `maxSampledTexturesPerShaderStage` (in WebGPU), giving us some extra leeway to add more view bind group textures. (This PR is extracted from—and unblocks—bevyengine#8015) ## Solution - We replace the existing `view_layout` and `view_layout_multisampled` pair with an array of 32 bind group layouts, generated ahead of time; - For now, these layouts cover all the possible combinations of: `multisampled`, `depth_prepass`, `normal_prepass`, `motion_vector_prepass` and `deferred_prepass`: - In the future, as @JMS55 pointed out, we can likely take out `motion_vector_prepass` and `deferred_prepass`, as these are not really needed for the mesh pipeline and can use separate pipelines. This would bring the possible combinations down to 8; - We can also add more "optional" textures as they become needed, allowing the engine to scale to a wider variety of use cases in lower end/web environments (e.g. some apps might just want normal and depth prepasses, others might only want light probes), while still keeping a high ceiling for high end native environments where more textures are supported. - While preallocating bind group layouts is relatively cheap, the number of combinations grows exponentially, so we should likely limit ourselves to something like at most 256–1024 total layouts until we find a better solution (like generating them lazily) - To make this mechanism a little bit more explicit/discoverable, so that compatibility with WebGPU/WebGL is not broken by accident, we add a `MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES` const and warn whenever the number of textures in the layout crosses it. - The warning is gated by `#[cfg(debug_assertions)]` and not issued in release builds; - We're counting the actual textures in the bind group layout instead of using some roundabout metric so it should be accurate; - Right now `MESH_PIPELINE_VIEW_LAYOUT_SAFE_MAX_TEXTURES` is set to 10 in order to leave 6 textures free for other groups; - Currently there's no combination that would cause us to go over the limit, but that will change once bevyengine#8015 lands. --- ## Changelog - `MeshPipeline` view bind group layouts now vary based on the current multisampling and prepass states, saving a couple of texture binding entries when prepasses are not in use. ## Migration Guide - `MeshPipeline::view_layout` and `MeshPipeline::view_layout_multisampled` have been replaced with a private array to accomodate for variable view bind group layouts. To obtain a view bind group layout for the current pipeline state, use the new `MeshPipeline::get_view_layout()` or `MeshPipeline::get_view_layout_from_key()` methods.

coreh added 3 commits October 16, 2023 23:32

Create and choose bind group layouts based on enabled prepasses

2c44195

Count textures and issue a warning when we go over a safe limit

f46e7f6

Fix clippy errors

19ca341

coreh changed the title ~~Variable Mesh Pipeline View Bind Group Layout~~ Variable MeshPipeline View Bind Group Layout Oct 17, 2023

superdump requested changes Oct 17, 2023

View reviewed changes

crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved

crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved

pcwalton reviewed Oct 17, 2023

View reviewed changes

crates/bevy_pbr/src/render/mesh.rs Outdated Show resolved Hide resolved

alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Oct 17, 2023

JMS55 suggested changes Oct 17, 2023

View reviewed changes

coreh added 2 commits October 17, 2023 23:45

Use bitflags!() for MeshPipelineViewLayoutKey

8db3082

Merge branch 'main' into variable-bind-group-layout

6949f8a

coreh requested review from IceSentry, JMS55, pcwalton and superdump October 18, 2023 04:34

pcwalton reviewed Oct 19, 2023

View reviewed changes

Use SmallVec to avoid needless heap allocation

392c3ca

nicopap reviewed Oct 19, 2023

View reviewed changes

coreh added 9 commits October 19, 2023 22:25

Use MeshPipelineViewLayoutKey::COUNT instead of 32

06dd7b0

Use array::from_fn()

d884622

Extract mesh_pipeline_layout file

6909e70

Use shorter label generation code suggested in PR

d8741c5

Move more logic to mesh_pipeline_layout.rs

ba17d90

Rename mesh_pipeline_layout.rs to mesh_view_layout.rs

0f43c7c

Rename mesh_View_layout.rs to mesh_view_bindings.rs

b6923e4

Extract prepass_bindings.rs

f20672a

Move binding creation logic to mesh_view_bindings.rs

0b76720

nicopap approved these changes Oct 20, 2023

View reviewed changes

superdump added this to the 0.12 milestone Oct 20, 2023

pcwalton approved these changes Oct 21, 2023

View reviewed changes

nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 21, 2023

robtfm approved these changes Oct 21, 2023

View reviewed changes

superdump approved these changes Oct 21, 2023

View reviewed changes

superdump added this pull request to the merge queue Oct 21, 2023

Merged via the queue into bevyengine:main with commit 9b80205 Oct 21, 2023

nicopap mentioned this pull request Oct 23, 2023

Add motion vectors support for animated meshes #9902

Closed

Bcompartment mentioned this pull request Mar 27, 2024

Update to bevy 0.13+ pinkponk/bevy_efficient_forest_rendering#6

Open

		@@ -636,127 +635,151 @@ where

		pub fn get_bind_group_layout_entries(

Uh oh!

Conversation

coreh commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Migration Guide

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicopap commented Oct 17, 2023

Uh oh!

Uh oh!

superdump commented Oct 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coreh commented Oct 18, 2023

Uh oh!

pcwalton left a comment

Choose a reason for hiding this comment

Uh oh!

nicopap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreh Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicopap Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicopap left a comment

Choose a reason for hiding this comment

Uh oh!

pcwalton commented Oct 21, 2023

Uh oh!

robtfm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreh commented Oct 17, 2023 •

edited

Loading

coreh Oct 20, 2023 •

edited

Loading

nicopap Oct 20, 2023 •

edited

Loading