Skip to content

Commit

Permalink
Refactor AsBindGroup to use a associated SystemParam. (#14909)
Browse files Browse the repository at this point in the history
# Objective

Adding more features to `AsBindGroup` proc macro means making the trait
arguments uglier. Downstream implementors of the trait without the proc
macro might want to do different things than our default arguments.

## Solution

Make `AsBindGroup` take an associated `Param` type.

## Migration Guide

`AsBindGroup` now allows the user to specify a `SystemParam` to be used
for creating bind groups.
  • Loading branch information
tychedelia authored Aug 25, 2024
1 parent 3892adc commit 1caa64d
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 65 deletions.
18 changes: 6 additions & 12 deletions crates/bevy_pbr/src/extended_material.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
use bevy_asset::{Asset, Handle};
use bevy_ecs::system::SystemParamItem;
use bevy_reflect::{impl_type_path, Reflect};
use bevy_render::{
mesh::MeshVertexBufferLayoutRef,
render_asset::RenderAssets,
render_resource::{
AsBindGroup, AsBindGroupError, BindGroupLayout, RenderPipelineDescriptor, Shader,
ShaderRef, SpecializedMeshPipelineError, UnpreparedBindGroup,
},
renderer::RenderDevice,
texture::{FallbackImage, GpuImage},
};

use crate::{Material, MaterialPipeline, MaterialPipelineKey, MeshPipeline, MeshPipelineKey};
Expand Down Expand Up @@ -147,26 +146,21 @@ impl_type_path!((in bevy_pbr::extended_material) ExtendedMaterial<B: Material, E

impl<B: Material, E: MaterialExtension> AsBindGroup for ExtendedMaterial<B, E> {
type Data = (<B as AsBindGroup>::Data, <E as AsBindGroup>::Data);
type Param = (<B as AsBindGroup>::Param, <E as AsBindGroup>::Param);

fn unprepared_bind_group(
&self,
layout: &BindGroupLayout,
render_device: &RenderDevice,
images: &RenderAssets<GpuImage>,
fallback_image: &FallbackImage,
(base_param, extended_param): &mut SystemParamItem<'_, '_, Self::Param>,
) -> Result<UnpreparedBindGroup<Self::Data>, AsBindGroupError> {
// add together the bindings of the base material and the user material
let UnpreparedBindGroup {
mut bindings,
data: base_data,
} = B::unprepared_bind_group(&self.base, layout, render_device, images, fallback_image)?;
let extended_bindgroup = E::unprepared_bind_group(
&self.extension,
layout,
render_device,
images,
fallback_image,
)?;
} = B::unprepared_bind_group(&self.base, layout, render_device, base_param)?;
let extended_bindgroup =
E::unprepared_bind_group(&self.extension, layout, render_device, extended_param)?;

bindings.extend(extended_bindgroup.bindings);

Expand Down
13 changes: 3 additions & 10 deletions crates/bevy_pbr/src/material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ use bevy_render::{
render_phase::*,
render_resource::*,
renderer::RenderDevice,
texture::FallbackImage,
view::{ExtractedView, Msaa, RenderVisibilityRanges, VisibleEntities, WithMesh},
};
use bevy_utils::tracing::error;
Expand Down Expand Up @@ -908,22 +907,16 @@ impl<M: Material> RenderAsset for PreparedMaterial<M> {

type Param = (
SRes<RenderDevice>,
SRes<RenderAssets<GpuImage>>,
SRes<FallbackImage>,
SRes<MaterialPipeline<M>>,
SRes<DefaultOpaqueRendererMethod>,
M::Param,
);

fn prepare_asset(
material: Self::SourceAsset,
(render_device, images, fallback_image, pipeline, default_opaque_render_method): &mut SystemParamItem<Self::Param>,
(render_device, pipeline, default_opaque_render_method, ref mut material_param): &mut SystemParamItem<Self::Param>,
) -> Result<Self, PrepareAssetError<Self::SourceAsset>> {
match material.as_bind_group(
&pipeline.material_layout,
render_device,
images,
fallback_image,
) {
match material.as_bind_group(&pipeline.material_layout, render_device, material_param) {
Ok(prepared) => {
let method = match material.opaque_render_method() {
OpaqueRendererMethod::Forward => OpaqueRendererMethod::Forward,
Expand Down
11 changes: 8 additions & 3 deletions crates/bevy_render/macros/src/as_bind_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result<TokenStream> {
let manifest = BevyManifest::default();
let render_path = manifest.get_path("bevy_render");
let asset_path = manifest.get_path("bevy_asset");
let ecs_path = manifest.get_path("bevy_ecs");

let mut binding_states: Vec<BindingState> = Vec::new();
let mut binding_impls = Vec::new();
Expand All @@ -62,7 +63,7 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result<TokenStream> {
binding_impls.push(quote! {{
use #render_path::render_resource::AsBindGroupShaderType;
let mut buffer = #render_path::render_resource::encase::UniformBuffer::new(Vec::new());
let converted: #converted_shader_type = self.as_bind_group_shader_type(images);
let converted: #converted_shader_type = self.as_bind_group_shader_type(&images);
buffer.write(&converted).unwrap();
(
#binding_index,
Expand Down Expand Up @@ -523,6 +524,11 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result<TokenStream> {
impl #impl_generics #render_path::render_resource::AsBindGroup for #struct_name #ty_generics #where_clause {
type Data = #prepared_data;

type Param = (
#ecs_path::system::lifetimeless::SRes<#render_path::render_asset::RenderAssets<#render_path::texture::GpuImage>>,
#ecs_path::system::lifetimeless::SRes<#render_path::texture::FallbackImage>,
);

fn label() -> Option<&'static str> {
Some(#struct_name_literal)
}
Expand All @@ -531,8 +537,7 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result<TokenStream> {
&self,
layout: &#render_path::render_resource::BindGroupLayout,
render_device: &#render_path::renderer::RenderDevice,
images: &#render_path::render_asset::RenderAssets<#render_path::texture::GpuImage>,
fallback_image: &#render_path::texture::FallbackImage,
(images, fallback_image): &mut #ecs_path::system::SystemParamItem<'_, '_, Self::Param>,
) -> Result<#render_path::render_resource::UnpreparedBindGroup<Self::Data>, #render_path::render_resource::AsBindGroupError> {
let bindings = vec![#(#binding_impls,)*];

Expand Down
23 changes: 12 additions & 11 deletions crates/bevy_render/src/render_resource/bind_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ use crate::{
render_asset::RenderAssets,
render_resource::{resource_macros::*, BindGroupLayout, Buffer, Sampler, TextureView},
renderer::RenderDevice,
texture::{FallbackImage, GpuImage},
texture::GpuImage,
};
use bevy_ecs::system::{SystemParam, SystemParamItem};
pub use bevy_render_macros::AsBindGroup;
use encase::ShaderType;
use std::ops::Deref;
Expand Down Expand Up @@ -57,7 +58,7 @@ impl Deref for BindGroup {
///
/// This is an opinionated trait that is intended to make it easy to generically
/// convert a type into a [`BindGroup`]. It provides access to specific render resources,
/// such as [`RenderAssets<GpuImage>`] and [`FallbackImage`]. If a type has a [`Handle<Image>`](bevy_asset::Handle),
/// such as [`RenderAssets<GpuImage>`] and [`crate::texture::FallbackImage`]. If a type has a [`Handle<Image>`](bevy_asset::Handle),
/// these can be used to retrieve the corresponding [`Texture`](crate::render_resource::Texture) resource.
///
/// [`AsBindGroup::as_bind_group`] is intended to be called once, then the result cached somewhere. It is generally
Expand Down Expand Up @@ -115,7 +116,7 @@ impl Deref for BindGroup {
/// * This field's [`Handle<Image>`](bevy_asset::Handle) will be used to look up the matching [`Texture`](crate::render_resource::Texture)
/// GPU resource, which will be bound as a texture in shaders. The field will be assumed to implement [`Into<Option<Handle<Image>>>`]. In practice,
/// most fields should be a [`Handle<Image>`](bevy_asset::Handle) or [`Option<Handle<Image>>`]. If the value of an [`Option<Handle<Image>>`] is
/// [`None`], the [`FallbackImage`] resource will be used instead. This attribute can be used in conjunction with a `sampler` binding attribute
/// [`None`], the [`crate::texture::FallbackImage`] resource will be used instead. This attribute can be used in conjunction with a `sampler` binding attribute
/// (with a different binding index) if a binding of the sampler for the [`Image`](crate::texture::Image) is also required.
///
/// | Arguments | Values | Default |
Expand All @@ -130,7 +131,7 @@ impl Deref for BindGroup {
/// * This field's [`Handle<Image>`](bevy_asset::Handle) will be used to look up the matching [`Texture`](crate::render_resource::Texture)
/// GPU resource, which will be bound as a storage texture in shaders. The field will be assumed to implement [`Into<Option<Handle<Image>>>`]. In practice,
/// most fields should be a [`Handle<Image>`](bevy_asset::Handle) or [`Option<Handle<Image>>`]. If the value of an [`Option<Handle<Image>>`] is
/// [`None`], the [`FallbackImage`] resource will be used instead.
/// [`None`], the [`crate::texture::FallbackImage`] resource will be used instead.
///
/// | Arguments | Values | Default |
/// |------------------------|--------------------------------------------------------------------------------------------|---------------|
Expand All @@ -143,7 +144,7 @@ impl Deref for BindGroup {
/// * This field's [`Handle<Image>`](bevy_asset::Handle) will be used to look up the matching [`Sampler`] GPU
/// resource, which will be bound as a sampler in shaders. The field will be assumed to implement [`Into<Option<Handle<Image>>>`]. In practice,
/// most fields should be a [`Handle<Image>`](bevy_asset::Handle) or [`Option<Handle<Image>>`]. If the value of an [`Option<Handle<Image>>`] is
/// [`None`], the [`FallbackImage`] resource will be used instead. This attribute can be used in conjunction with a `texture` binding attribute
/// [`None`], the [`crate::texture::FallbackImage`] resource will be used instead. This attribute can be used in conjunction with a `texture` binding attribute
/// (with a different binding index) if a binding of the texture for the [`Image`](crate::texture::Image) is also required.
///
/// | Arguments | Values | Default |
Expand Down Expand Up @@ -187,7 +188,7 @@ impl Deref for BindGroup {
/// color_texture: Option<Handle<Image>>,
/// }
/// ```
/// This is useful if you want a texture to be optional. When the value is [`None`], the [`FallbackImage`] will be used for the binding instead, which defaults
/// This is useful if you want a texture to be optional. When the value is [`None`], the [`crate::texture::FallbackImage`] will be used for the binding instead, which defaults
/// to "pure white".
///
/// Field uniforms with the same index will be combined into a single binding:
Expand Down Expand Up @@ -284,6 +285,8 @@ pub trait AsBindGroup {
/// Data that will be stored alongside the "prepared" bind group.
type Data: Send + Sync;

type Param: SystemParam + 'static;

/// label
fn label() -> Option<&'static str> {
None
Expand All @@ -294,11 +297,10 @@ pub trait AsBindGroup {
&self,
layout: &BindGroupLayout,
render_device: &RenderDevice,
images: &RenderAssets<GpuImage>,
fallback_image: &FallbackImage,
param: &mut SystemParamItem<'_, '_, Self::Param>,
) -> Result<PreparedBindGroup<Self::Data>, AsBindGroupError> {
let UnpreparedBindGroup { bindings, data } =
Self::unprepared_bind_group(self, layout, render_device, images, fallback_image)?;
Self::unprepared_bind_group(self, layout, render_device, param)?;

let entries = bindings
.iter()
Expand All @@ -325,8 +327,7 @@ pub trait AsBindGroup {
&self,
layout: &BindGroupLayout,
render_device: &RenderDevice,
images: &RenderAssets<GpuImage>,
fallback_image: &FallbackImage,
param: &mut SystemParamItem<'_, '_, Self::Param>,
) -> Result<UnpreparedBindGroup<Self::Data>, AsBindGroupError>;

/// Creates the bind group layout matching all bind groups returned by [`AsBindGroup::as_bind_group`]
Expand Down
17 changes: 3 additions & 14 deletions crates/bevy_sprite/src/mesh2d/material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ use bevy_render::{
SpecializedMeshPipeline, SpecializedMeshPipelineError, SpecializedMeshPipelines,
},
renderer::RenderDevice,
texture::{FallbackImage, GpuImage},
view::{ExtractedView, InheritedVisibility, Msaa, ViewVisibility, Visibility, VisibleEntities},
Extract, ExtractSchedule, Render, RenderApp, RenderSet,
};
Expand Down Expand Up @@ -581,23 +580,13 @@ impl<T: Material2d> PreparedMaterial2d<T> {
impl<M: Material2d> RenderAsset for PreparedMaterial2d<M> {
type SourceAsset = M;

type Param = (
SRes<RenderDevice>,
SRes<RenderAssets<GpuImage>>,
SRes<FallbackImage>,
SRes<Material2dPipeline<M>>,
);
type Param = (SRes<RenderDevice>, SRes<Material2dPipeline<M>>, M::Param);

fn prepare_asset(
material: Self::SourceAsset,
(render_device, images, fallback_image, pipeline): &mut SystemParamItem<Self::Param>,
(render_device, pipeline, material_param): &mut SystemParamItem<Self::Param>,
) -> Result<Self, PrepareAssetError<Self::SourceAsset>> {
match material.as_bind_group(
&pipeline.material2d_layout,
render_device,
images,
fallback_image,
) {
match material.as_bind_group(&pipeline.material2d_layout, render_device, material_param) {
Ok(prepared) => {
let mut mesh_pipeline_key_bits = Mesh2dPipelineKey::empty();
mesh_pipeline_key_bits.insert(alpha_mode_pipeline_key(material.alpha_mode()));
Expand Down
13 changes: 4 additions & 9 deletions crates/bevy_ui/src/render/ui_material_pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use bevy_render::{
render_phase::*,
render_resource::{binding_types::uniform_buffer, *},
renderer::{RenderDevice, RenderQueue},
texture::{BevyDefault, FallbackImage, GpuImage},
texture::BevyDefault,
view::*,
Extract, ExtractSchedule, Render, RenderSet,
};
Expand Down Expand Up @@ -604,18 +604,13 @@ pub struct PreparedUiMaterial<T: UiMaterial> {
impl<M: UiMaterial> RenderAsset for PreparedUiMaterial<M> {
type SourceAsset = M;

type Param = (
SRes<RenderDevice>,
SRes<RenderAssets<GpuImage>>,
SRes<FallbackImage>,
SRes<UiMaterialPipeline<M>>,
);
type Param = (SRes<RenderDevice>, SRes<UiMaterialPipeline<M>>, M::Param);

fn prepare_asset(
material: Self::SourceAsset,
(render_device, images, fallback_image, pipeline): &mut SystemParamItem<Self::Param>,
(render_device, pipeline, ref mut material_param): &mut SystemParamItem<Self::Param>,
) -> Result<Self, PrepareAssetError<Self::SourceAsset>> {
match material.as_bind_group(&pipeline.ui_layout, render_device, images, fallback_image) {
match material.as_bind_group(&pipeline.ui_layout, render_device, material_param) {
Ok(prepared) => Ok(PreparedUiMaterial {
bindings: prepared.bindings,
bind_group: prepared.bind_group,
Expand Down
14 changes: 8 additions & 6 deletions examples/shader/texture_binding_array.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! A shader that binds several textures onto one
//! `binding_array<texture<f32>>` shader binding slot and sample non-uniformly.
use bevy::ecs::system::lifetimeless::SRes;
use bevy::ecs::system::SystemParamItem;
use bevy::{
prelude::*,
reflect::TypePath,
Expand Down Expand Up @@ -97,12 +99,13 @@ struct BindlessMaterial {
impl AsBindGroup for BindlessMaterial {
type Data = ();

type Param = (SRes<RenderAssets<GpuImage>>, SRes<FallbackImage>);

fn as_bind_group(
&self,
layout: &BindGroupLayout,
render_device: &RenderDevice,
image_assets: &RenderAssets<GpuImage>,
fallback_image: &FallbackImage,
(image_assets, fallback_image): &mut SystemParamItem<'_, '_, Self::Param>,
) -> Result<PreparedBindGroup<Self::Data>, AsBindGroupError> {
// retrieve the render resources from handles
let mut images = vec![];
Expand Down Expand Up @@ -140,10 +143,9 @@ impl AsBindGroup for BindlessMaterial {

fn unprepared_bind_group(
&self,
_: &BindGroupLayout,
_: &RenderDevice,
_: &RenderAssets<GpuImage>,
_: &FallbackImage,
_layout: &BindGroupLayout,
_render_device: &RenderDevice,
_param: &mut SystemParamItem<'_, '_, Self::Param>,
) -> Result<UnpreparedBindGroup<Self::Data>, AsBindGroupError> {
// we implement as_bind_group directly because
panic!("bindless texture arrays can't be owned")
Expand Down

0 comments on commit 1caa64d

Please sign in to comment.