From 5ecff1e2dc772a4780b6c1e6c2783a8f53095e48 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Tue, 3 Dec 2024 21:25:43 -0600 Subject: [PATCH 1/8] turn apply_deferred into a ZST system --- crates/bevy_ecs/src/schedule/executor/mod.rs | 130 ++++++++++++++++--- 1 file changed, 114 insertions(+), 16 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 37fd9ff7246c8..f3629c82c159a 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -2,6 +2,9 @@ mod multi_threaded; mod simple; mod single_threaded; +use alloc::borrow::Cow; +use core::any::TypeId; + pub use self::{ multi_threaded::{MainThreadExecutor, MultiThreadedExecutor}, simple::SimpleExecutor, @@ -11,9 +14,13 @@ pub use self::{ use fixedbitset::FixedBitSet; use crate::{ - schedule::{BoxedCondition, NodeId}, - system::BoxedSystem, - world::World, + archetype::ArchetypeComponentId, + component::{ComponentId, Tick}, + prelude::{IntoSystemSet, SystemSet}, + query::Access, + schedule::{BoxedCondition, InternedSystemSet, NodeId, SystemTypeSet}, + system::{BoxedSystem, System, SystemIn}, + world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World}, }; /// Types that can run a [`SystemSchedule`] on a [`World`]. @@ -100,32 +107,123 @@ impl SystemSchedule { } } -/// Instructs the executor to call [`System::apply_deferred`](crate::system::System::apply_deferred) -/// on the systems that have run but not applied their [`Deferred`](crate::system::Deferred) system parameters -/// (like [`Commands`](crate::prelude::Commands)) or other system buffers. +/// A special [`System`] that instructs the executor to call +/// [`System::apply_deferred`] on the systems that have run but not applied +/// their [`Deferred`] system parameters (like [`Commands`]) or other system buffers. /// /// ## Scheduling /// /// `apply_deferred` systems are scheduled *by default* /// - later in the same schedule run (for example, if a system with `Commands` param /// is scheduled in `Update`, all the changes will be visible in `PostUpdate`) -/// - between systems with dependencies if the dependency -/// [has deferred buffers](crate::system::System::has_deferred) -/// (if system `bar` directly or indirectly depends on `foo`, and `foo` uses `Commands` param, -/// changes to the world in `foo` will be visible in `bar`) +/// - between systems with dependencies if the dependency [has deferred buffers] +/// (if system `bar` directly or indirectly depends on `foo`, and `foo` uses +/// `Commands` param, changes to the world in `foo` will be visible in `bar`) /// /// ## Notes -/// - This function (currently) does nothing if it's called manually or wrapped inside a [`PipeSystem`](crate::system::PipeSystem). -/// - Modifying a [`Schedule`](super::Schedule) may change the order buffers are applied. +/// - This system (currently) does nothing if it's called manually or wrapped +/// inside a [`PipeSystem`]. +/// - Modifying a [`Schedule`] may change the order buffers are applied. +/// +/// [`System::apply_deferred`]: crate::system::System::apply_deferred +/// [`Deferred`]: crate::system::Deferred +/// [`Commands`]: crate::prelude::Commands +/// [has deferred buffers]: crate::system::System::has_deferred +/// [`PipeSystem`]: crate::system::PipeSystem +/// [`Schedule`]: super::Schedule #[doc(alias = "apply_system_buffers")] -#[allow(unused_variables)] -pub fn apply_deferred(world: &mut World) {} +#[allow(unused_variables, non_camel_case_types)] +pub struct apply_deferred; /// Returns `true` if the [`System`](crate::system::System) is an instance of [`apply_deferred`]. pub(super) fn is_apply_deferred(system: &BoxedSystem) -> bool { - use crate::system::IntoSystem; // deref to use `System::type_id` instead of `Any::type_id` - system.as_ref().type_id() == apply_deferred.system_type_id() + system.as_ref().type_id() == TypeId::of::() +} + +impl System for apply_deferred { + type In = (); + type Out = (); + + fn name(&self) -> Cow<'static, str> { + Cow::Borrowed("bevy_ecs::apply_deferred") + } + + fn component_access(&self) -> &Access { + // This system accesses no components. + const { &Access::new() } + } + + fn archetype_component_access(&self) -> &Access { + // This system accesses no archetype components. + const { &Access::new() } + } + + fn is_send(&self) -> bool { + false + } + + fn is_exclusive(&self) -> bool { + // This system is labeled exclusive because it is used by the system + // executor to find places where deferred commands should be applied. + true + } + + fn has_deferred(&self) -> bool { + // This system itself doesn't have any commands to apply, but when it + // is pulled from the schedule to be ran, the executor will apply + // deferred commands from other systems. + false + } + + unsafe fn run_unsafe( + &mut self, + _input: SystemIn<'_, Self>, + _world: UnsafeWorldCell, + ) -> Self::Out { + // This system does nothing on its own. The executor will apply deferred + // commands from other systems instead of running this system. + } + + fn run(&mut self, _input: SystemIn<'_, Self>, _world: &mut World) -> Self::Out { + // This system does nothing on its own. The executor will apply deferred + // commands from other systems instead of running this system. + } + + fn apply_deferred(&mut self, _world: &mut World) {} + + fn queue_deferred(&mut self, _world: DeferredWorld) {} + + unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { + // This system is always valid to run because it doesn't do anything, + // and only used as a marker for the executor. + true + } + + fn initialize(&mut self, _world: &mut World) {} + + fn update_archetype_component_access(&mut self, _world: UnsafeWorldCell) {} + + fn check_change_tick(&mut self, _change_tick: Tick) {} + + fn default_system_sets(&self) -> Vec { + vec![SystemTypeSet::::new().intern()] + } + + fn get_last_run(&self) -> Tick { + // This system is never run, so it has no last run tick. + Tick::MAX + } + + fn set_last_run(&mut self, _last_run: Tick) {} +} + +impl IntoSystemSet<()> for apply_deferred { + type Set = SystemTypeSet; + + fn into_system_set(self) -> Self::Set { + SystemTypeSet::::new() + } } /// These functions hide the bottom of the callstack from `RUST_BACKTRACE=1` (assuming the default panic handler is used). From 7205c603915cb4c4ef222b8bb04084ff02fe1c0f Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Tue, 3 Dec 2024 21:51:50 -0600 Subject: [PATCH 2/8] elaborate on is_exclusive comment --- crates/bevy_ecs/src/schedule/executor/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index f3629c82c159a..d8aec0eed1a19 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -165,7 +165,8 @@ impl System for apply_deferred { fn is_exclusive(&self) -> bool { // This system is labeled exclusive because it is used by the system - // executor to find places where deferred commands should be applied. + // executor to find places where deferred commands should be applied, + // and commands can only be applied with exclusive access to the world. true } From 5163b1b627a6e2558e422768356a1e5c809c2a0c Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Tue, 3 Dec 2024 22:12:51 -0600 Subject: [PATCH 3/8] remove allowed unused_variables lint --- crates/bevy_ecs/src/schedule/executor/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index d8aec0eed1a19..4f7491ab44dde 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -132,7 +132,7 @@ impl SystemSchedule { /// [`PipeSystem`]: crate::system::PipeSystem /// [`Schedule`]: super::Schedule #[doc(alias = "apply_system_buffers")] -#[allow(unused_variables, non_camel_case_types)] +#[allow(non_camel_case_types)] pub struct apply_deferred; /// Returns `true` if the [`System`](crate::system::System) is an instance of [`apply_deferred`]. From 477eb0af1f1e7affafcfb4afc225b9d872f7dcf5 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Tue, 3 Dec 2024 22:31:36 -0600 Subject: [PATCH 4/8] remove redundant doc link --- crates/bevy_ecs/src/schedule/executor/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 4f7491ab44dde..88062f88fa8ae 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -135,7 +135,7 @@ impl SystemSchedule { #[allow(non_camel_case_types)] pub struct apply_deferred; -/// Returns `true` if the [`System`](crate::system::System) is an instance of [`apply_deferred`]. +/// Returns `true` if the [`System`] is an instance of [`apply_deferred`]. pub(super) fn is_apply_deferred(system: &BoxedSystem) -> bool { // deref to use `System::type_id` instead of `Any::type_id` system.as_ref().type_id() == TypeId::of::() From 150266dc61e08a54cb70c3d720e06c746a0a9186 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Tue, 3 Dec 2024 02:44:22 -0600 Subject: [PATCH 5/8] unify exclusive system params with normal system params --- crates/bevy_ecs/macros/src/lib.rs | 4 + crates/bevy_ecs/src/schedule/set.rs | 19 +- .../src/system/exclusive_function_system.rs | 330 ------------------ .../src/system/exclusive_system_param.rs | 156 --------- crates/bevy_ecs/src/system/function_system.rs | 2 +- crates/bevy_ecs/src/system/mod.rs | 4 - crates/bevy_ecs/src/system/system_name.rs | 15 +- crates/bevy_ecs/src/system/system_param.rs | 151 +++++++- crates/bevy_ecs/src/world/identifier.rs | 15 +- 9 files changed, 154 insertions(+), 542 deletions(-) delete mode 100644 crates/bevy_ecs/src/system/exclusive_function_system.rs delete mode 100644 crates/bevy_ecs/src/system/exclusive_system_param.rs diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index c3a256fef8629..2f0c79bebdeee 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -403,6 +403,10 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { change_tick, } } + + fn is_exclusive() -> bool { + false #(|| <#param as SystemParam>::is_exclusive())* + } } impl<'w, 's, #(#param: SystemParam,)*> ParamSet<'w, 's, (#(#param,)*)> diff --git a/crates/bevy_ecs/src/schedule/set.rs b/crates/bevy_ecs/src/schedule/set.rs index 244dd436351ec..b805e60e47b3c 100644 --- a/crates/bevy_ecs/src/schedule/set.rs +++ b/crates/bevy_ecs/src/schedule/set.rs @@ -11,10 +11,7 @@ pub use bevy_ecs_macros::{ScheduleLabel, SystemSet}; use crate::{ define_label, intern::Interned, - system::{ - ExclusiveFunctionSystem, ExclusiveSystemParamFunction, FunctionSystem, - IsExclusiveFunctionSystem, IsFunctionSystem, SystemParamFunction, - }, + system::{FunctionSystem, IsFunctionSystem, SystemParamFunction}, }; define_label!( @@ -187,20 +184,6 @@ where } } -// exclusive systems -impl IntoSystemSet<(IsExclusiveFunctionSystem, Marker)> for F -where - Marker: 'static, - F: ExclusiveSystemParamFunction, -{ - type Set = SystemTypeSet>; - - #[inline] - fn into_system_set(self) -> Self::Set { - SystemTypeSet::>::new() - } -} - #[cfg(test)] mod tests { use crate::{ diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs deleted file mode 100644 index 2face3a9f0cb3..0000000000000 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ /dev/null @@ -1,330 +0,0 @@ -use crate::{ - archetype::ArchetypeComponentId, - component::{ComponentId, Tick}, - query::Access, - schedule::{InternedSystemSet, SystemSet}, - system::{ - check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, IntoSystem, - System, SystemIn, SystemInput, SystemMeta, - }, - world::{unsafe_world_cell::UnsafeWorldCell, World}, -}; - -use alloc::borrow::Cow; -use core::marker::PhantomData; -use variadics_please::all_tuples; - -/// A function system that runs with exclusive [`World`] access. -/// -/// You get this by calling [`IntoSystem::into_system`] on a function that only accepts -/// [`ExclusiveSystemParam`]s. -/// -/// [`ExclusiveFunctionSystem`] must be `.initialized` before they can be run. -pub struct ExclusiveFunctionSystem -where - F: ExclusiveSystemParamFunction, -{ - func: F, - param_state: Option<::State>, - system_meta: SystemMeta, - // NOTE: PhantomData T> gives this safe Send/Sync impls - marker: PhantomData Marker>, -} - -impl ExclusiveFunctionSystem -where - F: ExclusiveSystemParamFunction, -{ - /// Return this system with a new name. - /// - /// Useful to give closure systems more readable and unique names for debugging and tracing. - pub fn with_name(mut self, new_name: impl Into>) -> Self { - self.system_meta.set_name(new_name.into()); - self - } -} - -/// A marker type used to distinguish exclusive function systems from regular function systems. -#[doc(hidden)] -pub struct IsExclusiveFunctionSystem; - -impl IntoSystem for F -where - Marker: 'static, - F: ExclusiveSystemParamFunction, -{ - type System = ExclusiveFunctionSystem; - fn into_system(func: Self) -> Self::System { - ExclusiveFunctionSystem { - func, - param_state: None, - system_meta: SystemMeta::new::(), - marker: PhantomData, - } - } -} - -const PARAM_MESSAGE: &str = "System's param_state was not found. Did you forget to initialize this system before running it?"; - -impl System for ExclusiveFunctionSystem -where - Marker: 'static, - F: ExclusiveSystemParamFunction, -{ - type In = F::In; - type Out = F::Out; - - #[inline] - fn name(&self) -> Cow<'static, str> { - self.system_meta.name.clone() - } - - #[inline] - fn component_access(&self) -> &Access { - self.system_meta.component_access_set.combined_access() - } - - #[inline] - fn archetype_component_access(&self) -> &Access { - &self.system_meta.archetype_component_access - } - - #[inline] - fn is_send(&self) -> bool { - // exclusive systems should have access to non-send resources - // the executor runs exclusive systems on the main thread, so this - // field reflects that constraint - false - } - - #[inline] - fn is_exclusive(&self) -> bool { - true - } - - #[inline] - fn has_deferred(&self) -> bool { - // exclusive systems have no deferred system params - false - } - - #[inline] - unsafe fn run_unsafe( - &mut self, - _input: SystemIn<'_, Self>, - _world: UnsafeWorldCell, - ) -> Self::Out { - panic!("Cannot run exclusive systems with a shared World reference"); - } - - fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out { - world.last_change_tick_scope(self.system_meta.last_run, |world| { - #[cfg(feature = "trace")] - let _span_guard = self.system_meta.system_span.enter(); - - let params = F::Param::get_param( - self.param_state.as_mut().expect(PARAM_MESSAGE), - &self.system_meta, - ); - let out = self.func.run(world, input, params); - - world.flush(); - self.system_meta.last_run = world.increment_change_tick(); - - out - }) - } - - #[inline] - fn apply_deferred(&mut self, _world: &mut World) { - // "pure" exclusive systems do not have any buffers to apply. - // Systems made by piping a normal system with an exclusive system - // might have buffers to apply, but this is handled by `PipeSystem`. - } - - #[inline] - fn queue_deferred(&mut self, _world: crate::world::DeferredWorld) { - // "pure" exclusive systems do not have any buffers to apply. - // Systems made by piping a normal system with an exclusive system - // might have buffers to apply, but this is handled by `PipeSystem`. - } - - #[inline] - unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool { - // All exclusive system params are always available. - true - } - - #[inline] - fn initialize(&mut self, world: &mut World) { - self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); - self.param_state = Some(F::Param::init(world, &mut self.system_meta)); - } - - fn update_archetype_component_access(&mut self, _world: UnsafeWorldCell) {} - - #[inline] - fn check_change_tick(&mut self, change_tick: Tick) { - check_system_change_tick( - &mut self.system_meta.last_run, - change_tick, - self.system_meta.name.as_ref(), - ); - } - - fn default_system_sets(&self) -> Vec { - let set = crate::schedule::SystemTypeSet::::new(); - vec![set.intern()] - } - - fn get_last_run(&self) -> Tick { - self.system_meta.last_run - } - - fn set_last_run(&mut self, last_run: Tick) { - self.system_meta.last_run = last_run; - } -} - -/// A trait implemented for all exclusive system functions that can be used as [`System`]s. -/// -/// This trait can be useful for making your own systems which accept other systems, -/// sometimes called higher order systems. -#[diagnostic::on_unimplemented( - message = "`{Self}` is not an exclusive system", - label = "invalid system" -)] -pub trait ExclusiveSystemParamFunction: Send + Sync + 'static { - /// The input type to this system. See [`System::In`]. - type In: SystemInput; - - /// The return type of this system. See [`System::Out`]. - type Out; - - /// The [`ExclusiveSystemParam`]'s defined by this system's `fn` parameters. - type Param: ExclusiveSystemParam; - - /// Executes this system once. See [`System::run`]. - fn run( - &mut self, - world: &mut World, - input: ::Inner<'_>, - param_value: ExclusiveSystemParamItem, - ) -> Self::Out; -} - -/// A marker type used to distinguish exclusive function systems with and without input. -#[doc(hidden)] -pub struct HasExclusiveSystemInput; - -macro_rules! impl_exclusive_system_function { - ($($param: ident),*) => { - #[allow(non_snake_case)] - impl ExclusiveSystemParamFunction Out> for Func - where - Func: Send + Sync + 'static, - for <'a> &'a mut Func: - FnMut(&mut World, $($param),*) -> Out + - FnMut(&mut World, $(ExclusiveSystemParamItem<$param>),*) -> Out, - Out: 'static, - { - type In = (); - type Out = Out; - type Param = ($($param,)*); - #[inline] - fn run(&mut self, world: &mut World, _in: (), param_value: ExclusiveSystemParamItem< ($($param,)*)>) -> Out { - // Yes, this is strange, but `rustc` fails to compile this impl - // without using this function. It fails to recognize that `func` - // is a function, potentially because of the multiple impls of `FnMut` - #[allow(clippy::too_many_arguments)] - fn call_inner( - mut f: impl FnMut(&mut World, $($param,)*) -> Out, - world: &mut World, - $($param: $param,)* - ) -> Out { - f(world, $($param,)*) - } - let ($($param,)*) = param_value; - call_inner(self, world, $($param),*) - } - } - - #[allow(non_snake_case)] - impl ExclusiveSystemParamFunction<(HasExclusiveSystemInput, fn(In, $($param,)*) -> Out)> for Func - where - Func: Send + Sync + 'static, - for <'a> &'a mut Func: - FnMut(In, &mut World, $($param),*) -> Out + - FnMut(In::Param<'_>, &mut World, $(ExclusiveSystemParamItem<$param>),*) -> Out, - In: SystemInput + 'static, - Out: 'static, - { - type In = In; - type Out = Out; - type Param = ($($param,)*); - #[inline] - fn run(&mut self, world: &mut World, input: In::Inner<'_>, param_value: ExclusiveSystemParamItem< ($($param,)*)>) -> Out { - // Yes, this is strange, but `rustc` fails to compile this impl - // without using this function. It fails to recognize that `func` - // is a function, potentially because of the multiple impls of `FnMut` - #[allow(clippy::too_many_arguments)] - fn call_inner( - mut f: impl FnMut(In::Param<'_>, &mut World, $($param,)*) -> Out, - input: In::Inner<'_>, - world: &mut World, - $($param: $param,)* - ) -> Out { - f(In::wrap(input), world, $($param,)*) - } - let ($($param,)*) = param_value; - call_inner(self, input, world, $($param),*) - } - } - }; -} -// Note that we rely on the highest impl to be <= the highest order of the tuple impls -// of `SystemParam` created. -all_tuples!(impl_exclusive_system_function, 0, 16, F); - -#[cfg(test)] -mod tests { - use crate::system::input::SystemInput; - - use super::*; - - #[test] - fn into_system_type_id_consistency() { - fn test(function: T) - where - T: IntoSystem + Copy, - { - fn reference_system(_world: &mut World) {} - - use core::any::TypeId; - - let system = IntoSystem::into_system(function); - - assert_eq!( - system.type_id(), - function.system_type_id(), - "System::type_id should be consistent with IntoSystem::system_type_id" - ); - - assert_eq!( - system.type_id(), - TypeId::of::(), - "System::type_id should be consistent with TypeId::of::()" - ); - - assert_ne!( - system.type_id(), - IntoSystem::into_system(reference_system).type_id(), - "Different systems should have different TypeIds" - ); - } - - fn exclusive_function_system(_world: &mut World) {} - - test(exclusive_function_system); - } -} diff --git a/crates/bevy_ecs/src/system/exclusive_system_param.rs b/crates/bevy_ecs/src/system/exclusive_system_param.rs deleted file mode 100644 index cc24cb7904304..0000000000000 --- a/crates/bevy_ecs/src/system/exclusive_system_param.rs +++ /dev/null @@ -1,156 +0,0 @@ -use crate::{ - prelude::{FromWorld, QueryState}, - query::{QueryData, QueryFilter}, - system::{Local, SystemMeta, SystemParam, SystemState}, - world::World, -}; -use bevy_utils::synccell::SyncCell; -use core::marker::PhantomData; -use variadics_please::all_tuples; - -/// A parameter that can be used in an exclusive system (a system with an `&mut World` parameter). -/// Any parameters implementing this trait must come after the `&mut World` parameter. -#[diagnostic::on_unimplemented( - message = "`{Self}` can not be used as a parameter for an exclusive system", - label = "invalid system parameter" -)] -pub trait ExclusiveSystemParam: Sized { - /// Used to store data which persists across invocations of a system. - type State: Send + Sync + 'static; - /// The item type returned when constructing this system param. - /// See [`SystemParam::Item`]. - type Item<'s>: ExclusiveSystemParam; - - /// Creates a new instance of this param's [`State`](Self::State). - fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self::State; - - /// Creates a parameter to be passed into an [`ExclusiveSystemParamFunction`]. - /// - /// [`ExclusiveSystemParamFunction`]: super::ExclusiveSystemParamFunction - fn get_param<'s>(state: &'s mut Self::State, system_meta: &SystemMeta) -> Self::Item<'s>; -} - -/// Shorthand way of accessing the associated type [`ExclusiveSystemParam::Item`] -/// for a given [`ExclusiveSystemParam`]. -pub type ExclusiveSystemParamItem<'s, P> =

::Item<'s>; - -impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> ExclusiveSystemParam - for &'a mut QueryState -{ - type State = QueryState; - type Item<'s> = &'s mut QueryState; - - fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - QueryState::new(world) - } - - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - state - } -} - -impl<'a, P: SystemParam + 'static> ExclusiveSystemParam for &'a mut SystemState

{ - type State = SystemState

; - type Item<'s> = &'s mut SystemState

; - - fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - SystemState::new(world) - } - - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - state - } -} - -impl<'_s, T: FromWorld + Send + 'static> ExclusiveSystemParam for Local<'_s, T> { - type State = SyncCell; - type Item<'s> = Local<'s, T>; - - fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - SyncCell::new(T::from_world(world)) - } - - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - Local(state.get()) - } -} - -impl ExclusiveSystemParam for PhantomData { - type State = (); - type Item<'s> = PhantomData; - - fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {} - - fn get_param<'s>(_state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - PhantomData - } -} - -macro_rules! impl_exclusive_system_param_tuple { - ($(#[$meta:meta])* $($param: ident),*) => { - #[allow(unused_variables)] - #[allow(non_snake_case)] - $(#[$meta])* - impl<$($param: ExclusiveSystemParam),*> ExclusiveSystemParam for ($($param,)*) { - type State = ($($param::State,)*); - type Item<'s> = ($($param::Item<'s>,)*); - - #[inline] - fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - (($($param::init(_world, _system_meta),)*)) - } - - #[inline] - #[allow(clippy::unused_unit)] - fn get_param<'s>( - state: &'s mut Self::State, - system_meta: &SystemMeta, - ) -> Self::Item<'s> { - - let ($($param,)*) = state; - ($($param::get_param($param, system_meta),)*) - } - } - }; -} - -all_tuples!( - #[doc(fake_variadic)] - impl_exclusive_system_param_tuple, - 0, - 16, - P -); - -#[cfg(test)] -mod tests { - use crate as bevy_ecs; - use crate::{schedule::Schedule, system::Local, world::World}; - use bevy_ecs_macros::Resource; - use core::marker::PhantomData; - - #[test] - fn test_exclusive_system_params() { - #[derive(Resource, Default)] - struct Res { - test_value: u32, - } - - fn my_system(world: &mut World, mut local: Local, _phantom: PhantomData>) { - assert_eq!(world.resource::().test_value, *local); - *local += 1; - world.resource_mut::().test_value += 1; - } - - let mut schedule = Schedule::default(); - schedule.add_systems(my_system); - - let mut world = World::default(); - world.init_resource::(); - - schedule.run(&mut world); - schedule.run(&mut world); - - assert_eq!(2, world.get_resource::().unwrap().test_value); - } -} diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 7c5806abbcd6a..ef8b8177532ff 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -714,7 +714,7 @@ where #[inline] fn is_exclusive(&self) -> bool { - false + F::Param::is_exclusive() } #[inline] diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 394e03c205ba3..1b301587bd5ab 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -115,8 +115,6 @@ mod adapter_system; mod builder; mod combinator; mod commands; -mod exclusive_function_system; -mod exclusive_system_param; mod function_system; mod input; mod observer_system; @@ -133,8 +131,6 @@ pub use adapter_system::*; pub use builder::*; pub use combinator::*; pub use commands::*; -pub use exclusive_function_system::*; -pub use exclusive_system_param::*; pub use function_system::*; pub use input::*; pub use observer_system::*; diff --git a/crates/bevy_ecs/src/system/system_name.rs b/crates/bevy_ecs/src/system/system_name.rs index 3ecc901baae2b..3a03d1c9d2fae 100644 --- a/crates/bevy_ecs/src/system/system_name.rs +++ b/crates/bevy_ecs/src/system/system_name.rs @@ -1,7 +1,7 @@ use crate::{ component::Tick, prelude::World, - system::{ExclusiveSystemParam, ReadOnlySystemParam, SystemMeta, SystemParam}, + system::{ReadOnlySystemParam, SystemMeta, SystemParam}, world::unsafe_world_cell::UnsafeWorldCell, }; use alloc::borrow::Cow; @@ -75,19 +75,6 @@ unsafe impl SystemParam for SystemName<'_> { // SAFETY: Only reads internal system state unsafe impl<'s> ReadOnlySystemParam for SystemName<'s> {} -impl ExclusiveSystemParam for SystemName<'_> { - type State = Cow<'static, str>; - type Item<'s> = SystemName<'s>; - - fn init(_world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - system_meta.name.clone() - } - - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - SystemName(state) - } -} - #[cfg(test)] mod tests { use crate::{ diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 7519a1e30a089..1f5b55ac25700 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -10,7 +10,7 @@ use crate::{ QueryState, ReadOnlyQueryData, }, storage::ResourceData, - system::{Query, Single, SystemMeta}, + system::{Query, Single, SystemMeta, SystemState}, world::{ unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FilteredResources, FilteredResourcesMut, FromWorld, World, @@ -273,6 +273,15 @@ pub unsafe trait SystemParam: Sized { world: UnsafeWorldCell<'world>, change_tick: Tick, ) -> Self::Item<'world, 'state>; + + /// Returns true if this [`SystemParam`] requires exclusive access to the + /// entire [`World`]. [`System`]s with exclusive parameters cannot run in + /// parallel with other systems. + /// + /// [`System`]: crate::system::System + fn is_exclusive() -> bool { + false + } } /// A [`SystemParam`] that only reads a given [`World`]. @@ -962,7 +971,7 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { } } -/// SAFETY: only reads world +// SAFETY: only reads world unsafe impl<'w> ReadOnlySystemParam for &'w World {} // SAFETY: `read_all` access is set and conflicts result in a panic @@ -1006,14 +1015,37 @@ unsafe impl SystemParam for &'_ World { } } -/// SAFETY: `DeferredWorld` can read all components and resources but cannot be used to gain any other mutable references. +const MUT_DEFERRED_WORLD_ERROR: &str = "DeferredWorld requires exclusive access + to the entire world, but a previous system parameter already registered \ + access to a part of it. Allowing this would break Rust's mutability rules"; + +// SAFETY: `DeferredWorld` registers exclusive access to the entire world. unsafe impl<'w> SystemParam for DeferredWorld<'w> { type State = (); type Item<'world, 'state> = DeferredWorld<'world>; fn init_state(_world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - system_meta.component_access_set.read_all(); - system_meta.component_access_set.write_all(); + let mut access = Access::default(); + access.read_all(); + if !system_meta + .archetype_component_access + .is_compatible(&access) + { + panic!("{}", MUT_DEFERRED_WORLD_ERROR); + } + system_meta.archetype_component_access.extend(&access); + + let mut filtered_access = FilteredAccess::default(); + + filtered_access.read_all(); + if !system_meta + .component_access_set + .get_conflicts_single(&filtered_access) + .is_empty() + { + panic!("{}", MUT_DEFERRED_WORLD_ERROR); + } + system_meta.component_access_set.add(filtered_access); system_meta.set_has_deferred(); } @@ -1025,6 +1057,111 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { ) -> Self::Item<'world, 'state> { world.into_deferred() } + + fn is_exclusive() -> bool { + true + } +} + +const MUT_WORLD_ERROR: &str = "&mut World requires exclusive access to the \ + entire world, but a previous system parameter already registered access to \ + a part of it. Allowing this would break Rust's mutability rules"; + +// SAFETY: `&mut World` registers exclusive access to the entire world. +unsafe impl SystemParam for &mut World { + type State = (); + type Item<'w, 's> = &'w mut World; + + fn init_state(_world: &mut World, system_meta: &mut SystemMeta) -> Self::State { + let mut access = Access::default(); + access.read_all(); + if !system_meta + .archetype_component_access + .is_compatible(&access) + { + panic!("{}", MUT_WORLD_ERROR); + } + system_meta.archetype_component_access.extend(&access); + + let mut filtered_access = FilteredAccess::default(); + + filtered_access.read_all(); + if !system_meta + .component_access_set + .get_conflicts_single(&filtered_access) + .is_empty() + { + panic!("{}", MUT_WORLD_ERROR); + } + system_meta.component_access_set.add(filtered_access); + system_meta.set_has_deferred(); + } + + unsafe fn get_param<'world, 'state>( + _state: &'state mut Self::State, + _system_meta: &SystemMeta, + world: UnsafeWorldCell<'world>, + _change_tick: Tick, + ) -> Self::Item<'world, 'state> { + // SAFETY: Exclusive access to the entire world was registered in `init_state`. + unsafe { world.world_mut() } + } + + fn is_exclusive() -> bool { + true + } +} + +// SAFETY: `&mut QueryState` only accesses internal state +unsafe impl SystemParam + for &mut QueryState +{ + type State = QueryState; + type Item<'w, 's> = &'s mut QueryState; + + fn init_state(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { + QueryState::new(world) + } + + unsafe fn new_archetype( + state: &mut Self::State, + archetype: &Archetype, + system_meta: &mut SystemMeta, + ) { + state.new_archetype(archetype, &mut system_meta.archetype_component_access); + } + + unsafe fn get_param<'world, 'state>( + state: &'state mut Self::State, + _system_meta: &SystemMeta, + _world: UnsafeWorldCell<'world>, + _change_tick: Tick, + ) -> Self::Item<'world, 'state> { + state + } +} + +// SAFETY: `&mut SystemState` only accesses internal state +unsafe impl SystemParam for &mut SystemState

{ + type State = SystemState

; + type Item<'w, 's> = &'s mut SystemState

; + + fn init_state(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { + SystemState::new(world) + } + + fn apply(state: &mut Self::State, _system_meta: &SystemMeta, world: &mut World) { + state.apply(world); + } + + unsafe fn get_param<'world, 'state>( + state: &'state mut Self::State, + _system_meta: &SystemMeta, + _world: UnsafeWorldCell<'world>, + _change_tick: Tick, + ) -> Self::Item<'world, 'state> { + state + } } /// A system local [`SystemParam`]. @@ -1953,6 +2090,10 @@ macro_rules! impl_system_param_tuple { let ($($param,)*) = state; ($($param::get_param($param, _system_meta, _world, _change_tick),)*) } + + fn is_exclusive() -> bool { + false $(|| $param::is_exclusive())* + } } }; } diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index 9187b818504b5..0e172775355b5 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -1,7 +1,7 @@ use crate::{ component::Tick, storage::SparseSetIndex, - system::{ExclusiveSystemParam, ReadOnlySystemParam, SystemMeta, SystemParam}, + system::{ReadOnlySystemParam, SystemMeta, SystemParam}, world::{FromWorld, World}, }; use core::sync::atomic::{AtomicUsize, Ordering}; @@ -66,19 +66,6 @@ unsafe impl SystemParam for WorldId { } } -impl ExclusiveSystemParam for WorldId { - type State = WorldId; - type Item<'s> = WorldId; - - fn init(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State { - world.id() - } - - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - *state - } -} - impl SparseSetIndex for WorldId { #[inline] fn sparse_set_index(&self) -> usize { From ed4d0fd8bac5c152526b6c2afda5c63d7420ddf2 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Wed, 4 Dec 2024 01:27:21 -0600 Subject: [PATCH 6/8] make mutable world access params register write_all --- crates/bevy_ecs/src/system/system_param.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 1f5b55ac25700..f6b77388763f4 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1015,7 +1015,7 @@ unsafe impl SystemParam for &'_ World { } } -const MUT_DEFERRED_WORLD_ERROR: &str = "DeferredWorld requires exclusive access +const MUT_DEFERRED_WORLD_ERROR: &str = "DeferredWorld requires exclusive access \ to the entire world, but a previous system parameter already registered \ access to a part of it. Allowing this would break Rust's mutability rules"; @@ -1027,6 +1027,7 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { fn init_state(_world: &mut World, system_meta: &mut SystemMeta) -> Self::State { let mut access = Access::default(); access.read_all(); + access.write_all(); if !system_meta .archetype_component_access .is_compatible(&access) @@ -1038,6 +1039,7 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { let mut filtered_access = FilteredAccess::default(); filtered_access.read_all(); + filtered_access.write_all(); if !system_meta .component_access_set .get_conflicts_single(&filtered_access) @@ -1075,6 +1077,7 @@ unsafe impl SystemParam for &mut World { fn init_state(_world: &mut World, system_meta: &mut SystemMeta) -> Self::State { let mut access = Access::default(); access.read_all(); + access.write_all(); if !system_meta .archetype_component_access .is_compatible(&access) @@ -1086,6 +1089,7 @@ unsafe impl SystemParam for &mut World { let mut filtered_access = FilteredAccess::default(); filtered_access.read_all(); + filtered_access.write_all(); if !system_meta .component_access_set .get_conflicts_single(&filtered_access) From c2eb4fa2d1488b1621f6992b9cc809d6cb0a1a02 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Wed, 4 Dec 2024 02:39:14 -0600 Subject: [PATCH 7/8] add some conflict tests --- crates/bevy_ecs/src/system/system_param.rs | 133 ++++++++++++++++++++- 1 file changed, 131 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f6b77388763f4..1a8c39a1fd24d 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -2637,8 +2637,9 @@ unsafe impl SystemParam for FilteredResourcesMut<'_, '_> { mod tests { use super::*; use crate::{ - self as bevy_ecs, // Necessary for the `SystemParam` Derive when used inside `bevy_ecs`. - system::assert_is_system, + self as bevy_ecs, + prelude::Component, + system::{assert_is_system, IntoSystem, System}, }; use core::cell::RefCell; @@ -2860,4 +2861,132 @@ mod tests { let _query: Query<()> = p.downcast_mut_inner().unwrap(); let _query: Query<()> = p.downcast().unwrap(); } + + #[derive(Component, Resource)] + struct Foo; + + fn check_conflict(system: impl IntoSystem<(), (), M>) { + let mut world = World::new(); + let mut system = IntoSystem::into_system(system); + system.initialize(&mut world); + } + + #[test] + #[should_panic] + fn conflict_mut_world_multiple() { + fn system(_: &mut World, _: &mut World) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_mut_world_deferred_world() { + fn system(_: &mut World, _: DeferredWorld) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_mut_world_query() { + fn system(_: &mut World, _: Query<&Foo>) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_query_mut_world() { + fn system(_: Query<&Foo>, _: &mut World) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_mut_world_resource() { + fn system(_: &mut World, _: Res) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_resource_mut_world() { + fn system(_: Res, _: &mut World) {} + check_conflict(system); + } + + #[test] + fn no_conflict_mut_world_local() { + fn system(_: &mut World, _: Local) {} + check_conflict(system); + } + + #[test] + fn no_conflict_mut_world_query_state() { + fn system(_: &mut World, _: &mut QueryState<&Foo>) {} + check_conflict(system); + } + + #[test] + fn no_conflict_mut_world_system_state() { + fn system(_: &mut World, _: &mut SystemState>) {} + check_conflict(system); + } + + #[test] + fn no_conflict_mut_world_system_state_recursive() { + fn system(_: &mut World, _: &mut SystemState<&mut SystemState>>) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_deferred_world_multiple() { + fn system(_: DeferredWorld, _: DeferredWorld) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_deferred_world_query() { + fn system(_: DeferredWorld, _: Query<&Foo>) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_query_deferred_world() { + fn system(_: Query<&Foo>, _: DeferredWorld) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_deferred_world_resource() { + fn system(_: DeferredWorld, _: Res) {} + check_conflict(system); + } + + #[test] + #[should_panic] + fn conflict_resource_deferred_world() { + fn system(_: Res, _: DeferredWorld) {} + check_conflict(system); + } + + #[test] + fn no_conflict_deferred_world_local() { + fn system(_: DeferredWorld, _: Local) {} + check_conflict(system); + } + + #[test] + fn no_conflict_deferred_world_query_state() { + fn system(_: DeferredWorld, _: &mut QueryState<&Foo>) {} + check_conflict(system); + } + + #[test] + fn no_conflict_deferred_world_system_state() { + fn system(_: DeferredWorld, _: &mut SystemState>) {} + check_conflict(system); + } } From 0fe16c60d0d7ac2cd8c5d574998aef6b680136b4 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Wed, 4 Dec 2024 10:37:58 -0600 Subject: [PATCH 8/8] mark &mut World and DeferredWorld system params as non-send --- crates/bevy_ecs/src/system/system_param.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 1a8c39a1fd24d..78e3e2bae5562 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -991,7 +991,6 @@ unsafe impl SystemParam for &'_ World { system_meta.archetype_component_access.extend(&access); let mut filtered_access = FilteredAccess::default(); - filtered_access.read_all(); if !system_meta .component_access_set @@ -1037,7 +1036,6 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { system_meta.archetype_component_access.extend(&access); let mut filtered_access = FilteredAccess::default(); - filtered_access.read_all(); filtered_access.write_all(); if !system_meta @@ -1048,7 +1046,9 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { panic!("{}", MUT_DEFERRED_WORLD_ERROR); } system_meta.component_access_set.add(filtered_access); + system_meta.set_has_deferred(); + system_meta.set_non_send(); } unsafe fn get_param<'world, 'state>( @@ -1087,7 +1087,6 @@ unsafe impl SystemParam for &mut World { system_meta.archetype_component_access.extend(&access); let mut filtered_access = FilteredAccess::default(); - filtered_access.read_all(); filtered_access.write_all(); if !system_meta @@ -1098,7 +1097,9 @@ unsafe impl SystemParam for &mut World { panic!("{}", MUT_WORLD_ERROR); } system_meta.component_access_set.add(filtered_access); + system_meta.set_has_deferred(); + system_meta.set_non_send(); } unsafe fn get_param<'world, 'state>(