From 9d48785f485673b7b0466a041960e29107bb8213 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Sat, 29 May 2021 15:35:08 -0700 Subject: [PATCH 1/6] Rename SystemState to SystemMeta. Add new SystemState --- crates/bevy_ecs/macros/src/lib.rs | 37 +-- .../{into_system.rs => function_system.rs} | 127 ++++++++-- crates/bevy_ecs/src/system/mod.rs | 56 ++++- crates/bevy_ecs/src/system/system_param.rs | 221 +++++++++++------- 4 files changed, 314 insertions(+), 127 deletions(-) rename crates/bevy_ecs/src/system/{into_system.rs => function_system.rs} (66%) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 1d4cb94c2a7c0..de79099c1cda0 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -221,40 +221,45 @@ pub fn impl_query_set(_input: TokenStream) -> TokenStream { type Fetch = QuerySetState<(#(QueryState<#query, #filter>,)*)>; } - // SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemState. If any QueryState conflicts + // SAFE: All Queries are constrained to ReadOnlyFetch, so World is only read + unsafe impl<#(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> ReadOnlySystemParamFetch for QuerySetState<(#(QueryState<#query, #filter>,)*)> + where #(#query::Fetch: ReadOnlyFetch,)* #(#filter::Fetch: FilterFetch,)* + { } + + // SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If any QueryState conflicts // with any prior access, a panic will occur. unsafe impl<#(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParamState for QuerySetState<(#(QueryState<#query, #filter>,)*)> where #(#filter::Fetch: FilterFetch,)* { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, config: Self::Config) -> Self { + fn init(world: &mut World, system_meta: &mut SystemMeta, config: Self::Config) -> Self { #( let mut #query = QueryState::<#query, #filter>::new(world); assert_component_access_compatibility( - &system_state.name, + &system_meta.name, std::any::type_name::<#query>(), std::any::type_name::<#filter>(), - &system_state.component_access_set, + &system_meta.component_access_set, &#query.component_access, world, ); )* #( - system_state + system_meta .component_access_set .add(#query.component_access.clone()); - system_state + system_meta .archetype_component_access .extend(&#query.archetype_component_access); )* QuerySetState((#(#query,)*)) } - fn new_archetype(&mut self, archetype: &Archetype, system_state: &mut SystemState) { + fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) { let (#(#query,)*) = &mut self.0; #( #query.new_archetype(archetype); - system_state + system_meta .archetype_component_access .extend(&#query.archetype_component_access); )* @@ -271,12 +276,12 @@ pub fn impl_query_set(_input: TokenStream) -> TokenStream { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { let (#(#query,)*) = &state.0; - QuerySet((#(Query::new(world, #query, system_state.last_change_tick, change_tick),)*)) + QuerySet((#(Query::new(world, #query, system_meta.last_change_tick, change_tick),)*)) } } @@ -388,15 +393,15 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { unsafe impl #path::system::SystemParamState for #fetch_struct_name { type Config = TSystemParamState::Config; - fn init(world: &mut #path::world::World, system_state: &mut #path::system::SystemState, config: Self::Config) -> Self { + fn init(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta, config: Self::Config) -> Self { Self { - state: TSystemParamState::init(world, system_state, config), + state: TSystemParamState::init(world, system_meta, config), marker: std::marker::PhantomData, } } - fn new_archetype(&mut self, archetype: &#path::archetype::Archetype, system_state: &mut #path::system::SystemState) { - self.state.new_archetype(archetype, system_state) + fn new_archetype(&mut self, archetype: &#path::archetype::Archetype, system_meta: &mut #path::system::SystemMeta) { + self.state.new_archetype(archetype, system_meta) } fn default_config() -> TSystemParamState::Config { @@ -412,12 +417,12 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { type Item = #struct_name#ty_generics; unsafe fn get_param( state: &'a mut Self, - system_state: &'a #path::system::SystemState, + system_meta: &'a #path::system::SystemMeta, world: &'a #path::world::World, change_tick: u32, ) -> Self::Item { #struct_name { - #(#fields: <<#field_types as SystemParam>::Fetch as #path::system::SystemParamFetch>::get_param(&mut state.state.#field_indices, system_state, world, change_tick),)* + #(#fields: <<#field_types as SystemParam>::Fetch as #path::system::SystemParamFetch>::get_param(&mut state.state.#field_indices, system_meta, world, change_tick),)* #(#ignored_fields: <#ignored_field_types>::default(),)* } } diff --git a/crates/bevy_ecs/src/system/into_system.rs b/crates/bevy_ecs/src/system/function_system.rs similarity index 66% rename from crates/bevy_ecs/src/system/into_system.rs rename to crates/bevy_ecs/src/system/function_system.rs index 97f3e0f62f0f2..4c3bcf885f136 100644 --- a/crates/bevy_ecs/src/system/into_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -3,26 +3,27 @@ use crate::{ component::ComponentId, query::{Access, FilteredAccessSet}, system::{ - check_system_change_tick, System, SystemId, SystemParam, SystemParamFetch, SystemParamState, + check_system_change_tick, ReadOnlySystemParamFetch, System, SystemId, SystemParam, + SystemParamFetch, SystemParamState, }, world::World, }; use bevy_ecs_macros::all_tuples; use std::{borrow::Cow, marker::PhantomData}; -/// The state of a [`System`]. -pub struct SystemState { +/// The metadata of a [`System`]. +pub struct SystemMeta { pub(crate) id: SystemId, pub(crate) name: Cow<'static, str>, pub(crate) component_access_set: FilteredAccessSet, pub(crate) archetype_component_access: Access, - // NOTE: this must be kept private. making a SystemState non-send is irreversible to prevent + // NOTE: this must be kept private. making a SystemMeta non-send is irreversible to prevent // SystemParams from overriding each other is_send: bool, pub(crate) last_change_tick: u32, } -impl SystemState { +impl SystemMeta { fn new() -> Self { Self { name: std::any::type_name::().into(), @@ -49,6 +50,86 @@ impl SystemState { } } +// TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference +// (to avoid the need for unwrapping to retrieve SystemMeta) +/// Holds on to persistent state required to drive [`SystemParam`] for a [`System`]. +pub struct SystemState { + meta: SystemMeta, + param_state: ::Fetch, + change_tick: u32, +} + +impl SystemState { + pub fn new(world: &mut World) -> Self { + let config = ::default_config(); + Self::with_config(world, config) + } + + pub fn with_config( + world: &mut World, + config: ::Config, + ) -> Self { + let mut meta = SystemMeta::new::(); + let param_state = ::init(world, &mut meta, config); + Self { + meta, + param_state, + change_tick: 0, + } + } + + #[inline] + pub fn meta(&self) -> &SystemMeta { + &self.meta + } + + /// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only. + pub fn get<'a>(&'a mut self, world: &'a World) -> >::Item + where + Param::Fetch: ReadOnlySystemParamFetch, + { + // SAFE: Param is read-only and doesn't allow mutable access to World + unsafe { self.get_unchecked(world) } + } + + /// Retrieve the mutable [`SystemParam`] values. + pub fn get_mut<'a>( + &'a mut self, + world: &'a mut World, + ) -> >::Item { + // SAFE: World is uniquely borrowed + unsafe { self.get_unchecked(world) } + } + + /// Applies all state queued up for [`SystemParam`] values. For example, this will apply commands queued up + /// by a [`Commands`](`super::Commands`) parameter to the given [`World`]. + /// This function should be called manually after the values returned by [`SystemState::get`] and [`SystemState::get_mut`] + /// are finished being used. + pub fn apply(&mut self, world: &mut World) { + self.param_state.apply(world); + } + + /// Retrieve the [`SystemParam`] values. + /// + /// # Safety + /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data + /// access is safe in the context of global [`World`] access. + #[inline] + pub unsafe fn get_unchecked<'a>( + &'a mut self, + world: &'a World, + ) -> >::Item { + let change_tick = world.increment_change_tick(); + self.change_tick = change_tick; + ::get_param( + &mut self.param_state, + &self.meta, + world, + 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 @@ -116,7 +197,7 @@ where { func: F, param_state: Option, - system_state: SystemState, + system_meta: SystemMeta, config: Option<::Config>, // NOTE: PhantomData T> gives this safe Send/Sync impls #[allow(clippy::type_complexity)] @@ -161,7 +242,7 @@ where func: self, param_state: None, config: Some(::default_config()), - system_state: SystemState::new::(), + system_meta: SystemMeta::new::(), marker: PhantomData, } } @@ -180,33 +261,33 @@ where #[inline] fn name(&self) -> Cow<'static, str> { - self.system_state.name.clone() + self.system_meta.name.clone() } #[inline] fn id(&self) -> SystemId { - self.system_state.id + self.system_meta.id } #[inline] fn new_archetype(&mut self, archetype: &Archetype) { let param_state = self.param_state.as_mut().unwrap(); - param_state.new_archetype(archetype, &mut self.system_state); + param_state.new_archetype(archetype, &mut self.system_meta); } #[inline] fn component_access(&self) -> &Access { - &self.system_state.component_access_set.combined_access() + &self.system_meta.component_access_set.combined_access() } #[inline] fn archetype_component_access(&self) -> &Access { - &self.system_state.archetype_component_access + &self.system_meta.archetype_component_access } #[inline] fn is_send(&self) -> bool { - self.system_state.is_send + self.system_meta.is_send } #[inline] @@ -215,11 +296,11 @@ where let out = self.func.run( input, self.param_state.as_mut().unwrap(), - &self.system_state, + &self.system_meta, world, change_tick, ); - self.system_state.last_change_tick = change_tick; + self.system_meta.last_change_tick = change_tick; out } @@ -233,7 +314,7 @@ where fn initialize(&mut self, world: &mut World) { self.param_state = Some(::init( world, - &mut self.system_state, + &mut self.system_meta, self.config.take().unwrap(), )); } @@ -241,9 +322,9 @@ where #[inline] fn check_change_tick(&mut self, change_tick: u32) { check_system_change_tick( - &mut self.system_state.last_change_tick, + &mut self.system_meta.last_change_tick, change_tick, - self.system_state.name.as_ref(), + self.system_meta.name.as_ref(), ); } } @@ -254,7 +335,7 @@ pub trait SystemParamFunction: Send + Sync &mut self, input: In, state: &mut Param::Fetch, - system_state: &SystemState, + system_meta: &SystemMeta, world: &World, change_tick: u32, ) -> Out; @@ -270,9 +351,9 @@ macro_rules! impl_system_function { FnMut($(<<$param as SystemParam>::Fetch as SystemParamFetch>::Item),*) -> Out + Send + Sync + 'static, Out: 'static { #[inline] - fn run(&mut self, _input: (), state: &mut <($($param,)*) as SystemParam>::Fetch, system_state: &SystemState, world: &World, change_tick: u32) -> Out { + fn run(&mut self, _input: (), state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out { unsafe { - let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_state, world, change_tick); + let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick); self($($param),*) } } @@ -286,9 +367,9 @@ macro_rules! impl_system_function { FnMut(In, $(<<$param as SystemParam>::Fetch as SystemParamFetch>::Item),*) -> Out + Send + Sync + 'static, Out: 'static { #[inline] - fn run(&mut self, input: Input, state: &mut <($($param,)*) as SystemParam>::Fetch, system_state: &SystemState, world: &World, change_tick: u32) -> Out { + fn run(&mut self, input: Input, state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out { unsafe { - let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_state, world, change_tick); + let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick); self(In(input), $($param),*) } } diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index b10ec03c6506e..6ba08a8a71c5b 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1,6 +1,6 @@ mod commands; mod exclusive_system; -mod into_system; +mod function_system; mod query; #[allow(clippy::module_inception)] mod system; @@ -9,7 +9,7 @@ mod system_param; pub use commands::*; pub use exclusive_system::*; -pub use into_system::*; +pub use function_system::*; pub use query::*; pub use system::*; pub use system_chaining::*; @@ -28,7 +28,7 @@ mod tests { schedule::{Schedule, Stage, SystemStage}, system::{ IntoExclusiveSystem, IntoSystem, Local, Query, QuerySet, RemovedComponents, Res, - ResMut, System, + ResMut, System, SystemState, }, world::{FromWorld, World}, }; @@ -485,4 +485,54 @@ mod tests { x.initialize(&mut world); y.initialize(&mut world); } + + #[test] + fn read_system_param() { + #[derive(Eq, PartialEq, Debug)] + struct A(usize); + + #[derive(Eq, PartialEq, Debug)] + struct B(usize); + + let mut world = World::default(); + world.insert_resource(A(42)); + world.spawn().insert(B(7)); + + let mut system_state: SystemState<(Res, Query<&B>, QuerySet<(Query<&C>, Query<&D>)>)> = + SystemState::new(&mut world); + let (a, query, _) = system_state.get(&world); + assert_eq!(*a, A(42), "returned resource matches initial value"); + assert_eq!( + *query.single().unwrap(), + B(7), + "returned component matches initial value" + ); + } + + #[test] + fn write_system_param() { + #[derive(Eq, PartialEq, Debug)] + struct A(usize); + + #[derive(Eq, PartialEq, Debug)] + struct B(usize); + + let mut world = World::default(); + world.insert_resource(A(42)); + world.spawn().insert(B(7)); + + let mut system_state: SystemState<(ResMut, Query<&mut B>)> = + SystemState::new(&mut world); + + // The following line shouldn't compile because the parameters used are not ReadOnlySystemParam + // let (a, query) = system_state.get(&world); + + let (a, mut query) = system_state.get_mut(&mut world); + assert_eq!(*a, A(42), "returned resource matches initial value"); + assert_eq!( + *query.single_mut().unwrap(), + B(7), + "returned component matches initial value" + ); + } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 07a8499971396..9cd46476593bc 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -5,8 +5,10 @@ use crate::{ change_detection::Ticks, component::{Component, ComponentId, ComponentTicks, Components}, entity::{Entities, Entity}, - query::{FilterFetch, FilteredAccess, FilteredAccessSet, QueryState, WorldQuery}, - system::{CommandQueue, Commands, Query, SystemState}, + query::{ + FilterFetch, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyFetch, WorldQuery, + }, + system::{CommandQueue, Commands, Query, SystemMeta}, world::{FromWorld, World}, }; pub use bevy_ecs_macros::SystemParam; @@ -46,7 +48,7 @@ pub trait SystemParam: Sized { /// /// # Safety /// -/// It is the implementor's responsibility to ensure `system_state` is populated with the _exact_ +/// It is the implementor's responsibility to ensure `system_meta` is populated with the _exact_ /// [`World`] access used by the `SystemParamState` (and associated [`SystemParamFetch`]). /// Additionally, it is the implementor's responsibility to ensure there is no /// conflicting access across all SystemParams. @@ -64,14 +66,20 @@ pub unsafe trait SystemParamState: Send + Sync + 'static { /// See [`FunctionSystem::config`](super::FunctionSystem::config) /// for more information and examples. type Config: Send + Sync; - fn init(world: &mut World, system_state: &mut SystemState, config: Self::Config) -> Self; + fn init(world: &mut World, system_meta: &mut SystemMeta, config: Self::Config) -> Self; #[inline] - fn new_archetype(&mut self, _archetype: &Archetype, _system_state: &mut SystemState) {} + fn new_archetype(&mut self, _archetype: &Archetype, _system_meta: &mut SystemMeta) {} #[inline] fn apply(&mut self, _world: &mut World) {} fn default_config() -> Self::Config; } +/// A [`SystemParamFetch`] that only reads a given [`World`]. +/// +/// # Safety +/// This must only be implemented for [`SystemParamFetch`] impls that exclusively read the World passed in to [`SystemParamFetch::get_param`] +pub unsafe trait ReadOnlySystemParamFetch {} + pub trait SystemParamFetch<'a>: SystemParamState { type Item; /// # Safety @@ -80,14 +88,12 @@ pub trait SystemParamFetch<'a>: SystemParamState { /// access is safe in the context of the system scheduler. unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item; } -pub struct QueryFetch(PhantomData<(Q, F)>); - impl<'a, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParam for Query<'a, Q, F> where F::Fetch: FilterFetch, @@ -95,7 +101,15 @@ where type Fetch = QueryState; } -// SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemState. If +// SAFE: QueryState is constrained to read-only fetches, so it only reads World. +unsafe impl ReadOnlySystemParamFetch for QueryState +where + Q::Fetch: ReadOnlyFetch, + F::Fetch: FilterFetch, +{ +} + +// SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this QueryState conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for QueryState where @@ -103,28 +117,28 @@ where { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { let state = QueryState::new(world); assert_component_access_compatibility( - &system_state.name, + &system_meta.name, std::any::type_name::(), std::any::type_name::(), - &system_state.component_access_set, + &system_meta.component_access_set, &state.component_access, world, ); - system_state + system_meta .component_access_set .add(state.component_access.clone()); - system_state + system_meta .archetype_component_access .extend(&state.archetype_component_access); state } - fn new_archetype(&mut self, archetype: &Archetype, system_state: &mut SystemState) { + fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta) { self.new_archetype(archetype); - system_state + system_meta .archetype_component_access .extend(&self.archetype_component_access); } @@ -141,11 +155,11 @@ where #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { - Query::new(world, state, system_state.last_change_tick, change_tick) + Query::new(world, state, system_meta.last_change_tick, change_tick) } } @@ -189,6 +203,9 @@ pub struct Res<'w, T: Component> { change_tick: u32, } +// SAFE: Res only reads a single World resource +unsafe impl ReadOnlySystemParamFetch for ResState {} + impl<'w, T: Component> Debug for Res<'w, T> where T: Debug, @@ -238,18 +255,18 @@ impl<'a, T: Component> SystemParam for Res<'a, T> { type Fetch = ResState; } -// SAFE: Res ComponentId and ArchetypeComponentId access is applied to SystemState. If this Res +// SAFE: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res // conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for ResState { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { let component_id = world.initialize_resource::(); - let combined_access = system_state.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( "Res<{}> in system {} conflicts with a previous ResMut<{0}> access. Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", - std::any::type_name::(), system_state.name); + std::any::type_name::(), system_meta.name); } combined_access.add_read(component_id); @@ -257,7 +274,7 @@ unsafe impl SystemParamState for ResState { let archetype_component_id = resource_archetype .get_archetype_component_id(component_id) .unwrap(); - system_state + system_meta .archetype_component_access .add_read(archetype_component_id); Self { @@ -275,7 +292,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -284,14 +301,14 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResState { .unwrap_or_else(|| { panic!( "Resource requested by {} does not exist: {}", - system_state.name, + system_meta.name, std::any::type_name::() ) }); Res { value: &*column.get_data_ptr().cast::().as_ptr(), ticks: column.get_ticks_unchecked(0), - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, } } @@ -304,11 +321,14 @@ impl<'a, T: Component> SystemParam for Option> { type Fetch = OptionResState; } +// SAFE: Only reads a single World resource +unsafe impl ReadOnlySystemParamFetch for OptionResState {} + unsafe impl SystemParamState for OptionResState { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { - Self(ResState::init(world, system_state, ())) + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { + Self(ResState::init(world, system_meta, ())) } fn default_config() {} @@ -320,7 +340,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -329,7 +349,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState { .map(|column| Res { value: &*column.get_data_ptr().cast::().as_ptr(), ticks: column.get_ticks_unchecked(0), - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, }) } @@ -345,22 +365,22 @@ impl<'a, T: Component> SystemParam for ResMut<'a, T> { type Fetch = ResMutState; } -// SAFE: Res ComponentId and ArchetypeComponentId access is applied to SystemState. If this Res +// SAFE: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res // conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for ResMutState { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { let component_id = world.initialize_resource::(); - let combined_access = system_state.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( "ResMut<{}> in system {} conflicts with a previous ResMut<{0}> access. Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", - std::any::type_name::(), system_state.name); + std::any::type_name::(), system_meta.name); } else if combined_access.has_read(component_id) { panic!( "ResMut<{}> in system {} conflicts with a previous Res<{0}> access. Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", - std::any::type_name::(), system_state.name); + std::any::type_name::(), system_meta.name); } combined_access.add_write(component_id); @@ -368,7 +388,7 @@ unsafe impl SystemParamState for ResMutState { let archetype_component_id = resource_archetype .get_archetype_component_id(component_id) .unwrap(); - system_state + system_meta .archetype_component_access .add_write(archetype_component_id); Self { @@ -386,7 +406,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResMutState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -395,7 +415,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResMutState { .unwrap_or_else(|| { panic!( "Resource requested by {} does not exist: {}", - system_state.name, + system_meta.name, std::any::type_name::() ) }); @@ -403,7 +423,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResMutState { value: value.value, ticks: Ticks { component_ticks: value.ticks.component_ticks, - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, }, } @@ -420,8 +440,8 @@ impl<'a, T: Component> SystemParam for Option> { unsafe impl SystemParamState for OptionResMutState { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { - Self(ResMutState::init(world, system_state, ())) + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { + Self(ResMutState::init(world, system_meta, ())) } fn default_config() {} @@ -433,7 +453,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResMutState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -443,7 +463,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResMutState { value: value.value, ticks: Ticks { component_ticks: value.ticks.component_ticks, - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, }, }) @@ -454,11 +474,14 @@ impl<'a> SystemParam for Commands<'a> { type Fetch = CommandQueue; } +// SAFE: Commands only accesses internal state +unsafe impl ReadOnlySystemParamFetch for CommandQueue {} + // SAFE: only local state is accessed unsafe impl SystemParamState for CommandQueue { type Config = (); - fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Default::default() } @@ -475,7 +498,7 @@ impl<'a> SystemParamFetch<'a> for CommandQueue { #[inline] unsafe fn get_param( state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -511,6 +534,9 @@ impl<'a> SystemParamFetch<'a> for CommandQueue { /// ``` pub struct Local<'a, T: Component>(&'a mut T); +// SAFE: Local only accesses internal state +unsafe impl ReadOnlySystemParamFetch for LocalState {} + impl<'a, T: Component> Debug for Local<'a, T> where T: Debug, @@ -547,7 +573,7 @@ impl<'a, T: Component + FromWorld> SystemParam for Local<'a, T> { unsafe impl SystemParamState for LocalState { type Config = Option; - fn init(world: &mut World, _system_state: &mut SystemState, config: Self::Config) -> Self { + fn init(world: &mut World, _system_meta: &mut SystemMeta, config: Self::Config) -> Self { Self(config.unwrap_or_else(|| T::from_world(world))) } @@ -562,7 +588,7 @@ impl<'a, T: Component + FromWorld> SystemParamFetch<'a> for LocalState { #[inline] unsafe fn get_param( state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, _world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -601,6 +627,9 @@ impl<'a, T> RemovedComponents<'a, T> { } } +// SAFE: Only reads World components +unsafe impl ReadOnlySystemParamFetch for RemovedComponentsState {} + /// The [`SystemParamState`] of [`RemovedComponents`]. pub struct RemovedComponentsState { component_id: ComponentId, @@ -616,7 +645,7 @@ impl<'a, T: Component> SystemParam for RemovedComponents<'a, T> { unsafe impl SystemParamState for RemovedComponentsState { type Config = (); - fn init(world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Self { component_id: world.components.get_or_insert_id::(), marker: PhantomData, @@ -632,7 +661,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for RemovedComponentsState { #[inline] unsafe fn get_param( state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -661,6 +690,9 @@ pub struct NonSend<'w, T> { change_tick: u32, } +// SAFE: Only reads a single World non-send resource +unsafe impl ReadOnlySystemParamFetch for NonSendState {} + impl<'w, T> Debug for NonSend<'w, T> where T: Debug, @@ -703,20 +735,20 @@ impl<'a, T: 'static> SystemParam for NonSend<'a, T> { type Fetch = NonSendState; } -// SAFE: NonSendComponentId and ArchetypeComponentId access is applied to SystemState. If this +// SAFE: NonSendComponentId and ArchetypeComponentId access is applied to SystemMeta. If this // NonSend conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for NonSendState { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { - system_state.set_non_send(); + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { + system_meta.set_non_send(); let component_id = world.initialize_non_send_resource::(); - let combined_access = system_state.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( "NonSend<{}> in system {} conflicts with a previous mutable resource access ({0}). Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", - std::any::type_name::(), system_state.name); + std::any::type_name::(), system_meta.name); } combined_access.add_read(component_id); @@ -724,7 +756,7 @@ unsafe impl SystemParamState for NonSendState { let archetype_component_id = resource_archetype .get_archetype_component_id(component_id) .unwrap(); - system_state + system_meta .archetype_component_access .add_read(archetype_component_id); Self { @@ -742,7 +774,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -752,7 +784,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { .unwrap_or_else(|| { panic!( "Non-send resource requested by {} does not exist: {}", - system_state.name, + system_meta.name, std::any::type_name::() ) }); @@ -760,7 +792,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { NonSend { value: &*column.get_data_ptr().cast::().as_ptr(), ticks: column.get_ticks_unchecked(0).clone(), - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, } } @@ -831,24 +863,24 @@ impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { type Fetch = NonSendMutState; } -// SAFE: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemState. If this +// SAFE: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this // NonSendMut conflicts with any prior access, a panic will occur. unsafe impl SystemParamState for NonSendMutState { type Config = (); - fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self { - system_state.set_non_send(); + fn init(world: &mut World, system_meta: &mut SystemMeta, _config: Self::Config) -> Self { + system_meta.set_non_send(); let component_id = world.components.get_or_insert_non_send_resource_id::(); - let combined_access = system_state.component_access_set.combined_access_mut(); + let combined_access = system_meta.component_access_set.combined_access_mut(); if combined_access.has_write(component_id) { panic!( "NonSendMut<{}> in system {} conflicts with a previous mutable resource access ({0}). Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", - std::any::type_name::(), system_state.name); + std::any::type_name::(), system_meta.name); } else if combined_access.has_read(component_id) { panic!( "NonSendMut<{}> in system {} conflicts with a previous immutable resource access ({0}). Allowing this would break Rust's mutability rules. Consider removing the duplicate access.", - std::any::type_name::(), system_state.name); + std::any::type_name::(), system_meta.name); } combined_access.add_write(component_id); @@ -856,7 +888,7 @@ unsafe impl SystemParamState for NonSendMutState { let archetype_component_id = resource_archetype .get_archetype_component_id(component_id) .unwrap(); - system_state + system_meta .archetype_component_access .add_write(archetype_component_id); Self { @@ -874,7 +906,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendMutState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -884,14 +916,14 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendMutState { .unwrap_or_else(|| { panic!( "Non-send resource requested by {} does not exist: {}", - system_state.name, + system_meta.name, std::any::type_name::() ) }); NonSendMut { value: &mut *column.get_data_ptr().cast::().as_ptr(), ticks: &mut *column.get_ticks_mut_ptr_unchecked(0), - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, } } @@ -901,6 +933,9 @@ impl<'a> SystemParam for &'a Archetypes { type Fetch = ArchetypesState; } +// SAFE: Only reads World archetypes +unsafe impl ReadOnlySystemParamFetch for ArchetypesState {} + /// The [`SystemParamState`] of [`Archetypes`]. pub struct ArchetypesState; @@ -908,7 +943,7 @@ pub struct ArchetypesState; unsafe impl SystemParamState for ArchetypesState { type Config = (); - fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Self } @@ -921,7 +956,7 @@ impl<'a> SystemParamFetch<'a> for ArchetypesState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -933,6 +968,9 @@ impl<'a> SystemParam for &'a Components { type Fetch = ComponentsState; } +// SAFE: Only reads World components +unsafe impl ReadOnlySystemParamFetch for ComponentsState {} + /// The [`SystemParamState`] of [`Components`]. pub struct ComponentsState; @@ -940,7 +978,7 @@ pub struct ComponentsState; unsafe impl SystemParamState for ComponentsState { type Config = (); - fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Self } @@ -953,7 +991,7 @@ impl<'a> SystemParamFetch<'a> for ComponentsState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -965,6 +1003,9 @@ impl<'a> SystemParam for &'a Entities { type Fetch = EntitiesState; } +// SAFE: Only reads World entities +unsafe impl ReadOnlySystemParamFetch for EntitiesState {} + /// The [`SystemParamState`] of [`Entities`]. pub struct EntitiesState; @@ -972,7 +1013,7 @@ pub struct EntitiesState; unsafe impl SystemParamState for EntitiesState { type Config = (); - fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Self } @@ -985,7 +1026,7 @@ impl<'a> SystemParamFetch<'a> for EntitiesState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -997,6 +1038,9 @@ impl<'a> SystemParam for &'a Bundles { type Fetch = BundlesState; } +// SAFE: Only reads World bundles +unsafe impl ReadOnlySystemParamFetch for BundlesState {} + /// The [`SystemParamState`] of [`Bundles`]. pub struct BundlesState; @@ -1004,7 +1048,7 @@ pub struct BundlesState; unsafe impl SystemParamState for BundlesState { type Config = (); - fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Self } @@ -1017,7 +1061,7 @@ impl<'a> SystemParamFetch<'a> for BundlesState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_state: &'a SystemState, + _system_meta: &'a SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -1031,6 +1075,9 @@ pub struct SystemChangeTick { pub change_tick: u32, } +// SAFE: Only reads internal system state +unsafe impl ReadOnlySystemParamFetch for SystemChangeTickState {} + impl SystemParam for SystemChangeTick { type Fetch = SystemChangeTickState; } @@ -1041,7 +1088,7 @@ pub struct SystemChangeTickState {} unsafe impl SystemParamState for SystemChangeTickState { type Config = (); - fn init(_world: &mut World, _system_state: &mut SystemState, _config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, _config: Self::Config) -> Self { Self {} } @@ -1053,12 +1100,12 @@ impl<'a> SystemParamFetch<'a> for SystemChangeTickState { unsafe fn get_param( _state: &mut Self, - system_state: &SystemState, + system_meta: &SystemMeta, _world: &World, change_tick: u32, ) -> Self::Item { SystemChangeTick { - last_change_tick: system_state.last_change_tick, + last_change_tick: system_meta.last_change_tick, change_tick, } } @@ -1069,6 +1116,10 @@ macro_rules! impl_system_param_tuple { impl<$($param: SystemParam),*> SystemParam for ($($param,)*) { type Fetch = ($($param::Fetch,)*); } + + // SAFE: tuple consists only of ReadOnlySystemParamFetches + unsafe impl<$($param: ReadOnlySystemParamFetch),*> ReadOnlySystemParamFetch for ($($param,)*) {} + #[allow(unused_variables)] #[allow(non_snake_case)] impl<'a, $($param: SystemParamFetch<'a>),*> SystemParamFetch<'a> for ($($param,)*) { @@ -1077,13 +1128,13 @@ macro_rules! impl_system_param_tuple { #[inline] unsafe fn get_param( state: &'a mut Self, - system_state: &'a SystemState, + system_meta: &'a SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { let ($($param,)*) = state; - ($($param::get_param($param, system_state, world, change_tick),)*) + ($($param::get_param($param, system_meta, world, change_tick),)*) } } @@ -1092,15 +1143,15 @@ macro_rules! impl_system_param_tuple { unsafe impl<$($param: SystemParamState),*> SystemParamState for ($($param,)*) { type Config = ($(<$param as SystemParamState>::Config,)*); #[inline] - fn init(_world: &mut World, _system_state: &mut SystemState, config: Self::Config) -> Self { + fn init(_world: &mut World, _system_meta: &mut SystemMeta, config: Self::Config) -> Self { let ($($param,)*) = config; - (($($param::init(_world, _system_state, $param),)*)) + (($($param::init(_world, _system_meta, $param),)*)) } #[inline] - fn new_archetype(&mut self, _archetype: &Archetype, _system_state: &mut SystemState) { + fn new_archetype(&mut self, _archetype: &Archetype, _system_meta: &mut SystemMeta) { let ($($param,)*) = self; - $($param.new_archetype(_archetype, _system_state);)* + $($param.new_archetype(_archetype, _system_meta);)* } #[inline] From fc720c6a5910c5178781c3bd18c0c40e0a93dd5a Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 1 Jun 2021 13:03:37 -0700 Subject: [PATCH 2/6] add change detection test --- crates/bevy_ecs/src/system/mod.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 6ba08a8a71c5b..b053b6cef825e 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -535,4 +535,30 @@ mod tests { "returned component matches initial value" ); } + + #[test] + fn system_param_change_detection() { + #[derive(Eq, PartialEq, Debug)] + struct A(usize); + + let mut world = World::default(); + let entity = world.spawn().insert(A(1)).id(); + + let mut system_state: SystemState>> = SystemState::new(&mut world); + { + let query = system_state.get(&world); + assert_eq!(*query.single().unwrap(), A(1)); + } + + { + let query = system_state.get(&world); + assert!(query.single().is_err()); + } + + world.entity_mut(entity).get_mut::().unwrap().0 = 2; + { + let query = system_state.get(&world); + assert_eq!(*query.single().unwrap(), A(2)); + } + } } From aded69e5f8e87a52fff0e713a31552442af649c9 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 1 Jun 2021 13:04:14 -0700 Subject: [PATCH 3/6] fix SystemState change detection --- crates/bevy_ecs/macros/src/lib.rs | 4 +-- crates/bevy_ecs/src/system/function_system.rs | 14 +++----- crates/bevy_ecs/src/system/system_param.rs | 32 +++++++++---------- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index de79099c1cda0..27383ff71ee55 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -276,7 +276,7 @@ pub fn impl_query_set(_input: TokenStream) -> TokenStream { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -417,7 +417,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { type Item = #struct_name#ty_generics; unsafe fn get_param( state: &'a mut Self, - system_meta: &'a #path::system::SystemMeta, + system_meta: &#path::system::SystemMeta, world: &'a #path::world::World, change_tick: u32, ) -> Self::Item { diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 4c3bcf885f136..d7aad8e42a32a 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -56,7 +56,6 @@ impl SystemMeta { pub struct SystemState { meta: SystemMeta, param_state: ::Fetch, - change_tick: u32, } impl SystemState { @@ -71,11 +70,7 @@ impl SystemState { ) -> Self { let mut meta = SystemMeta::new::(); let param_state = ::init(world, &mut meta, config); - Self { - meta, - param_state, - change_tick: 0, - } + Self { meta, param_state } } #[inline] @@ -120,13 +115,14 @@ impl SystemState { world: &'a World, ) -> >::Item { let change_tick = world.increment_change_tick(); - self.change_tick = change_tick; - ::get_param( + let param = ::get_param( &mut self.param_state, &self.meta, world, change_tick, - ) + ); + self.meta.last_change_tick = change_tick; + param } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 9cd46476593bc..95dd4cee1bc83 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -88,7 +88,7 @@ pub trait SystemParamFetch<'a>: SystemParamState { /// access is safe in the context of the system scheduler. unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item; @@ -155,7 +155,7 @@ where #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -292,7 +292,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -340,7 +340,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -406,7 +406,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResMutState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -453,7 +453,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResMutState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -498,7 +498,7 @@ impl<'a> SystemParamFetch<'a> for CommandQueue { #[inline] unsafe fn get_param( state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -588,7 +588,7 @@ impl<'a, T: Component + FromWorld> SystemParamFetch<'a> for LocalState { #[inline] unsafe fn get_param( state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, _world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -661,7 +661,7 @@ impl<'a, T: Component> SystemParamFetch<'a> for RemovedComponentsState { #[inline] unsafe fn get_param( state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -774,7 +774,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -906,7 +906,7 @@ impl<'a, T: 'static> SystemParamFetch<'a> for NonSendMutState { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { @@ -956,7 +956,7 @@ impl<'a> SystemParamFetch<'a> for ArchetypesState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -991,7 +991,7 @@ impl<'a> SystemParamFetch<'a> for ComponentsState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -1026,7 +1026,7 @@ impl<'a> SystemParamFetch<'a> for EntitiesState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -1061,7 +1061,7 @@ impl<'a> SystemParamFetch<'a> for BundlesState { #[inline] unsafe fn get_param( _state: &'a mut Self, - _system_meta: &'a SystemMeta, + _system_meta: &SystemMeta, world: &'a World, _change_tick: u32, ) -> Self::Item { @@ -1128,7 +1128,7 @@ macro_rules! impl_system_param_tuple { #[inline] unsafe fn get_param( state: &'a mut Self, - system_meta: &'a SystemMeta, + system_meta: &SystemMeta, world: &'a World, change_tick: u32, ) -> Self::Item { From 4b038ed066be5d2745714cc1d409034d321197ab Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 1 Jun 2021 13:12:24 -0700 Subject: [PATCH 4/6] Mark SystemParamFunction::run as unsafe. inline get/get_mut --- crates/bevy_ecs/src/system/function_system.rs | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index d7aad8e42a32a..06186347d5ce5 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -79,6 +79,7 @@ impl SystemState { } /// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only. + #[inline] pub fn get<'a>(&'a mut self, world: &'a World) -> >::Item where Param::Fetch: ReadOnlySystemParamFetch, @@ -88,6 +89,7 @@ impl SystemState { } /// Retrieve the mutable [`SystemParam`] values. + #[inline] pub fn get_mut<'a>( &'a mut self, world: &'a mut World, @@ -327,7 +329,11 @@ where /// A trait implemented for all functions that can be used as [`System`]s. pub trait SystemParamFunction: Send + Sync + 'static { - fn run( + /// # Safety + /// + /// This call might access any of the input parameters in an unsafe way. Make sure the data + /// access is safe in the context of the system scheduler. + unsafe fn run( &mut self, input: In, state: &mut Param::Fetch, @@ -347,11 +353,9 @@ macro_rules! impl_system_function { FnMut($(<<$param as SystemParam>::Fetch as SystemParamFetch>::Item),*) -> Out + Send + Sync + 'static, Out: 'static { #[inline] - fn run(&mut self, _input: (), state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out { - unsafe { - let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick); - self($($param),*) - } + unsafe fn run(&mut self, _input: (), state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out { + let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick); + self($($param),*) } } @@ -363,11 +367,9 @@ macro_rules! impl_system_function { FnMut(In, $(<<$param as SystemParam>::Fetch as SystemParamFetch>::Item),*) -> Out + Send + Sync + 'static, Out: 'static { #[inline] - fn run(&mut self, input: Input, state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out { - unsafe { - let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick); - self(In(input), $($param),*) - } + unsafe fn run(&mut self, input: Input, state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out { + let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick); + self(In(input), $($param),*) } } }; From 4a1524907c50495207a12b2c2cd4d9f828013fbd Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 1 Jun 2021 17:16:48 -0700 Subject: [PATCH 5/6] World validation --- crates/bevy_ecs/src/system/function_system.rs | 28 +++++++++++++++---- crates/bevy_ecs/src/system/mod.rs | 15 ++++++++-- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 06186347d5ce5..7307aef5f3482 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -6,7 +6,7 @@ use crate::{ check_system_change_tick, ReadOnlySystemParamFetch, System, SystemId, SystemParam, SystemParamFetch, SystemParamState, }, - world::World, + world::{World, WorldId}, }; use bevy_ecs_macros::all_tuples; use std::{borrow::Cow, marker::PhantomData}; @@ -56,6 +56,7 @@ impl SystemMeta { pub struct SystemState { meta: SystemMeta, param_state: ::Fetch, + world_id: WorldId, } impl SystemState { @@ -70,7 +71,11 @@ impl SystemState { ) -> Self { let mut meta = SystemMeta::new::(); let param_state = ::init(world, &mut meta, config); - Self { meta, param_state } + Self { + meta, + param_state, + world_id: world.id(), + } } #[inline] @@ -84,7 +89,8 @@ impl SystemState { where Param::Fetch: ReadOnlySystemParamFetch, { - // SAFE: Param is read-only and doesn't allow mutable access to World + self.validate_world(world); + // SAFE: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with unsafe { self.get_unchecked(world) } } @@ -94,7 +100,8 @@ impl SystemState { &'a mut self, world: &'a mut World, ) -> >::Item { - // SAFE: World is uniquely borrowed + self.validate_world(world); + // SAFE: World is uniquely borrowed and matches the World this SystemState was created with unsafe { self.get_unchecked(world) } } @@ -106,11 +113,22 @@ impl SystemState { self.param_state.apply(world); } + #[inline] + pub fn matches_world(&self, world: &World) -> bool { + self.world_id == world.id() + } + + #[inline] + fn validate_world(&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.") + } + /// Retrieve the [`SystemParam`] values. /// /// # Safety /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data - /// access is safe in the context of global [`World`] access. + /// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was + /// created with. #[inline] pub unsafe fn get_unchecked<'a>( &'a mut self, diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index b053b6cef825e..2c12e422c19d3 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -487,7 +487,7 @@ mod tests { } #[test] - fn read_system_param() { + fn read_system_state() { #[derive(Eq, PartialEq, Debug)] struct A(usize); @@ -510,7 +510,7 @@ mod tests { } #[test] - fn write_system_param() { + fn write_system_state() { #[derive(Eq, PartialEq, Debug)] struct A(usize); @@ -537,7 +537,7 @@ mod tests { } #[test] - fn system_param_change_detection() { + fn system_state_change_detection() { #[derive(Eq, PartialEq, Debug)] struct A(usize); @@ -561,4 +561,13 @@ mod tests { assert_eq!(*query.single().unwrap(), A(2)); } } + + #[test] + #[should_panic] + fn system_state_invalid_world() { + let mut world = World::default(); + let mut system_state = SystemState::>::new(&mut world); + let mismatched_world = World::default(); + system_state.get(&mismatched_world); + } } From a3f1b20f680bf50d1d3b692f018eb3326657d8ed Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 1 Jun 2021 17:38:53 -0700 Subject: [PATCH 6/6] Update archetypes automatically --- crates/bevy_ecs/src/system/function_system.rs | 36 ++++++++++++------- crates/bevy_ecs/src/system/mod.rs | 32 +++++++++++++++++ 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 7307aef5f3482..414ba2c3fec26 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -1,5 +1,5 @@ use crate::{ - archetype::{Archetype, ArchetypeComponentId}, + archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, component::ComponentId, query::{Access, FilteredAccessSet}, system::{ @@ -57,6 +57,7 @@ pub struct SystemState { meta: SystemMeta, param_state: ::Fetch, world_id: WorldId, + archetype_generation: ArchetypeGeneration, } impl SystemState { @@ -75,6 +76,7 @@ impl SystemState { meta, param_state, world_id: world.id(), + archetype_generation: ArchetypeGeneration::initial(), } } @@ -89,9 +91,9 @@ impl SystemState { where Param::Fetch: ReadOnlySystemParamFetch, { - self.validate_world(world); - // SAFE: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with - unsafe { self.get_unchecked(world) } + self.validate_world_and_update_archetypes(world); + // SAFE: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. + unsafe { self.get_unchecked_manual(world) } } /// Retrieve the mutable [`SystemParam`] values. @@ -100,9 +102,9 @@ impl SystemState { &'a mut self, world: &'a mut World, ) -> >::Item { - self.validate_world(world); - // SAFE: World is uniquely borrowed and matches the World this SystemState was created with - unsafe { self.get_unchecked(world) } + self.validate_world_and_update_archetypes(world); + // SAFE: World is uniquely borrowed and matches the World this SystemState was created with. + unsafe { self.get_unchecked_manual(world) } } /// Applies all state queued up for [`SystemParam`] values. For example, this will apply commands queued up @@ -118,19 +120,29 @@ impl SystemState { self.world_id == world.id() } - #[inline] - fn validate_world(&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.") + 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(); + let new_generation = archetypes.generation(); + let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); + let archetype_index_range = old_generation.value()..new_generation.value(); + + for archetype_index in archetype_index_range { + self.param_state.new_archetype( + &archetypes[ArchetypeId::new(archetype_index)], + &mut self.meta, + ); + } } - /// Retrieve the [`SystemParam`] values. + /// Retrieve the [`SystemParam`] values. This will not update archetypes automatically. /// /// # Safety /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data /// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was /// created with. #[inline] - pub unsafe fn get_unchecked<'a>( + pub unsafe fn get_unchecked_manual<'a>( &'a mut self, world: &'a World, ) -> >::Item { diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 2c12e422c19d3..06fdf53ce6eaa 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -570,4 +570,36 @@ mod tests { let mismatched_world = World::default(); system_state.get(&mismatched_world); } + + #[test] + fn system_state_archetype_update() { + #[derive(Eq, PartialEq, Debug)] + struct A(usize); + + #[derive(Eq, PartialEq, Debug)] + struct B(usize); + + let mut world = World::default(); + world.spawn().insert(A(1)); + + let mut system_state = SystemState::>::new(&mut world); + { + let query = system_state.get(&world); + assert_eq!( + query.iter().collect::>(), + vec![&A(1)], + "exactly one component returned" + ); + } + + world.spawn().insert_bundle((A(2), B(2))); + { + let query = system_state.get(&world); + assert_eq!( + query.iter().collect::>(), + vec![&A(1), &A(2)], + "components from both archetypes returned" + ); + } + } }