From 7d02561f41ebc5325294cb7c8ad68027ec84b535 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sun, 30 Jan 2022 22:38:17 +0000 Subject: [PATCH 1/6] Tidy up the `SystemParam` derive's requirements --- crates/bevy_ecs/macros/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 679df90154a07..f2f0cddf38514 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -382,7 +382,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { #[doc(hidden)] #fetch_struct_visibility struct #fetch_struct_name { state: TSystemParamState, - marker: std::marker::PhantomData<(#punctuated_generic_idents)> + marker: std::marker::PhantomData(#punctuated_generic_idents)> } unsafe impl #path::system::SystemParamState for #fetch_struct_name { From e8b7faa8d8c11ae2ca14d09ce8d4937bbb3e7ed8 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sun, 30 Jan 2022 22:41:51 +0000 Subject: [PATCH 2/6] Add the StaticSystemParam trick Add missing `DerefMut` impl and `inner` fn --- crates/bevy_ecs/src/system/system_param.rs | 83 ++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f242892b873d3..3f90ffc6714b6 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1254,6 +1254,89 @@ pub mod lifetimeless { pub type SCommands = crate::system::Commands<'static, 'static>; } +/// A helper for using system parameters in generic contexts +/// +/// This type is a system parameter which is statically proven to have +/// `Self::Fetch::Item == Self` (ignoring lifetimes for brevity) +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// use bevy_ecs::system::{SystemParam, StaticSystemParam}; +/// +/// fn do_thing_generically(t: StaticSystemParam) {} +/// +/// fn test_always_is(){ +/// do_thing_generically::.system(); +/// } +/// ``` +pub struct StaticSystemParam<'w, 's, P: SystemParam>(SystemParamItem<'w, 's, P>); + +impl<'w, 's, P: SystemParam> Deref for StaticSystemParam<'w, 's, P> { + type Target = SystemParamItem<'w, 's, P>; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl<'w, 's, P: SystemParam> DerefMut for StaticSystemParam<'w, 's, P> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl<'w, 's, P: SystemParam> StaticSystemParam<'w, 's, P> { + pub fn inner(self) -> SystemParamItem<'w, 's, P> { + self.0 + } +} + +pub struct StaticSystemParamState(S, PhantomData P>); + +impl<'world, 'state, P: SystemParam + 'static> SystemParam + for StaticSystemParam<'world, 'state, P> +{ + type Fetch = StaticSystemParamState; +} + +impl<'world, 'state, S: SystemParamFetch<'world, 'state>, P: SystemParam + 'static> + SystemParamFetch<'world, 'state> for StaticSystemParamState +where + P: SystemParam, +{ + type Item = StaticSystemParam<'world, 'state, P>; + + unsafe fn get_param( + state: &'state mut Self, + system_meta: &SystemMeta, + world: &'world World, + change_tick: u32, + ) -> Self::Item { + StaticSystemParam(S::get_param(&mut state.0, system_meta, world, change_tick)) + } +} + +unsafe impl<'w, 's, S: SystemParamState, P: SystemParam + 'static> SystemParamState + for StaticSystemParamState +{ + type Config = S::Config; + + fn init(world: &mut World, system_meta: &mut SystemMeta, config: Self::Config) -> Self { + Self(S::init(world, system_meta, config), PhantomData) + } + + fn default_config() -> Self::Config { + S::default_config() + } + fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) { + self.0.new_archetype(archetype, system_meta) + } + + fn apply(&mut self, world: &mut World) { + self.0.apply(world) + } +} + #[cfg(test)] mod tests { use super::SystemParam; From 898da83640522ef6d0b828f515cd5dbeaabae503 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sun, 30 Jan 2022 22:53:09 +0000 Subject: [PATCH 3/6] Migrate to `StaticSystemParam` Remove `RunSystem` --- crates/bevy_ecs/src/system/function_system.rs | 79 +------------------ crates/bevy_render/src/render_asset.rs | 67 +++++++--------- crates/bevy_render/src/render_component.rs | 36 +++------ 3 files changed, 41 insertions(+), 141 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 233b7ed6e2434..d66689d956d6b 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -4,7 +4,7 @@ use crate::{ query::{Access, FilteredAccessSet}, system::{ check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch, - SystemParamItem, SystemParamState, + SystemParamState, }, world::{World, WorldId}, }; @@ -46,11 +46,6 @@ impl SystemMeta { pub fn set_non_send(&mut self) { self.is_send = false; } - - #[inline] - pub(crate) fn check_change_tick(&mut self, change_tick: u32) { - check_system_change_tick(&mut self.last_change_tick, change_tick, self.name.as_ref()); - } } // TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference @@ -194,10 +189,6 @@ impl SystemState { self.world_id == world.id() } - pub(crate) fn new_archetype(&mut self, archetype: &Archetype) { - self.param_state.new_archetype(archetype, &mut self.meta); - } - fn validate_world_and_update_archetypes(&mut self, world: &World) { assert!(self.matches_world(world), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with."); let archetypes = world.archetypes(); @@ -236,74 +227,6 @@ impl SystemState { } } -/// A trait for defining systems with a [`SystemParam`] associated type. -/// -/// This facilitates the creation of systems that are generic over some trait -/// and that use that trait's associated types as `SystemParam`s. -pub trait RunSystem: Send + Sync + 'static { - /// The `SystemParam` type passed to the system when it runs. - type Param: SystemParam; - - /// Runs the system. - fn run(param: SystemParamItem); - - /// Creates a concrete instance of the system for the specified `World`. - fn system(world: &mut World) -> ParamSystem { - ParamSystem { - run: Self::run, - state: SystemState::new(world), - } - } -} - -pub struct ParamSystem { - state: SystemState

, - run: fn(SystemParamItem

), -} - -impl System for ParamSystem

{ - type In = (); - - type Out = (); - - fn name(&self) -> Cow<'static, str> { - self.state.meta().name.clone() - } - - fn new_archetype(&mut self, archetype: &Archetype) { - self.state.new_archetype(archetype); - } - - fn component_access(&self) -> &Access { - self.state.meta().component_access_set.combined_access() - } - - fn archetype_component_access(&self) -> &Access { - &self.state.meta().archetype_component_access - } - - fn is_send(&self) -> bool { - self.state.meta().is_send() - } - - unsafe fn run_unsafe(&mut self, _input: Self::In, world: &World) -> Self::Out { - let param = self.state.get_unchecked_manual(world); - (self.run)(param); - } - - fn apply_buffers(&mut self, world: &mut World) { - self.state.apply(world); - } - - fn initialize(&mut self, _world: &mut World) { - // already initialized by nature of the SystemState being constructed - } - - fn check_change_tick(&mut self, change_tick: u32) { - self.state.meta.check_change_tick(change_tick); - } -} - /// Conversion trait to turn something into a [`System`]. /// /// Use this to get a system from a function. Also note that every system implements this trait as diff --git a/crates/bevy_render/src/render_asset.rs b/crates/bevy_render/src/render_asset.rs index 12861ae6f4e64..6741b1af745d1 100644 --- a/crates/bevy_render/src/render_asset.rs +++ b/crates/bevy_render/src/render_asset.rs @@ -3,7 +3,7 @@ use bevy_app::{App, Plugin}; use bevy_asset::{Asset, AssetEvent, Assets, Handle}; use bevy_ecs::{ prelude::*, - system::{lifetimeless::*, RunSystem, SystemParam, SystemParamItem}, + system::{StaticSystemParam, SystemParam, SystemParamItem}, }; use bevy_utils::{HashMap, HashSet}; use std::marker::PhantomData; @@ -55,13 +55,12 @@ impl Default for RenderAssetPlugin { impl Plugin for RenderAssetPlugin { fn build(&self, app: &mut App) { if let Ok(render_app) = app.get_sub_app_mut(RenderApp) { - let prepare_asset_system = PrepareAssetSystem::::system(&mut render_app.world); render_app .init_resource::>() .init_resource::>() .init_resource::>() .add_system_to_stage(RenderStage::Extract, extract_render_asset::) - .add_system_to_stage(RenderStage::Prepare, prepare_asset_system); + .add_system_to_stage(RenderStage::Prepare, prepare_assets::); } } } @@ -122,14 +121,6 @@ fn extract_render_asset( }); } -/// Specifies all ECS data required by [`PrepareAssetSystem`]. -pub type RenderAssetParams = ( - SResMut>, - SResMut>, - SResMut>, - ::Param, -); - // TODO: consider storing inside system? /// All assets that should be prepared next frame. pub struct PrepareNextFrameAssets { @@ -146,38 +137,36 @@ impl Default for PrepareNextFrameAssets { /// This system prepares all assets of the corresponding [`RenderAsset`] type /// which where extracted this frame for the GPU. -pub struct PrepareAssetSystem(PhantomData); - -impl RunSystem for PrepareAssetSystem { - type Param = RenderAssetParams; - - fn run( - (mut extracted_assets, mut render_assets, mut prepare_next_frame, mut param): SystemParamItem, - ) { - let mut queued_assets = std::mem::take(&mut prepare_next_frame.assets); - for (handle, extracted_asset) in queued_assets.drain(..) { - match R::prepare_asset(extracted_asset, &mut param) { - Ok(prepared_asset) => { - render_assets.insert(handle, prepared_asset); - } - Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => { - prepare_next_frame.assets.push((handle, extracted_asset)); - } +fn prepare_assets( + mut extracted_assets: ResMut>, + mut render_assets: ResMut>, + mut prepare_next_frame: ResMut>, + param: StaticSystemParam<::Param>, +) { + let mut param = param.inner(); + let mut queued_assets = std::mem::take(&mut prepare_next_frame.assets); + for (handle, extracted_asset) in queued_assets.drain(..) { + match R::prepare_asset(extracted_asset, &mut param) { + Ok(prepared_asset) => { + render_assets.insert(handle, prepared_asset); + } + Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => { + prepare_next_frame.assets.push((handle, extracted_asset)); } } + } - for removed in std::mem::take(&mut extracted_assets.removed) { - render_assets.remove(&removed); - } + for removed in std::mem::take(&mut extracted_assets.removed) { + render_assets.remove(&removed); + } - for (handle, extracted_asset) in std::mem::take(&mut extracted_assets.extracted) { - match R::prepare_asset(extracted_asset, &mut param) { - Ok(prepared_asset) => { - render_assets.insert(handle, prepared_asset); - } - Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => { - prepare_next_frame.assets.push((handle, extracted_asset)); - } + for (handle, extracted_asset) in std::mem::take(&mut extracted_assets.extracted) { + match R::prepare_asset(extracted_asset, &mut param) { + Ok(prepared_asset) => { + render_assets.insert(handle, prepared_asset); + } + Err(PrepareAssetError::RetryNextUpdate(extracted_asset)) => { + prepare_next_frame.assets.push((handle, extracted_asset)); } } } diff --git a/crates/bevy_render/src/render_component.rs b/crates/bevy_render/src/render_component.rs index d6a72e337c4c7..f4e901587ee8a 100644 --- a/crates/bevy_render/src/render_component.rs +++ b/crates/bevy_render/src/render_component.rs @@ -9,10 +9,7 @@ use bevy_ecs::{ component::Component, prelude::*, query::{FilterFetch, QueryItem, WorldQuery}, - system::{ - lifetimeless::{Read, SCommands, SQuery}, - RunSystem, SystemParamItem, - }, + system::{lifetimeless::Read, StaticSystemParam}, }; use std::{marker::PhantomData, ops::Deref}; @@ -147,9 +144,8 @@ where ::Fetch: FilterFetch, { fn build(&self, app: &mut App) { - let system = ExtractComponentSystem::::system(&mut app.world); if let Ok(render_app) = app.get_sub_app_mut(RenderApp) { - render_app.add_system_to_stage(RenderStage::Extract, system); + render_app.add_system_to_stage(RenderStage::Extract, extract_components::); } } } @@ -165,25 +161,17 @@ impl ExtractComponent for Handle { } /// This system extracts all components of the corresponding [`ExtractComponent`] type. -pub struct ExtractComponentSystem(PhantomData); - -impl RunSystem for ExtractComponentSystem -where +fn extract_components( + mut commands: Commands, + mut previous_len: Local, + mut query: StaticSystemParam>, +) where ::Fetch: FilterFetch, { - type Param = ( - SCommands, - // the previous amount of extracted components - Local<'static, usize>, - SQuery<(Entity, C::Query), C::Filter>, - ); - - fn run((mut commands, mut previous_len, mut query): SystemParamItem) { - let mut values = Vec::with_capacity(*previous_len); - for (entity, query_item) in query.iter_mut() { - values.push((entity, (C::extract_component(query_item),))); - } - *previous_len = values.len(); - commands.insert_or_spawn_batch(values); + let mut values = Vec::with_capacity(*previous_len); + for (entity, query_item) in query.iter_mut() { + values.push((entity, (C::extract_component(query_item),))); } + *previous_len = values.len(); + commands.insert_or_spawn_batch(values); } From 054ce4a17c3a5b5e8ad18ee278543459b4656335 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 1 Feb 2022 12:50:19 +0000 Subject: [PATCH 4/6] Add some more docs, and rename `inner` Also add a proper read-only delegation --- crates/bevy_ecs/src/system/system_param.rs | 57 ++++++++++++++++++++-- crates/bevy_render/src/render_asset.rs | 2 +- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 3f90ffc6714b6..6bb3bfd26ca92 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1256,19 +1256,57 @@ pub mod lifetimeless { /// A helper for using system parameters in generic contexts /// -/// This type is a system parameter which is statically proven to have -/// `Self::Fetch::Item == Self` (ignoring lifetimes for brevity) +/// This type is a [`SystemParam`] adapter which always has +/// `Self::Fetch::Item == Self` (ignoring lifetimes for brevity), +/// no matter the argument [`SystemParam`] (`P`) (other than +/// that `P` must be `'static`) +/// +/// This makes it useful for having arbitrary [`SystemParam`] type arguments +/// to function systems, or for generic types using the [`derive@SystemParam`] +/// derive: /// /// ``` /// # use bevy_ecs::prelude::*; /// use bevy_ecs::system::{SystemParam, StaticSystemParam}; -/// +/// #[derive(SystemParam)] +/// struct GenericParam<'w,'s, T: SystemParam + 'static> { +/// field: StaticSystemParam<'w, 's, T>, +/// } /// fn do_thing_generically(t: StaticSystemParam) {} /// -/// fn test_always_is(){ +/// fn check_always_is_system(){ /// do_thing_generically::.system(); /// } /// ``` +/// Note that in a real case you'd generally want +/// additional bounds on `P`, for your use of the parameter +/// to have a reason to be generic. +/// +/// For example, using this would allow a type to be generic over +/// whether a resource is accessed mutably or not, with +/// impls being bounded on [`P: Deref`](Deref), and +/// [`P: DerefMut`](DerefMut) depending on whether the +/// method requires mutable access or not. +/// +/// The method which doesn't use this type will not compile: +/// ```compile_fail +/// # use bevy_ecs::prelude::*; +/// # use bevy_ecs::system::{SystemParam, StaticSystemParam}; +/// +/// fn do_thing_generically(t: T) {} +/// +/// #[derive(SystemParam)] +/// struct GenericParam<'w,'s, T: SystemParam> { +/// field: T, +/// #[system_param(ignore)] +/// // Use the lifetimes, as the `SystemParam` derive requires them +/// phantom: core::marker::PhantomData<&'w &'s ()> +/// } +/// # fn check_always_is_system(){ +/// # do_thing_generically::.system(); +/// # } +/// ``` +/// pub struct StaticSystemParam<'w, 's, P: SystemParam>(SystemParamItem<'w, 's, P>); impl<'w, 's, P: SystemParam> Deref for StaticSystemParam<'w, 's, P> { @@ -1286,13 +1324,21 @@ impl<'w, 's, P: SystemParam> DerefMut for StaticSystemParam<'w, 's, P> { } impl<'w, 's, P: SystemParam> StaticSystemParam<'w, 's, P> { - pub fn inner(self) -> SystemParamItem<'w, 's, P> { + /// Get the value of the parameter + pub fn into_inner(self) -> SystemParamItem<'w, 's, P> { self.0 } } +/// The [`SystemParamState`] of [`SystemChangeTick`]. pub struct StaticSystemParamState(S, PhantomData P>); +// Safe: This doesn't add any more reads, and the delegated fetch confirms it +unsafe impl<'w, 's, S: ReadOnlySystemParamFetch, P> ReadOnlySystemParamFetch + for StaticSystemParamState +{ +} + impl<'world, 'state, P: SystemParam + 'static> SystemParam for StaticSystemParam<'world, 'state, P> { @@ -1312,6 +1358,7 @@ where world: &'world World, change_tick: u32, ) -> Self::Item { + // Safe: We properly delegate SystemParamState StaticSystemParam(S::get_param(&mut state.0, system_meta, world, change_tick)) } } diff --git a/crates/bevy_render/src/render_asset.rs b/crates/bevy_render/src/render_asset.rs index 6741b1af745d1..d2fe8ed064951 100644 --- a/crates/bevy_render/src/render_asset.rs +++ b/crates/bevy_render/src/render_asset.rs @@ -143,7 +143,7 @@ fn prepare_assets( mut prepare_next_frame: ResMut>, param: StaticSystemParam<::Param>, ) { - let mut param = param.inner(); + let mut param = param.into_inner(); let mut queued_assets = std::mem::take(&mut prepare_next_frame.assets); for (handle, extracted_asset) in queued_assets.drain(..) { match R::prepare_asset(extracted_asset, &mut param) { From 8a300c489731ed0d9bb130350599fb2a1c61e486 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Fri, 11 Feb 2022 20:17:33 +0000 Subject: [PATCH 5/6] Use `assert_is_system` instead --- crates/bevy_ecs/src/system/system_param.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 6bb3bfd26ca92..1219f36d2c7dc 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1275,7 +1275,7 @@ pub mod lifetimeless { /// fn do_thing_generically(t: StaticSystemParam) {} /// /// fn check_always_is_system(){ -/// do_thing_generically::.system(); +/// bevy_ecs::system::assert_is_system(do_thing_generically::); /// } /// ``` /// Note that in a real case you'd generally want @@ -1303,7 +1303,7 @@ pub mod lifetimeless { /// phantom: core::marker::PhantomData<&'w &'s ()> /// } /// # fn check_always_is_system(){ -/// # do_thing_generically::.system(); +/// # bevy_ecs::system::assert_is_system(do_thing_generically::); /// # } /// ``` /// From 59067f911478547c02705e05c332f035b8e2353c Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Mon, 14 Mar 2022 19:07:08 -0700 Subject: [PATCH 6/6] remove old Config api usage --- crates/bevy_ecs/src/system/system_param.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 1219f36d2c7dc..965db7d0994fd 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1366,15 +1366,10 @@ where unsafe impl<'w, 's, S: SystemParamState, P: SystemParam + 'static> SystemParamState for StaticSystemParamState { - type Config = S::Config; - - fn init(world: &mut World, system_meta: &mut SystemMeta, config: Self::Config) -> Self { - Self(S::init(world, system_meta, config), PhantomData) + fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { + Self(S::init(world, system_meta), PhantomData) } - fn default_config() -> Self::Config { - S::default_config() - } fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) { self.0.new_archetype(archetype, system_meta) }