Custom SVG renderer for the offline DAG#588
Conversation
b2098b7 to
befbcfa
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the DOT-based graph rendering system with a custom SVG renderer for visualizing DAG configurations. The implementation uses the layout-rs library for graph layout and generates SVG directly, eliminating the dependency on external dot and inkscape tools.
- Centralizes DAG rendering into a single justfile recipe that works from any directory
- Implements comprehensive custom SVG rendering with support for self-loops, back-edges, and edge crossings
- Adds platform-specific file opening logic for better cross-platform support
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| justfile | Adds centralized dag recipe that renders copperconfig.ron from the current working directory with optional mission parameter |
| examples/modular_config_example/copperconfig.ron | New example configuration demonstrating modular includes feature |
| examples/cu_rp_balancebot/justfile | Removes local dag recipe in favor of centralized version |
| examples/cu_elrs_bdshot_demo/justfile | Removes local dag recipe in favor of centralized version |
| examples/cu_caterpillar/justfile | Removes local dag recipe in favor of centralized version |
| examples/cu_basic_fc/justfile | Removes local dag recipe in favor of centralized version |
| core/cu29_runtime/tests/render_edge_cases.ron | Adds comprehensive test cases for edge rendering scenarios (self-loops, back-edges, crossings) |
| core/cu29_runtime/tests/justfile | Adds dag recipe for testing edge case rendering |
| core/cu29_runtime/src/rendercfg.rs | Complete rewrite: replaces DOT generation with custom SVG renderer including layout, path generation, label placement, and multi-platform file opening |
| core/cu29_runtime/src/config.rs | Changes internal structures to pub(crate), refactors build_render_topology to use Vec instead of HashMap for deterministic ordering |
| core/cu29_runtime/Cargo.toml | Adds layout-rs dependency for graph layout functionality |
Comments suppressed due to low confidence (1)
core/cu29_runtime/src/config.rs:1755
- The algorithm change from HashMap to Vec with separate lookup HashMap alters the iteration order of nodes. The original code used
into_values()which had non-deterministic ordering, while the new Vec-based approach maintains insertion order. However, there's a subtle bug: when mutating nodes during edge processing, the code gets a mutable reference to a node by index, but if the same node appears as both src and dst in an edge, the sequential mutable borrows could cause issues. While the current implementation works because src and dst are different in practice, it would be more robust to complete all src mutations before dst mutations, or to collect the changes first.
let node = &mut nodes[idx];
if node.flavor == Flavor::Task && cnx.src_channel.is_none() && node.outputs.is_empty() {
node.outputs.push("out0".to_string());
}
}
if let Some(&idx) = node_lookup.get(&cnx.dst) {
let node = &mut nodes[idx];
if node.flavor == Flavor::Task && cnx.dst_channel.is_none() {
let next = format!("in{}", node.inputs.len());
node.inputs.push(next);
}
}
connections.push(RenderConnection {
src: cnx.src.clone(),
src_port: cnx.src_channel.clone(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let t = 0.5; | ||
| best_mid = Point::new(a.x + (b.x - a.x) * t, a.y + (b.y - a.y) * t); | ||
| best_dir = Point::new(dx / len, dy / len); | ||
| } |
There was a problem hiding this comment.
The division width / count as f64 on line 2432 could result in division by zero if the edges vector is empty. While the grouping logic might prevent this in practice, defensive programming would suggest checking for empty groups or ensuring count > 0.
|
|
||
| let mut port_anchors = HashMap::new(); | ||
| for node in &nodes { | ||
| let element = graph.element(node.handle); |
There was a problem hiding this comment.
The infinity check only validates if min is finite, but if no nodes exist in the section, both min and max would remain at their infinity values. Setting max to (0.0, 0.0) when min is infinite creates an inconsistent state where min is INFINITY but max is 0.0. This could cause unexpected behavior in subsequent calculations. Consider setting both to (0.0, 0.0) or handling the empty case explicitly.
| }; | ||
| let seg2 = straight_segment(start_corner, end_corner); | ||
| let seg3 = BezierSegment { | ||
| start: end_corner, |
There was a problem hiding this comment.
The variable lane_span is assigned but never used in this function. The underscore prefix let _ = lane_span; on line 2020 explicitly ignores it. If this parameter is not needed, consider removing it from the function signature, or if it's intended for future use, document why it's currently unused.
| let mut length = 0.0; | ||
| for seg in path { | ||
| length += seg.start.distance_to(seg.c1); |
There was a problem hiding this comment.
Potential divide-by-zero issue: if base_size is 0, the division max_width / base_size as f64 on line 2463 will result in infinity. While usize font sizes are unlikely to be zero in practice, this should be validated or handled defensively.
b172a70 to
94b36de
Compare
(the last one with some last minute tweaks: Mission: Default if no mission is defined, uniform colors for stuff out of the same port and only one label for the type, better deterministic color cycling)