Bind group descriptor macro

[JMS55]

Objective

• Simplify bind group creation code

Solution

• Add a macro written by a combination of @Emilgardis and @nicopap (I just designed the API and wrote this PR, very little credit should go to me)

---

Changelog

• Added bevy_render::bind_group_descriptor!()
• Added bevy_render::renderer::RenderContext::create_bind_group()

[JMS55]

I didn't use the macro in the post processing example, as I felt it was a bit too magic. Let me know if you think otherwise though.

[JMS55]

Idk if this needs to be pub or not. I copied what resource_macros did below.

[github-actions]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[github-actions]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[IceSentry]

So, I really like the direction, but I'd kinda prefer something like this:

trait BindGroupDescriptorHelper<'a> {
    fn new(label: &str, layout: &'a BindGroupLayout, entries: Vec<BindGroupEntry>) -> Self;
}

impl<'a> BindGroupDescriptorHelper<'a> for BindGroupDescriptor<'a> {
    fn new(label: &str, layout: &'a BindGroupLayout, entries: Vec<BindGroupEntry>) -> Self {
        todo!()
    }
}

and then only have a macro for the entries. It would be a tiny bit more verbose though but not by that much I think.

It would look something like this when using it. So still pretty close.

render_context.create_bind_group(BindGroupDescriptor::new(
    "bloom_downsampling_first_bind_group",
    &downsampling_pipeline_res.bind_group_layout,
	bind_group_entries![
	    texture(view_target.main_texture_view()),
	    sampler(&bind_groups.sampler),
	    buffer(uniforms.clone()),
	]
))

I would prefer something like this because it would make rust-analyzer happier since it could actually suggest types and it would also make it easier to add some docs to the function.

With that said, I also really like this as-is. The create_bind_group on render_context is really nice too.

[Emilgardis]

this could be

Suggested change

	(@d $_:ident($e:expr)) => (compile_error!("only 'texture', 'sampler', and 'buffer' bindings are supported"));
	(@d $i:ident($e:expr)) => (compile_error!(concat!("only 'texture', 'sampler', and 'buffer' are supported, found '", stringify!($i), "'")));

[cart]

I'm very uncertain about this one / the macros.

I didn't use the macro in the post processing example, as I felt it was a bit too magic. Let me know if you think otherwise though.

This is why. If something is "too magic" for our users (and I do think this probably qualifies), then imo it is pretty much always "too magic" for our built in plugins (because one of our core principles is "engine code is user code").

This macro is "just" optimizing Rust type system syntax (removing field names, shortening type names, removing array syntax, etc). And "optimizing" those things make it harder to inspect and understand the code directly (Ex: you can no longer press F12 in your IDE to navigate to symbol definitions, autocomplete won't necessarily work, etc). You need to first inspect the macro (nontrivial for some classes of users), understand how the mapping works, and manually look up symbols / docs / etc.

If this pr is the bar for when to use macros, I think we could justify using macros in a lot more places.

If we really want to remove field names, maybe we should just come up with new types / helper functions?

// Before
render_device.create_bind_group(&BindGroupDescriptor {
    label: Some("bloom_downsampling_first_bind_group"),
    layout: &downsampling_pipeline_res.bind_group_layout,
    entries: &[
        BindGroupEntry {
            binding: 0,
            // Read from main texture directly
            resource: BindingResource::TextureView(
                view_target.main_texture_view(),
            ),
        },
        BindGroupEntry {
            binding: 1,
            resource: BindingResource::Sampler(&bind_groups.sampler),
        },
        BindGroupEntry {
            binding: 2,
            resource: uniforms.clone(),
        },
    ],
})

// After (Variant 1: Bevy-wgpu-resource-type-wrapper-method that generates BindGroupEntry)
// create_bind_group on render_device automatically does the conversion 
render_device.create_bind_group(IntoBindGroupDescriptor(
    "bloom_downsampling_first_bind_group",
    &downsampling_pipeline_res.bind_group_layout,
    &[
        view_target.main_texture_view().binding(0),
        bind_groups.sampler.binding(1),
        uniforms.binding(2),
    ]
))

// After (Variant 2: Bevy-wgpu-resource-type Into conversion for BindingResource)
// Indices are computed in the `Into` conversion
// Not sure ownership would play out here
render_device.create_bind_group(IntoBindGroupDescriptor(
    "bloom_downsampling_first_bind_group",
    &downsampling_pipeline_res.bind_group_layout,
    [
        view_target.main_texture_view().into(),
        bind_groups.sampler.into(),
        uniforms.into(),
    ]
))

[JMS55]

The magic part I felt was the automatic binding indices, not the other parts. The macro is really just shorthand for the full wgpu types, because they're far too verbose for my liking. Look at prepare_generate_environment_map_lights_for_skyboxes_bind_groups from #9414 in generate_from_skybox.rs for an example of how bad this is. I could make the bindings explicit, but I like keeping them automatic because it makes writing code easier when I'm developing. I'm willing to bend on that part though.

The proposal @IceSentry and you brought up aren't bad... One concern is the amount of imports necessary. Perhaps a prelude would help. There's lots of other stuff that would be nice to have in a prelude anyways, now that I think of it. Another is I don't love the extra indentation from the []. It makes formatting much worse in a lot of scenarios, which was something I was trying to avoid :/.

IceSentry also brought up AsBindGroup, I'll take a look at that and see if that's usable for my use cases.

Something else though, is that after this PR I also wanted to make a followup macro for bind group layouts, which are much more complicated. The trait based approach won't work there. The function based approach @IceSentry listed might be ok, but I think we'll end up with a lot of functions to import, as there's a lot of variety necessary. I felt like a macro might be better, but idk.

[cart]

My only concern is the amount of imports necessary.

I don't see this as a concern. The conversion methods would already be in scope (as they would exist on the types being used). The only import required build be the top level IntoBindGroupDescriptor, which is the same amount of imports required for bind_group_descriptor!

[JMS55]

I don't see this as a concern. The conversion methods would already be in scope (as they would exist on the types being used)

Except for the buffer, wouldn't these be traits on wgpu types? Or am I misunderstanding something. It would only be two imports really though, sampler and texture, and I often don't need sampler anyways.

[cart]

Wgpu resources created via Bevy's RenderDevice are wrappers over the actual wgpu types. We have the ability to add methods to them.

[superdump]

I haven’t read the discussion here yet and can’t right now, but from just looking at the diff I wonder if this would make it far more difficult to understand what each of the fields being passed to the macro are meant to be, what type they are meant to be, and what is produced by the macro. I’m not really a fan of using macros here if IDEs don’t know the types of the arguments let alone the names of them. Using code like functions may help instead as then the arguments are clearly typed. texture/sampler/buffer names seem like they will collide easily and would benefit from a more descriptive name of what they do, or a namespace prefix that does that. And then I wonder if you’d basically be back to the same as we have but without some parentheses formatted onto their own lines?

[JMS55]

I tried (Variant 1: Bevy-wgpu-resource-type-wrapper-method that generates BindGroupEntry) in the latest commit. I only bothered to port SSAO and I think bloom. Check out SSAO, I think it's a fairly large downgrade over the macro...

Another option would be to make something more declarative, where you describe the resources and bindings, and then it gives you a function to create a bind group layout, and a function to pass resources MyType::bind(my_texture1, my_sampler, mytexture_2, ..., device) to create a bind group. This would lean even further into macros, though.

I really think macros are fine. Yes they can be magic, but macros are good for making DSLs, which is essentially what this is.

[nicopap]

What about instead of using identifiers in the macro, do a re-naming import in the macro? I found out that rust analyzer is very good at keeping track of names in macros. For example, with the complex nonsense you find in the dsl! macro of my crate, you would think no IDE functionalities would work with it. But in actuality, go-to-definition, doc on hover, rename, error messages, even suggestions (admittedly only when the open parenthesis of the macro is closed) all work fine. This is because all identifiers used in the macro are existing methods, not identifiers matched in macro.

Also looking at the diff, I find the macro is a great help here.

I understand the critical view of macros, but I feel like the heuristic is not nuanced enough.

Here is how you could fix the macro proposed in this pull request to work with IDE tooling:

macro_rules! bind_group_descriptor {
    ($label:expr, $layout:expr, $($entries:expr),* $(,)?) => {{
        use $crate::render_resource::{
            BindingResource::TextureView,
            BindingResource::Sampler,
            BindGroupEntry, BindGroupDescriptor,
        };
        let mut i = 0;
        BindGroupDescriptor {
            label: Some($label),
            layout: $layout,
            entries: &[ $(
                BindGroupEntry {
                    binding: { i += 1; i - 1},
                    resource: $entries,
                },
            )* ],
        }
    }};
}
// usage:
&bind_group_descriptor!(
    "upscaling_bind_group",
    &blit_pipeline.texture_bind_group,
    TextureView(upscaled_texture),
    Sampler(&sampler),
);

Also please pay attention how now the macro is (1) literally a single variant, no need for the hidden macro trick! This is a very easy-to-understand macro_rules definition. (2) when you do a typo, the compiler helpfully recommends a correction.

See the playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=150930545e7b2a75d08aff5b2dc22b7a

Of course, we need great doc for macros. It's not trivial to understand what a macro does and how you are supposed to use it. Again, the dsl! macro in cuicui_dsl is a great example. It has 418 lines of comment for 85 lines of macro!

Would people consider a macro if:

1. It's well documented with example, exhaustive list of the syntax and code showing the expanded version
2. Works with IDE tooling, thanks to using import renames

I think this addresses all the complaints that were raised in this discussion. So please substantiate your objections if it's not convincing enough still.

Edit: Maybe it's worthwhile to re-use the variant names like TextureView and Sampler instead of renaming them to texture and sampler. It would make the relation even clearer.

[cart]

One more non-macro proposal (I pushed a commit):

let bind_group = render_context.create_bind_group(
    "fxaa_bind_group",
    &fxaa_pipeline.texture_bind_group,
    [source.binding(), sampler.binding()],
);

This also works:

let bind_group = render_context.create_bind_group(
    "fxaa_bind_group",
    &fxaa_pipeline.texture_bind_group,
    [source.binding_index(0), sampler.binding_index(1)],
);

[JMS55]

Haven't reviewed what cart just tried, but a new thought occurred to me:

You can bind either textures, samplers, or buffers. They each have a unique type you can bind from. Why do we even need to specify the types by hand again at all? That's not really important , you can just check the type of the variable you're trying to bind.

EDIT: That's pretty much what @cart did actually. It's just an unfortunate side effect of using postfix method chaining that rustfmt tends to format it in an ugly way, vs texture() :/. If we go with a macro, and automatic indices, then we could remove .binding() entirely and have the macro call it.

[nicopap]

@JMS55 That's a nice insight. We could trivially implement From<&'a [Sampler|TextureView|SamplerArray|etc…]> for BindingResource and use it in the macro (or non-macro based approach) to avoid having to import any of the BindingResource variants. It would look as follow:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4f5412dbf05951a444314d52f2787257

I think what you suggest here @cart is good enough and gets rid of 99% of the boilerplate. I would just drop the implicit binding index option though, it doesn't add much value.

I still think that either the Describe struct I define in the link above or the macro is better, because it plays well with rustfmt, but at this point it's purely esthetic.

[IceSentry]

I really like the latest proposal from @cart but there's one issue. It only works with RenderContext. If you are creating a bind group in a system with the RenderDevice directly you won't have access to this api

Actually nevermind, we also control the RenderDevice api

[JMS55]

Going to close this. I think we're better served by trying to automate resource/bind group creation, rather than making it easier to do manually.

[robtfm]

i still think this is worthwhile. there's no value to the boilerplate and we can reduce it down to providing just the minimum required info, which i guess will always be needed regardless of the abstraction.

one more suggestion over here, looks like :

let bind_group = render_context.render_device().create_bind_group(&BindGroupDescriptor {
    label: Some("fxaa_bind_group"),
    layout: &fxaa_pipeline.texture_bind_group,
    entries: &BindGroupEntries::sequential((source, &sampler)),
});

no user facing macros, leaves the current apis unchanged, just adds a helper struct. type-based so no allocation, no noticeable impact on compile time.

edit: diff

[IceSentry]

Yeah, I've been using essentially the version cart suggested in my own crates and it's been really nice to not have half my files be bind group stuff. It makes it much nicer.

That BindGroupEntries impl is pretty neat too. And the diff using it on your branch is really nice to look at.

[IceSentry]

For anyone that was following this PR but missed an update, here's a PR with an updated version #9694