diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 4bdc7572e02e0..c5215775e5baf 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -324,6 +324,7 @@ pub struct ComponentDescriptor { // actually Send + Sync is_send_and_sync: bool, type_id: Option, + is_resource: bool, layout: Layout, // SAFETY: this function must be safe to call with pointers pointing to items of the type // this descriptor describes. @@ -357,6 +358,7 @@ impl ComponentDescriptor { storage_type: T::Storage::STORAGE_TYPE, is_send_and_sync: true, type_id: Some(TypeId::of::()), + is_resource: false, layout: Layout::new::(), drop: needs_drop::().then_some(Self::drop_ptr:: as _), } @@ -378,6 +380,7 @@ impl ComponentDescriptor { storage_type, is_send_and_sync: true, type_id: None, + is_resource: false, layout, drop, } @@ -394,6 +397,7 @@ impl ComponentDescriptor { storage_type: StorageType::Table, is_send_and_sync: true, type_id: Some(TypeId::of::()), + is_resource: true, layout: Layout::new::(), drop: needs_drop::().then_some(Self::drop_ptr:: as _), } @@ -405,6 +409,7 @@ impl ComponentDescriptor { storage_type, is_send_and_sync: false, type_id: Some(TypeId::of::()), + is_resource: true, layout: Layout::new::(), drop: needs_drop::().then_some(Self::drop_ptr:: as _), } @@ -468,7 +473,18 @@ impl Components { storages: &mut Storages, descriptor: ComponentDescriptor, ) -> ComponentId { - let index = Components::init_component_inner(&mut self.components, storages, descriptor); + let index = if let Some(type_id) = descriptor.type_id { + let indices = if descriptor.is_resource { + &mut self.resource_indices + } else { + &mut self.indices + }; + *indices.entry(type_id).or_insert_with(|| { + Components::init_component_inner(&mut self.components, storages, descriptor) + }) + } else { + Components::init_component_inner(&mut self.components, storages, descriptor) + }; ComponentId(index) } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 06001b584dbdc..d582b3f6b7694 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -65,6 +65,10 @@ type TypeIdMap = rustc_hash::FxHashMap; mod tests { use crate as bevy_ecs; use crate::prelude::Or; + use crate::schedule::{ + ComponentAccessSet, IntoSystemConfigs, ResourceAccessSet, Schedule, ScheduleBuildSettings, + }; + use crate::system::{Query, Res, ResMut}; use crate::{ bundle::Bundle, change_detection::Ref, @@ -1725,4 +1729,129 @@ mod tests { "new entity was spawned and received C component" ); } + + #[test] + fn read_component_with_access_set() { + #[derive(Component)] + struct Conflict; + + fn before_read(_: Query<&mut Conflict>) {} + fn does_read(_: Query<&A>, _: Query<&mut Conflict>) {} + + let mut world = World::new(); + let mut schedule = Schedule::default(); + + schedule.add_systems(( + before_read.before(ComponentAccessSet::Reads::), + does_read, + )); + + // Panic if there are ambiguities. + schedule.set_build_settings(ScheduleBuildSettings { + ambiguity_detection: bevy_ecs::schedule::LogLevel::Error, + ..Default::default() + }); + + schedule.run(&mut world); + } + + #[test] + fn write_component_with_access_set() { + fn does_write(_q: Query<&mut A>) {} + fn after_write(_q: Query<&A>) {} + + let mut world = World::new(); + let mut schedule = Schedule::default(); + + schedule.add_systems(does_write); + schedule.add_systems(after_write.after(ComponentAccessSet::Writes::)); + + // Panic if there are ambiguities. + schedule.set_build_settings(ScheduleBuildSettings { + ambiguity_detection: bevy_ecs::schedule::LogLevel::Error, + ..Default::default() + }); + + schedule.run(&mut world); + } + + #[test] + fn resource_with_access_set() { + #[derive(Resource, Default)] + struct R; + + fn send_system(_: ResMut) {} + fn read_system(_: Res) {} + + let mut world = World::new(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + schedule.add_systems(( + send_system.before(ResourceAccessSet::Reads::), + read_system, + )); + + // Panic if there are ambiguities. + schedule.set_build_settings(ScheduleBuildSettings { + ambiguity_detection: bevy_ecs::schedule::LogLevel::Error, + ..Default::default() + }); + + schedule.run(&mut world); + } + + #[test] + fn access_set_late_init() { + #[derive(Component)] + struct C; + + fn read_system(_: Query<&C>) {} + fn write_system(_: Query<&mut C>) {} + + let mut world = World::new(); + let mut schedule = Schedule::default(); + + // Make sure the system with write-access is initialized *before* the access set is mentioned. + schedule.add_systems(write_system); + schedule.run(&mut world); + + schedule.add_systems(read_system.after(ComponentAccessSet::Writes::)); + + // Panic if there are ambiguities. + schedule.set_build_settings(ScheduleBuildSettings { + ambiguity_detection: bevy_ecs::schedule::LogLevel::Error, + ..Default::default() + }); + + schedule.run(&mut world); + } + + #[test] + fn access_set_reads_all_not_included() { + #[derive(Resource, Default)] + struct R; + + fn read_system(_: Res) {} + fn read_all_system(_: &World) {} + + let mut world = World::new(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + schedule.add_systems(( + read_system, + // This will cause a dependency cycle if `read_all_system` is included + // in the read-access set. + read_all_system.after(ResourceAccessSet::Reads::), + )); + + // Panic if there are ambiguities. + schedule.set_build_settings(ScheduleBuildSettings { + ambiguity_detection: bevy_ecs::schedule::LogLevel::Error, + ..Default::default() + }); + + schedule.run(&mut world); + } } diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 808d31d2c48e7..b33f1ea56109e 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -121,6 +121,13 @@ impl Access { self.reads_all || self.reads_and_writes.contains(index.sparse_set_index()) } + /// Returns `true` if this has read-only access to the element given by `index`, + /// unless the access comes from [`Self::has_read_all`]. + pub(crate) fn has_granular_read_only(&self, index: T) -> bool { + let index = index.sparse_set_index(); + self.reads_and_writes.contains(index) && !self.writes.contains(index) + } + /// Returns `true` if this can access anything. pub fn has_any_read(&self) -> bool { self.reads_all || !self.reads_and_writes.is_clear() diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 76ddc7fbafa32..4e8e6e523bb15 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -93,6 +93,10 @@ impl SystemConfigs { impl NodeConfigs { /// Adds a new boxed system set to the systems. pub fn in_set_dyn(&mut self, set: BoxedSystemSet) { + assert!( + set.reads_component().is_none() && set.writes_component().is_none(), + "adding arbitrary systems to a world access set is not allowed" + ); match self { Self::NodeConfig(config) => { config.graph_info.sets.push(set); @@ -329,6 +333,10 @@ impl IntoSystemConfigs<()> for SystemConfigs { set.system_type().is_none(), "adding arbitrary systems to a system type set is not allowed" ); + assert!( + set.reads_component().is_none() && set.writes_component().is_none(), + "adding arbitrary systems to a world access set is not allowed" + ); self.in_set_dyn(set.dyn_clone()); @@ -409,6 +417,10 @@ impl SystemSetConfig { set.system_type().is_none(), "configuring system type sets is not allowed" ); + assert!( + set.reads_component().is_none() && set.writes_component().is_none(), + "configuring world access sets is not allowed" + ); Self { node: set, diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index a70df83c011bc..5f8966e86d8cd 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -19,6 +19,7 @@ use crate::{ self as bevy_ecs, component::{ComponentId, Components, Tick}, schedule::*, + storage::SparseSet, system::{BoxedSystem, Resource, System}, world::World, }; @@ -184,7 +185,7 @@ impl Schedule { Self { name: label.dyn_clone(), graph: ScheduleGraph::new(), - executable: SystemSchedule::new(), + executable: SystemSchedule::default(), executor: make_executor(ExecutorKind::default()), executor_initialized: false, } @@ -406,7 +407,12 @@ pub struct ScheduleGraph { system_sets: Vec, system_set_conditions: Vec>, system_set_ids: HashMap, - uninit: Vec<(NodeId, usize)>, + read_system_sets: SparseSet>, + write_system_sets: SparseSet>, + uninit: Vec, + /// Access sets that have to be checked against the systems that have already been + /// moved into the [`SystemSchedule`]. + new_access_sets: Vec, hierarchy: Dag, dependency: Dag, ambiguous_with: UnGraphMap, @@ -416,6 +422,22 @@ pub struct ScheduleGraph { settings: ScheduleBuildSettings, } +enum PendingInit { + System(usize), + SetCondition { + set_index: usize, + condition_index: usize, + }, + AccessSet(usize), +} + +struct AccessSet { + node: NodeId, + component: ComponentId, + reads: bool, + writes: bool, +} + impl ScheduleGraph { /// Creates an empty [`ScheduleGraph`] with default settings. pub fn new() -> Self { @@ -425,7 +447,10 @@ impl ScheduleGraph { system_sets: Vec::new(), system_set_conditions: Vec::new(), system_set_ids: HashMap::new(), + read_system_sets: SparseSet::new(), + write_system_sets: SparseSet::new(), uninit: Vec::new(), + new_access_sets: Vec::new(), hierarchy: Dag::new(), dependency: Dag::new(), ambiguous_with: UnGraphMap::new(), @@ -682,7 +707,7 @@ impl ScheduleGraph { self.update_graphs(id, config.graph_info)?; // system init has to be deferred (need `&mut World`) - self.uninit.push((id, 0)); + self.uninit.push(PendingInit::System(id.index())); self.systems.push(SystemNode::new(config.node)); self.system_conditions.push(config.conditions); @@ -711,16 +736,23 @@ impl ScheduleGraph { // system init has to be deferred (need `&mut World`) let system_set_conditions = &mut self.system_set_conditions[id.index()]; - self.uninit.push((id, system_set_conditions.len())); + self.uninit.push(PendingInit::SetCondition { + set_index: id.index(), + condition_index: system_set_conditions.len(), + }); system_set_conditions.append(&mut conditions); Ok(id) } fn add_set(&mut self, set: BoxedSystemSet) -> NodeId { - let id = NodeId::Set(self.system_sets.len()); + let index = self.system_sets.len(); self.system_sets.push(SystemSetNode::new(set.dyn_clone())); self.system_set_conditions.push(Vec::new()); + if set.is_access_set() { + self.uninit.push(PendingInit::AccessSet(index)); + } + let id = NodeId::Set(index); self.system_set_ids.insert(set, id); id } @@ -845,19 +877,107 @@ impl ScheduleGraph { /// Initializes any newly-added systems and conditions by calling [`System::initialize`] pub fn initialize(&mut self, world: &mut World) { - for (id, i) in self.uninit.drain(..) { - match id { - NodeId::System(index) => { - self.systems[index].get_mut().unwrap().initialize(world); + for uninit in self.uninit.drain(..) { + match uninit { + PendingInit::System(index) => { + let id = NodeId::System(index); + let system = self.systems[index].get_mut().unwrap(); + system.initialize(world); + + let system_access = system.component_access(); + for &set in system_access + .reads() + .filter_map(|id| self.read_system_sets.get(id)) + .flatten() + { + self.hierarchy.graph.add_edge(set, id, ()); + } + for &set in system_access + .writes() + .filter_map(|id| self.write_system_sets.get(id)) + .flatten() + { + self.hierarchy.graph.add_edge(set, id, ()); + } + for condition in &mut self.system_conditions[index] { condition.initialize(world); } } - NodeId::Set(index) => { + PendingInit::SetCondition { + set_index: index, + condition_index: i, + } => { for condition in self.system_set_conditions[index].iter_mut().skip(i) { condition.initialize(world); } } + PendingInit::AccessSet(index) => { + let id = NodeId::Set(index); + let set = self.system_sets[index].inner.as_ref(); + if let Some(component) = set.reads_component() { + let component = world + .components + .init_component_with_descriptor(&mut world.storages, component); + self.read_system_sets + .get_or_insert_with(component, Vec::new) + .push(id); + + // If true, that means there are old systems that need to be initialized. + let mut init_old = false; + + // Add any previously-added systems with read access to the set. + for (i, system) in self.systems.iter().enumerate() { + let Some(system) = &system.inner else { + init_old = true; + continue; + }; + if system.component_access().has_granular_read_only(component) { + self.hierarchy.graph.add_edge(id, NodeId::System(i), ()); + } + } + + if init_old { + self.new_access_sets.push(AccessSet { + node: id, + component, + reads: true, + writes: false, + }); + } + } + if let Some(component) = set.writes_component() { + let component = world + .components + .init_component_with_descriptor(&mut world.storages, component); + self.write_system_sets + .get_or_insert_with(component, Vec::new) + .push(id); + + // If true, that means there are old systems that need to be initialized. + let mut init_old = false; + + // Add any previously-added systems with write access to the set. + for (i, system) in self.systems.iter().enumerate() { + let Some(system) = &system.inner else { + init_old = true; + continue; + }; + if system.component_access().has_write(component) { + self.hierarchy.graph.add_edge(id, NodeId::System(i), ()); + } + } + + if init_old { + self.new_access_sets.push(AccessSet { + node: id, + component, + reads: false, + writes: true, + }); + } + } + } } } } @@ -1177,6 +1297,21 @@ impl ScheduleGraph { return Err(ScheduleBuildError::Uninitialized); } + // If any new access sets have been added, iterate over any systems in the old schedule + // to check if they should be added to the new sets. + for access_set in self.new_access_sets.drain(..) { + for (i, system) in &mut schedule.systems.iter_mut().enumerate() { + let system_access = system.component_access(); + if access_set.reads && system_access.has_granular_read_only(access_set.component) + || access_set.writes && system_access.has_write(access_set.component) + { + self.hierarchy + .graph + .add_edge(access_set.node, NodeId::System(i), ()); + } + } + } + // move systems out of old schedule for ((id, system), conditions) in schedule .system_ids diff --git a/crates/bevy_ecs/src/schedule/set.rs b/crates/bevy_ecs/src/schedule/set.rs index aa7ef806155f8..c82b8ce2d07c5 100644 --- a/crates/bevy_ecs/src/schedule/set.rs +++ b/crates/bevy_ecs/src/schedule/set.rs @@ -8,8 +8,11 @@ pub use bevy_ecs_macros::{ScheduleLabel, SystemSet}; use bevy_utils::define_boxed_label; use bevy_utils::label::DynHash; +use crate::component::ComponentDescriptor; +use crate::prelude::Component; use crate::system::{ - ExclusiveSystemParamFunction, IsExclusiveFunctionSystem, IsFunctionSystem, SystemParamFunction, + ExclusiveSystemParamFunction, IsExclusiveFunctionSystem, IsFunctionSystem, Resource, + SystemParamFunction, }; define_boxed_label!(ScheduleLabel); @@ -32,6 +35,28 @@ pub trait SystemSet: DynHash + Debug + Send + Sync + 'static { } /// Creates a boxed clone of the label corresponding to this system set. fn dyn_clone(&self) -> Box; + + /// Returns `true` if this system set is automatically populated based on + /// systems' world accesses. + /// The component tracked by this set is specified in either [`Self::reads_component`] + /// or [`Self::writes_component`]. + fn is_access_set(&self) -> bool { + self.reads_component().is_some() || self.writes_component().is_some() + } + + /// If this returns `Some`, then that means any systems that have read-only access + /// to the returned component will automatically be added to this set. + /// This also means that outside configuration of this system set is disallowed. + fn reads_component(&self) -> Option { + None + } + + /// If this returns `Some`, then that means any systems that have mutable access + /// to the returned component will automatically be added to this set. + /// This also means that outside configuration of this system set is disallowed. + fn writes_component(&self) -> Option { + None + } } impl PartialEq for dyn SystemSet { @@ -133,6 +158,128 @@ impl SystemSet for AnonymousSet { } } +macro_rules! access_set_enum { + ( $(#[$($tt:tt)*])* $name:ident) => { + $(#[$($tt)*])* + #[non_exhaustive] // Discourages people from matching on `_Marker`. + pub enum $name { + /// Any systems with read-only access to `T` will be added to this set. + Reads, + /// Any systems with mutable access to `T` will be added to this set. + Writes, + /// Any systems with access to `T` will be added to this set. + ReadsOrWrites, + #[doc(hidden)] + _Marker(PhantomData, std::convert::Infallible), + } + + impl Clone for $name { + fn clone(&self) -> Self { + *self + } + } + + impl Copy for $name {} + + impl PartialEq for $name { + fn eq(&self, other: &Self) -> bool { + use $name::*; + match (self, other) { + (Reads, Reads) | (Writes, Writes) | (ReadsOrWrites, ReadsOrWrites) => true, + _ => false, + } + } + } + + impl Eq for $name {} + + impl Hash for $name { + fn hash(&self, state: &mut H) { + match self { + $name::Reads => 0, + $name::Writes => 1, + $name::ReadsOrWrites => 2, + _ => 3, + }.hash(state); + } + } + }; +} + +access_set_enum!( + /// [`SystemSet`]s that are automatically populated with systems that access the component `T`. + ComponentAccessSet +); + +impl std::fmt::Debug for ComponentAccessSet { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Reads => write!(f, "ReadsComponent")?, + Self::Writes => write!(f, "WritesComponent")?, + Self::ReadsOrWrites => write!(f, "ReadsOrWritesComponent")?, + _ => {} + } + write!(f, "<{}>", std::any::type_name::()) + } +} + +impl SystemSet for ComponentAccessSet { + fn dyn_clone(&self) -> Box { + Box::new(*self) + } + + fn reads_component(&self) -> Option { + match self { + Self::Reads | Self::ReadsOrWrites => Some(ComponentDescriptor::new::()), + _ => None, + } + } + + fn writes_component(&self) -> Option { + match self { + Self::Writes | Self::ReadsOrWrites => Some(ComponentDescriptor::new::()), + _ => None, + } + } +} + +access_set_enum!( + /// [`SystemSet`]s that are automatically populated with systems that access the resource `T`. + ResourceAccessSet +); + +impl std::fmt::Debug for ResourceAccessSet { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Reads => write!(f, "ReadsResource")?, + Self::Writes => write!(f, "WritesResource")?, + Self::ReadsOrWrites => write!(f, "ReadsOrWritesResource")?, + _ => {} + } + write!(f, "<{}>", std::any::type_name::()) + } +} + +impl SystemSet for ResourceAccessSet { + fn dyn_clone(&self) -> Box { + Box::new(*self) + } + + fn reads_component(&self) -> Option { + match self { + Self::Reads | Self::ReadsOrWrites => Some(ComponentDescriptor::new_resource::()), + _ => None, + } + } + + fn writes_component(&self) -> Option { + match self { + Self::Writes | Self::ReadsOrWrites => Some(ComponentDescriptor::new_resource::()), + _ => None, + } + } +} + /// Types that can be converted into a [`SystemSet`]. pub trait IntoSystemSet: Sized { /// The type of [`SystemSet`] this instance converts into.