improve dependency graph dot rendering and add more tests

Author: makspll

assets/tests/add_system/added_systems_run_in_parallel.lua

@@ -0,0 +1,43 @@
+
+function on_test()
+    local post_update_schedule = world.get_schedule_by_name("PostUpdate")
+    
+    local test_system = post_update_schedule:get_system_by_name("on_test_post_update")
+    
+    local system_a = world.add_system(
+        post_update_schedule,
+        system_builder("custom_system_a", script_id)
+            :after(test_system)
+    )
+        
+    local system_b = world.add_system(
+        post_update_schedule,
+        system_builder("custom_system_b", script_id)
+            :after(test_system)
+    )    
+
+    -- generate a schedule graph and verify it's what we expect
+    local dot_graph = post_update_schedule:render_dot()
+
+    local expected_dot_graph = [[
+digraph {
+  node_0 [label="bevy_mod_scripting_core::bindings::allocator::garbage_collector"];
+  node_1 [label="on_test_post_update"];
+  node_2 [label="script_integration_test_harness::dummy_before_post_update_system"];
+  node_3 [label="script_integration_test_harness::dummy_post_update_system"];
+  node_4 [label="custom_system_a"];
+  node_5 [label="custom_system_b"];
+  node_6 [label="SystemSet GarbageCollection"];
+  node_7 [label="SystemSet ScriptSystem(custom_system_a)"];
+  node_8 [label="SystemSet ScriptSystem(custom_system_b)"];
+  node_0 -> node_6 [color=red, label="child of", arrowhead=diamond];
+  node_4 -> node_7 [color=red, label="child of", arrowhead=diamond];
+  node_5 -> node_8 [color=red, label="child of", arrowhead=diamond];
+  node_1 -> node_4 [color=blue, label="runs before", arrowhead=normal];
+  node_1 -> node_5 [color=blue, label="runs before", arrowhead=normal];
+  node_2 -> node_3 [color=blue, label="runs before", arrowhead=normal];
+}
+    ]]
+
+    assert_str_eq(dot_graph, expected_dot_graph, "Expected the schedule graph to match the expected graph")
+end

crates/bevy_mod_scripting_core/src/bindings/script_system.rs

@@ -26,7 +26,7 @@ use bevy::{
         entity::Entity,
         query::{Access, FilteredAccess, FilteredAccessSet, QueryState},
         reflect::AppTypeRegistry,
-        schedule::{IntoSystemConfigs, IntoSystemSetConfigs, SystemSet},
+        schedule::{IntoSystemConfigs, SystemSet},
         system::{IntoSystem, System},
         world::{unsafe_world_cell::UnsafeWorldCell, World},
     },
@@ -650,7 +650,7 @@ mod test {
 
     make_test_plugin!(crate);
 
-    fn test_system_rust(world: &mut World) {}
+    fn test_system_rust(_world: &mut World) {}
 
     #[test]
     fn test_script_system_with_existing_system_dependency_can_execute() {

crates/bevy_mod_scripting_functions/src/core.rs

@@ -762,6 +762,33 @@ impl ReflectSchedule {
             .into_iter()
             .find_map(|s| (s.identifier() == name || s.path() == name).then_some(s.into())))
     }
+
+    /// Renders the schedule as a dot graph string.
+    ///
+    /// Useful for debugging scheduling.
+    ///
+    /// Arguments:
+    /// * `ctxt`: The function call context
+    /// * `self_`: The schedule to render.
+    /// Returns:
+    /// * `dot`: The dot graph string.
+    fn render_dot(
+        ctxt: FunctionCallContext,
+        self_: Ref<ReflectSchedule>,
+    ) -> Result<String, InteropError> {
+        profiling::function_scope!("render_dot");
+        let world = ctxt.world()?;
+        world.with_resource(|schedules: &Schedules| {
+            let schedule = schedules
+                .get(*self_.label())
+                .ok_or_else(|| InteropError::missing_schedule(self_.identifier()))?;
+            let mut graph = bevy_system_reflection::schedule_to_reflect_graph(schedule);
+            graph.absorb_type_system_sets();
+            graph.sort();
+            let graph = bevy_system_reflection::reflect_graph_to_dot(graph);
+            Ok(graph)
+        })?
+    }
 }
 
 #[script_bindings(

crates/bevy_system_reflection/Cargo.toml

@@ -8,5 +8,9 @@ bevy = { workspace = true, default-features = false }
 dot-writer = "0.1.4"
 petgraph = "*"
 
+
+[dev-dependencies]
+pretty_assertions = "1.4"
+manifest-dir-macros = "0.1.18"
 [lints]
 workspace = true

crates/bevy_system_reflection/src/lib.rs

@@ -7,8 +7,7 @@ use bevy::ecs::schedule::{
 };
 use bevy::ecs::system::{System, SystemInput};
 use bevy::reflect::Reflect;
-use bevy::utils::HashSet;
-use bevy::utils::hashbrown::HashMap;
+use bevy::utils::hashbrown::{HashMap, HashSet};
 use dot_writer::{Attributes, DotWriter};
 
 #[derive(Reflect, Debug, Clone)]
@@ -76,7 +75,7 @@ impl ReflectSystem {
     }
 }
 
-#[derive(Reflect, Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Reflect, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
 #[reflect(opaque)]
 pub(crate) struct ReflectNodeId(pub NodeId);
 
@@ -158,7 +157,11 @@ impl ReflectSystemSet {
 /// Renders a schedule to a dot graph using the optimized schedule.
 pub fn schedule_to_dot_graph(schedule: &Schedule) -> String {
     let graph = schedule_to_reflect_graph(schedule);
+    reflect_graph_to_dot(graph)
+}
 
+/// Renders a reflectable system graph to a dot graph
+pub fn reflect_graph_to_dot(graph: ReflectSystemGraph) -> String {
     // create a dot graph with:
     // - hierarchy represented by red composition arrows
     // - dependencies represented by blue arrows
@@ -191,7 +194,6 @@ pub fn schedule_to_dot_graph(schedule: &Schedule) -> String {
             }
         }
 
-        // go through hierarchy edges
         // go through hierarchy edges
         for edge in graph.hierarchy {
             let from = node_id_map.get(&edge.from).cloned().unwrap_or_else(|| {
@@ -321,6 +323,132 @@ pub struct ReflectSystemGraph {
     hierarchy: Vec<Edge>,
 }
 
+impl ReflectSystemGraph {
+    /// Sorts the graph nodes and edges.
+    ///
+    /// Useful if you want a stable order and deterministic graphs.
+    pub fn sort(&mut self) {
+        // sort everything in this graph
+        self.nodes.sort_by_key(|node| match node {
+            ReflectSystemGraphNode::System(system) => system.node_id.0,
+            ReflectSystemGraphNode::SystemSet(system_set) => system_set.node_id.0,
+        });
+
+        self.dependencies.sort();
+
+        self.hierarchy.sort();
+    }
+
+    /// removes the set and bridges the edges connecting to it
+    fn absorb_set(&mut self, node_id: NodeId) {
+        // collect hierarchy parents and children in one pass
+        let mut hierarchy_parents = Vec::new();
+        let mut hierarchy_children = Vec::new();
+        // the relation ship expressed by edges is "is child of"
+        for edge in &self.hierarchy {
+            // these are children
+            if edge.to.0 == node_id {
+                hierarchy_children.push(edge.from.clone());
+            }
+            // these are parents
+            if edge.from.0 == node_id {
+                hierarchy_parents.push(edge.to.clone());
+            }
+        }
+
+        // 
+        let mut dependencies = Vec::new();
+        let mut dependents = Vec::new();
+        // the relationship expressed is "runs before" i.e. "is depended on by"
+        for edge in &self.dependencies {
+            if edge.to.0 == node_id {
+                dependencies.push(edge.from.clone());
+            }
+            if edge.from.0 == node_id {
+                dependents.push(edge.to.clone());
+            }
+        }
+
+        let mut new_hierarchy_edges =
+            HashSet::with_capacity(hierarchy_parents.len() * hierarchy_children.len());
+        let mut new_dependency_edges =
+            HashSet::with_capacity(dependencies.len() * dependents.len());
+
+        // each parent, becomes a parent to the sets children
+        for parent in hierarchy_parents.iter() {
+            for child in hierarchy_children.iter() {
+                new_hierarchy_edges.insert(Edge {
+                    from: child.clone(),
+                    to: parent.clone(),
+                });
+            }
+        }
+
+        for child in hierarchy_parents.iter() {
+            // bridge dependencies
+            for dependency in dependencies.iter() {
+                new_dependency_edges.insert(Edge {
+                    from: dependency.clone(),
+                    to: child.clone(),
+                });
+            }
+
+            for dependent in dependents.iter() {
+                new_dependency_edges.insert(Edge {
+                    from: child.clone(),
+                    to: dependent.clone(),
+                });
+            }
+        }
+
+
+        // remove any edges involving the set to absorb
+        self.hierarchy
+            .retain(|edge| edge.from.0 != node_id && edge.to.0 != node_id);
+        self.dependencies
+            .retain(|edge| edge.from.0 != node_id && edge.to.0 != node_id);
+
+        // remove the set from nodes
+        self.nodes.retain(|node| match node {
+            ReflectSystemGraphNode::SystemSet(system_set) => system_set.node_id.0 != node_id,
+            _ => true,
+        });
+
+        // add new bridging edges
+        self.hierarchy.extend(new_hierarchy_edges);
+        self.dependencies.extend(new_dependency_edges);
+    }
+
+    /// type system sets, are not really important to us, for all intents and purposes
+    /// they are one and the same as the underlying systems
+    /// Adapter and pipe systems might have multiple default system sets attached, but we want all them gone from the graph.
+    /// 
+    /// Inlines every type system set into its children, replacing anything pointing to those sets by edges to every system contained in the set
+    pub fn absorb_type_system_sets(&mut self) {
+
+        let type_sets = self
+            .nodes
+            .iter()
+            .filter_map(|node| match node {
+                ReflectSystemGraphNode::SystemSet(system_set) => {
+                    if system_set.type_id.is_some() {
+                        Some(system_set.node_id.0)
+                    } else {
+                        None
+                    }
+                }
+                _ => None,
+            })
+            .collect::<Vec<_>>();
+
+        // yes this is very inefficient, no this isn't a big hot path, these graphs mostly serve a debugging role.
+        for node_id in type_sets {
+            self.absorb_set(node_id);
+        }
+        
+    }
+}
+
 /// A node in the reflectable system graph
 #[derive(Reflect)]
 pub enum ReflectSystemGraphNode {
@@ -331,10 +459,83 @@ pub enum ReflectSystemGraphNode {
 }
 
 /// An edge in the graph
-#[derive(Reflect)]
+#[derive(Reflect, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct Edge {
     /// The id of the node this edge is coming from
     from: ReflectNodeId,
     /// The id of the node this edge is going to
     to: ReflectNodeId,
 }
+
+#[cfg(test)]
+mod test {
+    use bevy::{
+        app::Update,
+        ecs::{
+            schedule::{IntoSystemConfigs, IntoSystemSetConfigs},
+            world::World,
+        },
+    };
+
+    use super::*;
+
+    fn system_a() {}
+
+    fn system_b() {}
+
+    fn system_c() {}
+
+    fn system_d() {}
+
+    fn system_e() {}
+
+    const BLESS_MODE: bool = false;
+
+    #[test]
+    fn test_graph_is_as_expected() {
+        // create empty schedule and graph it
+
+        let mut schedule = Schedule::new(Update);
+
+        #[derive(SystemSet, Hash, PartialEq, Eq, PartialOrd, Ord, Debug, Clone, Copy)]
+        enum SystemSet {
+            SystemSetG,
+            SystemSetH,
+        }
+
+        // add a few systems that depend on each other, some via system sets
+
+        schedule
+            .add_systems(system_a)
+            .add_systems(system_b.before(system_a))
+            .add_systems(system_c.after(system_b).before(SystemSet::SystemSetH))
+            .add_systems(system_d.in_set(SystemSet::SystemSetG))
+            .add_systems(system_e.in_set(SystemSet::SystemSetH))
+            .configure_sets(SystemSet::SystemSetG.after(SystemSet::SystemSetH));
+        let mut world = World::new();
+        schedule.initialize(&mut world).unwrap();
+
+        let mut graph = schedule_to_reflect_graph(&schedule);
+        graph.absorb_type_system_sets();
+        graph.sort();
+        let dot = reflect_graph_to_dot(graph);
+
+        let normalize = |s: &str| {
+            // trim each line individually from the start, and replace " = " with "=" to deal with formatters
+            let lines: Vec<&str> = s.lines().map(|line| line.trim_start()).collect();
+            lines.join("\n").replace(" = ", "").replace(";", "").replace(",", "").trim().to_string()
+        };
+
+        // check that the dot graph is as expected
+        // the expected file is found next to the src/lib.rs folder
+        let expected = include_str!("../test_graph.dot");
+        let expected_path = manifest_dir_macros::file_path!("test_graph.dot");
+
+        if BLESS_MODE {
+            std::fs::write(expected_path, normalize(&dot)).unwrap();
+            panic!("Bless mode is active");
+        } else {
+            pretty_assertions::assert_eq!(normalize(&dot), normalize(expected));
+        }
+    }
+}

crates/bevy_system_reflection/test_graph.dot

@@ -0,0 +1,15 @@
+digraph {
+node_0 [label="bevy_system_reflection::test::system_a"]
+node_1 [label="bevy_system_reflection::test::system_b"]
+node_2 [label="bevy_system_reflection::test::system_c"]
+node_3 [label="bevy_system_reflection::test::system_d"]
+node_4 [label="bevy_system_reflection::test::system_e"]
+node_5 [label="SystemSet SystemSetH"]
+node_6 [label="SystemSet SystemSetG"]
+node_4 -> node_5 [color=red label="child of" arrowhead=diamond]
+node_3 -> node_6 [color=red label="child of" arrowhead=diamond]
+node_1 -> node_0 [color=blue label="runs before" arrowhead=normal]
+node_1 -> node_2 [color=blue label="runs before" arrowhead=normal]
+node_2 -> node_5 [color=blue label="runs before" arrowhead=normal]
+node_5 -> node_6 [color=blue label="runs before" arrowhead=normal]
+}

crates/testing_crates/script_integration_test_harness/Cargo.toml

@@ -15,3 +15,4 @@ bevy_mod_scripting_functions = { workspace = true, features = [
     "lua_bindings",
 ] }
 regex = { version = "1.11" }
+pretty_assertions = "1.*"