Fix the texture_binding_array, specialized_mesh_pipeline, and custom_shader_instancing examples after the bindless change.#16641
Merged
alice-i-cecile merged 2 commits intobevyengine:mainfrom Dec 5, 2024
Conversation
`custom_shader_instancing` examples after the bindless change. The bindless PR (bevyengine#16368) broke some examples: * `specialized_mesh_pipeline` and `custom_shader_instancing` failed because they expect to be able to render a mesh with no material, by overriding enough of the render pipeline to be able to do so. This PR fixes the issue by restoring the old behavior in which we extract meshes even if they have no material. * `texture_binding_array` broke because it doesn't implement `AsBindGroup::unprepared_bind_group`. This was tricky to fix because there's a very good reason why `texture_binding_array` doesn't implement that method: there's no sensible way to do so with `wgpu`'s current bindless API, due to its multiple levels of borrowed references. To fix the example, I split `MaterialBindGroup` into `MaterialBindlessBindGroup` and `MaterialNonBindlessBindGroup`, and allow direct custom implementations of `AsBindGroup::as_bind_group` for the latter type of bind groups. To opt in to the new behavior, return the `AsBindGroupError::CreateBindGroupDirectly` error from your `AsBindGroup::unprepared_bind_group` implementation, and Bevy will call your custom `AsBindGroup::as_bind_group` method as before.
pcwalton
added
A-Rendering
Elabajaba
reviewed
Dec 5, 2024
| fn init_custom(&mut self, bind_group: BindGroup, extra_data: M::Data) { | ||
| match *self { | ||
| MaterialBindGroup::Bindless(_) => { | ||
| error!("Custom bind groups aren't supported in bindless mode"); |
Contributor
There was a problem hiding this comment.
Dumb question (not asking for any changes), does limiting custom bind groups to non-bindless mean eg. custom materials (thinking about either an extended material, or maybe something like a full StandardMaterial toon shader replacement?) won't benefit from bindless currently?
Contributor
Author
There was a problem hiding this comment.
Bindless works for any material as long as you write #[bindless] on it; there's nothing specific to StandardMaterial. (Actually, StandardMaterial doesn't benefit from bindless yet; that's #16644.)
The restriction you quoted only applies in very unusual circumstances, basically only in the custom_shader_instancing example.
Elabajaba
approved these changes
Dec 5, 2024
Contributor
Elabajaba
left a comment
Elabajaba
left a comment
There was a problem hiding this comment.
Examples are working on my system (windows amd 6800xt, tested dx12 and vulkan), code looks good, comments are great as always.
JMS55
approved these changes
Dec 5, 2024
pcwalton
added
S-Ready-For-Final-Review
ecoskey
pushed a commit
to ecoskey/bevy
that referenced
this pull request
Jan 6, 2025
…stom_shader_instancing` examples after the bindless change. (bevyengine#16641) The bindless PR (bevyengine#16368) broke some examples: * `specialized_mesh_pipeline` and `custom_shader_instancing` failed because they expect to be able to render a mesh with no material, by overriding enough of the render pipeline to be able to do so. This PR fixes the issue by restoring the old behavior in which we extract meshes even if they have no material. * `texture_binding_array` broke because it doesn't implement `AsBindGroup::unprepared_bind_group`. This was tricky to fix because there's a very good reason why `texture_binding_array` doesn't implement that method: there's no sensible way to do so with `wgpu`'s current bindless API, due to its multiple levels of borrowed references. To fix the example, I split `MaterialBindGroup` into `MaterialBindlessBindGroup` and `MaterialNonBindlessBindGroup`, and allow direct custom implementations of `AsBindGroup::as_bind_group` for the latter type of bind groups. To opt in to the new behavior, return the `AsBindGroupError::CreateBindGroupDirectly` error from your `AsBindGroup::unprepared_bind_group` implementation, and Bevy will call your custom `AsBindGroup::as_bind_group` method as before. ## Migration Guide * Bevy will now unconditionally call `AsBindGroup::unprepared_bind_group` for your materials, so you must no longer panic in that function. Instead, return the new `AsBindGroupError::CreateBindGroupDirectly` error, and Bevy will fall back to calling `AsBindGroup::as_bind_group` as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bindless PR (#16368) broke some examples:
specialized_mesh_pipelineandcustom_shader_instancingfailed because they expect to be able to render a mesh with no material, by overriding enough of the render pipeline to be able to do so. This PR fixes the issue by restoring the old behavior in which we extract meshes even if they have no material.texture_binding_arraybroke because it doesn't implementAsBindGroup::unprepared_bind_group. This was tricky to fix because there's a very good reason whytexture_binding_arraydoesn't implement that method: there's no sensible way to do so withwgpu's current bindless API, due to its multiple levels of borrowed references. To fix the example, I splitMaterialBindGroupintoMaterialBindlessBindGroupandMaterialNonBindlessBindGroup, and allow direct custom implementations ofAsBindGroup::as_bind_groupfor the latter type of bind groups. To opt in to the new behavior, return theAsBindGroupError::CreateBindGroupDirectlyerror from yourAsBindGroup::unprepared_bind_groupimplementation, and Bevy will call your customAsBindGroup::as_bind_groupmethod as before.Migration Guide
AsBindGroup::unprepared_bind_groupfor your materials, so you must no longer panic in that function. Instead, return the newAsBindGroupError::CreateBindGroupDirectlyerror, and Bevy will fall back to callingAsBindGroup::as_bind_groupas before.