From 3cc6c567a8298566225df210d801e79ca282df26 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 15 Mar 2022 02:16:55 +0000 Subject: [PATCH] Obviate the need for `RunSystem`, and remove it (#3817) # Objective - Fixes #3300 - `RunSystem` is messy ## Solution - Adds the trick theorised in https://github.com/bevyengine/bevy/issues/3300#issuecomment-991791234 P.S. I also want this for an experimental refactoring of `Assets`, to remove the duplication of `Events>` Co-authored-by: Carter Anderson --- crates/bevy_ecs/macros/src/lib.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 79 +---------- crates/bevy_ecs/src/system/system_param.rs | 125 ++++++++++++++++++ crates/bevy_render/src/render_asset.rs | 67 ++++------ crates/bevy_render/src/render_component.rs | 36 ++--- 5 files changed, 167 insertions(+), 142 deletions(-) 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 { 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_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f242892b873d3..965db7d0994fd 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1254,6 +1254,131 @@ pub mod lifetimeless { pub type SCommands = crate::system::Commands<'static, 'static>; } +/// A helper for using system parameters in generic contexts +/// +/// 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 check_always_is_system(){ +/// bevy_ecs::system::assert_is_system(do_thing_generically::); +/// } +/// ``` +/// 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(){ +/// # bevy_ecs::system::assert_is_system(do_thing_generically::); +/// # } +/// ``` +/// +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> { + /// 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> +{ + 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 { + // Safe: We properly delegate SystemParamState + StaticSystemParam(S::get_param(&mut state.0, system_meta, world, change_tick)) + } +} + +unsafe impl<'w, 's, S: SystemParamState, P: SystemParam + 'static> SystemParamState + for StaticSystemParamState +{ + fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { + Self(S::init(world, system_meta), PhantomData) + } + + 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; diff --git a/crates/bevy_render/src/render_asset.rs b/crates/bevy_render/src/render_asset.rs index 12861ae6f4e64..d2fe8ed064951 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.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) { + 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); }