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

Fix meshlet shaders for bindless mode. #16825

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pcwalton
Copy link
Contributor

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

We have to extract the material ID from the mesh and stuff it in the
vertex during visibility buffer resolution.
@pcwalton pcwalton requested review from JMS55 and Elabajaba December 15, 2024 04:11
@pcwalton 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
crates/bevy_pbr/src/meshlet/visibility_buffer_resolve.wgsl Outdated Show resolved Hide resolved
@@ -5,8 +5,7 @@
mesh_bindings::mesh
}

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

@@ -12,6 +12,7 @@
}
#import bevy_render::maths::{affine3_to_square, mat2x4_f32_to_mat3x3_unpack}

#ifndef MESHLET_MESH_MATERIAL_PASS
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@JMS55
Copy link
Contributor

JMS55 commented Dec 15, 2024

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.

@pcwalton pcwalton requested a review from IceSentry December 15, 2024 04:54
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-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this! S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants