From 1caa64d94820d7148fa4b9a04e5436d17098ab1b Mon Sep 17 00:00:00 2001 From: charlotte Date: Sun, 25 Aug 2024 13:16:34 -0700 Subject: [PATCH] Refactor `AsBindGroup` to use a associated `SystemParam`. (#14909) # 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. --- crates/bevy_pbr/src/extended_material.rs | 18 +++++---------- crates/bevy_pbr/src/material.rs | 13 +++-------- .../bevy_render/macros/src/as_bind_group.rs | 11 ++++++--- .../src/render_resource/bind_group.rs | 23 ++++++++++--------- crates/bevy_sprite/src/mesh2d/material.rs | 17 +++----------- .../src/render/ui_material_pipeline.rs | 13 ++++------- examples/shader/texture_binding_array.rs | 14 ++++++----- 7 files changed, 44 insertions(+), 65 deletions(-) diff --git a/crates/bevy_pbr/src/extended_material.rs b/crates/bevy_pbr/src/extended_material.rs index ff21314f36..81b3c6ebbb 100644 --- a/crates/bevy_pbr/src/extended_material.rs +++ b/crates/bevy_pbr/src/extended_material.rs @@ -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}; @@ -147,26 +146,21 @@ impl_type_path!((in bevy_pbr::extended_material) ExtendedMaterial AsBindGroup for ExtendedMaterial { type Data = (::Data, ::Data); + type Param = (::Param, ::Param); fn unprepared_bind_group( &self, layout: &BindGroupLayout, render_device: &RenderDevice, - images: &RenderAssets, - fallback_image: &FallbackImage, + (base_param, extended_param): &mut SystemParamItem<'_, '_, Self::Param>, ) -> Result, 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); diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index a000b19a2c..d5b601d651 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -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; @@ -908,22 +907,16 @@ impl RenderAsset for PreparedMaterial { type Param = ( SRes, - SRes>, - SRes, SRes>, SRes, + M::Param, ); fn prepare_asset( material: Self::SourceAsset, - (render_device, images, fallback_image, pipeline, default_opaque_render_method): &mut SystemParamItem, + (render_device, pipeline, default_opaque_render_method, ref mut material_param): &mut SystemParamItem, ) -> Result> { - 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, diff --git a/crates/bevy_render/macros/src/as_bind_group.rs b/crates/bevy_render/macros/src/as_bind_group.rs index 8fa69d29f1..81fb920f3c 100644 --- a/crates/bevy_render/macros/src/as_bind_group.rs +++ b/crates/bevy_render/macros/src/as_bind_group.rs @@ -42,6 +42,7 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result { 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 = Vec::new(); let mut binding_impls = Vec::new(); @@ -62,7 +63,7 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result { 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, @@ -523,6 +524,11 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result { 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) } @@ -531,8 +537,7 @@ pub fn derive_as_bind_group(ast: syn::DeriveInput) -> Result { &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, #render_path::render_resource::AsBindGroupError> { let bindings = vec![#(#binding_impls,)*]; diff --git a/crates/bevy_render/src/render_resource/bind_group.rs b/crates/bevy_render/src/render_resource/bind_group.rs index 6371678e48..4380890e17 100644 --- a/crates/bevy_render/src/render_resource/bind_group.rs +++ b/crates/bevy_render/src/render_resource/bind_group.rs @@ -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; @@ -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`] and [`FallbackImage`]. If a type has a [`Handle`](bevy_asset::Handle), +/// such as [`RenderAssets`] and [`crate::texture::FallbackImage`]. If a type has a [`Handle`](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 @@ -115,7 +116,7 @@ impl Deref for BindGroup { /// * This field's [`Handle`](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>>`]. In practice, /// most fields should be a [`Handle`](bevy_asset::Handle) or [`Option>`]. If the value of an [`Option>`] 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 | @@ -130,7 +131,7 @@ impl Deref for BindGroup { /// * This field's [`Handle`](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>>`]. In practice, /// most fields should be a [`Handle`](bevy_asset::Handle) or [`Option>`]. If the value of an [`Option>`] is -/// [`None`], the [`FallbackImage`] resource will be used instead. +/// [`None`], the [`crate::texture::FallbackImage`] resource will be used instead. /// /// | Arguments | Values | Default | /// |------------------------|--------------------------------------------------------------------------------------------|---------------| @@ -143,7 +144,7 @@ impl Deref for BindGroup { /// * This field's [`Handle`](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>>`]. In practice, /// most fields should be a [`Handle`](bevy_asset::Handle) or [`Option>`]. If the value of an [`Option>`] 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 | @@ -187,7 +188,7 @@ impl Deref for BindGroup { /// color_texture: Option>, /// } /// ``` -/// 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: @@ -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 @@ -294,11 +297,10 @@ pub trait AsBindGroup { &self, layout: &BindGroupLayout, render_device: &RenderDevice, - images: &RenderAssets, - fallback_image: &FallbackImage, + param: &mut SystemParamItem<'_, '_, Self::Param>, ) -> Result, 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() @@ -325,8 +327,7 @@ pub trait AsBindGroup { &self, layout: &BindGroupLayout, render_device: &RenderDevice, - images: &RenderAssets, - fallback_image: &FallbackImage, + param: &mut SystemParamItem<'_, '_, Self::Param>, ) -> Result, AsBindGroupError>; /// Creates the bind group layout matching all bind groups returned by [`AsBindGroup::as_bind_group`] diff --git a/crates/bevy_sprite/src/mesh2d/material.rs b/crates/bevy_sprite/src/mesh2d/material.rs index 849fd980fb..17743f5aab 100644 --- a/crates/bevy_sprite/src/mesh2d/material.rs +++ b/crates/bevy_sprite/src/mesh2d/material.rs @@ -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, }; @@ -581,23 +580,13 @@ impl PreparedMaterial2d { impl RenderAsset for PreparedMaterial2d { type SourceAsset = M; - type Param = ( - SRes, - SRes>, - SRes, - SRes>, - ); + type Param = (SRes, SRes>, M::Param); fn prepare_asset( material: Self::SourceAsset, - (render_device, images, fallback_image, pipeline): &mut SystemParamItem, + (render_device, pipeline, material_param): &mut SystemParamItem, ) -> Result> { - 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())); diff --git a/crates/bevy_ui/src/render/ui_material_pipeline.rs b/crates/bevy_ui/src/render/ui_material_pipeline.rs index 2eaa2f583b..bcdccf15fc 100644 --- a/crates/bevy_ui/src/render/ui_material_pipeline.rs +++ b/crates/bevy_ui/src/render/ui_material_pipeline.rs @@ -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, }; @@ -604,18 +604,13 @@ pub struct PreparedUiMaterial { impl RenderAsset for PreparedUiMaterial { type SourceAsset = M; - type Param = ( - SRes, - SRes>, - SRes, - SRes>, - ); + type Param = (SRes, SRes>, M::Param); fn prepare_asset( material: Self::SourceAsset, - (render_device, images, fallback_image, pipeline): &mut SystemParamItem, + (render_device, pipeline, ref mut material_param): &mut SystemParamItem, ) -> Result> { - 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, diff --git a/examples/shader/texture_binding_array.rs b/examples/shader/texture_binding_array.rs index 0698c488bf..53e774df04 100644 --- a/examples/shader/texture_binding_array.rs +++ b/examples/shader/texture_binding_array.rs @@ -1,6 +1,8 @@ //! A shader that binds several textures onto one //! `binding_array>` shader binding slot and sample non-uniformly. +use bevy::ecs::system::lifetimeless::SRes; +use bevy::ecs::system::SystemParamItem; use bevy::{ prelude::*, reflect::TypePath, @@ -97,12 +99,13 @@ struct BindlessMaterial { impl AsBindGroup for BindlessMaterial { type Data = (); + type Param = (SRes>, SRes); + fn as_bind_group( &self, layout: &BindGroupLayout, render_device: &RenderDevice, - image_assets: &RenderAssets, - fallback_image: &FallbackImage, + (image_assets, fallback_image): &mut SystemParamItem<'_, '_, Self::Param>, ) -> Result, AsBindGroupError> { // retrieve the render resources from handles let mut images = vec![]; @@ -140,10 +143,9 @@ impl AsBindGroup for BindlessMaterial { fn unprepared_bind_group( &self, - _: &BindGroupLayout, - _: &RenderDevice, - _: &RenderAssets, - _: &FallbackImage, + _layout: &BindGroupLayout, + _render_device: &RenderDevice, + _param: &mut SystemParamItem<'_, '_, Self::Param>, ) -> Result, AsBindGroupError> { // we implement as_bind_group directly because panic!("bindless texture arrays can't be owned")