From 3b72d84efc42c0b07e35ce65cfae81980aa75287 Mon Sep 17 00:00:00 2001 From: Cameron <51241057+maniwani@users.noreply.github.com> Date: Mon, 14 Nov 2022 18:55:34 -0800 Subject: [PATCH] reorder init steps for tuple methods --- crates/bevy_ecs/src/schedule_v3/config.rs | 34 +- .../bevy_ecs/src/schedule_v3/graph/utils.rs | 4 +- crates/bevy_ecs/src/schedule_v3/schedule.rs | 386 +++++++++--------- 3 files changed, 203 insertions(+), 221 deletions(-) diff --git a/crates/bevy_ecs/src/schedule_v3/config.rs b/crates/bevy_ecs/src/schedule_v3/config.rs index cfa237a7d4aef3..ace2941f7ef54f 100644 --- a/crates/bevy_ecs/src/schedule_v3/config.rs +++ b/crates/bevy_ecs/src/schedule_v3/config.rs @@ -3,7 +3,7 @@ use bevy_utils::HashSet; use crate::{ schedule_v3::{ condition::{BoxedCondition, Condition}, - graph::{DependencyEdgeKind, DependencyInfo}, + graph::{DependencyEdgeKind, GraphInfo}, set::{BoxedSystemSet, IntoSystemSet, SystemSet}, }, system::{BoxedSystem, IntoSystem, System}, @@ -12,21 +12,21 @@ use crate::{ /// A [`SystemSet`] with scheduling metadata. pub struct SystemSetConfig { pub(super) set: BoxedSystemSet, - pub(super) info: DependencyInfo, + pub(super) graph_info: GraphInfo, pub(super) conditions: Vec, } /// A [`System`] with scheduling metadata. pub struct SystemConfig { pub(super) system: BoxedSystem, - pub(super) info: DependencyInfo, + pub(super) graph_info: GraphInfo, pub(super) conditions: Vec, } pub(super) fn new_set_unchecked(set: BoxedSystemSet) -> SystemSetConfig { SystemSetConfig { set, - info: DependencyInfo { + graph_info: GraphInfo { sets: HashSet::new(), edges: HashSet::new(), }, @@ -44,7 +44,7 @@ fn new_system(system: BoxedSystem) -> SystemConfig { let sets = system.default_system_sets().into_iter().collect(); SystemConfig { system, - info: DependencyInfo { + graph_info: GraphInfo { sets, edges: HashSet::new(), }, @@ -153,12 +153,12 @@ impl IntoSystemSetConfig for SystemSetConfig { fn in_set(mut self, set: impl SystemSet) -> Self { assert!(!set.is_system_type(), "invalid use of system type set"); - self.info.sets.insert(set.dyn_clone()); + self.graph_info.sets.insert(set.dyn_clone()); self } fn before(mut self, set: impl IntoSystemSet) -> Self { - self.info.edges.insert(( + self.graph_info.edges.insert(( DependencyEdgeKind::Before, set.into_system_set().dyn_clone(), )); @@ -166,7 +166,7 @@ impl IntoSystemSetConfig for SystemSetConfig { } fn after(mut self, set: impl IntoSystemSet) -> Self { - self.info + self.graph_info .edges .insert((DependencyEdgeKind::After, set.into_system_set().dyn_clone())); self @@ -279,12 +279,12 @@ impl IntoSystemConfig<()> for SystemConfig { fn in_set(mut self, set: impl SystemSet) -> Self { assert!(!set.is_system_type(), "invalid use of system type set"); - self.info.sets.insert(set.dyn_clone()); + self.graph_info.sets.insert(set.dyn_clone()); self } fn before(mut self, set: impl IntoSystemSet) -> Self { - self.info.edges.insert(( + self.graph_info.edges.insert(( DependencyEdgeKind::Before, set.into_system_set().dyn_clone(), )); @@ -292,7 +292,7 @@ impl IntoSystemConfig<()> for SystemConfig { } fn after(mut self, set: impl IntoSystemSet) -> Self { - self.info + self.graph_info .edges .insert((DependencyEdgeKind::After, set.into_system_set().dyn_clone())); self @@ -384,7 +384,7 @@ impl IntoSystemCollection<()> for SystemCollection { fn in_set(mut self, set: impl SystemSet) -> Self { assert!(!set.is_system_type(), "invalid use of system type set"); for config in self.inner.iter_mut() { - config.info.sets.insert(set.dyn_clone()); + config.graph_info.sets.insert(set.dyn_clone()); } self @@ -394,7 +394,7 @@ impl IntoSystemCollection<()> for SystemCollection { let set = set.into_system_set(); for config in self.inner.iter_mut() { config - .info + .graph_info .edges .insert((DependencyEdgeKind::Before, set.dyn_clone())); } @@ -406,7 +406,7 @@ impl IntoSystemCollection<()> for SystemCollection { let set = set.into_system_set(); for config in self.inner.iter_mut() { config - .info + .graph_info .edges .insert((DependencyEdgeKind::After, set.dyn_clone())); } @@ -464,7 +464,7 @@ impl IntoSystemSetCollection for SystemSetCollection { fn in_set(mut self, set: impl SystemSet) -> Self { assert!(!set.is_system_type(), "invalid use of system type set"); for config in self.inner.iter_mut() { - config.info.sets.insert(set.dyn_clone()); + config.graph_info.sets.insert(set.dyn_clone()); } self @@ -474,7 +474,7 @@ impl IntoSystemSetCollection for SystemSetCollection { let set = set.into_system_set(); for config in self.inner.iter_mut() { config - .info + .graph_info .edges .insert((DependencyEdgeKind::Before, set.dyn_clone())); } @@ -486,7 +486,7 @@ impl IntoSystemSetCollection for SystemSetCollection { let set = set.into_system_set(); for config in self.inner.iter_mut() { config - .info + .graph_info .edges .insert((DependencyEdgeKind::After, set.dyn_clone())); } diff --git a/crates/bevy_ecs/src/schedule_v3/graph/utils.rs b/crates/bevy_ecs/src/schedule_v3/graph/utils.rs index b23dfab3a162a1..870b17c2a14093 100644 --- a/crates/bevy_ecs/src/schedule_v3/graph/utils.rs +++ b/crates/bevy_ecs/src/schedule_v3/graph/utils.rs @@ -35,13 +35,13 @@ pub(crate) enum DependencyEdgeKind { } #[derive(Clone)] -pub(crate) struct DependencyInfo { +pub(crate) struct GraphInfo { pub(crate) sets: HashSet, pub(crate) edges: HashSet<(DependencyEdgeKind, BoxedSystemSet)>, } #[derive(Clone)] -pub(crate) struct IndexedDependencyInfo { +pub(crate) struct IndexedGraphInfo { pub(crate) sets: HashSet, pub(crate) edges: HashSet<(DependencyEdgeKind, NodeId)>, } diff --git a/crates/bevy_ecs/src/schedule_v3/schedule.rs b/crates/bevy_ecs/src/schedule_v3/schedule.rs index 7073c5383a2680..f2bba797b01893 100644 --- a/crates/bevy_ecs/src/schedule_v3/schedule.rs +++ b/crates/bevy_ecs/src/schedule_v3/schedule.rs @@ -20,69 +20,6 @@ use crate::{ world::World, }; -/// TBD -pub struct Schedule { - graph: ScheduleMeta, - executable: SystemSchedule, - executor: Box, -} - -impl Schedule { - pub fn new() -> Self { - Self { - graph: ScheduleMeta::new(), - executable: SystemSchedule::new(), - executor: Box::new(MultiThreadedExecutor::new()), - } - } - - pub fn add_system

(&mut self, system: impl IntoSystemConfig

) { - self.graph.add_system(system); - } - - pub fn configure_set(&mut self, set: impl IntoSystemSetConfig) { - self.graph.configure_set(set); - } - - pub fn set_default_set(&mut self, set: impl SystemSet) { - self.graph.set_default_set(set); - } - - pub fn set_executor(&mut self, executor: ExecutorKind) { - self.executor = match executor { - ExecutorKind::Simple => Box::new(SimpleExecutor::new()), - ExecutorKind::SingleThreaded => Box::new(SingleThreadedExecutor::new()), - ExecutorKind::MultiThreaded => Box::new(MultiThreadedExecutor::new()), - }; - } - - pub(crate) fn initialize(&mut self, world: &mut World) -> Result<(), BuildError> { - if self.graph.changed { - self.graph.initialize(world)?; - self.graph.update_schedule(&mut self.executable)?; - self.executor.init(&self.executable); - } - - Ok(()) - } - - pub fn run(&mut self, world: &mut World) { - self.initialize(world).unwrap(); - self.executor.run(&mut self.executable, world); - } - - /// Iterates all component change ticks and clamps any older than [`MAX_CHANGE_AGE`](crate::change_detection::MAX_CHANGE_AGE). - /// This prevents overflow and thus prevents false positives. - /// - /// **Note:** Does nothing if the [`World`] counter has not been incremented at least [`CHECK_TICK_THRESHOLD`](crate::change_detection::CHECK_TICK_THRESHOLD) - /// times since the previous pass. - pub(crate) fn check_change_ticks(&mut self, change_tick: u32, last_change_tick: u32) { - #[cfg(feature = "trace")] - let _span = bevy_utils::tracing::info_span!("check schedule ticks").entered(); - todo!(); - } -} - /// Stores [`ScheduleLabel`]-[`Schedule`] pairs. #[derive(Default, Resource)] pub struct Schedules { @@ -178,7 +115,69 @@ impl Schedules { } } -/// A directed acyclic graph. +/// TBD +pub struct Schedule { + graph: ScheduleMeta, + executable: SystemSchedule, + executor: Box, +} + +impl Schedule { + pub fn new() -> Self { + Self { + graph: ScheduleMeta::new(), + executable: SystemSchedule::new(), + executor: Box::new(MultiThreadedExecutor::new()), + } + } + + pub fn add_system

(&mut self, system: impl IntoSystemConfig

) { + self.graph.add_system(system); + } + + pub fn configure_set(&mut self, set: impl IntoSystemSetConfig) { + self.graph.configure_set(set); + } + + pub fn set_default_set(&mut self, set: impl SystemSet) { + self.graph.set_default_set(set); + } + + pub fn set_executor(&mut self, executor: ExecutorKind) { + self.executor = match executor { + ExecutorKind::Simple => Box::new(SimpleExecutor::new()), + ExecutorKind::SingleThreaded => Box::new(SingleThreadedExecutor::new()), + ExecutorKind::MultiThreaded => Box::new(MultiThreadedExecutor::new()), + }; + } + + pub(crate) fn initialize(&mut self, world: &mut World) -> Result<(), BuildError> { + if self.graph.changed { + self.graph.initialize(world); + self.graph.update_schedule(&mut self.executable)?; + self.executor.init(&self.executable); + } + + Ok(()) + } + + pub fn run(&mut self, world: &mut World) { + self.initialize(world).unwrap(); + self.executor.run(&mut self.executable, world); + } + + /// Iterates all component change ticks and clamps any older than [`MAX_CHANGE_AGE`](crate::change_detection::MAX_CHANGE_AGE). + /// This prevents overflow and thus prevents false positives. + /// + /// **Note:** Does nothing if the [`World`] counter has not been incremented at least [`CHECK_TICK_THRESHOLD`](crate::change_detection::CHECK_TICK_THRESHOLD) + /// times since the previous pass. + pub(crate) fn check_change_ticks(&mut self, change_tick: u32, last_change_tick: u32) { + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!("check schedule ticks").entered(); + todo!(); + } +} + #[derive(Default)] struct DAG { /// A directed graph. @@ -212,6 +211,11 @@ impl SystemSetMeta { } } +enum UninitNode { + System(BoxedSystem, Vec), + SystemSet(Vec), +} + #[derive(Default)] struct ScheduleMeta { system_set_ids: HashMap, @@ -226,7 +230,7 @@ struct ScheduleMeta { default_set: Option, next_node_id: u64, changed: bool, - uninit_nodes: Vec<(NodeId, DependencyInfo)>, + uninit: Vec<(NodeId, UninitNode)>, } impl ScheduleMeta { @@ -242,7 +246,7 @@ impl ScheduleMeta { default_set: None, next_node_id: 0, changed: false, - uninit_nodes: Vec::new(), + uninit: Vec::new(), } } @@ -252,178 +256,162 @@ impl ScheduleMeta { id } + fn set_default_set(&mut self, set: impl SystemSet) { + assert!(!set.is_system_type(), "invalid use of system type set"); + self.default_set = Some(set.dyn_clone()); + } + fn add_system

(&mut self, system: impl IntoSystemConfig

) { + self.add_system_inner(system).unwrap(); + } + + fn add_system_inner

(&mut self, system: impl IntoSystemConfig

) -> Result<(), BuildError> { let SystemConfig { system, - mut info, + mut graph_info, conditions, } = system.into_config(); - if info.sets.is_empty() { + let id = NodeId::System(self.next_id()); + + if graph_info.sets.is_empty() { if let Some(default) = self.default_set.as_ref() { - info.sets.insert(default.dyn_clone()); + graph_info.sets.insert(default.dyn_clone()); } } - let id = NodeId::System(self.next_id()); - self.systems.insert(id, system); - self.conditions.insert(id, conditions); - self.uninit_nodes.push((id, info)); + // graph updates are immediate + self.update_graphs(id, graph_info)?; + + // system init has to be deferred (need `&mut World`) + self.uninit + .push((id, UninitNode::System(system, conditions))); self.changed = true; + + Ok(()) + } + + fn configure_set(&mut self, set: impl IntoSystemSetConfig) { + self.configure_set_inner(set).unwrap(); } - fn add_set(&mut self, set: impl IntoSystemSetConfig, use_default: bool) { + fn configure_set_inner(&mut self, set: impl IntoSystemSetConfig) -> Result<(), BuildError> { let SystemSetConfig { set, - mut info, - conditions, + mut graph_info, + mut conditions, } = set.into_config(); - if use_default && info.sets.is_empty() { + let id = match self.system_set_ids.get(&set) { + Some(&id) => id, + None => { + let id = NodeId::Set(self.next_id()); + self.system_set_ids.insert(set.dyn_clone(), id); + self.system_sets.insert(id, SystemSetMeta::new(set)); + id + } + }; + + // TODO: only if this is the first time configure_set has been called for this set + if graph_info.sets.is_empty() { if let Some(default) = self.default_set.as_ref() { - info.sets.insert(default.dyn_clone()); + graph_info.sets.insert(default.dyn_clone()); } } - let id = NodeId::Set(self.next_id()); - self.system_set_ids.insert(set.dyn_clone(), id); - self.system_sets.insert(id, SystemSetMeta::new(set)); - self.conditions.insert(id, conditions); - self.uninit_nodes.push((id, info)); + // graph updates are immediate + self.update_graphs(id, graph_info)?; + + // system init has to be deferred (need `&mut World`) + self.uninit.push((id, UninitNode::SystemSet(conditions))); self.changed = true; - } - fn configure_set(&mut self, set: impl IntoSystemSetConfig) { - let config = set.into_config(); - match self.system_set_ids.get(&config.set) { - Some(&id) => { - let SystemSetConfig { - set: _, - info, - mut conditions, - } = config; - - // TODO: only initialize these newly-added conditions - self.conditions - .get_mut(&id) - .unwrap() - .append(&mut conditions); - self.uninit_nodes.push((id, info)); - } - None => { - self.add_set(config, true); - } - } + Ok(()) } - fn set_default_set(&mut self, set: impl SystemSet) { - assert!(!set.is_system_type(), "invalid use of system type set"); - self.default_set = Some(set.dyn_clone()); + fn add_set(&mut self, set: BoxedSystemSet) { + let id = NodeId::Set(self.next_id()); + self.system_set_ids.insert(set.dyn_clone(), id); + self.system_sets.insert(id, SystemSetMeta::new(set)); } - fn check_dependency_info(&mut self) -> Result<(), BuildError> { - let mut unknown_system_sets = HashSet::new(); - for (id, info) in self.uninit_nodes.iter() { - for set in info.sets.iter() { - match self.system_set_ids.get(set) { - Some(set_id) => { - if id == set_id { - let looping_set = self.system_sets[set_id].0.dyn_clone(); - return Err(BuildError::HierarchyLoop(looping_set)); - } - } - None => { - unknown_system_sets.insert(set.dyn_clone()); + fn check_sets(&mut self, id: &NodeId, graph_info: &GraphInfo) -> Result<(), BuildError> { + for set in graph_info.sets.iter() { + match self.system_set_ids.get(set) { + Some(set_id) => { + if id == set_id { + return Err(BuildError::HierarchyLoop(set.dyn_clone())); } } + None => self.add_set(set.dyn_clone()), } + } - // TODO: SystemCollection edges - for (_order, dep) in info.edges.iter() { - match self.system_set_ids.get(dep) { - Some(dep_id) => { - if id == dep_id { - let looping_set = self.system_sets[dep_id].0.dyn_clone(); - return Err(BuildError::DependencyLoop(looping_set)); - } - } - None => { - unknown_system_sets.insert(dep.dyn_clone()); + Ok(()) + } + + fn check_edges(&mut self, id: &NodeId, graph_info: &GraphInfo) -> Result<(), BuildError> { + for (_, dep) in graph_info.edges.iter() { + match self.system_set_ids.get(dep) { + Some(dep_id) => { + if id == dep_id { + return Err(BuildError::DependencyLoop(dep.dyn_clone())); } } - } - } - - for set in unknown_system_sets.drain() { - if set.is_system_type() { - self.add_set(new_set_unchecked(set), false); - } else { - self.add_set(set, false); + None => self.add_set(dep.dyn_clone()), } } Ok(()) } - fn initialize(&mut self, world: &mut World) -> Result<(), BuildError> { - // check info for errors - self.check_dependency_info()?; + fn update_graphs(&mut self, id: NodeId, graph_info: GraphInfo) -> Result<(), BuildError> { + self.check_sets(&id, &graph_info)?; + self.check_edges(&id, &graph_info)?; - // convert to node ids - let nodes = self - .uninit_nodes - .drain(..) - .map(|(id, info)| { - let sets = info - .sets - .into_iter() - .map(|set| self.system_set_ids[&set]) - .collect::>(); - let edges = info - .edges - .into_iter() - .map(|(order, set)| (order, self.system_set_ids[&set])) - .collect::>(); - - (id, IndexedDependencyInfo { sets, edges }) - }) - .collect::>(); - - // initialize - for id in nodes.keys() { - if id.is_system() { - let system = (self.systems.get_mut(id)).unwrap(); - system.initialize(world); - } - - let conditions = (self.conditions.get_mut(id)).unwrap(); - for system in conditions.iter_mut() { - system.initialize(world); - } + let GraphInfo { sets, edges } = graph_info; + self.hierarchy.graph.add_node(id); + for set in sets.into_iter().map(|set| self.system_set_ids[&set]) { + self.hierarchy.graph.add_edge(set, id, ()); } - // update hierarchy graph (do this first) - for (&id, info) in nodes.iter() { - self.hierarchy.graph.add_node(id); - for &set in info.sets.iter() { - self.hierarchy.graph.add_edge(set, id, ()); - } + self.dependency.graph.add_node(id); + for (kind, oid) in edges + .into_iter() + .map(|(kind, set)| (kind, self.system_set_ids[&set])) + { + let (lhs, rhs) = match kind { + DependencyEdgeKind::Before => (id, oid), + DependencyEdgeKind::After => (oid, id), + }; + self.dependency.graph.add_edge(lhs, rhs, ()); } - // update dependency graph - for (&id, info) in nodes.iter() { - self.dependency.graph.add_node(id); - for &(order, other) in info.edges.iter() { - let (lhs, rhs) = match order { - DependencyEdgeKind::Before => (id, other), - DependencyEdgeKind::After => (other, id), - }; + Ok(()) + } - self.dependency.graph.add_edge(lhs, rhs, ()); + fn initialize(&mut self, world: &mut World) { + for (id, uninit) in self.uninit.drain(..) { + match uninit { + UninitNode::System(mut system, mut conditions) => { + system.initialize(world); + self.systems.insert(id, system); + for condition in conditions.iter_mut() { + condition.initialize(world); + } + self.conditions.insert(id, conditions); + } + UninitNode::SystemSet(mut conditions) => { + for condition in conditions.iter_mut() { + condition.initialize(world); + } + self.conditions + .entry(id) + .or_insert_with(Vec::new) + .append(&mut conditions); + } } } - - Ok(()) } fn build_dependency_graph(&mut self) -> Result<(), BuildError> { @@ -437,7 +425,6 @@ impl ScheduleMeta { let result = check_graph(&self.hierarchy.graph, &self.hierarchy.topsort); if self.contains_redundant_edges(&result.transitive_edges) { - // TODO: warning return Err(BuildError::HierarchyConflict); } @@ -646,7 +633,7 @@ impl ScheduleMeta { fn update_schedule(&mut self, schedule: &mut SystemSchedule) -> Result<(), BuildError> { use std::cell::RefCell; - if !self.uninit_nodes.is_empty() { + if !self.uninit.is_empty() { return Err(BuildError::Uninitialized); } @@ -871,16 +858,13 @@ mod tests { } #[test] + #[should_panic] fn dependency_loop() { - // TODO: ensure that loops cannot appear after flattening. - // no adding dependencies between things connected hierarchically + // TODO: error on cross dependencies to avoid loops after flattening let mut world = World::new(); let mut schedule = Schedule::new(); schedule.configure_set(TestSet::X.after(TestSet::X)); - - let result = schedule.initialize(&mut world); - assert!(matches!(result, Err(BuildError::DependencyLoop(_)))); } #[test] @@ -896,14 +880,12 @@ mod tests { } #[test] + #[should_panic] fn hierarchy_loop() { let mut world = World::new(); let mut schedule = Schedule::new(); schedule.configure_set(TestSet::X.in_set(TestSet::X)); - - let result = schedule.initialize(&mut world); - assert!(matches!(result, Err(BuildError::HierarchyLoop(_)))); } #[test] @@ -962,7 +944,7 @@ mod tests { } #[test] - fn redundant_hierarchical_edges() { + fn hierarchy_conflict() { let mut world = World::new(); let mut schedule = Schedule::new();