From e133d94eeb158bfbc3ce4038877b09e05ea238fb Mon Sep 17 00:00:00 2001 From: Christian Hughes Date: Thu, 17 Jul 2025 20:12:35 -0500 Subject: [PATCH] address feedback --- .../bevy_ecs/src/schedule/graph/graph_map.rs | 76 +++++++++++-------- crates/bevy_ecs/src/schedule/graph/mod.rs | 2 +- crates/bevy_ecs/src/schedule/graph/node.rs | 58 ++------------ .../bevy_ecs/src/schedule/graph/tarjan_scc.rs | 48 +++++++----- crates/bevy_ecs/src/schedule/node.rs | 69 +++++++---------- 5 files changed, 108 insertions(+), 145 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/graph/graph_map.rs b/crates/bevy_ecs/src/schedule/graph/graph_map.rs index 9ffc179af6..7eae0f8fff 100644 --- a/crates/bevy_ecs/src/schedule/graph/graph_map.rs +++ b/crates/bevy_ecs/src/schedule/graph/graph_map.rs @@ -13,24 +13,24 @@ use core::{ use indexmap::IndexMap; use smallvec::SmallVec; -use crate::schedule::graph::node::{DirectedGraphNodeId, GraphNodeId, GraphNodeIdPair}; +use crate::schedule::graph::node::GraphNodeId; use Direction::{Incoming, Outgoing}; -/// A `Graph` with undirected edges. +/// A `Graph` with undirected edges of some [`GraphNodeId`] `N`. /// /// For example, an edge between *1* and *2* is equivalent to an edge between /// *2* and *1*. pub type UnGraph = Graph; -/// A `Graph` with directed edges. +/// A `Graph` with directed edges of some [`GraphNodeId`] `N`. /// /// For example, an edge from *1* to *2* is distinct from an edge from *2* to /// *1*. pub type DiGraph = Graph; /// `Graph` is a graph datastructure using an associative array -/// of its node weights `NodeId`. +/// of its node weights of some [`GraphNodeId`]. /// /// It uses a combined adjacency list and sparse adjacency matrix /// representation, using **O(|N| + |E|)** space, and allows testing for edge @@ -40,6 +40,7 @@ pub type DiGraph = Graph; /// /// - Constant generic bool `DIRECTED` determines whether the graph edges are directed or /// undirected. +/// - The `GraphNodeId` type `N`, which is used as the node weight. /// - The `BuildHasher` `S`. /// /// You can use the type aliases `UnGraph` and `DiGraph` for convenience. @@ -50,8 +51,8 @@ pub struct Graph where S: BuildHasher, { - nodes: IndexMap, S>, - edges: HashSet, + nodes: IndexMap, S>, + edges: HashSet, } impl fmt::Debug for Graph { @@ -74,10 +75,10 @@ impl Graph /// Use their natural order to map the node pair (a, b) to a canonical edge id. #[inline] - fn edge_key(a: N, b: N) -> N::Pair { + fn edge_key(a: N, b: N) -> N::Edge { let (a, b) = if DIRECTED || a <= b { (a, b) } else { (b, a) }; - N::Pair::new(a, b) + N::Edge::from((a, b)) } /// Return the number of nodes in the graph. @@ -103,7 +104,7 @@ impl Graph return; }; - let links = links.into_iter().map(N::Directed::unwrap); + let links = links.into_iter().map(N::Adjacent::into); for (succ, dir) in links { let edge = if dir == Outgoing { @@ -133,18 +134,18 @@ impl Graph self.nodes .entry(a) .or_insert_with(|| Vec::with_capacity(1)) - .push(N::Directed::new(b, Outgoing)); + .push(N::Adjacent::from((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(N::Directed::new(a, Incoming)); + .push(N::Adjacent::from((a, Incoming))); } } } - /// Remove edge relation from a to b + /// Remove edge relation from a to b. /// /// Return `true` if it did exist. fn remove_single_edge(&mut self, a: N, b: N, dir: Direction) -> bool { @@ -155,7 +156,7 @@ impl Graph let Some(index) = sus .iter() .copied() - .map(N::Directed::unwrap) + .map(N::Adjacent::into) .position(|elt| (DIRECTED && elt == (b, dir)) || (!DIRECTED && elt.0 == b)) else { return false; @@ -198,7 +199,7 @@ impl Graph }; iter.copied() - .map(N::Directed::unwrap) + .map(N::Adjacent::into) .filter_map(|(n, dir)| (!DIRECTED || dir == Outgoing).then_some(n)) } @@ -216,7 +217,7 @@ impl Graph }; iter.copied() - .map(N::Directed::unwrap) + .map(N::Adjacent::into) .filter_map(move |(n, d)| (!DIRECTED || d == dir || n == a).then_some(n)) } @@ -249,7 +250,7 @@ impl Graph /// 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(N::Pair::unwrap) + self.edges.iter().copied().map(N::Edge::into) } pub(crate) fn to_index(&self, ix: N) -> usize { @@ -266,29 +267,38 @@ impl Graph where S: Default, { + // Converts the node key and every adjacency list entry from `N` to `T`. + fn try_convert_node>( + (key, adj): (N, Vec), + ) -> Result<(T, Vec), T::Error> { + let key = key.try_into()?; + let adj = adj + .into_iter() + .map(|node| { + let (id, dir) = node.into(); + Ok(T::Adjacent::from((id.try_into()?, dir))) + }) + .collect::>()?; + Ok((key, adj)) + } + // Unpacks the edge pair, converts the nodes from `N` to `T`, and repacks them. + fn try_convert_edge>( + edge: N::Edge, + ) -> Result { + let (a, b) = edge.into(); + Ok(T::Edge::from((a.try_into()?, b.try_into()?))) + } + let nodes = self .nodes .into_iter() - .map(|(k, v)| { - Ok(( - k.try_into()?, - v.into_iter() - .map(|v| { - let (id, dir) = v.unwrap(); - Ok(T::Directed::new(id.try_into()?, dir)) - }) - .collect::, T::Error>>()?, - )) - }) - .collect::, S>, T::Error>>()?; + .map(try_convert_node::) + .collect::>()?; let edges = self .edges .into_iter() - .map(|e| { - let (a, b) = e.unwrap(); - Ok(T::Pair::new(a.try_into()?, b.try_into()?)) - }) - .collect::, T::Error>>()?; + .map(try_convert_edge::) + .collect::>()?; Ok(Graph { nodes, edges }) } } diff --git a/crates/bevy_ecs/src/schedule/graph/mod.rs b/crates/bevy_ecs/src/schedule/graph/mod.rs index a1606c6159..dbb5edc3f3 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}; +pub use node::GraphNodeId; /// 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 a41b5808c0..c3ab19c9da 100644 --- a/crates/bevy_ecs/src/schedule/graph/node.rs +++ b/crates/bevy_ecs/src/schedule/graph/node.rs @@ -7,56 +7,10 @@ use crate::schedule::graph::Direction; /// [`DiGraph`]: crate::schedule::graph::DiGraph /// [`UnGraph`]: crate::schedule::graph::UnGraph pub trait GraphNodeId: Copy + Eq + Hash + Ord + Debug { - /// This [`GraphNodeId`] and a [`Direction`]. - type Directed: DirectedGraphNodeId; - /// Two of these [`GraphNodeId`]s. - type Pair: GraphNodeIdPair; -} - -/// Types that are a [`GraphNodeId`] with a [`Direction`]. -pub trait DirectedGraphNodeId: Copy + Debug { - /// The type of [`GraphNodeId`] a [`Direction`] is paired with. - type Id: GraphNodeId; - - /// Packs a [`GraphNodeId`] and a [`Direction`] into a single type. - fn new(id: Self::Id, direction: Direction) -> Self; - - /// Unpacks a [`GraphNodeId`] and a [`Direction`] from this type. - fn unwrap(self) -> (Self::Id, Direction); -} - -/// Types that are a pair of [`GraphNodeId`]s. -pub trait GraphNodeIdPair: Copy + Eq + Hash + Debug { - /// The type of [`GraphNodeId`] for each element of the pair. - type Id: GraphNodeId; - - /// Packs two [`GraphNodeId`]s into a single type. - fn new(a: Self::Id, b: Self::Id) -> Self; - - /// Unpacks two [`GraphNodeId`]s from this type. - fn unwrap(self) -> (Self::Id, Self::Id); -} - -impl DirectedGraphNodeId for (N, Direction) { - type Id = N; - - fn new(id: N, direction: Direction) -> Self { - (id, direction) - } - - fn unwrap(self) -> (N, Direction) { - self - } -} - -impl GraphNodeIdPair for (N, N) { - type Id = N; - - fn new(a: N, b: N) -> Self { - (a, b) - } - - fn unwrap(self) -> (N, N) { - self - } + /// The type that packs and unpacks this [`GraphNodeId`] with a [`Direction`]. + /// This is used to save space in the graph's adjacency list. + type Adjacent: Copy + Debug + From<(Self, Direction)> + Into<(Self, Direction)>; + /// The type that packs and unpacks this [`GraphNodeId`] with another + /// [`GraphNodeId`]. This is used to save space in the graph's edge list. + type Edge: Copy + Eq + Hash + Debug + From<(Self, Self)> + Into<(Self, Self)>; } diff --git a/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs b/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs index 35e2dd4e5d..309ec321ba 100644 --- a/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs +++ b/crates/bevy_ecs/src/schedule/graph/tarjan_scc.rs @@ -17,9 +17,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(); @@ -47,9 +47,9 @@ pub(crate) fn new_tarjan_scc( } } -struct NodeData> { +struct NodeData> { root_index: Option, - neighbors: N, + neighbors: Neighbors, } /// A state for computing the *strongly connected components* using [Tarjan's algorithm][1]. @@ -59,15 +59,15 @@ 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, Id, Hasher, AllNodes, Neighbors> +struct TarjanScc<'graph, N, Hasher, AllNodes, Neighbors> where - Id: GraphNodeId, + N: GraphNodeId, Hasher: BuildHasher, - AllNodes: Iterator, - Neighbors: Iterator, + AllNodes: Iterator, + Neighbors: Iterator, { /// Source of truth [`DiGraph`] - graph: &'graph DiGraph, + 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 @@ -78,17 +78,22 @@ where /// [`Iterator`] of possibly unvisited neighbors. nodes: Vec>, /// A stack of [`GraphNodeId`]s where a SCC will be found starting at the top of the stack. - stack: Vec, + stack: Vec, /// A stack of [`GraphNodeId`]s which need to be visited to determine which SCC they belong to. - visitation_stack: Vec<(Id, bool)>, + visitation_stack: Vec<(N, 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, Id: GraphNodeId, S: BuildHasher, A: Iterator, N: Iterator> - TarjanScc<'graph, Id, S, A, N> +impl< + 'graph, + N: GraphNodeId, + S: BuildHasher, + A: Iterator, + Neighbors: Iterator, + > TarjanScc<'graph, N, S, A, Neighbors> { /// Compute the next *strongly connected component* using Algorithm 3 in /// [A Space-Efficient Algorithm for Finding Strongly Connected Components][1] by David J. Pierce, @@ -101,7 +106,7 @@ impl<'graph, Id: GraphNodeId, S: BuildHasher, A: Iterator, N: Iterato /// Returns `Some` for 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). - fn next_scc(&mut self) -> Option<&[Id]> { + fn next_scc(&mut self) -> Option<&[N]> { // Cleanup from possible previous iteration if let (Some(start), Some(index_adjustment)) = (self.start.take(), self.index_adjustment.take()) @@ -141,7 +146,7 @@ impl<'graph, Id: GraphNodeId, S: BuildHasher, A: Iterator, N: Iterato /// If a visitation is required, this will return `None` and mark the required neighbor and the /// current node as in need of visitation again. /// If no SCC can be found in the current visitation stack, returns `None`. - fn visit_once(&mut self, v: Id, mut v_is_local_root: bool) -> Option { + fn visit_once(&mut self, v: N, 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() { @@ -205,13 +210,18 @@ impl<'graph, Id: GraphNodeId, S: BuildHasher, A: Iterator, N: Iterato } } -impl<'graph, Id: GraphNodeId, S: BuildHasher, A: Iterator, N: Iterator> - Iterator for TarjanScc<'graph, Id, S, A, N> +impl< + 'graph, + N: GraphNodeId, + S: BuildHasher, + A: Iterator, + Neighbors: Iterator, + > Iterator for TarjanScc<'graph, N, S, A, Neighbors> { // 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<[Id; 4]>; + type Item = SmallVec<[N; 4]>; fn next(&mut self) -> Option { let next = SmallVec::from_slice(self.next_scc()?); diff --git a/crates/bevy_ecs/src/schedule/node.rs b/crates/bevy_ecs/src/schedule/node.rs index 8bbbc99e92..36005cf0c9 100644 --- a/crates/bevy_ecs/src/schedule/node.rs +++ b/crates/bevy_ecs/src/schedule/node.rs @@ -14,7 +14,7 @@ use crate::{ prelude::{SystemIn, SystemSet}, query::FilteredAccessSet, schedule::{ - graph::{DirectedGraphNodeId, Direction, GraphNodeId, GraphNodeIdPair}, + graph::{Direction, GraphNodeId}, BoxedCondition, InternedSystemSet, }, system::{ @@ -256,8 +256,8 @@ new_key_type! { } impl GraphNodeId for SystemKey { - type Directed = (SystemKey, Direction); - type Pair = (SystemKey, SystemKey); + type Adjacent = (SystemKey, Direction); + type Edge = (SystemKey, SystemKey); } impl TryFrom for SystemKey { @@ -322,8 +322,8 @@ impl NodeId { } impl GraphNodeId for NodeId { - type Directed = CompactNodeIdAndDirection; - type Pair = CompactNodeIdPair; + type Adjacent = CompactNodeIdAndDirection; + type Edge = CompactNodeIdPair; } impl From for NodeId { @@ -348,14 +348,13 @@ pub struct CompactNodeIdAndDirection { impl Debug for CompactNodeIdAndDirection { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.unwrap().fmt(f) + let tuple: (_, _) = (*self).into(); + tuple.fmt(f) } } -impl DirectedGraphNodeId for CompactNodeIdAndDirection { - type Id = NodeId; - - fn new(id: NodeId, direction: Direction) -> Self { +impl From<(NodeId, Direction)> for CompactNodeIdAndDirection { + fn from((id, direction): (NodeId, Direction)) -> Self { let key = match id { NodeId::System(key) => key.data(), NodeId::Set(key) => key.data(), @@ -368,20 +367,16 @@ impl DirectedGraphNodeId for CompactNodeIdAndDirection { direction, } } +} - fn unwrap(self) -> (NodeId, Direction) { - let Self { - key, - is_system, - direction, - } = self; - - let node = match is_system { - true => NodeId::System(key.into()), - false => NodeId::Set(key.into()), +impl From for (NodeId, Direction) { + fn from(value: CompactNodeIdAndDirection) -> Self { + let node = match value.is_system { + true => NodeId::System(value.key.into()), + false => NodeId::Set(value.key.into()), }; - (node, direction) + (node, value.direction) } } @@ -396,14 +391,13 @@ pub struct CompactNodeIdPair { impl Debug for CompactNodeIdPair { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.unwrap().fmt(f) + let tuple: (_, _) = (*self).into(); + tuple.fmt(f) } } -impl GraphNodeIdPair for CompactNodeIdPair { - type Id = NodeId; - - fn new(a: NodeId, b: NodeId) -> Self { +impl From<(NodeId, NodeId)> for CompactNodeIdPair { + fn from((a, b): (NodeId, NodeId)) -> Self { let key_a = match a { NodeId::System(index) => index.data(), NodeId::Set(index) => index.data(), @@ -423,23 +417,18 @@ impl GraphNodeIdPair for CompactNodeIdPair { is_system_b, } } +} - fn unwrap(self) -> (NodeId, NodeId) { - let Self { - key_a, - key_b, - is_system_a, - is_system_b, - } = self; - - let a = match is_system_a { - true => NodeId::System(key_a.into()), - false => NodeId::Set(key_a.into()), +impl From for (NodeId, NodeId) { + fn from(value: CompactNodeIdPair) -> Self { + let a = match value.is_system_a { + true => NodeId::System(value.key_a.into()), + false => NodeId::Set(value.key_a.into()), }; - let b = match is_system_b { - true => NodeId::System(key_b.into()), - false => NodeId::Set(key_b.into()), + let b = match value.is_system_b { + true => NodeId::System(value.key_b.into()), + false => NodeId::Set(value.key_b.into()), }; (a, b)