Merge pull request #279 from mitchmindtree/refactor/plain-graph

Represent graphs with a plain `petgraph::Graph` (not `StableGraph`)

Author: mitchmindtree

crates/gantz_core/src/node/graph.rs

@@ -20,7 +20,12 @@ use serde::{Deserialize, Serialize};
 use std::hash::Hash;
 
 /// The graph type used to represent a nested graph.
-pub type Graph<N> = petgraph::stable_graph::StableGraph<N, Edge, Directed, Index>;
+///
+/// A plain (non-stable) `petgraph::Graph`: node indices stay contiguous (`0..n`)
+/// because `remove_node` swap-removes (the former-last node adopts the removed
+/// index). Callers that key persistent data by node index must migrate the
+/// swapped node on removal - see `gantz_core::node::state::move_value`.
+pub type Graph<N> = petgraph::graph::Graph<N, Edge, Directed, Index>;
 
 /// The type used for indexing into the graph.
 pub type Index = usize;

crates/gantz_core/src/node/state.rs

@@ -253,6 +253,20 @@ pub fn remove_value(vm: &mut Engine, node_path: &[usize]) -> Result<(), SteelErr
     Ok(())
 }
 
+/// Move the state value at `from` to `to`, clearing `from`.
+///
+/// The moved value carries any nested subtree with it, so this rekeys a whole
+/// node's state in one step. A no-op if there is no value at `from`. Used to
+/// migrate the swapped node after a plain `petgraph::Graph` swap-remove keeps
+/// node indices contiguous (see [`crate::node::graph::Graph`]).
+pub fn move_value(vm: &mut Engine, from: &[usize], to: &[usize]) -> Result<(), SteelErr> {
+    if let Some(val) = extract_value(vm, from)? {
+        update_value(vm, to, val)?;
+        remove_value(vm, from)?;
+    }
+    Ok(())
+}
+
 /// Extract the value for the node with the given ID.
 pub fn extract_value(vm: &Engine, node_path: &[usize]) -> Result<Option<SteelVal>, SteelErr> {
     let SteelVal::HashMapV(root_state) = vm.extract_value(ROOT_STATE)? else {

crates/gantz_core/tests/state.rs

@@ -233,6 +233,100 @@ fn test_graph_with_counters() {
     assert_eq!([a, b, c], [Counter(1), Counter(2), Counter(3)]);
 }
 
+// `move_value` rekeys a top-level node, carrying its whole nested subtree along.
+#[test]
+fn move_value_moves_nested_subtree() {
+    let mut vm = Engine::new_base();
+    vm.register_value(ROOT_STATE, SteelVal::empty_hashmap());
+
+    // Node 5 is a nested graph whose child 3 holds counter state.
+    node::state::update(&mut vm, &[5, 3], Counter(7)).unwrap();
+    assert_eq!(
+        node::state::extract::<Counter>(&vm, &[5, 3]).unwrap(),
+        Some(Counter(7)),
+    );
+
+    // Move node 5 -> 2; its child state must follow under the new key.
+    node::state::move_value(&mut vm, &[5], &[2]).unwrap();
+    assert_eq!(
+        node::state::extract::<Counter>(&vm, &[2, 3]).unwrap(),
+        Some(Counter(7)),
+    );
+    assert!(node::state::extract_value(&vm, &[5]).unwrap().is_none());
+
+    // Moving from an empty slot is a no-op.
+    node::state::move_value(&mut vm, &[9], &[1]).unwrap();
+    assert!(node::state::extract_value(&vm, &[1]).unwrap().is_none());
+}
+
+// A plain `petgraph::Graph` swap-removes: the former-last node adopts the removed
+// index. The GUI removal migration (`remove_value` for the deleted node, then
+// `move_value` for the swapped node) must carry the swapped stateful node's
+// accumulated state to its new index, so the recompiled code - which reads state
+// by the new index - still finds it.
+#[test]
+fn swap_remove_migrates_stateful_node_state() {
+    use gantz_core::node::graph::Graph;
+
+    // 0: standalone push (stateless), 1: push_b, 2: counter_b driven by push_b.
+    let mut g: Graph<Box<dyn DebugNode>> = Graph::default();
+    let p_a = g.add_node(Box::new(node_push()) as Box<dyn DebugNode>);
+    let p_b = g.add_node(Box::new(node_push()) as Box<_>);
+    let c_b = g.add_node(Box::new(node_counter()) as Box<_>);
+    g.add_edge(p_b, c_b, Edge::from((0, 0)));
+
+    let mut vm = Engine::new_base();
+    vm.register_value(ROOT_STATE, SteelVal::empty_hashmap());
+
+    // (Re)compile + register + load the eval fns for the current graph shape.
+    let load = |g: &Graph<Box<dyn DebugNode>>, vm: &mut Engine| {
+        gantz_core::graph::register(&no_lookup, g, &[], vm);
+        let eps = push_pull_entrypoints(&no_lookup, g);
+        let module = gantz_core::compile::module(&no_lookup, g, &eps, &Default::default()).unwrap();
+        for f in module {
+            vm.run(format!("{f}")).unwrap();
+        }
+    };
+    let ctx = node::MetaCtx::new(&no_lookup);
+    let push = |g: &Graph<Box<dyn DebugNode>>, vm: &mut Engine, n: usize| {
+        let ep = entrypoint::push(vec![n], g[node::graph::NodeIx::new(n)].n_outputs(ctx) as u8);
+        vm.call_function_by_name_with_args(&entry_fn_name(&ep.id()), vec![])
+            .unwrap();
+    };
+
+    // Push b twice -> counter_b (index 2) == 2.
+    load(&g, &mut vm);
+    push(&g, &mut vm, p_b.index());
+    push(&g, &mut vm, p_b.index());
+    assert_eq!(
+        node::state::extract::<Counter>(&vm, &[c_b.index()]).unwrap(),
+        Some(Counter(2)),
+    );
+
+    // Delete p_a (index 0). Swap-remove moves counter_b (last) into index 0.
+    let last = g.node_count() - 1;
+    assert_ne!(p_a.index(), last);
+    node::state::remove_value(&mut vm, &[p_a.index()]).unwrap();
+    g.remove_node(p_a);
+    node::state::move_value(&mut vm, &[last], &[p_a.index()]).unwrap();
+
+    // counter_b is now at index 0, p_b stayed at index 1. Recompile + re-register.
+    let c_b = node::graph::NodeIx::new(0);
+    let p_b = node::graph::NodeIx::new(1);
+    load(&g, &mut vm);
+
+    // The migrated state survived the move; pushing b once more -> 3.
+    assert_eq!(
+        node::state::extract::<Counter>(&vm, &[c_b.index()]).unwrap(),
+        Some(Counter(2)),
+    );
+    push(&g, &mut vm, p_b.index());
+    assert_eq!(
+        node::state::extract::<Counter>(&vm, &[c_b.index()]).unwrap(),
+        Some(Counter(3)),
+    );
+}
+
 // Manual regression check for #266: a stateful node must not leak memory per
 // evaluation. Drives one over a *single* persistent `Engine`, sampling RSS, and
 // asserts it stays bounded once warmed up.

crates/gantz_egui/src/ops.rs

@@ -123,8 +123,7 @@ where
     graph[node_ix].register(reg_ctx);
 
     // Position the new node under the pointer, falling back to the center of the
-    // current view. `insert` (not `entry`) so a reused stable-graph index can't
-    // inherit a removed node's stale position.
+    // current view.
     let pos = pos.unwrap_or_else(|| view.scene_rect.center());
     let egui_id = egui_graph::NodeId::from_u64(node_ix.index() as u64);
     view.layout.insert(egui_id, pos);
@@ -179,8 +178,7 @@ where
     let node_ix = graph.add_node(N::from(named_ref));
 
     // Position the new node under the pointer, falling back to the center of the
-    // current view. `insert` (not `entry`) so a reused stable-graph index can't
-    // inherit a removed node's stale position.
+    // current view.
     let pos = pos.unwrap_or_else(|| view.scene_rect.center());
     let egui_id = egui_graph::NodeId::from_u64(node_ix.index() as u64);
     view.layout.insert(egui_id, pos);
@@ -194,6 +192,60 @@ where
     Some(node_ix)
 }
 
+/// Remove `nodes` from `graph`, migrating the per-node state, layout and
+/// selection that are keyed by node index.
+///
+/// `petgraph::Graph::remove_node` swap-removes: the former-last node adopts the
+/// removed index, so exactly one surviving node changes index per removal.
+/// Targets are processed highest-index first, so a swap only ever pulls a
+/// surviving node down into an already-freed higher slot and never invalidates a
+/// pending target. The swapped node's state, layout entry and selection are then
+/// moved to its new index. Edge selection is cleared because removing a node
+/// drops its incident edges with compounded edge swaps. Returns whether any node
+/// was removed.
+///
+/// Run this before the next recompile (`vm::sync`): the regenerated code reads
+/// state by the new index, so the migration must already be in place.
+pub fn remove_nodes<N>(
+    graph: &mut Graph<N>,
+    vm: &mut Engine,
+    layout: &mut egui_graph::Layout,
+    selection: &mut crate::widget::graph_scene::Selection,
+    nodes: impl IntoIterator<Item = NodeIndex>,
+) -> bool {
+    let node_id = |ix: usize| egui_graph::NodeId::from_u64(ix as u64);
+    let mut targets: Vec<NodeIndex> = nodes.into_iter().collect();
+    targets.sort_unstable_by_key(|n| std::cmp::Reverse(n.index()));
+    targets.dedup();
+    let mut removed_any = false;
+    for t in targets {
+        if graph.node_weight(t).is_none() {
+            continue;
+        }
+        let last = graph.node_count() - 1;
+        // Drop the removed node's index-keyed data.
+        let _ = node::state::remove_value(vm, &[t.index()]);
+        layout.remove(&node_id(t.index()));
+        selection.nodes.remove(&t);
+        graph.remove_node(t);
+        // Migrate the node that swapped into `t` (the former `last`), if any.
+        if t.index() != last {
+            let _ = node::state::move_value(vm, &[last], &[t.index()]);
+            if let Some(pos) = layout.remove(&node_id(last)) {
+                layout.insert(node_id(t.index()), pos);
+            }
+            if selection.nodes.remove(&NodeIndex::new(last)) {
+                selection.nodes.insert(t);
+            }
+        }
+        removed_any = true;
+    }
+    if removed_any {
+        selection.edges.clear();
+    }
+    removed_any
+}
+
 /// Insert an Inspect node on the given edge, splicing it between the
 /// endpoints and positioning it at `cmd.pos`.
 pub fn inspect_edge<N>(
@@ -376,3 +428,56 @@ pub fn commit_layout<G>(
         head,
     ))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::widget::graph_scene::Selection;
+    use gantz_core::node::graph::NodeIx;
+
+    // Deleting a node swap-removes the former-last node into its slot; the
+    // swapped node's layout entry and selection must follow it to the new index.
+    #[test]
+    fn remove_nodes_migrates_layout_and_selection() {
+        let node_id = |i: usize| egui_graph::NodeId::from_u64(i as u64);
+
+        // Five nodes 0..5 (weights 10..15) each with a distinct layout x.
+        let mut graph: Graph<u32> = Graph::default();
+        for w in 10u32..15 {
+            graph.add_node(w);
+        }
+        let mut layout = egui_graph::Layout::default();
+        for i in 0..5 {
+            layout.insert(node_id(i), egui::pos2(i as f32, 0.0));
+        }
+        let mut selection = Selection::default();
+        selection.nodes.insert(NodeIx::new(4)); // select the (to-be-swapped) last
+
+        let mut vm = Engine::new_base();
+
+        // Delete index 1: node 4 (weight 14) swap-removes into slot 1.
+        let removed = remove_nodes(
+            &mut graph,
+            &mut vm,
+            &mut layout,
+            &mut selection,
+            [NodeIx::new(1)],
+        );
+        assert!(removed);
+
+        // The swapped node now sits at index 1.
+        assert_eq!(graph.node_count(), 4);
+        assert_eq!(graph[NodeIx::new(1)], 14);
+
+        // Layout followed the swap; the deleted and old-last slots are gone.
+        assert_eq!(layout.len(), 4);
+        assert_eq!(layout.get(&node_id(1)).copied(), Some(egui::pos2(4.0, 0.0)));
+        assert!(!layout.contains_key(&node_id(4)));
+
+        // Selection followed the swap: node 4 -> node 1.
+        assert_eq!(
+            selection.nodes.iter().copied().collect::<Vec<_>>(),
+            vec![NodeIx::new(1)],
+        );
+    }
+}