Fix meshlet shaders for bindless mode. #16825

pcwalton · 2024-12-15T00:36:08Z

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

crates/bevy_pbr/src/meshlet/visibility_buffer_resolve.wgsl

JMS55 · 2024-12-15T04:25:51Z

crates/bevy_pbr/src/render/parallax_mapping.wgsl


-fn sample_depth_map(uv: vec2<f32>, instance_index: u32) -> f32 {
-    let slot = mesh[instance_index].material_bind_group_slot;
+fn sample_depth_map(uv: vec2<f32>, material_bind_group_slot: u32) -> f32 {


Why did this change? Was it missed in an earlier PR?

Not quite sure what you mean. It changed because we don't use that mesh array in visbuffer resolution. If you're asking whether I didn't fix meshlets in an earlier PR, yes, I broke them (that's the whole point of this PR).

Ah nvm I see that it used to get the slot in the function itself, and all you did was move it outside the function so that the slot could be obtained differently for meshlet meshes.

JMS55 · 2024-12-15T04:26:21Z

crates/bevy_pbr/src/render/mesh_functions.wgsl

 }
 #import bevy_render::maths::{affine3_to_square, mat2x4_f32_to_mat3x3_unpack}

+#ifndef MESHLET_MESH_MATERIAL_PASS


Why were the #ifndef MESHLET_MESH_MATERIAL_PASS added here? Did they cause compilation issues?

The mesh array that's used in those functions isn't present during visbuffer resolution.

Right, but I don't think that causes any actual issues, unless you try and use them? It was never an issue in 0.15. Unless a recent PR changed that. Anyways I'm fine with the ifdef, I was just wondering why it was added.

It caused the shaders to fail to compile.

JMS55 · 2024-12-15T04:54:10Z

I can't test meshlets atm (don't have access to my desktop), but approving on the assumption that it was tested and this works. Code looks fine.

JMS55 · 2024-12-15T20:55:40Z

I'm also curious to know, is all 5 StandardMaterial bunnies shaded in one draw call with bindless? Because if there's one bind group and pipeline shared across all 5 bunnies, they should be able to be shaded in one pass. So there should be two draws total during the meshlet mesh material shading pass: one for the debug material, and one for standard material.

pcwalton · 2024-12-16T00:22:35Z

@JMS55 Bindless isn't enabled for meshlets. I don't know enough about the meshlet code to implement it; you'll need to implement it if you want it. It doesn't break, though.

JMS55 · 2024-12-23T00:27:10Z

Can confirm it works.

Also looking at the renderdoc trace, now we do bind PBR StandardMaterial BG, 5 draws for each of the PBR instances, bind MeshletDebugMaterial, 1 draw for the 5 debug instances. Before bindless this used to be a separate bind + draw for each PBR instance, now it's 1 bind + separate draws.

Ideally ofc this would be 1 draw for all 5 PBR instances since they share the same pipeline + BG, but I can fix that sometime in the future. Probably needs to wait for retained bins, and when we consolidate the binning code across mesh and meshlet mesh.

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

# Objective - Running example `load_gltf` when not using bindless gives this error ``` ERROR bevy_render::render_resource::pipeline_cache: failed to process shader: error: no definition in scope for identifier: 'slot' ┌─ crates/bevy_pbr/src/render/pbr_fragment.wgsl:153:13 │ 153 │ slot, │ ^^^^ unknown identifier │ = no definition in scope for identifier: 'slot' ``` - since #16825 ## Solution - Set `slot` to the expected value when not mindless - Also use it for `uv_b` ## Testing - Run example `load_gltf` on a Mac or in wasm

Currently, our batchable binned items are stored in a hash table that maps bin key, which includes the batch set key, to a list of entities. Multidraw is handled by sorting the bin keys and accumulating adjacent bins that can be multidrawn together (i.e. have the same batch set key) into multidraw commands during `batch_and_prepare_binned_render_phase`. This is reasonably efficient right now, but it will complicate future work to retain indirect draw parameters from frame to frame. Consider what must happen when we have retained indirect draw parameters and the application adds a bin (i.e. a new mesh) that shares a batch set key with some pre-existing meshes. (That is, the new mesh can be multidrawn with the pre-existing meshes.) To be maximally efficient, our goal in that scenario will be to update *only* the indirect draw parameters for the batch set (i.e. multidraw command) containing the mesh that was added, while leaving the others alone. That means that we have to quickly locate all the bins that belong to the batch set being modified. In the existing code, we would have to sort the list of bin keys so that bins that can be multidrawn together become adjacent to one another in the list. Then we would have to do a binary search through the sorted list to find the location of the bin that was just added. Next, we would have to widen our search to adjacent indexes that contain the same batch set, doing expensive comparisons against the batch set key every time. Finally, we would reallocate the indirect draw parameters and update the stored pointers to the indirect draw parameters that the bins store. By contrast, it'd be dramatically simpler if we simply changed the way bins are stored to first map from batch set key (i.e. multidraw command) to the bins (i.e. meshes) within that batch set key, and then from each individual bin to the mesh instances. That way, the scenario above in which we add a new mesh will be simpler to handle. First, we will look up the batch set key corresponding to that mesh in the outer map to find an inner map corresponding to the single multidraw command that will draw that batch set. We will know how many meshes the multidraw command is going to draw by the size of that inner map. Then we simply need to reallocate the indirect draw parameters and update the pointers to those parameters within the bins as necessary. There will be no need to do any binary search or expensive batch set key comparison: only a single hash lookup and an iteration over the inner map to update the pointers. This patch implements the above technique. Because we don't have retained bins yet, this PR provides no performance benefits. However, it opens the door to maximally efficient updates when only a small number of meshes change from frame to frame. The main churn that this patch causes is that the *batch set key* (which uniquely specifies a multidraw command) and *bin key* (which uniquely specifies a mesh *within* that multidraw command) are now separate, instead of the batch set key being embedded *within* the bin key. In order to isolate potential regressions, I think that at least #16890, #16836, and #16825 should land before this PR does. ## Migration Guide * The *batch set key* is now separate from the *bin key* in `BinnedPhaseItem`. The batch set key is used to collect multidrawable meshes together. If you aren't using the multidraw feature, you can safely set the batch set key to `()`.

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

# Objective - Running example `load_gltf` when not using bindless gives this error ``` ERROR bevy_render::render_resource::pipeline_cache: failed to process shader: error: no definition in scope for identifier: 'slot' ┌─ crates/bevy_pbr/src/render/pbr_fragment.wgsl:153:13 │ 153 │ slot, │ ^^^^ unknown identifier │ = no definition in scope for identifier: 'slot' ``` - since bevyengine#16825 ## Solution - Set `slot` to the expected value when not mindless - Also use it for `uv_b` ## Testing - Run example `load_gltf` on a Mac or in wasm

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

# Objective - Running example `load_gltf` when not using bindless gives this error ``` ERROR bevy_render::render_resource::pipeline_cache: failed to process shader: error: no definition in scope for identifier: 'slot' ┌─ crates/bevy_pbr/src/render/pbr_fragment.wgsl:153:13 │ 153 │ slot, │ ^^^^ unknown identifier │ = no definition in scope for identifier: 'slot' ``` - since bevyengine#16825 ## Solution - Set `slot` to the expected value when not mindless - Also use it for `uv_b` ## Testing - Run example `load_gltf` on a Mac or in wasm

Currently, our batchable binned items are stored in a hash table that maps bin key, which includes the batch set key, to a list of entities. Multidraw is handled by sorting the bin keys and accumulating adjacent bins that can be multidrawn together (i.e. have the same batch set key) into multidraw commands during `batch_and_prepare_binned_render_phase`. This is reasonably efficient right now, but it will complicate future work to retain indirect draw parameters from frame to frame. Consider what must happen when we have retained indirect draw parameters and the application adds a bin (i.e. a new mesh) that shares a batch set key with some pre-existing meshes. (That is, the new mesh can be multidrawn with the pre-existing meshes.) To be maximally efficient, our goal in that scenario will be to update *only* the indirect draw parameters for the batch set (i.e. multidraw command) containing the mesh that was added, while leaving the others alone. That means that we have to quickly locate all the bins that belong to the batch set being modified. In the existing code, we would have to sort the list of bin keys so that bins that can be multidrawn together become adjacent to one another in the list. Then we would have to do a binary search through the sorted list to find the location of the bin that was just added. Next, we would have to widen our search to adjacent indexes that contain the same batch set, doing expensive comparisons against the batch set key every time. Finally, we would reallocate the indirect draw parameters and update the stored pointers to the indirect draw parameters that the bins store. By contrast, it'd be dramatically simpler if we simply changed the way bins are stored to first map from batch set key (i.e. multidraw command) to the bins (i.e. meshes) within that batch set key, and then from each individual bin to the mesh instances. That way, the scenario above in which we add a new mesh will be simpler to handle. First, we will look up the batch set key corresponding to that mesh in the outer map to find an inner map corresponding to the single multidraw command that will draw that batch set. We will know how many meshes the multidraw command is going to draw by the size of that inner map. Then we simply need to reallocate the indirect draw parameters and update the pointers to those parameters within the bins as necessary. There will be no need to do any binary search or expensive batch set key comparison: only a single hash lookup and an iteration over the inner map to update the pointers. This patch implements the above technique. Because we don't have retained bins yet, this PR provides no performance benefits. However, it opens the door to maximally efficient updates when only a small number of meshes change from frame to frame. The main churn that this patch causes is that the *batch set key* (which uniquely specifies a multidraw command) and *bin key* (which uniquely specifies a mesh *within* that multidraw command) are now separate, instead of the batch set key being embedded *within* the bin key. In order to isolate potential regressions, I think that at least bevyengine#16890, bevyengine#16836, and bevyengine#16825 should land before this PR does. ## Migration Guide * The *batch set key* is now separate from the *bin key* in `BinnedPhaseItem`. The batch set key is used to collect multidrawable meshes together. If you aren't using the multidraw feature, you can safely set the batch set key to `()`.

Fix meshlet shaders for bindless mode.

57af624

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

pcwalton requested review from Elabajaba and JMS55 December 15, 2024 04:11

pcwalton added A-Rendering Drawing game state to the screen P-Regression Functionality that used to work but no longer does. Add a test for this! C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 15, 2024

JMS55 suggested changes Dec 15, 2024

View reviewed changes

Address review comment

185539e

JMS55 approved these changes Dec 15, 2024

View reviewed changes

pcwalton requested a review from IceSentry December 15, 2024 04:54

alice-i-cecile added the S-Needs-Testing Testing must be done before this is safe to merge label Dec 15, 2024

alice-i-cecile added this to the 0.16 milestone Dec 15, 2024

Merge remote-tracking branch 'origin/main' into bindless-meshlets

9d71763

pcwalton added 2 commits December 18, 2024 15:46

Merge remote-tracking branch 'origin/main' into bindless-meshlets

ede668c

Merge fallout

abd8f14

This was referenced Dec 19, 2024

Retain RenderMaterialInstances and RenderMeshMaterialIds from frame to frame. #16878

Closed

Introduce two-level bins for multidrawable meshes. #16898

Merged

IceSentry approved these changes Dec 19, 2024

View reviewed changes

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 Dec 19, 2024

alice-i-cecile added this pull request to the merge queue Dec 24, 2024

Merged via the queue into bevyengine:main with commit ddf4d9e Dec 24, 2024
29 checks passed

pcwalton added a commit to pcwalton/bevy that referenced this pull request Dec 25, 2024

Fix meshlet shaders for bindless mode. (bevyengine#16825)

e432ee0

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

mockersf mentioned this pull request Dec 25, 2024

Fix non-meshlet shaders for non-bindless mode #16966

Merged

ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025

Fix meshlet shaders for bindless mode. (bevyengine#16825)

a6f761c

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025

Fix meshlet shaders for bindless mode. (bevyengine#16825)

6a74999

We have to extract the material ID from the mesh and stuff it in the vertex during visibility buffer resolution.

Uh oh!

Fix meshlet shaders for bindless mode. #16825

Fix meshlet shaders for bindless mode. #16825

Uh oh!

Conversation

pcwalton commented Dec 15, 2024

Uh oh!

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!

JMS55 commented Dec 15, 2024

Uh oh!

JMS55 commented Dec 15, 2024

Uh oh!

pcwalton commented Dec 16, 2024

Uh oh!

JMS55 commented Dec 23, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants