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.

Sign up for GitHub

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

Jump to bottom

Pad out binding arrays to their full length if partially bound binding arrays aren't supported. #18126

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

Pad out binding arrays to their full length if partially bound binding arrays aren't supported. #18126

wants to merge 2 commits into from
+102 −46

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Mar 3, 2025 •
edited
Loading

I mistakenly thought that with the wgpu upgrade we'd have PARTIALLY_BOUND_BINDING_ARRAY everywhere. Unfortunately this is not the case. This PR adds a workaround back in.

Closes #18098.

JMS55
JMS55 reviewed Mar 3, 2025
View reviewed changes
crates/bevy_pbr/src/material_bind_groups.rs Outdated

if let Some(required_binding_array_size) = required_binding_array_size {
while sampler_bindings.len() < required_binding_array_size as usize {
sampler_bindings.push(&**fallback_sampler);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using vec.extend(iter::repeat(fallback).take(n)) will be a little bit clearer here imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the PR work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, see below comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW your suggestion isn't quite correct, but I can do what I think you meant.

@JMS55
Copy link
Contributor

JMS55 commented Mar 3, 2025

Didn't work :(

2025-03-03T05:38:04.982853Z ERROR wgpu::backend::wgpu_core: Handling wgpu errors as fatal by default    

thread 'Compute Task Pool (6)' panicked at D:\packages\cargo\registry\src\index.crates.io-1949cf8c6b5b557f\wgpu-24.0.1\src\backend\wgpu_core.rs:1187:26:    
wgpu error: Validation Error

Caused by:
  In Device::create_bind_group, label = 'StandardMaterial'
    Binding count declared with exactly 2048 items, but 1 items were provided


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_pbr::material::prepare_material_bind_groups<bevy_pbr::pbr_material::StandardMaterial>`!

I would also be totally fine with a patch to just disable bindless on systems without PARTIALLY_BOUND_BINDING_ARRAY support.

@pcwalton
Copy link
Contributor Author

pcwalton commented Mar 3, 2025

Someone who can repro will have to fix it then. Doesn't make sense for me to back and forth.

@JMS55
Copy link
Contributor

JMS55 commented Mar 3, 2025

If you want to reproduce on your machine, you can remove PARTIALLY_BOUND_BINDING_ARRAY from the requested device features. Bevy enables everything supported (minus RT and primary mappable buffers) by default, but you can modify that.

@pcwalton
Copy link
Contributor Author

pcwalton commented Mar 3, 2025

Ok, it should work now.

@pcwalton pcwalton marked this pull request as ready for review March 3, 2025 06:25
@pcwalton pcwalton requested review from JMS55 and superdump March 3, 2025 06:26
@pcwalton pcwalton changed the title Try to fix rendering if partially-bound binding arrays aren't supported Pad out binding arrays to their full length if partially bound binding arrays aren't supported. Mar 3, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Mar 3, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 3, 2025
@JMS55
Copy link
Contributor

JMS55 commented Mar 4, 2025 •
edited
Loading

@pcwalton it works now.

But actually, I think we should consider disabling bindless support if PARTIALLY_BOUND_BINDING_ARRAY is not supported. It should be supported basically everywhere afaik. I didn't remember until today, but the only reason it's not working on Arc is due to a wgpu bug that was fixed, and will be in wgpu v25.

Up to you, since you've already done all the work, but I'd personally be happy to drop the extra code to support this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JMS55 JMS55 Awaiting requested review from JMS55

@superdump superdump Awaiting requested review from superdump

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Rendering
Status: No status
Milestone
0.16
Development

Successfully merging this pull request may close these issues.

Broken rendering on GPUs that don't support PARTIALLY_BOUND_BINDING_ARRAY
3 participants
@pcwalton @JMS55 @alice-i-cecile