From fab83471b51fbe86aa2d450650db268658151e79 Mon Sep 17 00:00:00 2001 From: Charles Bournhonesque Date: Thu, 2 May 2024 14:31:32 -0400 Subject: [PATCH] add schedule docs (#13174) # Objective I'm reading through the schedule code, which is somewhat lacking documentation. I've been adding some docstrings to help me understand the code; I feel like some of them could be useful to also help others read this code. --------- Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/schedule/config.rs | 12 +++-- crates/bevy_ecs/src/schedule/executor/mod.rs | 6 +++ crates/bevy_ecs/src/schedule/graph_utils.rs | 5 ++- crates/bevy_ecs/src/schedule/schedule.rs | 47 ++++++++++++++++---- crates/bevy_ecs/src/system/system.rs | 2 + 5 files changed, 60 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 7b3419ceb7..3204b59e65 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -48,9 +48,15 @@ impl IntoSystemConfigs<()> for BoxedSystem<(), ()> { } } -/// Stores configuration for a single generic node. +/// Stores configuration for a single generic node (a system or a system set) +/// +/// The configuration includes the node itself, scheduling metadata +/// (hierarchy: in which sets is the node contained, +/// dependencies: before/after which other nodes should this node run) +/// and the run conditions associated with this node. pub struct NodeConfig { pub(crate) node: T, + /// Hierarchy and dependency metadata for this node pub(crate) graph_info: GraphInfo, pub(crate) conditions: Vec, } @@ -83,7 +89,7 @@ impl SystemConfigs { Self::NodeConfig(SystemConfig { node: system, graph_info: GraphInfo { - sets, + hierarchy: sets, ..Default::default() }, conditions: Vec::new(), @@ -96,7 +102,7 @@ impl NodeConfigs { pub fn in_set_inner(&mut self, set: InternedSystemSet) { match self { Self::NodeConfig(config) => { - config.graph_info.sets.push(set); + config.graph_info.hierarchy.push(set); } Self::Configs { configs, .. } => { for config in configs { diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 585ffa6358..3bf2f7edeb 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -62,16 +62,22 @@ pub struct SystemSchedule { /// Indexed by system node id. pub(super) system_conditions: Vec>, /// Indexed by system node id. + /// Number of systems that the system immediately depends on. pub(super) system_dependencies: Vec, /// Indexed by system node id. + /// List of systems that immediately depend on the system. pub(super) system_dependents: Vec>, /// Indexed by system node id. + /// List of sets containing the system that have conditions pub(super) sets_with_conditions_of_systems: Vec, /// List of system set node ids. pub(super) set_ids: Vec, /// Indexed by system set node id. pub(super) set_conditions: Vec>, /// Indexed by system set node id. + /// List of systems that are in sets that have conditions. + /// + /// If a set doesn't run because of its conditions, this is used to skip all systems in it. pub(super) systems_in_sets_with_conditions: Vec, } diff --git a/crates/bevy_ecs/src/schedule/graph_utils.rs b/crates/bevy_ecs/src/schedule/graph_utils.rs index b5d10b08e1..f3f1ade9b3 100644 --- a/crates/bevy_ecs/src/schedule/graph_utils.rs +++ b/crates/bevy_ecs/src/schedule/graph_utils.rs @@ -73,9 +73,12 @@ pub(crate) enum Ambiguity { IgnoreAll, } +/// Metadata about how the node fits in the schedule graph #[derive(Clone, Default)] pub(crate) struct GraphInfo { - pub(crate) sets: Vec, + /// the sets that the node belongs to (hierarchy) + pub(crate) hierarchy: Vec, + /// the sets that the node depends on (must run before or after) pub(crate) dependencies: Vec, pub(crate) ambiguous_with: Ambiguity, } diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index bdd48d7de8..380529e389 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -532,15 +532,27 @@ impl SystemNode { } /// Metadata for a [`Schedule`]. +/// +/// The order isn't optimized; calling `ScheduleGraph::build_schedule` will return a +/// `SystemSchedule` where the order is optimized for execution. #[derive(Default)] pub struct ScheduleGraph { + /// List of systems in the schedule systems: Vec, + /// List of conditions for each system, in the same order as `systems` system_conditions: Vec>, + /// List of system sets in the schedule system_sets: Vec, + /// List of conditions for each system set, in the same order as `system_sets` system_set_conditions: Vec>, + /// Map from system set to node id system_set_ids: HashMap, + /// Systems that have not been initialized yet; for system sets, we store the index of the first uninitialized condition + /// (all the conditions after that index still need to be initialized) uninit: Vec<(NodeId, usize)>, + /// Directed acyclic graph of the hierarchy (which systems/sets are children of which sets) hierarchy: Dag, + /// Directed acyclic graph of the dependency (which systems/sets have to run before which other systems/sets) dependency: Dag, ambiguous_with: UnGraphMap, ambiguous_with_all: HashSet, @@ -548,6 +560,7 @@ pub struct ScheduleGraph { anonymous_sets: usize, changed: bool, settings: ScheduleBuildSettings, + /// Dependency edges that will **not** automatically insert an instance of `apply_deferred` on the edge. no_sync_edges: BTreeSet<(NodeId, NodeId)>, auto_sync_node_ids: HashMap, } @@ -613,7 +626,7 @@ impl ScheduleGraph { .unwrap() } - /// Returns an iterator over all systems in this schedule. + /// Returns an iterator over all systems in this schedule, along with the conditions for each system. pub fn systems( &self, ) -> impl Iterator, &[BoxedCondition])> { @@ -627,7 +640,8 @@ impl ScheduleGraph { }) } - /// Returns an iterator over all system sets in this schedule. + /// Returns an iterator over all system sets in this schedule, along with the conditions for each + /// system set. pub fn system_sets(&self) -> impl Iterator { self.system_set_ids.iter().map(|(_, &node_id)| { let set_node = &self.system_sets[node_id.index()]; @@ -787,6 +801,7 @@ impl ScheduleGraph { } } + /// Add a [`SystemConfig`] to the graph, including its dependencies and conditions. fn add_system_inner(&mut self, config: SystemConfig) -> Result { let id = NodeId::System(self.systems.len()); @@ -806,6 +821,7 @@ impl ScheduleGraph { self.process_configs(sets.into_configs(), false); } + /// Add a single `SystemSetConfig` to the graph, including its dependencies and conditions. fn configure_set_inner(&mut self, set: SystemSetConfig) -> Result { let SystemSetConfig { node: set, @@ -837,7 +853,13 @@ impl ScheduleGraph { id } - fn check_set(&mut self, id: &NodeId, set: InternedSystemSet) -> Result<(), ScheduleBuildError> { + /// Checks that a system set isn't included in itself. + /// If not present, add the set to the graph. + fn check_hierarchy_set( + &mut self, + id: &NodeId, + set: InternedSystemSet, + ) -> Result<(), ScheduleBuildError> { match self.system_set_ids.get(&set) { Some(set_id) => { if id == set_id { @@ -858,18 +880,22 @@ impl ScheduleGraph { AnonymousSet::new(id) } - fn check_sets( + /// Check that no set is included in itself. + /// Add all the sets from the [`GraphInfo`]'s hierarchy to the graph. + fn check_hierarchy_sets( &mut self, id: &NodeId, graph_info: &GraphInfo, ) -> Result<(), ScheduleBuildError> { - for &set in &graph_info.sets { - self.check_set(id, set)?; + for &set in &graph_info.hierarchy { + self.check_hierarchy_set(id, set)?; } Ok(()) } + /// Checks that no system set is dependent on itself. + /// Add all the sets from the [`GraphInfo`]'s dependencies to the graph. fn check_edges( &mut self, id: &NodeId, @@ -899,17 +925,18 @@ impl ScheduleGraph { Ok(()) } + /// Update the internal graphs (hierarchy, dependency, ambiguity) by adding a single [`GraphInfo`] fn update_graphs( &mut self, id: NodeId, graph_info: GraphInfo, ) -> Result<(), ScheduleBuildError> { - self.check_sets(&id, &graph_info)?; + self.check_hierarchy_sets(&id, &graph_info)?; self.check_edges(&id, &graph_info)?; self.changed = true; let GraphInfo { - sets, + hierarchy: sets, dependencies, ambiguous_with, .. @@ -1137,6 +1164,9 @@ impl ScheduleGraph { .unwrap() } + /// Return a map from system set `NodeId` to a list of system `NodeId`s that are included in the set. + /// Also return a map from system set `NodeId` to a `FixedBitSet` of system `NodeId`s that are included in the set, + /// where the bitset order is the same as `self.systems` fn map_sets_to_systems( &self, hierarchy_topsort: &[NodeId], @@ -1397,6 +1427,7 @@ impl ScheduleGraph { } } + /// Updates the `SystemSchedule` from the `ScheduleGraph`. fn update_schedule( &mut self, schedule: &mut SystemSchedule, diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 9c67cdce39..ec1b08707c 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -105,6 +105,8 @@ pub trait System: Send + Sync + 'static { fn check_change_tick(&mut self, change_tick: Tick); /// Returns the system's default [system sets](crate::schedule::SystemSet). + /// + /// Each system will create a default system set that contains the system. fn default_system_sets(&self) -> Vec { Vec::new() }