Skip to content

Make the StandardMaterial bindless index table have a fixed size regardless of the features that are enabled.#18771

Merged
alice-i-cecile merged 16 commits intobevyengine:mainfrom
pcwalton:clearcoat-fix
Apr 9, 2025
Merged

Make the StandardMaterial bindless index table have a fixed size regardless of the features that are enabled.#18771
alice-i-cecile merged 16 commits intobevyengine:mainfrom
pcwalton:clearcoat-fix

Conversation

@pcwalton
Copy link
Copy Markdown
Contributor

@pcwalton pcwalton commented Apr 8, 2025

Due to the preprocessor usage in the shader, different combinations of features could cause the fields of StandardMaterialBindings to shift around. In certain cases, this could cause them to not line up with the bindings specified in StandardMaterial. This resulted in #18104.

This commit fixes the issue by making StandardMaterialBindings have a fixed size. On the CPU side, it uses the #[bindless(index_table(range(M..N)))] feature I added to AsBindGroup in #18025 to do so. Thus this patch has a dependency on #18025.

Closes #18104.

PR bevyengine#17898 disabled bindless support for `ExtendedMaterial`. This commit
adds it back. It also adds a new example, `extended_material_bindless`,
showing how to use it.
They were sized based on the total number of resources (base +
extended), which was incorrect. They should be based on the binding
range.
@pcwalton pcwalton requested review from superdump and tychedelia April 8, 2025 23:49
@pcwalton
Copy link
Copy Markdown
Contributor Author

pcwalton commented Apr 8, 2025

The only commit that's unique to this PR is 31db8e2.

Copy link
Copy Markdown
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Confirmed fix on windows.

pcwalton added 2 commits April 8, 2025 17:18
regardless of the features that are enabled.

Due to the preprocessor usage in the shader, different combinations of
features could cause the fields of `StandardMaterialBindings` to shift
around. In certain cases, this could cause them to not line up with the
bindings specified in `StandardMaterial`. This resulted in bevyengine#18104.

This commit fixes the issue by making `StandardMaterialBindings` have a
fixed size. On the CPU side, it uses the
`#[bindless(index_table(range(M..N)))]` feature I added to `AsBindGroup`
in bevyengine#18025 to do so. Thus this patch has a dependency on bevyengine#18025.

Closes bevyengine#18104.
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Apr 9, 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 Apr 9, 2025
@pcwalton pcwalton added the S-Blocked This cannot move forward until something else changes label Apr 9, 2025
@robtfm
Copy link
Copy Markdown
Contributor

robtfm commented Apr 9, 2025

not tested, but from the comments on the issue: is the #[bindless(index_table(range(0..31)))] necessary or does removing the ifdefs suffice?

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Apr 9, 2025
@pcwalton
Copy link
Copy Markdown
Contributor Author

pcwalton commented Apr 9, 2025

@robtfm It's necessary because without that attribute the index table that AsBindGroup generates will have a size just big enough to contain the largest binding. The feature flags use #[cfg_attr], so the AsBindGroup macro won't see any bindings that are cfg'd off. So if, for example, all the feature flags were turned off, the size that AsBindGroup thinks the index table is would be much smaller than the size declared in the shader. That's why we need the manual override.

@pcwalton pcwalton 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 Apr 9, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 9, 2025
Merged via the queue into bevyengine:main with commit 065a95e Apr 9, 2025
34 checks passed
mockersf pushed a commit that referenced this pull request Apr 9, 2025
…gardless of the features that are enabled. (#18771)

Due to the preprocessor usage in the shader, different combinations of
features could cause the fields of `StandardMaterialBindings` to shift
around. In certain cases, this could cause them to not line up with the
bindings specified in `StandardMaterial`. This resulted in #18104.

This commit fixes the issue by making `StandardMaterialBindings` have a
fixed size. On the CPU side, it uses the
`#[bindless(index_table(range(M..N)))]` feature I added to `AsBindGroup`
in #18025 to do so. Thus this patch has a dependency on #18025.

Closes #18104.

---------

Co-authored-by: Robert Swain <robert.swain@gmail.com>
jf908 pushed a commit to jf908/bevy that referenced this pull request May 12, 2025
…gardless of the features that are enabled. (bevyengine#18771)

Due to the preprocessor usage in the shader, different combinations of
features could cause the fields of `StandardMaterialBindings` to shift
around. In certain cases, this could cause them to not line up with the
bindings specified in `StandardMaterial`. This resulted in bevyengine#18104.

This commit fixes the issue by making `StandardMaterialBindings` have a
fixed size. On the CPU side, it uses the
`#[bindless(index_table(range(M..N)))]` feature I added to `AsBindGroup`
in bevyengine#18025 to do so. Thus this patch has a dependency on bevyengine#18025.

Closes bevyengine#18104.

---------

Co-authored-by: Robert Swain <robert.swain@gmail.com>
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Black materials on clearcoat example

5 participants