From da9307705ae8bc4ac53e4078a068a9da34101aa8 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Mon, 10 Mar 2025 19:51:40 -0500 Subject: [PATCH 1/4] generalize graphmap --- .../schedule/auto_insert_apply_deferred.rs | 4 +- .../bevy_ecs/src/schedule/graph/graph_map.rs | 186 +++++------------- crates/bevy_ecs/src/schedule/graph/mod.rs | 17 +- crates/bevy_ecs/src/schedule/graph/node.rs | 135 ++++++++++++- .../bevy_ecs/src/schedule/graph/tarjan_scc.rs | 60 +++--- crates/bevy_ecs/src/schedule/pass.rs | 12 +- crates/bevy_ecs/src/schedule/schedule.rs | 22 ++- 7 files changed, 251 insertions(+), 185 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs b/crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs index 171abee9641da..bccf5ff8e2dce 100644 --- a/crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs +++ b/crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs @@ -75,7 +75,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass { &mut self, _world: &mut World, graph: &mut ScheduleGraph, - dependency_flattened: &mut DiGraph, + dependency_flattened: &mut DiGraph, ) -> Result<(), ScheduleBuildError> { let mut sync_point_graph = dependency_flattened.clone(); let topo = graph.topsort_graph(dependency_flattened, ReportCycles::Dependency)?; @@ -217,7 +217,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass { &mut self, set: NodeId, systems: &[NodeId], - dependency_flattened: &DiGraph, + dependency_flattened: &DiGraph, ) -> impl Iterator { if systems.is_empty() { // collapse dependencies for empty sets diff --git a/crates/bevy_ecs/src/schedule/graph/graph_map.rs b/crates/bevy_ecs/src/schedule/graph/graph_map.rs index b255e55d26df9..61e04cc8df030 100644 --- a/crates/bevy_ecs/src/schedule/graph/graph_map.rs +++ b/crates/bevy_ecs/src/schedule/graph/graph_map.rs @@ -13,7 +13,7 @@ use core::{ use indexmap::IndexMap; use smallvec::SmallVec; -use super::NodeId; +use crate::schedule::graph::{DirectedGraphNodeId, GraphNodeId, GraphNodeIdPair}; use Direction::{Incoming, Outgoing}; @@ -21,13 +21,13 @@ use Direction::{Incoming, Outgoing}; /// /// For example, an edge between *1* and *2* is equivalent to an edge between /// *2* and *1*. -pub type UnGraph = Graph; +pub type UnGraph = Graph; /// A `Graph` with directed edges. /// /// For example, an edge from *1* to *2* is distinct from an edge from *2* to /// *1*. -pub type DiGraph = Graph; +pub type DiGraph = Graph; /// `Graph` is a graph datastructure using an associative array /// of its node weights `NodeId`. @@ -46,22 +46,28 @@ pub type DiGraph = Graph; /// /// `Graph` does not allow parallel edges, but self loops are allowed. #[derive(Clone)] -pub struct Graph +pub struct Graph where + Id: GraphNodeId, S: BuildHasher, { - nodes: IndexMap, S>, - edges: HashSet, + nodes: IndexMap, S>, + edges: HashSet, } -impl fmt::Debug for Graph { +impl fmt::Debug for Graph +where + Id: GraphNodeId, + S: BuildHasher, +{ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.nodes.fmt(f) } } -impl Graph +impl Graph where + Id: GraphNodeId, S: BuildHasher, { /// Create a new `Graph` with estimated capacity. @@ -77,10 +83,10 @@ where /// Use their natural order to map the node pair (a, b) to a canonical edge id. #[inline] - fn edge_key(a: NodeId, b: NodeId) -> CompactNodeIdPair { + fn edge_key(a: Id, b: Id) -> Id::Pair { let (a, b) = if DIRECTED || a <= b { (a, b) } else { (b, a) }; - CompactNodeIdPair::store(a, b) + Id::Pair::pack(a, b) } /// Return the number of nodes in the graph. @@ -89,19 +95,19 @@ where } /// Add node `n` to the graph. - pub fn add_node(&mut self, n: NodeId) { + pub fn add_node(&mut self, n: Id) { self.nodes.entry(n).or_default(); } /// Remove a node `n` from the graph. /// /// Computes in **O(N)** time, due to the removal of edges with other nodes. - pub fn remove_node(&mut self, n: NodeId) { + pub fn remove_node(&mut self, n: Id) { let Some(links) = self.nodes.swap_remove(&n) else { return; }; - let links = links.into_iter().map(CompactNodeIdAndDirection::load); + let links = links.into_iter().map(Id::Directed::unpack); for (succ, dir) in links { let edge = if dir == Outgoing { @@ -117,7 +123,7 @@ where } /// Return `true` if the node is contained in the graph. - pub fn contains_node(&self, n: NodeId) -> bool { + pub fn contains_node(&self, n: Id) -> bool { self.nodes.contains_key(&n) } @@ -125,19 +131,19 @@ where /// For a directed graph, the edge is directed from `a` to `b`. /// /// Inserts nodes `a` and/or `b` if they aren't already part of the graph. - pub fn add_edge(&mut self, a: NodeId, b: NodeId) { + pub fn add_edge(&mut self, a: Id, b: Id) { if self.edges.insert(Self::edge_key(a, b)) { // insert in the adjacency list if it's a new edge self.nodes .entry(a) .or_insert_with(|| Vec::with_capacity(1)) - .push(CompactNodeIdAndDirection::store(b, Outgoing)); + .push(Id::Directed::pack(b, Outgoing)); if a != b { // self loops don't have the Incoming entry self.nodes .entry(b) .or_insert_with(|| Vec::with_capacity(1)) - .push(CompactNodeIdAndDirection::store(a, Incoming)); + .push(Id::Directed::pack(a, Incoming)); } } } @@ -145,7 +151,7 @@ where /// Remove edge relation from a to b /// /// Return `true` if it did exist. - fn remove_single_edge(&mut self, a: NodeId, b: NodeId, dir: Direction) -> bool { + fn remove_single_edge(&mut self, a: Id, b: Id, dir: Direction) -> bool { let Some(sus) = self.nodes.get_mut(&a) else { return false; }; @@ -153,7 +159,7 @@ where let Some(index) = sus .iter() .copied() - .map(CompactNodeIdAndDirection::load) + .map(Id::Directed::unpack) .position(|elt| (DIRECTED && elt == (b, dir)) || (!DIRECTED && elt.0 == b)) else { return false; @@ -166,7 +172,7 @@ where /// Remove edge from `a` to `b` from the graph. /// /// Return `false` if the edge didn't exist. - pub fn remove_edge(&mut self, a: NodeId, b: NodeId) -> bool { + pub fn remove_edge(&mut self, a: Id, b: Id) -> bool { let exist1 = self.remove_single_edge(a, b, Outgoing); let exist2 = if a != b { self.remove_single_edge(b, a, Incoming) @@ -179,26 +185,24 @@ where } /// Return `true` if the edge connecting `a` with `b` is contained in the graph. - pub fn contains_edge(&self, a: NodeId, b: NodeId) -> bool { + pub fn contains_edge(&self, a: Id, b: Id) -> bool { self.edges.contains(&Self::edge_key(a, b)) } /// Return an iterator over the nodes of the graph. - pub fn nodes( - &self, - ) -> impl DoubleEndedIterator + ExactSizeIterator + '_ { + pub fn nodes(&self) -> impl DoubleEndedIterator + ExactSizeIterator + '_ { self.nodes.keys().copied() } /// Return an iterator of all nodes with an edge starting from `a`. - pub fn neighbors(&self, a: NodeId) -> impl DoubleEndedIterator + '_ { + pub fn neighbors(&self, a: Id) -> impl DoubleEndedIterator + '_ { let iter = match self.nodes.get(&a) { Some(neigh) => neigh.iter(), None => [].iter(), }; iter.copied() - .map(CompactNodeIdAndDirection::load) + .map(Id::Directed::unpack) .filter_map(|(n, dir)| (!DIRECTED || dir == Outgoing).then_some(n)) } @@ -207,22 +211,22 @@ where /// If the graph's edges are undirected, this is equivalent to *.neighbors(a)*. pub fn neighbors_directed( &self, - a: NodeId, + a: Id, dir: Direction, - ) -> impl DoubleEndedIterator + '_ { + ) -> impl DoubleEndedIterator + '_ { let iter = match self.nodes.get(&a) { Some(neigh) => neigh.iter(), None => [].iter(), }; iter.copied() - .map(CompactNodeIdAndDirection::load) + .map(Id::Directed::unpack) .filter_map(move |(n, d)| (!DIRECTED || d == dir || n == a).then_some(n)) } /// Return an iterator of target nodes with an edge starting from `a`, /// paired with their respective edge weights. - pub fn edges(&self, a: NodeId) -> impl DoubleEndedIterator + '_ { + pub fn edges(&self, a: Id) -> impl DoubleEndedIterator + '_ { self.neighbors(a) .map(move |b| match self.edges.get(&Self::edge_key(a, b)) { None => unreachable!(), @@ -234,9 +238,9 @@ where /// paired with their respective edge weights. pub fn edges_directed( &self, - a: NodeId, + a: Id, dir: Direction, - ) -> impl DoubleEndedIterator + '_ { + ) -> impl DoubleEndedIterator + '_ { self.neighbors_directed(a, dir).map(move |b| { let (a, b) = if dir == Incoming { (b, a) } else { (a, b) }; @@ -248,18 +252,19 @@ where } /// Return an iterator over all edges of the graph with their weight in arbitrary order. - pub fn all_edges(&self) -> impl ExactSizeIterator + '_ { - self.edges.iter().copied().map(CompactNodeIdPair::load) + pub fn all_edges(&self) -> impl ExactSizeIterator + '_ { + self.edges.iter().copied().map(Id::Pair::unpack) } - pub(crate) fn to_index(&self, ix: NodeId) -> usize { + pub(crate) fn to_index(&self, ix: Id) -> usize { self.nodes.get_index_of(&ix).unwrap() } } /// Create a new empty `Graph`. -impl Default for Graph +impl Default for Graph where + Id: GraphNodeId, S: BuildHasher + Default, { fn default() -> Self { @@ -267,9 +272,13 @@ where } } -impl DiGraph { +impl DiGraph +where + Id: GraphNodeId, + S: BuildHasher, +{ /// Iterate over all *Strongly Connected Components* in this graph. - pub(crate) fn iter_sccs(&self) -> impl Iterator> + '_ { + pub(crate) fn iter_sccs(&self) -> impl Iterator> + '_ { super::tarjan_scc::new_tarjan_scc(self) } } @@ -295,103 +304,10 @@ impl Direction { } } -/// Compact storage of a [`NodeId`] and a [`Direction`]. -#[derive(Clone, Copy)] -struct CompactNodeIdAndDirection { - index: usize, - is_system: bool, - direction: Direction, -} - -impl fmt::Debug for CompactNodeIdAndDirection { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.load().fmt(f) - } -} - -impl CompactNodeIdAndDirection { - const fn store(node: NodeId, direction: Direction) -> Self { - let index = node.index(); - let is_system = node.is_system(); - - Self { - index, - is_system, - direction, - } - } - - const fn load(self) -> (NodeId, Direction) { - let Self { - index, - is_system, - direction, - } = self; - - let node = match is_system { - true => NodeId::System(index), - false => NodeId::Set(index), - }; - - (node, direction) - } -} - -/// Compact storage of a [`NodeId`] pair. -#[derive(Clone, Copy, Hash, PartialEq, Eq)] -struct CompactNodeIdPair { - index_a: usize, - index_b: usize, - is_system_a: bool, - is_system_b: bool, -} - -impl fmt::Debug for CompactNodeIdPair { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.load().fmt(f) - } -} - -impl CompactNodeIdPair { - const fn store(a: NodeId, b: NodeId) -> Self { - let index_a = a.index(); - let is_system_a = a.is_system(); - - let index_b = b.index(); - let is_system_b = b.is_system(); - - Self { - index_a, - index_b, - is_system_a, - is_system_b, - } - } - - const fn load(self) -> (NodeId, NodeId) { - let Self { - index_a, - index_b, - is_system_a, - is_system_b, - } = self; - - let a = match is_system_a { - true => NodeId::System(index_a), - false => NodeId::Set(index_a), - }; - - let b = match is_system_b { - true => NodeId::System(index_b), - false => NodeId::Set(index_b), - }; - - (a, b) - } -} - #[cfg(test)] mod tests { + use crate::schedule::NodeId; + use super::*; use alloc::vec; @@ -402,7 +318,7 @@ mod tests { fn node_order_preservation() { use NodeId::System; - let mut graph = ::default(); + let mut graph = DiGraph::::default(); graph.add_node(System(1)); graph.add_node(System(2)); @@ -444,7 +360,7 @@ mod tests { fn strongly_connected_components() { use NodeId::System; - let mut graph = ::default(); + let mut graph = DiGraph::::default(); graph.add_edge(System(1), System(2)); graph.add_edge(System(2), System(1)); diff --git a/crates/bevy_ecs/src/schedule/graph/mod.rs b/crates/bevy_ecs/src/schedule/graph/mod.rs index a7ef79dbe113c..2947de19e17a2 100644 --- a/crates/bevy_ecs/src/schedule/graph/mod.rs +++ b/crates/bevy_ecs/src/schedule/graph/mod.rs @@ -17,7 +17,7 @@ mod node; mod tarjan_scc; pub use graph_map::{DiGraph, Direction, UnGraph}; -pub use node::NodeId; +pub use node::{DirectedGraphNodeId, GraphNodeId, GraphNodeIdPair, NodeId}; /// Specifies what kind of edge should be added to the dependency graph. #[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)] @@ -92,11 +92,11 @@ pub(crate) struct CheckGraphResults { /// Edges that are redundant because a longer path exists. pub(crate) transitive_edges: Vec<(NodeId, NodeId)>, /// Variant of the graph with no transitive edges. - pub(crate) transitive_reduction: DiGraph, + pub(crate) transitive_reduction: DiGraph, /// Variant of the graph with all possible transitive edges. // TODO: this will very likely be used by "if-needed" ordering #[expect(dead_code, reason = "See the TODO above this attribute.")] - pub(crate) transitive_closure: DiGraph, + pub(crate) transitive_closure: DiGraph, } impl Default for CheckGraphResults { @@ -123,7 +123,10 @@ impl Default for CheckGraphResults { /// ["On the calculation of transitive reduction-closure of orders"][1] by Habib, Morvan and Rampon. /// /// [1]: https://doi.org/10.1016/0012-365X(93)90164-O -pub(crate) fn check_graph(graph: &DiGraph, topological_order: &[NodeId]) -> CheckGraphResults { +pub(crate) fn check_graph( + graph: &DiGraph, + topological_order: &[NodeId], +) -> CheckGraphResults { if graph.node_count() == 0 { return CheckGraphResults::default(); } @@ -132,7 +135,7 @@ pub(crate) fn check_graph(graph: &DiGraph, topological_order: &[NodeId]) -> Chec // build a copy of the graph where the nodes and edges appear in topsorted order let mut map = >::with_capacity_and_hasher(n, Default::default()); - let mut topsorted = ::default(); + let mut topsorted = DiGraph::::default(); // iterate nodes in topological order for (i, &node) in topological_order.iter().enumerate() { map.insert(node, i); @@ -228,13 +231,13 @@ pub(crate) fn check_graph(graph: &DiGraph, topological_order: &[NodeId]) -> Chec /// ["Finding all the elementary circuits of a directed graph"][1] by D. B. Johnson. /// /// [1]: https://doi.org/10.1137/0204007 -pub fn simple_cycles_in_component(graph: &DiGraph, scc: &[NodeId]) -> Vec> { +pub fn simple_cycles_in_component(graph: &DiGraph, scc: &[NodeId]) -> Vec> { let mut cycles = vec![]; let mut sccs = vec![SmallVec::from_slice(scc)]; while let Some(mut scc) = sccs.pop() { // only look at nodes and edges in this strongly-connected component - let mut subgraph = ::default(); + let mut subgraph = DiGraph::::default(); for &node in &scc { subgraph.add_node(node); } diff --git a/crates/bevy_ecs/src/schedule/graph/node.rs b/crates/bevy_ecs/src/schedule/graph/node.rs index e4af143fc7a78..9ab597082d70e 100644 --- a/crates/bevy_ecs/src/schedule/graph/node.rs +++ b/crates/bevy_ecs/src/schedule/graph/node.rs @@ -1,4 +1,9 @@ -use core::fmt::Debug; +use core::{ + fmt::{self, Debug}, + hash::Hash, +}; + +use crate::schedule::graph::Direction; /// Unique identifier for a system or system set stored in a [`ScheduleGraph`]. /// @@ -45,3 +50,131 @@ impl NodeId { } } } + +impl GraphNodeId for NodeId { + type Pair = CompactNodeIdPair; + type Directed = CompactNodeIdAndDirection; +} + +/// Compact storage of a [`NodeId`] pair. +#[derive(Clone, Copy, Hash, PartialEq, Eq)] +pub struct CompactNodeIdPair { + index_a: usize, + index_b: usize, + is_system_a: bool, + is_system_b: bool, +} + +impl Debug for CompactNodeIdPair { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.unpack().fmt(f) + } +} + +impl GraphNodeIdPair for CompactNodeIdPair { + fn pack(a: NodeId, b: NodeId) -> Self { + Self { + index_a: a.index(), + index_b: b.index(), + is_system_a: a.is_system(), + is_system_b: b.is_system(), + } + } + + fn unpack(self) -> (NodeId, NodeId) { + let a = match self.is_system_a { + true => NodeId::System(self.index_a), + false => NodeId::Set(self.index_a), + }; + let b = match self.is_system_b { + true => NodeId::System(self.index_b), + false => NodeId::Set(self.index_b), + }; + (a, b) + } +} + +/// Compact storage of a [`NodeId`] and a [`Direction`]. +#[derive(Clone, Copy)] +pub struct CompactNodeIdAndDirection { + index: usize, + is_system: bool, + direction: Direction, +} + +impl Debug for CompactNodeIdAndDirection { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.unpack().fmt(f) + } +} + +impl DirectedGraphNodeId for CompactNodeIdAndDirection { + fn pack(node: NodeId, dir: Direction) -> Self { + Self { + index: node.index(), + is_system: node.is_system(), + direction: dir, + } + } + + fn unpack(self) -> (NodeId, Direction) { + let node = match self.is_system { + true => NodeId::System(self.index), + false => NodeId::Set(self.index), + }; + (node, self.direction) + } +} + +/// A node in a [`DiGraph`] or [`UnGraph`]. +/// +/// [`DiGraph`]: crate::schedule::graph::DiGraph +/// [`UnGraph`]: crate::schedule::graph::UnGraph +pub trait GraphNodeId: Copy + Eq + Ord + Debug + Hash { + /// A pair of [`GraphNodeId`]s for storing edge information. Typically + /// stored in a memory-efficient format. + type Pair: GraphNodeIdPair; + /// A pair of [`GraphNodeId`] and [`Direction`] for storing neighbor + /// information. Typically stored in a memory-efficient format. + type Directed: DirectedGraphNodeId; +} + +/// A pair of [`GraphNodeId`]s for storing edge information. Typically stored in +/// a memory-efficient format. +pub trait GraphNodeIdPair: Copy + Eq + Hash { + /// Packs the given identifiers into a pair. + fn pack(a: Id, b: Id) -> Self; + + /// Unpacks this pair into two identifiers. + fn unpack(self) -> (Id, Id); +} + +impl GraphNodeIdPair for (Id, Id) { + fn pack(a: Id, b: Id) -> Self { + (a, b) + } + + fn unpack(self) -> (Id, Id) { + (self.0, self.1) + } +} + +/// A pair of [`GraphNodeId`] and [`Direction`] for storing neighbor +/// information. Typically stored in a memory-efficient format. +pub trait DirectedGraphNodeId: Copy + Debug { + /// Packs the given identifier and direction into a pair. + fn pack(node: Id, dir: Direction) -> Self; + + /// Unpacks this pair into the identifier and direction. + fn unpack(self) -> (Id, Direction); +} + +impl DirectedGraphNodeId for (Id, Direction) { + fn pack(node: Id, dir: Direction) -> Self { + (node, dir) + } + + fn unpack(self) -> (Id, Direction) { + (self.0, self.1) + } +} diff --git a/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs b/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs index 5718dd2cfb83b..c7c157d78d88e 100644 --- a/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs +++ b/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs @@ -1,8 +1,7 @@ -use super::DiGraph; -use super::NodeId; use alloc::vec::Vec; -use core::hash::BuildHasher; -use core::num::NonZeroUsize; +use core::{hash::BuildHasher, num::NonZeroUsize}; + +use crate::schedule::graph::{DiGraph, GraphNodeId}; use smallvec::SmallVec; /// Create an iterator over *strongly connected components* using Algorithm 3 in @@ -16,9 +15,9 @@ use smallvec::SmallVec; /// Returns each strongly strongly connected component (scc). /// The order of node ids within each scc is arbitrary, but the order of /// the sccs is their postorder (reverse topological sort). -pub(crate) fn new_tarjan_scc( - graph: &DiGraph, -) -> impl Iterator> + '_ { +pub(crate) fn new_tarjan_scc( + graph: &DiGraph, +) -> impl Iterator> + '_ { // Create a list of all nodes we need to visit. let unchecked_nodes = graph.nodes(); @@ -46,7 +45,7 @@ pub(crate) fn new_tarjan_scc( } } -struct NodeData> { +struct NodeData> { root_index: Option, neighbors: N, } @@ -58,35 +57,40 @@ struct NodeData> { /// [1]: https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm /// [`petgraph`]: https://docs.rs/petgraph/0.6.5/petgraph/ /// [`TarjanScc`]: https://docs.rs/petgraph/0.6.5/petgraph/algo/struct.TarjanScc.html -struct TarjanScc<'graph, Hasher, AllNodes, Neighbors> +struct TarjanScc<'graph, Id, Hasher, AllNodes, Neighbors> where + Id: GraphNodeId, Hasher: BuildHasher, - AllNodes: Iterator, - Neighbors: Iterator, + AllNodes: Iterator, + Neighbors: Iterator, { /// Source of truth [`DiGraph`] - graph: &'graph DiGraph, - /// An [`Iterator`] of [`NodeId`]s from the `graph` which may not have been visited yet. + graph: &'graph DiGraph, + /// An [`Iterator`] of [`GraphNodeId`]s from the `graph` which may not have been visited yet. unchecked_nodes: AllNodes, /// The index of the next SCC index: usize, /// A count of potentially remaining SCCs component_count: usize, - /// Information about each [`NodeId`], including a possible SCC index and an + /// Information about each [`GraphNodeId`], including a possible SCC index and an /// [`Iterator`] of possibly unvisited neighbors. - nodes: Vec>, - /// A stack of [`NodeId`]s where a SCC will be found starting at the top of the stack. - stack: Vec, - /// A stack of [`NodeId`]s which need to be visited to determine which SCC they belong to. - visitation_stack: Vec<(NodeId, bool)>, + nodes: Vec>, + /// A stack of [`GraphNodeId`]s where a SCC will be found starting at the top of the stack. + stack: Vec, + /// A stack of [`GraphNodeId`]s which need to be visited to determine which SCC they belong to. + visitation_stack: Vec<(Id, bool)>, /// An index into the `stack` indicating the starting point of a SCC. start: Option, /// An adjustment to the `index` which will be applied once the current SCC is found. index_adjustment: Option, } -impl<'graph, S: BuildHasher, A: Iterator, N: Iterator> - TarjanScc<'graph, S, A, N> +impl<'graph, Id, S, A, N> TarjanScc<'graph, Id, S, A, N> +where + Id: GraphNodeId, + S: BuildHasher, + A: Iterator, + N: Iterator, { /// Compute the next *strongly connected component* using Algorithm 3 in /// [A Space-Efficient Algorithm for Finding Strongly Connected Components][1] by David J. Pierce, @@ -99,7 +103,7 @@ impl<'graph, S: BuildHasher, A: Iterator, N: Iterator Option<&[NodeId]> { + fn next_scc(&mut self) -> Option<&[Id]> { // Cleanup from possible previous iteration if let (Some(start), Some(index_adjustment)) = (self.start.take(), self.index_adjustment.take()) @@ -139,7 +143,7 @@ impl<'graph, S: BuildHasher, A: Iterator, N: Iterator Option { + fn visit_once(&mut self, v: Id, mut v_is_local_root: bool) -> Option { let node_v = &mut self.nodes[self.graph.to_index(v)]; if node_v.root_index.is_none() { @@ -203,13 +207,17 @@ impl<'graph, S: BuildHasher, A: Iterator, N: Iterator, N: Iterator> Iterator - for TarjanScc<'graph, S, A, N> +impl<'graph, Id, S, A, N> Iterator for TarjanScc<'graph, Id, S, A, N> +where + Id: GraphNodeId, + S: BuildHasher, + A: Iterator, + N: Iterator, { // It is expected that the `DiGraph` is sparse, and as such wont have many large SCCs. // Returning a `SmallVec` allows this iterator to skip allocation in cases where that // assumption holds. - type Item = SmallVec<[NodeId; 4]>; + type Item = SmallVec<[Id; 4]>; fn next(&mut self) -> Option { let next = SmallVec::from_slice(self.next_scc()?); diff --git a/crates/bevy_ecs/src/schedule/pass.rs b/crates/bevy_ecs/src/schedule/pass.rs index 20680e04e032c..13a5c7782eb03 100644 --- a/crates/bevy_ecs/src/schedule/pass.rs +++ b/crates/bevy_ecs/src/schedule/pass.rs @@ -21,7 +21,7 @@ pub trait ScheduleBuildPass: Send + Sync + Debug + 'static { &mut self, set: NodeId, systems: &[NodeId], - dependency_flattened: &DiGraph, + dependency_flattened: &DiGraph, ) -> impl Iterator; /// The implementation will be able to modify the `ScheduleGraph` here. @@ -29,7 +29,7 @@ pub trait ScheduleBuildPass: Send + Sync + Debug + 'static { &mut self, world: &mut World, graph: &mut ScheduleGraph, - dependency_flattened: &mut DiGraph, + dependency_flattened: &mut DiGraph, ) -> Result<(), ScheduleBuildError>; } @@ -39,14 +39,14 @@ pub(super) trait ScheduleBuildPassObj: Send + Sync + Debug { &mut self, world: &mut World, graph: &mut ScheduleGraph, - dependency_flattened: &mut DiGraph, + dependency_flattened: &mut DiGraph, ) -> Result<(), ScheduleBuildError>; fn collapse_set( &mut self, set: NodeId, systems: &[NodeId], - dependency_flattened: &DiGraph, + dependency_flattened: &DiGraph, dependencies_to_add: &mut Vec<(NodeId, NodeId)>, ); fn add_dependency(&mut self, from: NodeId, to: NodeId, all_options: &TypeIdMap>); @@ -56,7 +56,7 @@ impl ScheduleBuildPassObj for T { &mut self, world: &mut World, graph: &mut ScheduleGraph, - dependency_flattened: &mut DiGraph, + dependency_flattened: &mut DiGraph, ) -> Result<(), ScheduleBuildError> { self.build(world, graph, dependency_flattened) } @@ -64,7 +64,7 @@ impl ScheduleBuildPassObj for T { &mut self, set: NodeId, systems: &[NodeId], - dependency_flattened: &DiGraph, + dependency_flattened: &DiGraph, dependencies_to_add: &mut Vec<(NodeId, NodeId)>, ) { let iter = self.collapse_set(set, systems, dependency_flattened); diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index fbfd4010edcc3..be5978d1a233d 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -586,7 +586,7 @@ impl Schedule { #[derive(Default)] pub struct Dag { /// A directed graph. - graph: DiGraph, + graph: DiGraph, /// A cached topological ordering of the graph. topsort: Vec, } @@ -600,7 +600,7 @@ impl Dag { } /// The directed graph of the stored systems, connected by their ordering dependencies. - pub fn graph(&self) -> &DiGraph { + pub fn graph(&self) -> &DiGraph { &self.graph } @@ -682,7 +682,7 @@ pub struct ScheduleGraph { hierarchy: Dag, /// Directed acyclic graph of the dependency (which systems/sets have to run before which other systems/sets) dependency: Dag, - ambiguous_with: UnGraph, + ambiguous_with: UnGraph, /// Nodes that are allowed to have ambiguous ordering relationship with any other systems. pub ambiguous_with_all: HashSet, conflicting_systems: Vec<(NodeId, NodeId, Vec)>, @@ -1242,7 +1242,7 @@ impl ScheduleGraph { fn map_sets_to_systems( &self, hierarchy_topsort: &[NodeId], - hierarchy_graph: &DiGraph, + hierarchy_graph: &DiGraph, ) -> (HashMap>, HashMap) { let mut set_systems: HashMap> = HashMap::with_capacity_and_hasher(self.system_sets.len(), Default::default()); @@ -1277,7 +1277,10 @@ impl ScheduleGraph { (set_systems, set_system_bitsets) } - fn get_dependency_flattened(&mut self, set_systems: &HashMap>) -> DiGraph { + fn get_dependency_flattened( + &mut self, + set_systems: &HashMap>, + ) -> DiGraph { // flatten: combine `in_set` with `before` and `after` information // have to do it like this to preserve transitivity let mut dependency_flattened = self.dependency.graph.clone(); @@ -1316,7 +1319,10 @@ impl ScheduleGraph { dependency_flattened } - fn get_ambiguous_with_flattened(&self, set_systems: &HashMap>) -> UnGraph { + fn get_ambiguous_with_flattened( + &self, + set_systems: &HashMap>, + ) -> UnGraph { let mut ambiguous_with_flattened = UnGraph::default(); for (lhs, rhs) in self.ambiguous_with.all_edges() { match (lhs, rhs) { @@ -1349,7 +1355,7 @@ impl ScheduleGraph { fn get_conflicting_systems( &self, flat_results_disconnected: &Vec<(NodeId, NodeId)>, - ambiguous_with_flattened: &UnGraph, + ambiguous_with_flattened: &UnGraph, ignored_ambiguities: &BTreeSet, ) -> Vec<(NodeId, NodeId, Vec)> { let mut conflicting_systems = Vec::new(); @@ -1695,7 +1701,7 @@ impl ScheduleGraph { /// If the graph contain cycles, then an error is returned. pub fn topsort_graph( &self, - graph: &DiGraph, + graph: &DiGraph, report: ReportCycles, ) -> Result, ScheduleBuildError> { // Tarjan's SCC algorithm returns elements in *reverse* topological order. From 861845fbdf83db3bddc10dbec2d89da7970b462a Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Mon, 10 Mar 2025 19:56:53 -0500 Subject: [PATCH 2/4] generalize a couple algos --- crates/bevy_ecs/src/schedule/graph/mod.rs | 35 ++++++++++++----------- crates/bevy_ecs/src/schedule/schedule.rs | 2 +- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/graph/mod.rs b/crates/bevy_ecs/src/schedule/graph/mod.rs index 2947de19e17a2..5a314dd50a456 100644 --- a/crates/bevy_ecs/src/schedule/graph/mod.rs +++ b/crates/bevy_ecs/src/schedule/graph/mod.rs @@ -82,24 +82,24 @@ pub(crate) fn row_col(index: usize, num_cols: usize) -> (usize, usize) { } /// Stores the results of the graph analysis. -pub(crate) struct CheckGraphResults { +pub(crate) struct CheckGraphResults { /// Boolean reachability matrix for the graph. pub(crate) reachable: FixedBitSet, /// Pairs of nodes that have a path connecting them. - pub(crate) connected: HashSet<(NodeId, NodeId)>, + pub(crate) connected: HashSet<(Id, Id)>, /// Pairs of nodes that don't have a path connecting them. - pub(crate) disconnected: Vec<(NodeId, NodeId)>, + pub(crate) disconnected: Vec<(Id, Id)>, /// Edges that are redundant because a longer path exists. - pub(crate) transitive_edges: Vec<(NodeId, NodeId)>, + pub(crate) transitive_edges: Vec<(Id, Id)>, /// Variant of the graph with no transitive edges. - pub(crate) transitive_reduction: DiGraph, + pub(crate) transitive_reduction: DiGraph, /// Variant of the graph with all possible transitive edges. // TODO: this will very likely be used by "if-needed" ordering #[expect(dead_code, reason = "See the TODO above this attribute.")] - pub(crate) transitive_closure: DiGraph, + pub(crate) transitive_closure: DiGraph, } -impl Default for CheckGraphResults { +impl Default for CheckGraphResults { fn default() -> Self { Self { reachable: FixedBitSet::new(), @@ -123,10 +123,10 @@ impl Default for CheckGraphResults { /// ["On the calculation of transitive reduction-closure of orders"][1] by Habib, Morvan and Rampon. /// /// [1]: https://doi.org/10.1016/0012-365X(93)90164-O -pub(crate) fn check_graph( - graph: &DiGraph, - topological_order: &[NodeId], -) -> CheckGraphResults { +pub(crate) fn check_graph( + graph: &DiGraph, + topological_order: &[Id], +) -> CheckGraphResults { if graph.node_count() == 0 { return CheckGraphResults::default(); } @@ -135,7 +135,7 @@ pub(crate) fn check_graph( // build a copy of the graph where the nodes and edges appear in topsorted order let mut map = >::with_capacity_and_hasher(n, Default::default()); - let mut topsorted = DiGraph::::default(); + let mut topsorted = DiGraph::::default(); // iterate nodes in topological order for (i, &node) in topological_order.iter().enumerate() { map.insert(node, i); @@ -231,13 +231,16 @@ pub(crate) fn check_graph( /// ["Finding all the elementary circuits of a directed graph"][1] by D. B. Johnson. /// /// [1]: https://doi.org/10.1137/0204007 -pub fn simple_cycles_in_component(graph: &DiGraph, scc: &[NodeId]) -> Vec> { +pub fn simple_cycles_in_component( + graph: &DiGraph, + scc: &[Id], +) -> Vec> { let mut cycles = vec![]; let mut sccs = vec![SmallVec::from_slice(scc)]; while let Some(mut scc) = sccs.pop() { // only look at nodes and edges in this strongly-connected component - let mut subgraph = DiGraph::::default(); + let mut subgraph = DiGraph::::default(); for &node in &scc { subgraph.add_node(node); } @@ -257,12 +260,12 @@ pub fn simple_cycles_in_component(graph: &DiGraph, scc: &[NodeId]) -> Ve HashSet::with_capacity_and_hasher(subgraph.node_count(), Default::default()); // connects nodes along path segments that can't be part of a cycle (given current root) // those nodes can be unblocked at the same time - let mut unblock_together: HashMap> = + let mut unblock_together: HashMap> = HashMap::with_capacity_and_hasher(subgraph.node_count(), Default::default()); // stack for unblocking nodes let mut unblock_stack = Vec::with_capacity(subgraph.node_count()); // nodes can be involved in multiple cycles - let mut maybe_in_more_cycles: HashSet = + let mut maybe_in_more_cycles: HashSet = HashSet::with_capacity_and_hasher(subgraph.node_count(), Default::default()); // stack for DFS let mut stack = Vec::with_capacity(subgraph.node_count()); diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index be5978d1a233d..cc8a7b4305141 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1789,7 +1789,7 @@ impl ScheduleGraph { fn check_for_cross_dependencies( &self, - dep_results: &CheckGraphResults, + dep_results: &CheckGraphResults, hier_results_connected: &HashSet<(NodeId, NodeId)>, ) -> Result<(), ScheduleBuildError> { for &(a, b) in &dep_results.connected { From f321bf633181592260f7f2d50d13f4298e316d20 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Mon, 10 Mar 2025 20:26:54 -0500 Subject: [PATCH 3/4] rename associated types --- .../bevy_ecs/src/schedule/graph/graph_map.rs | 24 ++++++++-------- crates/bevy_ecs/src/schedule/graph/mod.rs | 2 +- crates/bevy_ecs/src/schedule/graph/node.rs | 28 +++++++++---------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/graph/graph_map.rs b/crates/bevy_ecs/src/schedule/graph/graph_map.rs index 61e04cc8df030..5d887051dd31a 100644 --- a/crates/bevy_ecs/src/schedule/graph/graph_map.rs +++ b/crates/bevy_ecs/src/schedule/graph/graph_map.rs @@ -13,7 +13,7 @@ use core::{ use indexmap::IndexMap; use smallvec::SmallVec; -use crate::schedule::graph::{DirectedGraphNodeId, GraphNodeId, GraphNodeIdPair}; +use crate::schedule::graph::{GraphNodeEdge, GraphNodeId, GraphNodeNeighbor}; use Direction::{Incoming, Outgoing}; @@ -51,8 +51,8 @@ where Id: GraphNodeId, S: BuildHasher, { - nodes: IndexMap, S>, - edges: HashSet, + nodes: IndexMap, S>, + edges: HashSet, } impl fmt::Debug for Graph @@ -83,10 +83,10 @@ where /// Use their natural order to map the node pair (a, b) to a canonical edge id. #[inline] - fn edge_key(a: Id, b: Id) -> Id::Pair { + fn edge_key(a: Id, b: Id) -> Id::Edge { let (a, b) = if DIRECTED || a <= b { (a, b) } else { (b, a) }; - Id::Pair::pack(a, b) + Id::Edge::pack(a, b) } /// Return the number of nodes in the graph. @@ -107,7 +107,7 @@ where return; }; - let links = links.into_iter().map(Id::Directed::unpack); + let links = links.into_iter().map(Id::Neighbor::unpack); for (succ, dir) in links { let edge = if dir == Outgoing { @@ -137,13 +137,13 @@ where self.nodes .entry(a) .or_insert_with(|| Vec::with_capacity(1)) - .push(Id::Directed::pack(b, Outgoing)); + .push(Id::Neighbor::pack(b, Outgoing)); if a != b { // self loops don't have the Incoming entry self.nodes .entry(b) .or_insert_with(|| Vec::with_capacity(1)) - .push(Id::Directed::pack(a, Incoming)); + .push(Id::Neighbor::pack(a, Incoming)); } } } @@ -159,7 +159,7 @@ where let Some(index) = sus .iter() .copied() - .map(Id::Directed::unpack) + .map(Id::Neighbor::unpack) .position(|elt| (DIRECTED && elt == (b, dir)) || (!DIRECTED && elt.0 == b)) else { return false; @@ -202,7 +202,7 @@ where }; iter.copied() - .map(Id::Directed::unpack) + .map(Id::Neighbor::unpack) .filter_map(|(n, dir)| (!DIRECTED || dir == Outgoing).then_some(n)) } @@ -220,7 +220,7 @@ where }; iter.copied() - .map(Id::Directed::unpack) + .map(Id::Neighbor::unpack) .filter_map(move |(n, d)| (!DIRECTED || d == dir || n == a).then_some(n)) } @@ -253,7 +253,7 @@ where /// Return an iterator over all edges of the graph with their weight in arbitrary order. pub fn all_edges(&self) -> impl ExactSizeIterator + '_ { - self.edges.iter().copied().map(Id::Pair::unpack) + self.edges.iter().copied().map(Id::Edge::unpack) } pub(crate) fn to_index(&self, ix: Id) -> usize { diff --git a/crates/bevy_ecs/src/schedule/graph/mod.rs b/crates/bevy_ecs/src/schedule/graph/mod.rs index 5a314dd50a456..741034d93fadb 100644 --- a/crates/bevy_ecs/src/schedule/graph/mod.rs +++ b/crates/bevy_ecs/src/schedule/graph/mod.rs @@ -17,7 +17,7 @@ mod node; mod tarjan_scc; pub use graph_map::{DiGraph, Direction, UnGraph}; -pub use node::{DirectedGraphNodeId, GraphNodeId, GraphNodeIdPair, NodeId}; +pub use node::{GraphNodeNeighbor, GraphNodeId, GraphNodeEdge, NodeId}; /// Specifies what kind of edge should be added to the dependency graph. #[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)] diff --git a/crates/bevy_ecs/src/schedule/graph/node.rs b/crates/bevy_ecs/src/schedule/graph/node.rs index 9ab597082d70e..67cc1b5a5a23e 100644 --- a/crates/bevy_ecs/src/schedule/graph/node.rs +++ b/crates/bevy_ecs/src/schedule/graph/node.rs @@ -52,8 +52,8 @@ impl NodeId { } impl GraphNodeId for NodeId { - type Pair = CompactNodeIdPair; - type Directed = CompactNodeIdAndDirection; + type Edge = CompactNodeIdPair; + type Neighbor = CompactNodeIdAndDirection; } /// Compact storage of a [`NodeId`] pair. @@ -71,7 +71,7 @@ impl Debug for CompactNodeIdPair { } } -impl GraphNodeIdPair for CompactNodeIdPair { +impl GraphNodeEdge for CompactNodeIdPair { fn pack(a: NodeId, b: NodeId) -> Self { Self { index_a: a.index(), @@ -108,7 +108,7 @@ impl Debug for CompactNodeIdAndDirection { } } -impl DirectedGraphNodeId for CompactNodeIdAndDirection { +impl GraphNodeNeighbor for CompactNodeIdAndDirection { fn pack(node: NodeId, dir: Direction) -> Self { Self { index: node.index(), @@ -133,23 +133,23 @@ impl DirectedGraphNodeId for CompactNodeIdAndDirection { pub trait GraphNodeId: Copy + Eq + Ord + Debug + Hash { /// A pair of [`GraphNodeId`]s for storing edge information. Typically /// stored in a memory-efficient format. - type Pair: GraphNodeIdPair; + type Edge: GraphNodeEdge; /// A pair of [`GraphNodeId`] and [`Direction`] for storing neighbor /// information. Typically stored in a memory-efficient format. - type Directed: DirectedGraphNodeId; + type Neighbor: GraphNodeNeighbor; } /// A pair of [`GraphNodeId`]s for storing edge information. Typically stored in /// a memory-efficient format. -pub trait GraphNodeIdPair: Copy + Eq + Hash { - /// Packs the given identifiers into a pair. +pub trait GraphNodeEdge: Copy + Eq + Hash { + /// Packs the given nodes into an edge. fn pack(a: Id, b: Id) -> Self; - /// Unpacks this pair into two identifiers. + /// Unpacks this edge into two nodes. fn unpack(self) -> (Id, Id); } -impl GraphNodeIdPair for (Id, Id) { +impl GraphNodeEdge for (Id, Id) { fn pack(a: Id, b: Id) -> Self { (a, b) } @@ -161,15 +161,15 @@ impl GraphNodeIdPair for (Id, Id) { /// A pair of [`GraphNodeId`] and [`Direction`] for storing neighbor /// information. Typically stored in a memory-efficient format. -pub trait DirectedGraphNodeId: Copy + Debug { - /// Packs the given identifier and direction into a pair. +pub trait GraphNodeNeighbor: Copy + Debug { + /// Packs the given identifier and direction into a neighbor. fn pack(node: Id, dir: Direction) -> Self; - /// Unpacks this pair into the identifier and direction. + /// Unpacks this neighbor into the identifier and direction. fn unpack(self) -> (Id, Direction); } -impl DirectedGraphNodeId for (Id, Direction) { +impl GraphNodeNeighbor for (Id, Direction) { fn pack(node: Id, dir: Direction) -> Self { (node, dir) } From 75136950c000db69bb3b5fb7feb02d5087893620 Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Mon, 10 Mar 2025 20:47:44 -0500 Subject: [PATCH 4/4] reorder imports --- crates/bevy_ecs/src/schedule/graph/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/graph/mod.rs b/crates/bevy_ecs/src/schedule/graph/mod.rs index 741034d93fadb..4c49fe1c4107b 100644 --- a/crates/bevy_ecs/src/schedule/graph/mod.rs +++ b/crates/bevy_ecs/src/schedule/graph/mod.rs @@ -17,7 +17,7 @@ mod node; mod tarjan_scc; pub use graph_map::{DiGraph, Direction, UnGraph}; -pub use node::{GraphNodeNeighbor, GraphNodeId, GraphNodeEdge, NodeId}; +pub use node::{GraphNodeEdge, GraphNodeId, GraphNodeNeighbor, NodeId}; /// Specifies what kind of edge should be added to the dependency graph. #[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)]