-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Add BindGroupLayout caching via descriptors #21205
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
base: main
Are you sure you want to change the base?
Conversation
crates/bevy_anti_alias/src/contrast_adaptive_sharpening/node.rs
Outdated
Show resolved
Hide resolved
render_device, | ||
material_param, | ||
false, | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is yuck, AsBindGroup::unprepared_bind_group
needs a BindGroupLayout
, so we fetch it from the cache - investigate if unprepared_bind_group
can use BindGroupLayoutDescriptor
instead
where | ||
Self: Sized, | ||
{ | ||
todo!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do similar to what the method right above this one does, something like
todo!(); | |
BindGroupLayoutDescriptor::new( | |
Self::label(), | |
&Self::bind_group_layout_entries(render_device, false), | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, the entries method takes render_device. Hmm. im not sure why that is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be related to #[derive(AsBindGroup)]
macro with bindless
, it needs the render_device
to see if its supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Zeophlite@1c9e74c - this disables the above check, but hopefully helps point in the right direction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I think the bindless_supported
check can be deferred but too much for this PR, I've added back render_device
for bindless cases, see 3bae3f4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zeophlite I think the way we should go about this is to remove the render device dependency in the other direction: we don't want to defer it but make it happen earlier, and inform as bind group by some other mechanism. For example, if we had a way of setting shader defs like BINDLESS and then checking them in as bind group, we can generate the right bind group layout etc for it. Because thats really what it is, shader-def-dependent bind group layouts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for the time being using a render device to implement it is fine though :)
Can you add more motivation to the PR description please? I'm having trouble following why this is a good idea. |
pub label: Option<Cow<'static, str>>, | ||
/// The layout of bind groups for this pipeline. | ||
pub layout: Vec<BindGroupLayout>, | ||
pub layout: Vec<BindGroupLayoutDescriptor>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers: this line (and the equivalent on ComputePipelineDescriptor) is the whole point of this change. It makes these two structs not depend on wgpu
, which means they can be moved out of bevy_render into a bevy_material, and created without the need of a renderdevice. Specifically, this allows describing a shader's inputs without bevy_render, which when combined with bevy_shader allows for renderless material abstraction.
Everything else in this PR is just to make these two line diffs happen.
Objective
BindGroupLayout
by using aBindGroupLayoutDescriptor
and cache the resultsbevy_material
(render-less material definitions)Solution
Testing