Move more functions off of ScheduleGraph (#21817)

## Objective

- Part of #20115
- Reduce `ScheduleGraph` implementation surface and extract reusable
parts.

## Solution

- Upgraded and exposed `Dag<N>` as a fully-fledged directed acyclic
graph type.
- The validity of the cached topological order is now tracked by a
`dirty` flag. All modifications to the graph (via `DerefMut` or
`Dag::graph_mut`) cause the DAG to be marked as dirty.
- Added `Dag::toposort`: if the DAG is dirty, computes topological
order, caches it, and marks the DAG as clean. If already clean, returns
the cached topological order. We now also reuse the previous toposort
`Vec` allocation.
- Added `Dag::get_toposort`: can be used to access the topological order
with `&self`, with the stipulation that it returns an `Option`, and
`None` if the DAG is dirty.
- Replaced `check_graph` with `Dag::analyze`, and made it publicly
accessible.
- Added `Dag::remove_redundant_edges`, which uses the output of
`Dag::analyze`.
- Renamed `CheckGraphResults` to `DagAnalysis`.
- Added `DagAnalysis::check_for_redundant_edges`, replacing
`ScheduleGraph::optionally_check_hierarchy_conflicts`.
- Added `DagAnalysis::check_for_cross_dependencies`, replacing
`ScheduleGraph::check_for_cross_dependencies`. It now takes two full
`DagAnalysis` for comparison.
- Added `DagAnalysis::check_for_cross_intersection`, replacing
`ScheduleGraph::check_order_but_intersect`.
- Added `DagGroups` to encapsulate the `HashMap<SystemSetKey,
Vec<SystemKey>>` with additional capabilities:
- `DagGroups::flatten` and `DagGroups::flatten_undirected` handle the
graph reduction operations previously performed by functions on
`ScheduleGraph`.
- Added `ConflictingSystems` to encapsulate `Vec<(SystemKey, SystemKey,
ComponentId)>` with additional capabilities and type safety.

See the included migration guide for breaking changes.

# Testing

- Ran examples
- Added new tests for `Dag`, `DagAnalysis`, and `DagGroups`
functionality.

# Future work

- Consider replacing `HashSet<SystemKey>` with a `FixedBitSet`-like type
for better performance.

Author: ItsDoot

crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs

@@ -1,9 +1,9 @@
 use alloc::{boxed::Box, collections::BTreeSet, vec::Vec};
 
-use bevy_platform::collections::HashMap;
+use bevy_platform::collections::{HashMap, HashSet};
 
 use crate::{
-    schedule::{SystemKey, SystemSetKey},
+    schedule::{graph::Dag, SystemKey, SystemSetKey},
     system::{IntoSystem, System},
     world::World,
 };
@@ -72,11 +72,11 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
         &mut self,
         _world: &mut World,
         graph: &mut ScheduleGraph,
-        dependency_flattened: &mut DiGraph<SystemKey>,
+        dependency_flattened: &mut Dag<SystemKey>,
     ) -> Result<(), ScheduleBuildError> {
-        let mut sync_point_graph = dependency_flattened.clone();
-        let topo = dependency_flattened
-            .toposort()
+        let mut sync_point_graph = dependency_flattened.graph().clone();
+        let (topo, flat_dependency) = dependency_flattened
+            .toposort_and_graph()
             .map_err(ScheduleBuildError::FlatDependencySort)?;
 
         fn set_has_conditions(graph: &ScheduleGraph, set: SystemSetKey) -> bool {
@@ -124,7 +124,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
         let mut distance_to_explicit_sync_node: HashMap<u32, SystemKey> = HashMap::default();
 
         // Determine the distance for every node and collect the explicit sync points.
-        for &key in &topo {
+        for &key in topo {
             let (node_distance, mut node_needs_sync) = distances_and_pending_sync
                 .get(&key)
                 .copied()
@@ -146,7 +146,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
                 node_needs_sync = graph.systems[key].has_deferred();
             }
 
-            for target in dependency_flattened.neighbors_directed(key, Direction::Outgoing) {
+            for target in flat_dependency.neighbors_directed(key, Direction::Outgoing) {
                 let (target_distance, target_pending_sync) =
                     distances_and_pending_sync.entry(target).or_default();
 
@@ -179,13 +179,13 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
 
         // Find any edges which have a different number of sync points between them and make sure
         // there is a sync point between them.
-        for &key in &topo {
+        for &key in topo {
             let (node_distance, _) = distances_and_pending_sync
                 .get(&key)
                 .copied()
                 .unwrap_or_default();
 
-            for target in dependency_flattened.neighbors_directed(key, Direction::Outgoing) {
+            for target in flat_dependency.neighbors_directed(key, Direction::Outgoing) {
                 let (target_distance, _) = distances_and_pending_sync
                     .get(&target)
                     .copied()
@@ -215,14 +215,14 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
             }
         }
 
-        *dependency_flattened = sync_point_graph;
+        **dependency_flattened = sync_point_graph;
         Ok(())
     }
 
     fn collapse_set(
         &mut self,
         set: SystemSetKey,
-        systems: &[SystemKey],
+        systems: &HashSet<SystemKey>,
         dependency_flattening: &DiGraph<NodeId>,
     ) -> impl Iterator<Item = (NodeId, NodeId)> {
         if systems.is_empty() {

crates/bevy_ecs/src/schedule/error.rs

@@ -4,10 +4,14 @@ use core::fmt::Write as _;
 use thiserror::Error;
 
 use crate::{
-    component::{ComponentId, Components},
+    component::Components,
     schedule::{
-        graph::{DiGraphToposortError, GraphNodeId},
-        NodeId, ScheduleGraph, SystemKey, SystemSetKey,
+        graph::{
+            DagCrossDependencyError, DagOverlappingGroupError, DagRedundancyError,
+            DiGraphToposortError, GraphNodeId,
+        },
+        AmbiguousSystemConflictsWarning, ConflictingSystems, NodeId, ScheduleGraph, SystemKey,
+        SystemSetKey, SystemTypeSetAmbiguityError,
     },
     world::World,
 };
@@ -26,14 +30,14 @@ pub enum ScheduleBuildError {
     #[error("Failed to topologically sort the flattened dependency graph: {0}")]
     FlatDependencySort(DiGraphToposortError<SystemKey>),
     /// Tried to order a system (set) relative to a system set it belongs to.
-    #[error("`{0:?}` and `{1:?}` have both `in_set` and `before`-`after` relationships (these might be transitive). This combination is unsolvable as a system cannot run before or after a set it belongs to.")]
-    CrossDependency(NodeId, NodeId),
+    #[error("`{:?}` and `{:?}` have both `in_set` and `before`-`after` relationships (these might be transitive). This combination is unsolvable as a system cannot run before or after a set it belongs to.", .0.0, .0.1)]
+    CrossDependency(#[from] DagCrossDependencyError<NodeId>),
     /// Tried to order system sets that share systems.
-    #[error("`{0:?}` and `{1:?}` have a `before`-`after` relationship (which may be transitive) but share systems.")]
-    SetsHaveOrderButIntersect(SystemSetKey, SystemSetKey),
+    #[error("`{:?}` and `{:?}` have a `before`-`after` relationship (which may be transitive) but share systems.", .0.0, .0.1)]
+    SetsHaveOrderButIntersect(#[from] DagOverlappingGroupError<SystemSetKey>),
     /// Tried to order a system (set) relative to all instances of some system function.
-    #[error("Tried to order against `{0:?}` in a schedule that has more than one `{0:?}` instance. `{0:?}` is a `SystemTypeSet` and cannot be used for ordering if ambiguous. Use a different set without this restriction.")]
-    SystemTypeSetAmbiguity(SystemSetKey),
+    #[error(transparent)]
+    SystemTypeSetAmbiguity(#[from] SystemTypeSetAmbiguityError),
     /// Tried to run a schedule before all of its systems have been initialized.
     #[error("Tried to run a schedule before all of its systems have been initialized.")]
     Uninitialized,
@@ -56,7 +60,7 @@ pub enum ScheduleBuildWarning {
     /// [`LogLevel::Ignore`]: crate::schedule::LogLevel::Ignore
     /// [`LogLevel::Error`]: crate::schedule::LogLevel::Error
     #[error("The hierarchy of system sets contains redundant edges: {0:?}")]
-    HierarchyRedundancy(Vec<(NodeId, NodeId)>),
+    HierarchyRedundancy(#[from] DagRedundancyError<NodeId>),
     /// Systems with conflicting access have indeterminate run order.
     ///
     /// This warning is **disabled** by default, but can be enabled by setting
@@ -66,8 +70,8 @@ pub enum ScheduleBuildWarning {
     /// [`ScheduleBuildSettings::ambiguity_detection`]: crate::schedule::ScheduleBuildSettings::ambiguity_detection
     /// [`LogLevel::Warn`]: crate::schedule::LogLevel::Warn
     /// [`LogLevel::Error`]: crate::schedule::LogLevel::Error
-    #[error("Systems with conflicting access have indeterminate run order: {0:?}")]
-    Ambiguity(Vec<(SystemKey, SystemKey, Vec<ComponentId>)>),
+    #[error(transparent)]
+    Ambiguity(#[from] AmbiguousSystemConflictsWarning),
 }
 
 impl ScheduleBuildError {
@@ -101,13 +105,13 @@ impl ScheduleBuildError {
             ScheduleBuildError::FlatDependencySort(DiGraphToposortError::Cycle(cycles)) => {
                 Self::dependency_cycle_to_string(cycles, graph)
             }
-            ScheduleBuildError::CrossDependency(a, b) => {
-                Self::cross_dependency_to_string(a, b, graph)
+            ScheduleBuildError::CrossDependency(error) => {
+                Self::cross_dependency_to_string(error, graph)
             }
-            ScheduleBuildError::SetsHaveOrderButIntersect(a, b) => {
+            ScheduleBuildError::SetsHaveOrderButIntersect(DagOverlappingGroupError(a, b)) => {
                 Self::sets_have_order_but_intersect_to_string(a, b, graph)
             }
-            ScheduleBuildError::SystemTypeSetAmbiguity(set) => {
+            ScheduleBuildError::SystemTypeSetAmbiguity(SystemTypeSetAmbiguityError(set)) => {
                 Self::system_type_set_ambiguity_to_string(set, graph)
             }
             ScheduleBuildError::Uninitialized => Self::uninitialized_to_string(),
@@ -195,7 +199,11 @@ impl ScheduleBuildError {
         message
     }
 
-    fn cross_dependency_to_string(a: &NodeId, b: &NodeId, graph: &ScheduleGraph) -> String {
+    fn cross_dependency_to_string(
+        error: &DagCrossDependencyError<NodeId>,
+        graph: &ScheduleGraph,
+    ) -> String {
+        let DagCrossDependencyError(a, b) = error;
         format!(
             "{} `{}` and {} `{}` have both `in_set` and `before`-`after` relationships (these might be transitive). \
             This combination is unsolvable as a system cannot run before or after a set it belongs to.",
@@ -227,7 +235,7 @@ impl ScheduleBuildError {
     }
 
     pub(crate) fn ambiguity_to_string(
-        ambiguities: &[(SystemKey, SystemKey, Vec<ComponentId>)],
+        ambiguities: &ConflictingSystems,
         graph: &ScheduleGraph,
         components: &Components,
     ) -> String {
@@ -236,7 +244,7 @@ impl ScheduleBuildError {
             "{n_ambiguities} pairs of systems with conflicting data access have indeterminate execution order. \
             Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n",
         );
-        let ambiguities = graph.conflicts_to_string(ambiguities, components);
+        let ambiguities = ambiguities.to_string(graph, components);
         for (name_a, name_b, conflicts) in ambiguities {
             writeln!(message, " -- {name_a} and {name_b}").unwrap();
 
@@ -261,10 +269,10 @@ impl ScheduleBuildWarning {
     /// replaced with their names.
     pub fn to_string(&self, graph: &ScheduleGraph, world: &World) -> String {
         match self {
-            ScheduleBuildWarning::HierarchyRedundancy(transitive_edges) => {
+            ScheduleBuildWarning::HierarchyRedundancy(DagRedundancyError(transitive_edges)) => {
                 ScheduleBuildError::hierarchy_redundancy_to_string(transitive_edges, graph)
             }
-            ScheduleBuildWarning::Ambiguity(ambiguities) => {
+            ScheduleBuildWarning::Ambiguity(AmbiguousSystemConflictsWarning(ambiguities)) => {
                 ScheduleBuildError::ambiguity_to_string(ambiguities, graph, world.components())
             }
         }