Bind group entries by robtfm · Pull Request #9694 · bevyengine/bevy

robtfm · 2023-09-04T23:38:59Z

Objective

Simplify bind group creation code. alternative to (and based on) #9476

Solution

Add a BindGroupEntries struct that can transparently be used where &[BindGroupEntry<'b>] is required in BindGroupDescriptors.

Allows constructing the descriptor's entries as:

render_device.create_bind_group(
    "my_bind_group",
    &my_layout,
    &BindGroupEntries::with_indexes((
        (2, &my_sampler),
        (3, my_uniform),
    )),
);

instead of

render_device.create_bind_group(
    "my_bind_group",
    &my_layout,
    &[
        BindGroupEntry {
            binding: 2,
            resource: BindingResource::Sampler(&my_sampler),
        },
        BindGroupEntry {
            binding: 3,
            resource: my_uniform,
        },
    ],
);

or

render_device.create_bind_group(
    "my_bind_group",
    &my_layout,
    &BindGroupEntries::sequential((&my_sampler, my_uniform)),
);

instead of

render_device.create_bind_group(
    "my_bind_group",
    &my_layout,
    &[
        BindGroupEntry {
            binding: 0,
            resource: BindingResource::Sampler(&my_sampler),
        },
        BindGroupEntry {
            binding: 1,
            resource: my_uniform,
        },
    ],
);

the structs has no user facing macros, is tuple-type-based so stack allocated, and has no noticeable impact on compile time.

Also adds a DynamicBindGroupEntries struct with a similar api that uses a Vec under the hood and allows extending the entries.
Modifies RenderDevice::create_bind_group to take separate arguments label, layout and entries instead of a BindGroupDescriptor struct. The struct can't be stored due to the internal references, and with only 3 members arguably does not add enough context to justify itself.
Modify the codebase to use the new api and the BindGroupEntries / DynamicBindGroupEntries structs where appropriate (whenever the entries slice contains more than 1 member).

Migration Guide

Calls to RenderDevice::create_bind_group({BindGroupDescriptor { label, layout, entries }) must be amended to RenderDevice::create_bind_group(label, layout, entries).
If labels have been specified as "bind_group_name".into(), they need to change to just "bind_group_name". Some("bind_group_name") and None will still work, but Some("bind_group_name") can optionally be simplified to just "bind_group_name".

crates/bevy_render/src/renderer/render_device.rs

IceSentry

Alright, I went over everything it looks really good. The diffs are so nice to look at!

LGTM

IceSentry · 2023-09-05T00:29:23Z

crates/bevy_pbr/src/render/mesh_bindings.rs

+        render_device.create_bind_group(
+            Some("model_only_mesh_bind_group"),
+            &self.model_only,
+            &[entry::model(0, model.clone())],


It makes sense to not change this in this PR but the pattern used here and the rest of the file will be easy to remove in a future PR.

examples/shader/post_processing.rs

IceSentry · 2023-09-05T00:41:13Z

Oh, I reviewed before the IntoLabel but that's really nice to see too!

IceSentry · 2023-09-05T00:42:31Z

I think you can remove the .as_slice() from the PR description

crates/bevy_render/src/render_resource/bind_group_entries.rs

robtfm · 2023-09-05T15:47:49Z

Not sure what went wrong with the ci, I tried on my repo and it’s green. I also don’t know how to rerun it any more (rip bors)

IceSentry · 2023-09-05T22:12:17Z

Yeah, build works fine on my side too. Must be a CI issue, I think only maintainers are allowed to force rerun CI. Alternatively you could push a new commit

superdump

Approving but do not merge until after #9258 is merged. I don't want to disrupt that large PR.

IceSentry · 2023-10-17T20:21:36Z

I've been using this in a codebase that uses UniformBuffer<T>. I added this impl to make it work for me.

impl<'a, T: ShaderType + WriteInto> IntoBinding<'a> for &'a UniformBuffer<T> {
    #[inline]
    fn into_binding(self) -> BindingResource<'a> {
        BindingResource::Buffer(
            self.buffer()
                .expect("Failed to get buffer")
                .as_entire_buffer_binding(),
        )
    }
}

We could also add this so it can work with any buffer binding

impl<'a> IntoBinding<'a> for BufferBinding<'a> {
    #[inline]
    fn into_binding(self) -> BindingResource<'a> {
        BindingResource::Buffer(self)
    }
}

I also have used this one for DynamicUniformBuffer but the the ShaderType bound could be a bit annoying

impl<'a, T: ShaderType + WriteInto> IntoBinding<'a> for &'a DynamicUniformBuffer<T> 
{ 
    #[inline] 
    fn into_binding(self) -> BindingResource<'a> { 
        BindingResource::Buffer(BufferBinding { 
            buffer: self.buffer().expect("Failed to get buffer"), 
            offset: 0, 
            size: Some(T::min_size()), 
        }) 
    } 
}

Co-authored-by: IceSentry <[email protected]>

robtfm · 2023-10-17T22:53:56Z

I added your impl for UniformBuffer and wgpu::BufferBinding, thanks!

I also have used this one for DynamicUniformBuffer

this one was already in the PR though

IceSentry · 2023-10-17T22:56:24Z

this one was already in the PR though

Ah, right, sorry, it was in uniform_buffer.rs. When using it I just copied the bind_group_entries.rs module so I missed that one.

IceSentry · 2023-10-17T23:00:08Z

crates/bevy_gizmos/src/lib.rs

+            bindgroup: render_device.create_bind_group(
+                "LineGizmoUniform bindgroup",
+                &line_gizmo_uniform_layout.layout,
+                &[BindGroupEntry {


With the UniformBuffer/BufferBinding impl you should be able to use it here too now I think?

i didn't use it anywhere there was only 1 entry, it doesn't really save any typing. i guess i could for consistency though?

Right, makes sense then. I'll personally use it everywhere in my own code but I think it's fine as is.

it ends up looking like &BindGroupEntries::sequential((binding,)), which is a bit weird...

Oof, yeah, nevermind then 😅

there's actually quite a few single-entry bindings at zero over the codebase ... i'll try adding a BindGroupEntries::single method.

added, i think it's nice

crates/bevy_render/src/render_resource/uniform_buffer.rs

Co-authored-by: IceSentry <[email protected]>

crates/bevy_render/src/render_resource/uniform_buffer.rs

IceSentry · 2023-10-18T00:47:06Z

Here's another impl that could be useful to have

impl<'a> IntoBinding<'a> for &'a CachedTexture {
    #[inline]
    fn into_binding(self) -> BindingResource<'a> {
        BindingResource::TextureView(&self.default_view)
    }
}

I needed that because ViewPrepassTextures holds Option<CachedTexture> and I wanted to create a bind group with them.

I wanted to suggest an impl that unwraps for Options but I think it makes sense to do unwrapping at the callsites.

# Objective Simplify bind group creation code. alternative to (and based on) bevyengine#9476 ## Solution - Add a `BindGroupEntries` struct that can transparently be used where `&[BindGroupEntry<'b>]` is required in BindGroupDescriptors. Allows constructing the descriptor's entries as: ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &BindGroupEntries::with_indexes(( (2, &my_sampler), (3, my_uniform), )), ); ``` instead of ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &[ BindGroupEntry { binding: 2, resource: BindingResource::Sampler(&my_sampler), }, BindGroupEntry { binding: 3, resource: my_uniform, }, ], ); ``` or ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &BindGroupEntries::sequential((&my_sampler, my_uniform)), ); ``` instead of ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &[ BindGroupEntry { binding: 0, resource: BindingResource::Sampler(&my_sampler), }, BindGroupEntry { binding: 1, resource: my_uniform, }, ], ); ``` the structs has no user facing macros, is tuple-type-based so stack allocated, and has no noticeable impact on compile time. - Also adds a `DynamicBindGroupEntries` struct with a similar api that uses a `Vec` under the hood and allows extending the entries. - Modifies `RenderDevice::create_bind_group` to take separate arguments `label`, `layout` and `entries` instead of a `BindGroupDescriptor` struct. The struct can't be stored due to the internal references, and with only 3 members arguably does not add enough context to justify itself. - Modify the codebase to use the new api and the `BindGroupEntries` / `DynamicBindGroupEntries` structs where appropriate (whenever the entries slice contains more than 1 member). ## Migration Guide - Calls to `RenderDevice::create_bind_group({BindGroupDescriptor { label, layout, entries })` must be amended to `RenderDevice::create_bind_group(label, layout, entries)`. - If `label`s have been specified as `"bind_group_name".into()`, they need to change to just `"bind_group_name"`. `Some("bind_group_name")` and `None` will still work, but `Some("bind_group_name")` can optionally be simplified to just `"bind_group_name"`. --------- Co-authored-by: IceSentry <[email protected]>

# Objective - Follow up to #9694 ## Solution - Same api as #9694 but adapted for `BindGroupLayoutEntry` - Use the same `ShaderStages` visibilty for all entries by default - Add `BindingType` helper function that mirror the wgsl equivalent and that make writing layouts much simpler. Before: ```rust let layout = render_device.create_bind_group_layout(&BindGroupLayoutDescriptor { label: Some("post_process_bind_group_layout"), entries: &[ BindGroupLayoutEntry { binding: 0, visibility: ShaderStages::FRAGMENT, ty: BindingType::Texture { sample_type: TextureSampleType::Float { filterable: true }, view_dimension: TextureViewDimension::D2, multisampled: false, }, count: None, }, BindGroupLayoutEntry { binding: 1, visibility: ShaderStages::FRAGMENT, ty: BindingType::Sampler(SamplerBindingType::Filtering), count: None, }, BindGroupLayoutEntry { binding: 2, visibility: ShaderStages::FRAGMENT, ty: BindingType::Buffer { ty: bevy::render::render_resource::BufferBindingType::Uniform, has_dynamic_offset: false, min_binding_size: Some(PostProcessSettings::min_size()), }, count: None, }, ], }); ``` After: ```rust let layout = render_device.create_bind_group_layout( "post_process_bind_group_layout"), &BindGroupLayoutEntries::sequential( ShaderStages::FRAGMENT, ( texture_2d_f32(), sampler(SamplerBindingType::Filtering), uniform_buffer(false, Some(PostProcessSettings::min_size())), ), ), ); ``` Here's a more extreme example in bevy_solari: JMS55@86dab7f --- ## Changelog - Added `BindGroupLayoutEntries` and all `BindingType` helper functions. ## Migration Guide `RenderDevice::create_bind_group_layout()` doesn't take a `BindGroupLayoutDescriptor` anymore. You need to provide the parameters separately ```rust // 0.12 let layout = render_device.create_bind_group_layout(&BindGroupLayoutDescriptor { label: Some("post_process_bind_group_layout"), entries: &[ BindGroupLayoutEntry { // ... }, ], }); // 0.13 let layout = render_device.create_bind_group_layout( "post_process_bind_group_layout", &[ BindGroupLayoutEntry { // ... }, ], ); ``` ## TODO - [x] implement a `Dynamic` variant - [x] update the `RenderDevice::create_bind_group_layout()` api to match the one from `RenderDevice::creat_bind_group()` - [x] docs

# Objective Simplify bind group creation code. alternative to (and based on) bevyengine#9476 ## Solution - Add a `BindGroupEntries` struct that can transparently be used where `&[BindGroupEntry<'b>]` is required in BindGroupDescriptors. Allows constructing the descriptor's entries as: ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &BindGroupEntries::with_indexes(( (2, &my_sampler), (3, my_uniform), )), ); ``` instead of ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &[ BindGroupEntry { binding: 2, resource: BindingResource::Sampler(&my_sampler), }, BindGroupEntry { binding: 3, resource: my_uniform, }, ], ); ``` or ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &BindGroupEntries::sequential((&my_sampler, my_uniform)), ); ``` instead of ```rust render_device.create_bind_group( "my_bind_group", &my_layout, &[ BindGroupEntry { binding: 0, resource: BindingResource::Sampler(&my_sampler), }, BindGroupEntry { binding: 1, resource: my_uniform, }, ], ); ``` the structs has no user facing macros, is tuple-type-based so stack allocated, and has no noticeable impact on compile time. - Also adds a `DynamicBindGroupEntries` struct with a similar api that uses a `Vec` under the hood and allows extending the entries. - Modifies `RenderDevice::create_bind_group` to take separate arguments `label`, `layout` and `entries` instead of a `BindGroupDescriptor` struct. The struct can't be stored due to the internal references, and with only 3 members arguably does not add enough context to justify itself. - Modify the codebase to use the new api and the `BindGroupEntries` / `DynamicBindGroupEntries` structs where appropriate (whenever the entries slice contains more than 1 member). ## Migration Guide - Calls to `RenderDevice::create_bind_group({BindGroupDescriptor { label, layout, entries })` must be amended to `RenderDevice::create_bind_group(label, layout, entries)`. - If `label`s have been specified as `"bind_group_name".into()`, they need to change to just `"bind_group_name"`. `Some("bind_group_name")` and `None` will still work, but `Some("bind_group_name")` can optionally be simplified to just `"bind_group_name"`. --------- Co-authored-by: IceSentry <[email protected]>

robtfm added 8 commits September 3, 2023 18:06

BindGroupEntries

ff63eb8

fmt

da60a77

ci

a9fb779

tweak prepass get_bindings

6800f24

destructure BindGroupDescriptor

b35c654

Merge remote-tracking branch 'upstream/main' into bind_group_entries

5d23b02

fmt

862ae47

fix comment

a57ff80

robtfm added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Needs-SME This type of work requires an SME to approve it. labels Sep 4, 2023

robtfm added 4 commits September 5, 2023 00:41

no bevy_internals

49e186a

no_run

90d5caa

ignore?

b59a64d

doc tests are hard

7b88136

IceSentry reviewed Sep 5, 2023

View reviewed changes

crates/bevy_render/src/renderer/render_device.rs Outdated Show resolved Hide resolved

IntoLabel

c43e753

IceSentry approved these changes Sep 5, 2023

View reviewed changes

simpler

56e4297

robtfm added 2 commits September 5, 2023 01:42

re-add example comments

b8dbd66

remove as_slice

3e3c9f6

JMS55 self-requested a review September 5, 2023 01:13

JMS55 suggested changes Sep 5, 2023

View reviewed changes

crates/bevy_render/src/render_resource/bind_group_entries.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/render_resource/bind_group_entries.rs Outdated Show resolved Hide resolved

crates/bevy_render/src/render_resource/bind_group_entries.rs Outdated Show resolved Hide resolved

indexes -> indices

22c1e67

JMS55 approved these changes Sep 5, 2023

View reviewed changes

IceSentry mentioned this pull request Sep 5, 2023

Bind group descriptor macro #9476

Closed

superdump approved these changes Oct 8, 2023

View reviewed changes

robtfm and others added 5 commits October 17, 2023 23:08

Merge remote-tracking branch 'upstream/main' into bind_group_entries

14ff145

merge merge

6f98c9a

Merge remote-tracking branch 'upstream/main' into bind_group_entries

624df5f

merge that merged merge till its really merged

b070027

add more IntoBinding impls

3c17ffc

Co-authored-by: IceSentry <[email protected]>

IceSentry reviewed Oct 17, 2023

View reviewed changes

crates/bevy_render/src/render_resource/uniform_buffer.rs Outdated Show resolved Hide resolved

Update crates/bevy_render/src/render_resource/uniform_buffer.rs

3875480

Co-authored-by: IceSentry <[email protected]>

IceSentry reviewed Oct 17, 2023

View reviewed changes

crates/bevy_render/src/render_resource/uniform_buffer.rs Outdated Show resolved Hide resolved

robtfm added 5 commits October 18, 2023 00:08

bad comma

3f0e121

add/use BindGroupEntries::single

31bd9fc

remove errant clone

9985276

ci

e958c26

ci ci

6a6a5a8

robtfm added 2 commits October 21, 2023 15:31

Merge remote-tracking branch 'upstream/main' into bind_group_entries

358c0dc

fix merge

d072729

superdump approved these changes Oct 21, 2023

View reviewed changes

superdump added this pull request to the merge queue Oct 21, 2023

Merged via the queue into bevyengine:main with commit 6f2a5cb Oct 21, 2023

IceSentry mentioned this pull request Oct 22, 2023

Bind group layout entries #10224

Merged

3 tasks

Uh oh!

Conversation

robtfm commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Migration Guide

Uh oh!

Uh oh!

IceSentry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IceSentry commented Sep 5, 2023

Uh oh!

IceSentry commented Sep 5, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robtfm commented Sep 5, 2023

Uh oh!

IceSentry commented Sep 5, 2023

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

IceSentry commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robtfm commented Oct 17, 2023

Uh oh!

IceSentry commented Oct 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IceSentry commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

robtfm commented Sep 4, 2023 •

edited

Loading

IceSentry commented Oct 17, 2023 •

edited

Loading

IceSentry commented Oct 18, 2023 •

edited

Loading