Grida Canvas - Layout (prep) - Scene Graph#432
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR replaces the NodeRepository-based scene model with a new SceneGraph across the grida-canvas crate. It adds a scene_graph module, a SceneGraph API (Parent, NodeId, traversal/mutation), updates Scene to hold a graph, and refactors IO, painter, runtime, examples, benches, tests, and utilities to use SceneGraph. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as Loader
participant Conv as Converter (Figma/Grida)
participant SG as SceneGraph
participant Scene as Scene
Client->>Conv: convert(document / snapshot)
Conv->>Conv: parse nodes, collect links map (child relationships)
Conv->>SG: SceneGraph::new_from_snapshot(nodes, links, roots)
SG-->>Conv: SceneGraph
Conv->>Scene: Scene { name, background_color, graph: SceneGraph }
Scene-->>Client: Scene
note right of SG: Graph holds nodes + adjacency (roots & children)
sequenceDiagram
autonumber
actor App as Renderer
participant Scene as Scene
participant SG as SceneGraph
participant Painter as Painter/Layer
participant Cache as GeometryCache
App->>Scene: request graph
Scene-->>App: &SceneGraph
App->>SG: roots()
loop for each root
App->>Painter: from_node(root_id, &SceneGraph, scene_cache, opacity)
Painter->>SG: get_node(root_id)
alt node has children
Painter->>SG: get_children(root_id)
loop traverse children
Painter->>SG: get_node(child_id)
Painter->>Cache: geometry lookup/build
end
end
end
note over Painter,SG: All traversal and lookups use SceneGraph APIs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| /// Add a child to a parent's children list | ||
| pub fn add_child(&mut self, parent: &NodeId, child: NodeId) -> SceneGraphResult<()> { | ||
| let children = self | ||
| .links | ||
| .get_mut(parent) | ||
| .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?; | ||
| children.push(child); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Insert a child at a specific index in the parent's children list | ||
| pub fn add_child_at( | ||
| &mut self, | ||
| parent: &NodeId, | ||
| child: NodeId, | ||
| index: usize, | ||
| ) -> SceneGraphResult<()> { | ||
| let children = self | ||
| .links | ||
| .get_mut(parent) | ||
| .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?; | ||
|
|
||
| if index > children.len() { | ||
| return Err(SceneGraphError::IndexOutOfBounds { | ||
| parent: parent.clone(), | ||
| index, | ||
| len: children.len(), | ||
| }); | ||
| } | ||
|
|
||
| children.insert(index, child); | ||
| Ok(()) |
There was a problem hiding this comment.
The new SceneGraph helpers (add_child/add_child_at) simply push the child ID into the target parent’s list without removing it from its previous location. When a node is first appended to the graph (e.g. as a root) and later reparented through these helpers, the node remains in roots or in another parent’s child list and is also added to the new parent. Traversals such as scene.graph.roots() → walk_preorder will therefore visit and render the same node twice, and the graph can end up with the same node under multiple parents. Reparent operations should detach the node from any existing parent and from roots before linking it to the new parent.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/grida-canvas/src/io/io_figma.rs (3)
476-483: StrokeAlign mapping bug: serde_json stringification breaks mapping (defaults to Center).You convert enum to JSON string and match "CENTER"/"INSIDE"/"OUTSIDE"; JSON adds quotes, so it won’t match and falls back to Center. Use the enum mapping directly.
Apply this diff (updates helper and call sites; replicate pattern across all convert_* using stroke_align):
- /// Convert Figma's stroke align to our StrokeAlign - fn convert_stroke_align(stroke_align: String) -> StrokeAlign { - match stroke_align.as_str() { - "INSIDE" => StrokeAlign::Inside, - "OUTSIDE" => StrokeAlign::Outside, - "CENTER" => StrokeAlign::Center, - _ => StrokeAlign::Center, - } - } + /// Convert Figma's stroke align to our StrokeAlign + fn convert_stroke_align(align: Option<&FigmaStrokeAlign>) -> StrokeAlign { + align.map(StrokeAlign::from).unwrap_or(StrokeAlign::Center) + }Example call-site fixes (do similarly in component/instance/section/frame/ellipse/rect/polygon/line/boolean/star):
- stroke_align: Self::convert_stroke_align( - component - .stroke_align - .as_ref() - .map(|a| serde_json::to_string(a).unwrap_or_default()) - .unwrap_or_else(|| "CENTER".to_string()), - ), + stroke_align: Self::convert_stroke_align(component.stroke_align.as_ref()),- stroke_align: Self::convert_stroke_align( - origin - .stroke_align - .as_ref() - .map(|a| serde_json::to_string(a).unwrap_or_default()) - .unwrap_or_else(|| "CENTER".to_string()), - ), + stroke_align: Self::convert_stroke_align(origin.stroke_align.as_ref()),Also applies to: 630-640, 722-735, 769-776, 895-904, 1163-1172, 1199-1206, 1233-1241, 1267-1276, 1311-1320, 1345-1354
1043-1114: Vector children are never linked; resulting group renders emptyconvert_vector creates path nodes and inserts into the repository but does not record parent→children links, so SceneGraph won’t attach them to the vector group.
Add link insertion for the vector group:
fn convert_vector(&mut self, origin: &Box<VectorNode>) -> Result<Node, String> { let mut children = Vec::new(); let mut path_index = 0; @@ - // Create a group node containing all the path nodes - Ok(Node::Container(ContainerNodeRec { - id: origin.id.clone(), + // Link children under the vector container + let node_id = origin.id.clone(); + if !children.is_empty() { + self.links.insert(node_id.clone(), children); + } + + // Create a group node containing all the path nodes + Ok(Node::Container(ContainerNodeRec { + id: node_id, name: Some(origin.name.clone()), active: origin.visible.unwrap_or(true), @@ })) }
88-92: Unused variable 'repeat'repeat is computed but unused in ImagePaint branches. Either wire it into the schema or silence the warning.
Minimal fix to silence warnings:
- let repeat = match image.scale_mode { + let _repeat = match image.scale_mode {Apply in both implementations (impl From<&FigmaPaint> and convert_paint).
Also applies to: 354-357
crates/grida-canvas/examples/bench_100k.rs (1)
44-45: Division-by-zero when n <= 1 in diagonal_progress(grid_height + grid_width - 2) can be 0 (e.g., n=1), causing a runtime panic.
Apply this fix:
- let diagonal_progress = (row + col) as f32 / (grid_height + grid_width - 2) as f32; + let denom = (grid_height + grid_width - 2).max(1) as f32; + let diagonal_progress = (row + col) as f32 / denom;crates/grida-canvas/examples/sys_camera.rs (1)
217-227: Double free of Skia Surface pointer on resize/cleanupsurface_ptr is freed on resize and again at program end. Because raw pointers are Copy, both the closure and outer scope hold the same value → potential double free.
Use Cell to own and swap the pointer safely, and free exactly once:
@@ -use std::{ffi::CString, num::NonZeroU32}; +use std::{cell::Cell, ffi::CString, num::NonZeroU32}; @@ - let ( - surface_ptr, + let ( + surface_raw_ptr, el, @@ - // Create renderer + // Wrap surface pointer for safe swaps across resizes + let surface_ptr = Cell::new(surface_raw_ptr); + + // Create renderer @@ - Backend::GL(surface_ptr), + Backend::GL(surface_ptr.get()), @@ - Event::WindowEvent { + Event::WindowEvent { event: WindowEvent::Resized(size), .. } => { // Recreate Skia surface @@ - // Update surface pointer - unsafe { _ = Box::from_raw(surface_ptr) }; - let new_surface_ptr = Box::into_raw(Box::new(surface)); - renderer.backend = Backend::GL(new_surface_ptr); + // Swap surface pointer safely and free the old one + let new_surface_ptr = Box::into_raw(Box::new(surface)); + let old_ptr = surface_ptr.replace(new_surface_ptr); + unsafe { _ = Box::from_raw(old_ptr) }; + renderer.backend = Backend::GL(surface_ptr.get()); } @@ - // Clean up - unsafe { - _ = Box::from_raw(surface_ptr); - } + // Clean up current surface once + unsafe { _ = Box::from_raw(surface_ptr.get()) };This prevents double free and ensures the latest surface is freed. As per coding guidelines
Also applies to: 291-296, 336-340
crates/grida-canvas/src/cache/geometry.rs (1)
261-306: Bug: TextSpan returns local bounds; should return world boundsbuild_recursive unions child bounds in world space. Returning intrinsic (local) bounds for TextSpan breaks parent unions and culling.
Apply this fix:
- intrinsic_bounds + world_bounds
🧹 Nitpick comments (30)
crates/grida-canvas/src/io/io_figma.rs (1)
49-105: Duplicate paint conversion logicYou have both impl From<&FigmaPaint> for Paint and FigmaConverter::convert_paint with diverging behavior (e.g., image URL mapping). Prefer a single source of truth to avoid drift.
- Keep only FigmaConverter::convert_paint (uses image URL remapping) and delete the impl From<&FigmaPaint>; or
- Make the impl delegate to a shared helper used by both.
Also applies to: 310-371
crates/grida-canvas/src/resources/mod.rs (1)
125-148: Broaden image extraction and deduplicate resultsCurrently only Rectangle nodes are inspected and URLs may repeat. Consider scanning all node types with paints and dedup via HashSet.
Example minimal dedup:
pub fn extract_image_urls(scene: &Scene) -> Vec<String> { - let mut urls = Vec::new(); + use std::collections::HashSet; + let mut urls = HashSet::new(); for (id, _) in scene.graph.iter() { if let Ok(n) = scene.graph.get_node(id) { - if let crate::node::schema::Node::Rectangle(rect) = n { + if let crate::node::schema::Node::Rectangle(rect) = n { for fill in &rect.fills { if let Paint::Image(img) = fill { match &img.image { - ResourceRef::RID(r) | ResourceRef::HASH(r) => urls.push(r.clone()), + ResourceRef::RID(r) | ResourceRef::HASH(r) => { urls.insert(r.clone()); }, } } } for stroke in &rect.strokes { if let Paint::Image(img) = stroke { match &img.image { - ResourceRef::RID(r) | ResourceRef::HASH(r) => urls.push(r.clone()), + ResourceRef::RID(r) | ResourceRef::HASH(r) => { urls.insert(r.clone()); }, } } } } } } - urls + urls.into_iter().collect() }Optionally, extract paints from other node types too (Ellipse, Rectangle, Vector paths, etc.) to catch all images.
crates/grida-canvas/examples/bench_100k.rs (1)
85-89: Clamp and round HSV→RGB conversion to avoid truncation artifactsCasting f32→u8 truncates. Clamp to [0,1] and round for better color fidelity.
- ( - ((r + m) * 255.0) as u8, - ((g + m) * 255.0) as u8, - ((b + m) * 255.0) as u8, - ) + ( + (255.0 * (r + m).clamp(0.0, 1.0)).round() as u8, + (255.0 * (g + m).clamp(0.0, 1.0)).round() as u8, + (255.0 * (b + m).clamp(0.0, 1.0)).round() as u8, + )crates/grida-canvas/examples/grida_paint.rs (1)
42-45: Avoid repeated clones of parent idIf NodeId is cheap (Copy) prefer passing directly; if not, bind once:
let parent = Parent::NodeId(root_container_id.clone()); // reuse `parent.clone()` in each append_childBased on learnings
Also applies to: 79-82, 117-120, 146-149, 188-191, 231-234
crates/grida-canvas/examples/grida_shapes.rs (1)
45-46: Reduce Parent::NodeId cloningBind once and reuse to cut allocs if NodeId is not Copy:
let parent = Parent::NodeId(root_container_id.clone()); // use parent.clone() in loopsBased on learnings
Also applies to: 63-64, 91-92, 111-112, 138-139, 159-160, 181-182
crates/grida-canvas/examples/grida_vector.rs (2)
28-53: Avoid manual ids; prefer factory helpers to ensure validity/uniquenessVectorNodeRec instances set id = "1", "2", etc. If SceneGraph or downstream relies on unique ids, hardcoding risks collisions. Prefer NodeFactory (if available) to create Vector nodes with generated ids or let the graph assign.
I can draft a helper that builds VectorNodeRec with a generated id and sane defaults; want that?
Also applies to: 58-84, 90-116, 121-147, 153-181, 190-221, 227-253, 262-292, 298-330
151-151: Leftover FIXME indicates broken shape; can help debugThe closed segment ab(3,0) “not working” likely needs a region defined for fill, or consistent winding/order. If desired, share the VectorNetwork semantics and I’ll propose a fix.
Also applies to: 166-171
crates/grida-canvas/examples/grida_blendmode.rs (1)
1-1: Top-level FIXME: broken demoIf this is intentionally broken, consider moving under golden_ or gating with a cargo feature to avoid confusing users.
crates/grida-canvas/src/painter/geometry.rs (1)
246-262: Traversal via SceneGraph is correct; consider minor robustness and perf tweaks
- Preallocate: reserve capacity for
shapes_with_opsfromchildren.len()to reduce reallocs.- Inversion fallback: using identity on non-invertible transforms can produce incorrect results. Consider skipping such children or bailing out.
Proposed diff:
- let mut shapes_with_ops = Vec::new(); - - let children = graph.get_children(&node.id)?; + let children = graph.get_children(&node.id)?; + if children.is_empty() { + return None; + } + let mut shapes_with_ops = Vec::with_capacity(children.len()); @@ - let relative = inv.compose(&child_world); + // If inversion failed above and returned identity, consider skipping to avoid wrong transforms. + let relative = inv.compose(&child_world);If you prefer skipping on inversion failure, replace the earlier
unwrap_or_else(AffineTransform::identity)with an early return or conditional skip for that child.crates/grida-canvas/tests/scene_cache.rs (1)
63-68: Layer ordering assumption may be brittleAssertions rely on rect layer preceding container. If painter changes ordering (z-order, stable sort), this test could flap. Consider asserting set membership or documenting the ordering contract.
crates/grida-canvas/src/cache/geometry.rs (1)
232-260: Clarify container bounds semantics (own vs union)Entry.absolute_bounding_box stores the container’s own bounds, while the function returns union_world_bounds (own ∪ children). Groups store union into the entry. If intentional (container represents its own shape for culling/hit‑test), please add a short comment; otherwise consider storing union in the entry for consistency.
crates/grida-canvas/examples/grida_shapes_ellipse.rs (1)
27-43: Optional: batch appends per row to reduce overheadCollect nodes then use append_children once per row to lower per-call overhead (repeat for other rows).
crates/grida-canvas/src/dummy/mod.rs (1)
63-83: Benchmark: consider chunked appends or preallocationFor large grids, build a Vec per row (or full scene) and call append_children to reduce repeated insert overhead. Optionally reserve Vec capacity: (rows*cols) to avoid reallocations.
crates/grida-canvas/src/node/schema.rs (1)
97-107: UnknownNodeProperties.children vs SceneGraphThis still exposes children even though hierarchy lives in SceneGraph. If only for IO/back-compat, annotate that, or plan deprecation to avoid confusion.
crates/grida-canvas/examples/app_grida.rs (2)
44-71: Snapshot → SceneGraph construction: add child filtering and handle unknown IDsCurrent build skips scene nodes as parents, but child lists may still reference scene or unknown node IDs. If SceneGraph::new_from_snapshot doesn’t sanitize, consider filtering children to existing, non‑scene nodes to avoid dangling edges.
Example tweak before calling new_from_snapshot:
- let filtered_links: std::collections::HashMap<String, Vec<String>> = links + let node_ids: std::collections::HashSet<String> = + nodes.iter().map(|n| n.id().to_string()).collect(); + let filtered_links: std::collections::HashMap<String, Vec<String>> = links .into_iter() .filter(|(parent_id, _)| !scene_node_ids.contains(parent_id)) - .filter_map(|(parent_id, children_opt)| children_opt.map(|children| (parent_id, children))) + .filter_map(|(parent_id, children_opt)| { + children_opt.map(|children| { + let children = children + .into_iter() + .filter(|c| node_ids.contains(c)) + .collect::<Vec<_>>(); + (parent_id, children) + }) + }) .collect();
71-75: Use parsed background color when availableYou extract the scene background color but then ignore it and hardcode a gray. Prefer the parsed value with a sensible fallback.
Apply:
- Scene { - name: scene_name, - background_color: Some(CGColor(230, 230, 230, 255)), - graph, - } + Scene { + name: scene_name, + background_color: _bg_color.or(Some(CGColor(230, 230, 230, 255))), + graph, + }Additionally, rename
_bg_colortobg_colorabove to avoid the leading underscore since it’s now used:- let (scene_name, _bg_color) = if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) = + let (scene_name, bg_color) = if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) = canvas_file.document.nodes.get(&scene_id)crates/grida-canvas/src/runtime/scene.rs (1)
66-80: Guard DFS against cycles; avoid potential stack overflowcollect_scene_font_families uses recursive DFS without a visited set. If malformed graphs or future features introduce cycles, this can recurse infinitely. Add a visited set to make traversal robust.
Apply:
-fn collect_scene_font_families(scene: &Scene) -> HashSet<String> { - fn walk(id: &NodeId, graph: &SceneGraph, set: &mut HashSet<String>) { +fn collect_scene_font_families(scene: &Scene) -> HashSet<String> { + fn walk( + id: &NodeId, + graph: &SceneGraph, + set: &mut HashSet<String>, + visited: &mut HashSet<NodeId>, + ) { + if !visited.insert(id.clone()) { + return; + } if let Ok(node) = graph.get_node(id) { match node { Node::TextSpan(n) => { set.insert(n.text_style.font_family.clone()); } _ => {} } } - if let Some(children) = graph.get_children(id) { - for child in children { - walk(child, graph, set); - } - } + if let Some(children) = graph.get_children(id) { + for child in children { + walk(child, graph, set, visited); + } + } } let mut set = HashSet::new(); - for id in scene.graph.roots() { - walk(&id, &scene.graph, &mut set); + let mut visited = HashSet::new(); + for id in scene.graph.roots() { + walk(&id, &scene.graph, &mut set, &mut visited); } set }Also applies to: 83-85
crates/grida-canvas/tests/geometry_cache.rs (1)
15-15: Avoid cloning the whole graph in testsYou clone the graph solely to keep node_count() available after moving it into Scene. Capture the count first and move the original graph to avoid the clone.
Apply:
- let scene = Scene { + let node_count = graph.node_count(); + let scene = Scene { name: "test".into(), background_color: None, - graph: graph.clone(), + graph, }; @@ - assert_eq!(cache.len(), graph.node_count()); + assert_eq!(cache.len(), node_count);Also applies to: 23-30, 31-41
crates/grida-canvas/examples/grida_effects.rs (1)
3-3: LGTM overall; reduce repeated clones of root parent (optional)Migration to SceneGraph looks correct. To cut repeated String clones in append_child calls, reuse a Parent enum instance for the root container.
Example:
let parent_root = Parent::NodeId(root_container_id.clone()); // ... graph.append_child(Node::Rectangle(rect), parent_root.clone()); // repeat with parent_root.clone()For further ergonomics, consider small helpers to spawn a shape with effects at (x, y).
Also applies to: 10-11, 20-21, 45-48, 67-71, 91-95, 110-114, 146-149, 168-171, 187-190, 213-216, 235-239, 305-308, 365-368, 374-375
crates/grida-canvas/src/window/application.rs (2)
9-9: Remove unused import
Parentisn’t used in this file. Drop it to keep warnings clean.-use crate::node::scene_graph::{Parent, SceneGraph}; +use crate::node::scene_graph::SceneGraph;
289-289: Avoid unnecessary String allocation when getting a node
&id.to_string()allocates. Prefer passing&strif supported byget_node, or add an overload taking&str.- if let Ok(node) = scene.graph.get_node(&id.to_string()) { + if let Ok(node) = scene.graph.get_node(id) {If
get_nodedoesn’t accept&str, consider addingpub fn get_node_str(&self, id: &str)in SceneGraph and use it here. [Based on learnings]crates/grida-canvas/examples/grida_images.rs (1)
3-3: SceneGraph migration LGTM; handle image load errorsGraph composition and Scene.graph wiring look correct.
Avoid
unwrap()on network image fetch to keep the demo resilient offline or on transient failures (log and fallback).- let bytes = load_image(&image_url).await.unwrap(); + let bytes = match load_image(&image_url).await { + Ok(b) => b, + Err(e) => { + eprintln!("failed to load demo image ({}): {}", &image_url, e); + Vec::new() // or early-return a simpler scene without image paints + } + };Note: If you fallback to empty bytes, associated RID won’t be registered; consider guarding
renderer.add_image(&bytes)accordingly.Also applies to: 237-256, 261-261
crates/grida-canvas/examples/grida_booleans.rs (2)
1-2: Address FIXME or rename the demoHeader says “broken demo”. If intended to move under golden_*, rename and update references, or remove the FIXME to avoid confusion. I can help with a follow-up PR.
12-12: Boolean ops composition via SceneGraph is correct; small ergonomic tweakThe append sequence (boolean node → operands as children → description text to root) is sound.
To reduce repeated allocations, cache the root parent once:
- let root_container_id = graph.append_child(Node::Container(root_container_node), Parent::Root); + let root_container_id = graph.append_child(Node::Container(root_container_node), Parent::Root); + let root_parent = Parent::NodeId(root_container_id.clone()); ... - let bool_id = graph.append_child( - Node::BooleanOperation(bool_node), - Parent::NodeId(root_container_id.clone()), - ); + let bool_id = graph.append_child(Node::BooleanOperation(bool_node), root_parent.clone()); ... - graph.append_child(Node::TextSpan(text), Parent::NodeId(root_container_id.clone())); + graph.append_child(Node::TextSpan(text), root_parent.clone());Also applies to: 22-22, 77-89, 142-156, 210-221, 276-287, 293-293
crates/grida-canvas/examples/grida_nested.rs (1)
16-21: Prefer usize for counts/indiceslevels as usize is idiomatic and removes some casts in ranges/enumeration.
- let levels: i32 = 6; // Number of nesting levels + let levels: usize = 6; // Number of nesting levelsNote: keep powi() using i32 via i as i32 where needed.
crates/grida-canvas/examples/grida_webfonts.rs (1)
127-146: Graph build is correct; consider batching top-level appendsReplace two append_child calls for heading/description with a single append_children for micro-efficiency and clarity.
- graph.append_child( - Node::TextSpan(heading_node), - Parent::NodeId(root_container_id.clone()), - ); - graph.append_child( - Node::TextSpan(description_node), - Parent::NodeId(root_container_id.clone()), - ); + graph.append_children( + vec![Node::TextSpan(heading_node), Node::TextSpan(description_node)], + Parent::NodeId(root_container_id.clone()), + );Optionally, batch the Albert Sans nodes similarly by collecting into a Vec and one append_children call.
crates/grida-canvas/src/node/scene_graph.rs (1)
136-143: Consider optimizingappend_childrento reduce allocations.The current implementation clones
parenton each iteration and callsappend_childin a loop, which performs incremental updates to the links map.Apply this diff to batch the parent updates:
pub fn append_children(&mut self, nodes: Vec<Node>, parent: Parent) -> Vec<NodeId> { - let mut ids = Vec::new(); + let mut ids = Vec::with_capacity(nodes.len()); + for node in nodes { - let id = self.append_child(node, parent.clone()); + let id = self.nodes.insert(node); ids.push(id); } + + match parent { + Parent::Root => { + self.roots.extend(ids.clone()); + } + Parent::NodeId(parent_id) => { + self.links + .entry(parent_id) + .or_insert_with(Vec::new) + .extend(ids.clone()); + } + } + ids }crates/grida-canvas/examples/grida_fills.rs (1)
41-41: Consider reducing allocations by reusing Parent.Each
append_childcall clonesroot_idto constructParent::NodeId(root_id.clone()). For clarity in examples this is fine, but in performance-critical code you could factor out the parent:let parent = Parent::NodeId(root_id.clone()); // Then use &parent or parent.clone() as needed graph.append_child(Node::Rectangle(multi_solid_rect), parent.clone());This is a nitpick for example code and does not require changes.
Also applies to: 72-72, 107-107, 155-155, 216-216, 271-271, 334-334, 369-369
crates/grida-canvas/examples/grida_strokes.rs (1)
53-56: Repetitive parent construction is acceptable for example clarity.While each
append_childcall clonesroot_idto construct the parent, this pattern is clear and readable for example code. In performance-critical paths, consider factoring out the parent as noted in other files.Also applies to: 77-80, 97-100, 113-116, 130-133, 148-151
crates/grida-canvas/examples/grida_mask.rs (1)
8-59: LGTM! Helper function migrated correctly.The function signature and implementation correctly:
- Accept
&mut SceneGraphandParentparameters- Use
graph.append_child()to add nodes- Return
Vec<NodeId>for the created contentThe
parent.clone()on lines 28, 40, and 56 is a minor inefficiency but acceptable for helper functions. You could pass&Parentand clone only when needed, but this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/grida-canvas/goldens/pdf.pdfis excluded by!**/*.pdfcrates/grida-canvas/goldens/svg.svgis excluded by!**/*.svgcrates/grida-canvas/goldens/type_stroke.pngis excluded by!**/*.png
📒 Files selected for processing (50)
crates/grida-canvas/Cargo.toml(2 hunks)crates/grida-canvas/benches/bench_rectangles.rs(1 hunks)crates/grida-canvas/examples/app_figma.rs(1 hunks)crates/grida-canvas/examples/app_grida.rs(2 hunks)crates/grida-canvas/examples/bench_100k.rs(2 hunks)crates/grida-canvas/examples/golden_container_stroke.rs(2 hunks)crates/grida-canvas/examples/golden_pdf.rs(3 hunks)crates/grida-canvas/examples/golden_svg.rs(4 hunks)crates/grida-canvas/examples/golden_type_stroke.rs(2 hunks)crates/grida-canvas/examples/grida_basic.rs(3 hunks)crates/grida-canvas/examples/grida_blendmode.rs(8 hunks)crates/grida-canvas/examples/grida_booleans.rs(6 hunks)crates/grida-canvas/examples/grida_container.rs(2 hunks)crates/grida-canvas/examples/grida_effects.rs(13 hunks)crates/grida-canvas/examples/grida_fills.rs(10 hunks)crates/grida-canvas/examples/grida_gradients.rs(6 hunks)crates/grida-canvas/examples/grida_image.rs(2 hunks)crates/grida-canvas/examples/grida_images.rs(2 hunks)crates/grida-canvas/examples/grida_lines.rs(3 hunks)crates/grida-canvas/examples/grida_mask.rs(9 hunks)crates/grida-canvas/examples/grida_nested.rs(1 hunks)crates/grida-canvas/examples/grida_paint.rs(8 hunks)crates/grida-canvas/examples/grida_shapes.rs(9 hunks)crates/grida-canvas/examples/grida_shapes_ellipse.rs(7 hunks)crates/grida-canvas/examples/grida_strokes.rs(18 hunks)crates/grida-canvas/examples/grida_texts.rs(2 hunks)crates/grida-canvas/examples/grida_vector.rs(11 hunks)crates/grida-canvas/examples/grida_webfonts.rs(3 hunks)crates/grida-canvas/examples/sys_camera.rs(2 hunks)crates/grida-canvas/examples/wd_animation.rs(2 hunks)crates/grida-canvas/src/cache/geometry.rs(5 hunks)crates/grida-canvas/src/dummy/mod.rs(3 hunks)crates/grida-canvas/src/io/io_figma.rs(10 hunks)crates/grida-canvas/src/io/io_grida.rs(0 hunks)crates/grida-canvas/src/node/factory.rs(0 hunks)crates/grida-canvas/src/node/mod.rs(1 hunks)crates/grida-canvas/src/node/scene_graph.rs(1 hunks)crates/grida-canvas/src/node/schema.rs(2 hunks)crates/grida-canvas/src/painter/geometry.rs(4 hunks)crates/grida-canvas/src/painter/layer.rs(26 hunks)crates/grida-canvas/src/painter/painter_debug_node.rs(8 hunks)crates/grida-canvas/src/resources/mod.rs(1 hunks)crates/grida-canvas/src/runtime/scene.rs(4 hunks)crates/grida-canvas/src/vectornetwork/vn_painter.rs(1 hunks)crates/grida-canvas/src/window/application.rs(3 hunks)crates/grida-canvas/tests/export_as_pdf.rs(3 hunks)crates/grida-canvas/tests/geometry_cache.rs(3 hunks)crates/grida-canvas/tests/hit_test.rs(4 hunks)crates/grida-canvas/tests/render_bounds.rs(6 hunks)crates/grida-canvas/tests/scene_cache.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/grida-canvas/src/node/factory.rs
- crates/grida-canvas/src/io/io_grida.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: Use the skia-safe crate for all painting in the 2D real-time rendering engine
Use the math2 crate for geometry and common math operations
Ensure source is formatted with rustfmt (cargo fmt)
Files:
crates/grida-canvas/src/node/mod.rscrates/grida-canvas/tests/scene_cache.rscrates/grida-canvas/examples/golden_container_stroke.rscrates/grida-canvas/src/vectornetwork/vn_painter.rscrates/grida-canvas/examples/grida_shapes.rscrates/grida-canvas/examples/grida_basic.rscrates/grida-canvas/tests/render_bounds.rscrates/grida-canvas/examples/grida_nested.rscrates/grida-canvas/tests/hit_test.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/examples/grida_image.rscrates/grida-canvas/examples/grida_blendmode.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/examples/grida_mask.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/examples/wd_animation.rscrates/grida-canvas/examples/bench_100k.rscrates/grida-canvas/examples/app_figma.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/examples/grida_webfonts.rscrates/grida-canvas/examples/grida_shapes_ellipse.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/dummy/mod.rscrates/grida-canvas/examples/golden_pdf.rscrates/grida-canvas/examples/grida_container.rscrates/grida-canvas/benches/bench_rectangles.rscrates/grida-canvas/examples/grida_fills.rscrates/grida-canvas/examples/grida_images.rscrates/grida-canvas/examples/golden_type_stroke.rscrates/grida-canvas/examples/grida_texts.rscrates/grida-canvas/examples/grida_paint.rscrates/grida-canvas/examples/grida_lines.rscrates/grida-canvas/examples/grida_vector.rscrates/grida-canvas/examples/app_grida.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/examples/sys_camera.rscrates/grida-canvas/examples/grida_gradients.rscrates/grida-canvas/examples/golden_svg.rscrates/grida-canvas/examples/grida_strokes.rscrates/grida-canvas/examples/grida_effects.rscrates/grida-canvas/tests/geometry_cache.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/tests/export_as_pdf.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/examples/grida_booleans.rs
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/src/node/mod.rscrates/grida-canvas/tests/scene_cache.rscrates/grida-canvas/examples/golden_container_stroke.rscrates/grida-canvas/src/vectornetwork/vn_painter.rscrates/grida-canvas/examples/grida_shapes.rscrates/grida-canvas/examples/grida_basic.rscrates/grida-canvas/tests/render_bounds.rscrates/grida-canvas/examples/grida_nested.rscrates/grida-canvas/tests/hit_test.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/examples/grida_image.rscrates/grida-canvas/examples/grida_blendmode.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/examples/grida_mask.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/examples/wd_animation.rscrates/grida-canvas/examples/bench_100k.rscrates/grida-canvas/examples/app_figma.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/examples/grida_webfonts.rscrates/grida-canvas/examples/grida_shapes_ellipse.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/dummy/mod.rscrates/grida-canvas/examples/golden_pdf.rscrates/grida-canvas/examples/grida_container.rscrates/grida-canvas/benches/bench_rectangles.rscrates/grida-canvas/examples/grida_fills.rscrates/grida-canvas/examples/grida_images.rscrates/grida-canvas/examples/golden_type_stroke.rscrates/grida-canvas/examples/grida_texts.rscrates/grida-canvas/examples/grida_paint.rscrates/grida-canvas/examples/grida_lines.rscrates/grida-canvas/Cargo.tomlcrates/grida-canvas/examples/grida_vector.rscrates/grida-canvas/examples/app_grida.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/examples/sys_camera.rscrates/grida-canvas/examples/grida_gradients.rscrates/grida-canvas/examples/golden_svg.rscrates/grida-canvas/examples/grida_strokes.rscrates/grida-canvas/examples/grida_effects.rscrates/grida-canvas/tests/geometry_cache.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/tests/export_as_pdf.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/examples/grida_booleans.rs
🧠 Learnings (3)
📚 Learning: 2025-09-26T09:30:18.313Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-09-26T09:30:18.313Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use the math2 crate for geometry and common math operations
Applied to files:
crates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/src/cache/geometry.rs
📚 Learning: 2025-09-26T09:30:18.313Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-09-26T09:30:18.313Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use the skia-safe crate for all painting in the 2D real-time rendering engine
Applied to files:
crates/grida-canvas/src/resources/mod.rscrates/grida-canvas/examples/grida_paint.rs
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Declare ttf-parser = "0.25" as a required dependency for the fonts crate
Applied to files:
crates/grida-canvas/Cargo.toml
🔇 Additional comments (76)
crates/grida-canvas/src/node/mod.rs (1)
3-3: Expose SceneGraph publicly — good changeMakes the new graph API accessible across the crate. No issues.
crates/grida-canvas/examples/grida_gradients.rs (1)
3-3: SceneGraph migration in example looks correctRoot creation + append_child usage is consistent; Scene now carries graph. LGTM.
Also applies to: 10-10, 20-21, 53-54, 85-86, 117-118, 151-152, 157-157
crates/grida-canvas/examples/golden_container_stroke.rs (1)
3-3: Graph-based scene construction is correctUsing SceneGraph with Parent::Root and NodeId for children is sound. No issues.
Also applies to: 11-11, 38-41, 44-44
crates/grida-canvas/Cargo.toml (1)
22-22: Addition of taffy is appropriate for layout; consider a feature gateLooks good. Optionally gate it behind a cargo feature (e.g., layout) to trim non-layout builds.
To confirm usage and check for the latest compatible version, run:
crates/grida-canvas/examples/golden_type_stroke.rs (1)
84-92: Batched append to SceneGraph looks goodUsing graph.append_children with Parent::Root is clear and efficient. No issues spotted.
crates/grida-canvas/examples/app_figma.rs (1)
244-246: Printing graph metrics is correctSwitch to scene.graph.roots().len() and node_count() aligns with the new API. Looks good.
crates/grida-canvas/examples/grida_container.rs (1)
44-48: SceneGraph construction is cleanappend_child + Parent semantics are used correctly; scene now exposes graph. No issues.
Also applies to: 52-53
crates/grida-canvas/src/io/io_figma.rs (1)
845-861: Repository/links accumulate across canvases; scenes can bleed into each otherFigmaConverter holds repository and links across convert_canvas calls. Converting multi-canvas documents will keep prior nodes/links unless explicitly cleared. Unless SceneGraph::new_from_snapshot prunes to the reachable subgraph from roots, this can leak nodes into unrelated scenes.
Either reset state per canvas or move links into a local map:
fn convert_canvas(&mut self, canvas: &CanvasNode) -> Result<Scene, String> { + // Reset state per canvas to avoid cross-canvas leakage + self.repository = NodeRepository::new(); + self.links.clear();Run to inspect SceneGraph::new_from_snapshot behavior (does it prune unreachable nodes?):
crates/grida-canvas/examples/bench_100k.rs (1)
3-3: SceneGraph migration looks goodImports, graph creation, appends, and Scene construction align with the new API.
Also applies to: 11-11, 54-54, 59-59
crates/grida-canvas/examples/grida_paint.rs (1)
3-3: SceneGraph migration LGTMGraph creation, root container handling, and child appends are consistent with the new API; Scene now carries graph. Nice.
Also applies to: 10-11, 20-21, 42-45, 79-82, 117-120, 146-149, 188-191, 231-234, 240-241
crates/grida-canvas/examples/sys_camera.rs (1)
4-4: SceneGraph integration LGTMGraph creation, root group, and appends are correct; Scene now carries graph. Rendering flow unaffected.
Also applies to: 29-29, 32-41, 43-44, 55-55, 63-63
crates/grida-canvas/examples/grida_shapes.rs (1)
3-3: SceneGraph migration LGTMRoot container + appends and Scene.graph usage are consistent and clean.
Also applies to: 19-20, 26-28, 45-46, 63-64, 91-92, 111-112, 138-139, 159-160, 181-182, 186-187
crates/grida-canvas/examples/grida_vector.rs (1)
3-3: SceneGraph migration LGTMGraph construction and appends look consistent; Scene.graph use matches the new model.
Also applies to: 11-11, 21-21, 54-55, 85-86, 117-118, 148-149, 182-183, 222-223, 254-255, 293-294, 330-330, 337-338
crates/grida-canvas/examples/grida_blendmode.rs (2)
5-5: SceneGraph migration LGTMUse of graph, Parent, and Scene.graph looks correct throughout.
Also applies to: 12-12, 22-22, 85-89, 143-147, 155-158, 173-177, 188-192, 204-208, 219-223, 228-229
103-104: Rotation is likely in radians; -90.0 should be -π/2Else you’re rotating by -90 radians. Align with grida_paint.rs where degrees are converted to radians.
- transform: AffineTransform::new(base_size / 2.0, base_size / 2.0, -90.0), // Rotate -90 degrees + transform: AffineTransform::new( + base_size / 2.0, + base_size / 2.0, + -std::f32::consts::FRAC_PI_2, // -90 degrees in radians + ),crates/grida-canvas/examples/wd_animation.rs (1)
11-12: SceneGraph migration looks correctGraph init, append to Root, and passing
graphintoSceneare consistent with the new API. No issues spotted.Also applies to: 24-30
crates/grida-canvas/examples/grida_image.rs (1)
49-53: Graph-based composition LGTMRoot container attached to
Parent::Root, rectangle attached under the root id, andScene { graph, ... }is correct.Also applies to: 56-56
crates/grida-canvas/examples/grida_lines.rs (1)
18-18: SceneGraph usage and batch append look goodRoot appended to
Parent::Rootand lines added viaappend_childrenunder that parent is correct. Scene now exposesgraphas expected.Also applies to: 98-109, 114-114
crates/grida-canvas/src/painter/geometry.rs (2)
2-2: Import switch to SceneGraphAligned with the project-wide migration. Good.
301-306: Wrapper updated to graph — LGTMSignature and forwarding align with the new API.
crates/grida-canvas/tests/render_bounds.rs (1)
3-7: Tests migrated to SceneGraph correctlyCreating a
SceneGraph, appending nodes underParent::Root, and constructingScene { graph, ... }are consistent. GeometryCache usage remains valid.Also applies to: 15-27, 42-55, 71-87, 102-119
crates/grida-canvas/tests/scene_cache.rs (1)
2-6: SceneGraph migration looks solidClean switch to SceneGraph: proper Parent usage, IDs captured, Scene.graph assigned. Matches project direction. Based on learnings.
Also applies to: 17-23, 31-33, 37-38
crates/grida-canvas/src/cache/geometry.rs (1)
61-65: Graph roots traversal LGTMUsing scene.graph.roots() and passing graph through recursion is correct and aligns with SceneGraph API.
crates/grida-canvas/examples/grida_shapes_ellipse.rs (1)
3-3: SceneGraph adoption in example looks goodRoot container + child wiring via Parent::NodeId is correct; Scene.graph set. Based on learnings.
Also applies to: 19-22, 122-129
crates/grida-canvas/src/dummy/mod.rs (2)
39-47: Nice use of append_children for the dummy sceneSimple, clear, and aligns with the new graph model.
Also applies to: 49-53
81-82: Math usage is on‑guidelineTransforms use math2; good adherence. Based on learnings.
crates/grida-canvas/src/node/schema.rs (1)
83-91: Scene now owns a SceneGraph — good directionCentralizing hierarchy/data in SceneGraph simplifies traversal and cache builds. Docs clarify intent. Based on learnings.
crates/grida-canvas/examples/app_grida.rs (1)
3-3: LGTM: SceneGraph importImport looks correct and aligns with the new graph-based scene model.
crates/grida-canvas/src/runtime/scene.rs (2)
3-3: LGTM: import consolidationImporting SceneGraph and schema types together is consistent with the migration.
672-676: LGTM: tests migrated to SceneGraph APIFactory usage, append_child, Parent, and Scene.graph wiring look correct.
Also applies to: 688-690, 693-693, 754-756, 759-759
crates/grida-canvas/tests/hit_test.rs (1)
3-7: LGTM: hit-test suite updated for SceneGraphSceneGraph construction and Scene.graph usage are correct. Assertions reflect expected z-order given insertion. No issues spotted.
If z-order semantics change later (e.g., stable sort by paint order), tests asserting order may need to compare sets or specify intended ordering explicitly.
Also applies to: 17-21, 29-32, 36-37, 61-61, 64-65, 69-70, 94-98, 106-109, 113-114
crates/grida-canvas/tests/geometry_cache.rs (1)
55-61: LGTM: container bounds test migratedGraph wiring and expectations look consistent with the new API.
Also applies to: 69-71, 75-76
crates/grida-canvas/examples/grida_texts.rs (1)
115-130: SceneGraph migration looks goodGraph creation and append semantics are correct, and Scene now owning graph is consistent with the new model.
Also applies to: 135-135
crates/grida-canvas/tests/export_as_pdf.rs (1)
3-7: LGTM: test updated to SceneGraphClean switch to SceneGraph with a root-level rectangle; wiring into Scene.graph is correct. Assertions are fine.
Also applies to: 17-17, 28-28, 33-33
crates/grida-canvas/src/window/application.rs (1)
494-513: Verify ID preservation and link integrity for new_from_snapshotYou convert JSON nodes with
into()and discard the map keys. Ensure:
- JSONNode::into() preserves the original node ID.
scene_childrenandfiltered_linksonly reference IDs present innodes, otherwise graph build may be inconsistent.If the API can fail on missing IDs, consider returning a Result and handling/logging errors here.
Run to confirm signatures and expectations:
crates/grida-canvas/examples/grida_basic.rs (4)
3-3: SceneGraph import switch looks goodAPI migration aligns with the new graph-based Scene.
152-152: Group node creation is fine without mutNo in-place mutations before append; no issues.
162-194: Graph build flow looks correct (root → group → shapes + other elements)Parenting and ordering are sensible. Please confirm child insertion order defines paint Z-order in SceneGraph to ensure expected stacking.
199-199: Scene now carries graph fieldConstruction matches the new Scene API.
crates/grida-canvas/examples/golden_pdf.rs (5)
3-3: SceneGraph import switch looks goodConsistent with the migration.
13-13: Graph initializationOK to build graph locally before assembling Scene.
264-281: Batch append under root looks goodCorrect parent and coherent ordering.
285-285: Scene.graph assignment OKMatches new API.
85-87: Typo likely causes compile error: from_rotatationAffineTransform method name appears misspelled; use from_rotation.
Apply this diff:
- transform: AffineTransform::from_rotatation(45.0), + transform: AffineTransform::from_rotation(45.0),crates/grida-canvas/examples/grida_nested.rs (5)
3-3: SceneGraph import switch looks good
10-10: Graph initializationGood.
22-68: Nested-container construction looks solidCumulative transform, sizing, fills, stroke, and label per level are coherent; parenting with current_parent is correct.
70-85: Final star under innermost containerCorrect placement and styling.
89-89: Scene.graph assignment OKcrates/grida-canvas/examples/golden_svg.rs (6)
3-3: SceneGraph import switch looks good
14-14: Graph initializationOK.
24-24: Root container appendCorrect parent is Parent::Root.
237-252: Batch append under root looks goodOrdering and parenting are correct.
257-258: Scene.graph assignment OK
73-75: Typo likely causes compile error: from_rotatationReplace with from_rotation.
- transform: AffineTransform::from_rotatation(45.0), + transform: AffineTransform::from_rotation(45.0),crates/grida-canvas/examples/grida_webfonts.rs (3)
4-4: SceneGraph import switch looks good
63-63: line_height: Factor(1.5) changeLooks correct if engine interprets Factor as a multiplier; verify rendering matches prior intent.
151-151: Scene.graph assignment OKcrates/grida-canvas/src/node/scene_graph.rs (2)
67-105: LGTM! Well-designed SceneGraph API.The core design is solid:
- Clear separation between structure (roots, links) and data (nodes)
- Comprehensive error handling with custom error types
new_from_snapshotprovides an efficient bulk-loading path for IO operations
349-673: Excellent test coverage.The test suite covers:
- Basic operations (append, add, remove)
- Traversal methods (preorder, postorder, ancestors, descendants)
- Error conditions (not found, out of bounds)
- Bulk operations (append_children, new_from_snapshot)
- Complex hierarchies
crates/grida-canvas/examples/grida_fills.rs (2)
8-20: LGTM! Clean migration to SceneGraph API.The migration correctly:
- Creates a
SceneGraphinstead ofNodeRepository- Uses
append_childwithParent::Rootfor the root container- Obtains and reuses the root ID for subsequent children
371-376: LGTM! Scene construction updated correctly.The
Scenenow containsgraphinstead of separateid,children, andnodesfields, aligning with the new SceneGraph-based architecture.crates/grida-canvas/src/painter/painter_debug_node.rs (4)
268-320: LGTM! Correctly refactored to use SceneGraph.The method signature and implementation properly:
- Accept
&SceneGraphparameter- Use
graph.get_children()to access child IDs- Use
graph.get_node()to retrieve node data- Preserve the clipping and rendering order logic
352-369: LGTM! Group node rendering updated correctly.The traversal logic correctly uses
graph.get_children()andgraph.get_node()to iterate and render child nodes.
371-408: LGTM! Boolean operation rendering updated correctly.The method now passes
graphtoboolean_operation_shapeand uses graph-based traversal for the fallback case.
432-456: LGTM! Dispatch method updated correctly.All recursive calls now pass the
graphparameter, ensuring consistent SceneGraph-based traversal throughout the rendering pipeline.crates/grida-canvas/benches/bench_rectangles.rs (1)
9-69: LGTM! Efficient benchmark migration.The benchmark correctly:
- Creates a
SceneGraph- Builds rectangles in a vector first
- Uses
append_childrenfor bulk insertion (avoiding repeated parent clones)- Constructs the
Scenewith thegraphfieldThis is an efficient migration pattern that should be followed in other bulk-insertion scenarios.
crates/grida-canvas/examples/grida_strokes.rs (2)
8-21: LGTM! Clean example migration to SceneGraph.The example correctly initializes a
SceneGraphand establishes the root container usingParent::Root.
512-517: LGTM! Scene construction aligned with new API.The
Scenenow contains thegraphfield, consistent with the SceneGraph-based architecture.crates/grida-canvas/src/painter/layer.rs (5)
217-226: LGTM! Layer flattening entry point updated correctly.The
from_scenemethod now iterates overscene.graph.roots()and passes&scene.graphtoflatten_node, correctly adapting to the SceneGraph API.
228-239: LGTM! Node-based flattening updated correctly.The method signature and implementation correctly use
&SceneGraphfor graph-based traversal.
245-364: LGTM! Comprehensive node flattening logic updated.The method correctly handles all node types:
- Uses
graph.get_node()for node retrieval- Uses
graph.get_children()for child iteration- Passes
graphto all helper methods (build_render_commands,compute_clip_path,boolean_operation_shape)- Preserves all rendering semantics (opacity, transforms, effects, clipping)
743-773: LGTM! Render command building updated correctly.The method signature and implementation correctly use
&SceneGraph, passing it through to recursiveflatten_nodecalls while maintaining the masking logic.
820-895: LGTM! Clip path computation updated correctly.The method correctly:
- Takes
&SceneGraphparameter- Uses
graph.get_node()to retrieve parent nodes- Passes
graphtoboolean_operation_path- Maintains the clip shape merging logic
crates/grida-canvas/examples/grida_mask.rs (2)
61-81: LGTM! Mask builder functions updated correctly.All three mask builders (
build_geometry_mask,build_alpha_mask,build_luminance_mask) correctly:
- Accept
&mut SceneGraphandParentparameters- Use
graph.append_child()to add the mask node- Return
NodeIdfor the created maskAlso applies to: 83-118, 120-155
157-245: LGTM! Demo scene assembly updated correctly.The function correctly:
- Creates a
SceneGraph- Appends the root container with
Parent::Root- Creates panels and content with appropriate parent contexts
- Builds masks as the last children per panel (for flat list masking model)
- Returns a
Scenewith thegraphfield
| /// Get all ancestors of a node (path to root) | ||
| pub fn ancestors(&self, id: &NodeId) -> SceneGraphResult<Vec<NodeId>> { | ||
| if !self.has_node(id) { | ||
| return Err(SceneGraphError::NodeNotFound(id.clone())); | ||
| } | ||
|
|
||
| let mut result = Vec::new(); | ||
| let mut current = id.clone(); | ||
|
|
||
| // Find parent by searching all links | ||
| loop { | ||
| let mut found_parent = false; | ||
| for (parent_id, children) in &self.links { | ||
| if children.contains(¤t) { | ||
| result.push(parent_id.clone()); | ||
| current = parent_id.clone(); | ||
| found_parent = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if !found_parent { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| Ok(result) | ||
| } |
There was a problem hiding this comment.
Performance issue in ancestors method: O(n×m) complexity.
The ancestors method searches through all entries in self.links for each step up the tree. For a tree with n nodes at depth d, this results in O(d×n) complexity, which degrades performance in large scene graphs.
Consider maintaining a reverse lookup (child→parent map) or storing parent references in nodes for O(1) ancestor lookup:
pub struct SceneGraph {
roots: Vec<NodeId>,
links: HashMap<NodeId, Vec<NodeId>>,
+ parents: HashMap<NodeId, NodeId>,
nodes: NodeRepository,
}Then update ancestors to use the parent map:
pub fn ancestors(&self, id: &NodeId) -> SceneGraphResult<Vec<NodeId>> {
if !self.has_node(id) {
return Err(SceneGraphError::NodeNotFound(id.clone()));
}
let mut result = Vec::new();
let mut current = id.clone();
- // Find parent by searching all links
loop {
- let mut found_parent = false;
- for (parent_id, children) in &self.links {
- if children.contains(¤t) {
- result.push(parent_id.clone());
- current = parent_id.clone();
- found_parent = true;
- break;
- }
- }
-
- if !found_parent {
+ if let Some(parent_id) = self.parents.get(¤t) {
+ result.push(parent_id.clone());
+ current = parent_id.clone();
+ } else {
break;
}
}
Ok(result)
}Don't forget to maintain the parents map in all mutation methods (append_child, add_child, add_child_at, remove_child, new_from_snapshot).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/grida-canvas/src/node/scene_graph.rs around lines 296 to 323, the
ancestors method currently scans self.links for each parent lookup causing
O(d×n) time; introduce and maintain a reverse map (e.g., parents:
HashMap<NodeId, NodeId>) or store parent reference on nodes so ancestors can
walk up via O(1) parent lookups; update ancestors to follow the parent map until
root and return collected NodeIds; ensure every mutating method that changes
links (append_child, add_child, add_child_at, remove_child, new_from_snapshot
and any other child/link operations) updates the parents map consistently
(insert on add, update on move, remove on delete) to keep it in sync.
| use crate::cg::types::{ | ||
| BlendMode, CGColor, FillRule, ImagePaint, ImageRepeat, Paint, ResourceRef, SolidPaint, | ||
| StrokeAlign, | ||
| BlendMode, CGColor, FillRule, ImagePaint, Paint, ResourceRef, SolidPaint, StrokeAlign, | ||
| }; |
There was a problem hiding this comment.
Tests: missing imports break compilation (Paints, ImageFilters, ImagePaintFit)
The tests use Paints::new, ImageFilters::default(), and ImagePaintFit::Fit but these aren’t imported into the test module. Add them to the cg::types import.
Apply:
- use crate::cg::types::{
- BlendMode, CGColor, FillRule, ImagePaint, Paint, ResourceRef, SolidPaint, StrokeAlign,
- };
+ use crate::cg::types::{
+ BlendMode, CGColor, FillRule, ImagePaint, ImageFilters, ImagePaintFit, Paint, Paints,
+ ResourceRef, SolidPaint, StrokeAlign,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use crate::cg::types::{ | |
| BlendMode, CGColor, FillRule, ImagePaint, ImageRepeat, Paint, ResourceRef, SolidPaint, | |
| StrokeAlign, | |
| BlendMode, CGColor, FillRule, ImagePaint, Paint, ResourceRef, SolidPaint, StrokeAlign, | |
| }; | |
| use crate::cg::types::{ | |
| BlendMode, CGColor, FillRule, ImagePaint, ImageFilters, ImagePaintFit, Paint, Paints, | |
| ResourceRef, SolidPaint, StrokeAlign, | |
| }; |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/vectornetwork/vn_painter.rs around lines 258 to 260,
the test module uses Paints::new, ImageFilters::default(), and
ImagePaintFit::Fit but those symbols are not imported from crate::cg::types;
update the use statement to also import Paints, ImageFilters, and ImagePaintFit
so the tests can compile by adding those names to the existing import list.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
crates/grida-canvas/src/io/io_figma.rs (3)
845-856: Reset repository/links per canvas to avoid cross‑scene contaminationconvert_canvas builds a graph from self.repository and self.links, which accumulate across canvases. Subsequent scenes will include nodes/links from previous canvases.
Apply at the start of convert_canvas:
fn convert_canvas(&mut self, canvas: &CanvasNode) -> Result<Scene, String> { + // Reset per canvas to avoid cross-scene contamination + self.repository = NodeRepository::new(); + self.links.clear(); let children = canvas
948-957: Option misuse in convert_text causes compile errorsstyle is Option<&TypeStyle>; code accesses fields directly (style.font_family, origin.style.text_case) and will not compile.
Apply minimal safe Option handling:
- let style = origin.style.as_ref(); + let style = origin.style.as_ref(); @@ - if let Some(font_family) = &style.font_family { - self.add_font( - font_family.clone(), - style.font_post_script_name.clone(), - style.font_style.clone(), - ); - } + if let Some(s) = style { + if let Some(font_family) = &s.font_family { + self.add_font( + font_family.clone(), + s.font_post_script_name.clone(), + s.font_style.clone(), + ); + } + } @@ - text_style: TextStyleRec { + text_style: TextStyleRec { text_decoration: Some(TextDecorationRec { text_decoration_line: Self::convert_text_decoration( - style.text_decoration.as_ref(), + style.and_then(|s| s.text_decoration.as_ref()), ), @@ - font_family: style - .font_family - .clone() - .unwrap_or_else(|| "Inter".to_string()), - font_size: style.font_size.unwrap_or(14.0) as f32, + font_family: style.and_then(|s| s.font_family.clone()).unwrap_or_else(|| "Inter".to_string()), + font_size: style.and_then(|s| s.font_size).unwrap_or(14.0) as f32, @@ - font_weight: FontWeight::new(style.font_weight.unwrap_or(400.0) as u32), - font_kerning: true, - letter_spacing: style - .letter_spacing - .map(|v| TextLetterSpacing::Factor(v as f32)) - .unwrap_or_default(), + font_weight: FontWeight::new(style.and_then(|s| s.font_weight).unwrap_or(400.0) as u32), + font_kerning: true, + letter_spacing: style.and_then(|s| s.letter_spacing).map(|v| TextLetterSpacing::Factor(v as f32)).unwrap_or_default(), @@ - font_style_italic: style.italic.unwrap_or(false), - line_height: style - .line_height_px - .map(|v| TextLineHeight::Fixed(v as f32)) - .unwrap_or_default(), - text_transform: match origin.style.text_case.as_ref() { + font_style_italic: style.and_then(|s| s.italic).unwrap_or(false), + line_height: style.and_then(|s| s.line_height_px).map(|v| TextLineHeight::Fixed(v as f32)).unwrap_or_default(), + text_transform: match style.and_then(|s| s.text_case.as_ref()) {Also applies to: 979-1008, 1010-1026
437-453: Fix convert_fills calls to match its signature
convert_fills(&self, Option<&Vec>) is being passed Some(&x.fills) or Some(&x.fills.as_ref()), resulting in Option<&Option<Vec<_>>> and a type mismatch. Drop theSome(&…)wrappers and call with the Option reference directly:- fills: self.convert_fills(Some(&component.fills.as_ref())), + fills: self.convert_fills(component.fills.as_ref()),Apply the same change to all
convert_fillscallsites incrates/grida-canvas/src/io/io_figma.rs(lines 630, 725, 769, 893, 1031, 1058, 1162, 1197, 1266, 1310, 1345).crates/grida-canvas/src/resources/mod.rs (1)
125-147: Deduplicate image refs and scaffold full paint traversal
- Swap
VecforBTreeSetto eliminate duplicates:- let mut urls = Vec::new(); + let mut urls = std::collections::BTreeSet::new(); @@ - ResourceRef::RID(r) | ResourceRef::HASH(r) => urls.push(r.clone()), + ResourceRef::RID(r) | ResourceRef::HASH(r) => { urls.insert(r.clone()); } @@ - urls + urls.into_iter().collect()
- Follow-up: factor out a
collect_paints(&Node) -> impl Iterator<Item=&Paint>to cover all paintable variants (Rectangle, Ellipse, Vector, etc.)crates/grida-canvas/tests/scene_cache.rs (1)
63-67: Make the layer assertions order-agnostic to avoid flakinessintersects() may not guarantee ordering. Compare IDs ignoring order.
- let layer0 = &cache.layers.layers[layer_indices[0]]; - let layer1 = &cache.layers.layers[layer_indices[1]]; - assert_eq!(layer0.id(), &rect_id); - assert_eq!(layer1.id(), &container_id); + let layer0 = &cache.layers.layers[layer_indices[0]]; + let layer1 = &cache.layers.layers[layer_indices[1]]; + let mut got = vec![layer0.id().clone(), layer1.id().clone()]; + got.sort(); + let mut expected = vec![rect_id.clone(), container_id.clone()]; + expected.sort(); + assert_eq!(got, expected);crates/grida-canvas/examples/sys_camera.rs (1)
291-295: Fix GL surface lifetime: double‑free risk and leaks on resize
- On resize, freeing surface_ptr (the initial pointer) but not updating it, then freeing it again at shutdown causes double‑free UB if a resize occurred.
- Subsequent resize surfaces are never freed (leak).
Free the current backend surface before replacing, and avoid manual frees of the captured pointer. Use renderer.free() at shutdown.
- // Update surface pointer - unsafe { _ = Box::from_raw(surface_ptr) }; - let new_surface_ptr = Box::into_raw(Box::new(surface)); - renderer.backend = Backend::GL(new_surface_ptr); + // Free current backend surface before replacing + let old_ptr = renderer.backend.get_surface(); + unsafe { let _ = Box::from_raw(old_ptr); } + // Install new surface + renderer.backend = Backend::GL(Box::into_raw(Box::new(surface)));At shutdown:
- // Clean up - unsafe { - _ = Box::from_raw(surface_ptr); - } + // Clean up + renderer.free();Also applies to: 336-340
crates/grida-canvas/src/cache/geometry.rs (3)
261-306: TextSpan recursion returns local bounds instead of world boundsReturning intrinsic_bounds corrupts parent unions/culling. Return world bounds like other branches.
- cache.entries.insert(id.clone(), entry.clone()); - - intrinsic_bounds + cache.entries.insert(id.clone(), entry.clone()); + entry.absolute_bounding_box
215-260: Container bounds should include children (world and render) when not clippingCurrent entry.absolute_bounding_box uses container-only world_bounds and render bounds ignore children. This underestimates for non‑clipping containers and is inconsistent with Group.
let world_bounds = transform_rect(&local_bounds, &world_transform); - let mut union_world_bounds = world_bounds; + let mut union_world_bounds = world_bounds; let render_bounds = compute_render_bounds_from_style( world_bounds, if n.has_stroke_geometry() { n.stroke_width } else { 0.0 }, n.stroke_align, &n.effects, ); + let mut union_render_bounds = render_bounds; if let Some(children) = graph.get_children(id) { for child_id in children { let child_bounds = Self::build_recursive( child_id, graph, &world_transform, Some(id.clone()), cache, paragraph_cache, fonts, ); union_world_bounds = rect::union(&[union_world_bounds, child_bounds]); + if let Some(rb) = cache.get_render_bounds(child_id) { + union_render_bounds = + rect::union(&[union_render_bounds, rb]); + } } } let entry = GeometryEntry { transform: local_transform, absolute_transform: world_transform, bounding_box: local_bounds, - absolute_bounding_box: world_bounds, - absolute_render_bounds: render_bounds, + absolute_bounding_box: union_world_bounds, + absolute_render_bounds: union_render_bounds, parent: parent_id.clone(), dirty_transform: false, dirty_bounds: false, }; cache.entries.insert(id.clone(), entry.clone()); union_world_bounds
582-587: Include TextSpan effects in render boundsUsing default effects underestimates TextSpan render bounds.
- Node::TextSpan(n) => compute_render_bounds_from_style( - world_bounds, - n.stroke_width, - n.stroke_align, - &LayerEffects::default(), - ), + Node::TextSpan(n) => compute_render_bounds_from_style( + world_bounds, + n.stroke_width, + n.stroke_align, + &n.effects, + ),As per coding guidelines (use math2 for geometry), this keeps the same math path while fixing effect inflation.
crates/grida-canvas/tests/export_as_pdf.rs (1)
61-66: Avoid potential panic when checking PDF headerSlicing
&data[0..4]will panic ifdata.len() < 4. Usestarts_withto be safe.Apply this diff:
- // Check that we have some data - assert!(!data.is_empty()); - - // Check that it starts with PDF magic bytes - assert_eq!(&data[0..4], b"%PDF"); + // Check it starts with PDF magic bytes safely + assert!( + data.starts_with(b"%PDF"), + "Export did not start with %PDF header" + );crates/grida-canvas/examples/grida_gradients.rs (2)
36-40: Fix typo and use radians for rotation.
from_rotatationwill not compile. Also, prior examples use radians. Replace withnew(0.0, 0.0, angle_rad)and convert degrees to radians.Apply:
- let angle = (i as f32) * 45.0; - rect.set_fill(Paint::LinearGradient(LinearGradientPaint { - transform: AffineTransform::from_rotatation(angle), + let angle_deg = (i as f32) * 45.0; + let angle_rad = angle_deg * std::f32::consts::PI / 180.0; + rect.set_fill(Paint::LinearGradient(LinearGradientPaint { + transform: AffineTransform::new(0.0, 0.0, angle_rad), stops: vec![And:
- let angle = (i as f32) * 45.0; - rect.strokes = Paints::new([Paint::LinearGradient(LinearGradientPaint { - transform: AffineTransform::from_rotatation(angle), + let angle_deg = (i as f32) * 45.0; + let angle_rad = angle_deg * std::f32::consts::PI / 180.0; + rect.strokes = Paints::new([Paint::LinearGradient(LinearGradientPaint { + transform: AffineTransform::new(0.0, 0.0, angle_rad),Also applies to: 100-104
66-71: Prefer constructor over struct literal for AffineTransform.For consistency with other examples, use
AffineTransform::new(tx, ty, 0.0)instead of a directmatrixliteral:- rect.set_fill(Paint::RadialGradient(RadialGradientPaint { - transform: AffineTransform { - matrix: [[1.0, 0.0, offset], [0.0, 1.0, offset]], - }, + rect.set_fill(Paint::RadialGradient(RadialGradientPaint { + transform: AffineTransform::new(offset, offset, 0.0), stops: vec![And for strokes:
- rect.strokes = Paints::new([Paint::RadialGradient(RadialGradientPaint { - transform: AffineTransform { - matrix: [[1.0, 0.0, offset], [0.0, 1.0, offset]], - }, + rect.strokes = Paints::new([Paint::RadialGradient(RadialGradientPaint { + transform: AffineTransform::new(offset, offset, 0.0),Also applies to: 132-135
crates/grida-canvas/examples/grida_strokes.rs (1)
295-307: Naming mismatch: “Conic Gradient Stroke” uses RadialGradientEither rename the label or use the intended conic gradient type (if supported).
Also applies to: 305-324
crates/grida-canvas/examples/app_grida.rs (2)
16-19: Avoid blocking I/O in async functionUse tokio::fs to prevent blocking the async runtime, or make the function sync.
-async fn load_scene_from_file(file_path: &str) -> Scene { - let file: String = fs::read_to_string(file_path).expect("failed to read file"); +async fn load_scene_from_file(file_path: &str) -> Scene { + let file: String = tokio::fs::read_to_string(file_path) + .await + .expect("failed to read file");
29-36: Use parsed scene background color (with fallback)Currently the parsed background_color is ignored. Prefer using it, with a sensible default fallback.
- // Extract scene metadata from SceneNode - let (scene_name, _bg_color) = if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) = + // Extract scene metadata from SceneNode + let (scene_name, bg_color) = if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) = canvas_file.document.nodes.get(&scene_id) { - (scene_node.name.clone(), scene_node.background_color.clone()) + ( + scene_node.name.clone(), + scene_node.background_color.clone().map(Into::into), + ) } else { (scene_id.clone(), None) };- Scene { - name: scene_name, - background_color: Some(CGColor(230, 230, 230, 255)), - graph, - } + Scene { + name: scene_name, + background_color: bg_color.or(Some(CGColor(230, 230, 230, 255))), + graph, + }Also applies to: 71-75
crates/grida-canvas/examples/grida_blendmode.rs (1)
103-104: Rotation likely expects radians — convert -90 degreesUse to_radians for consistency (other examples use to_radians).
Apply this diff:
- transform: AffineTransform::new(base_size / 2.0, base_size / 2.0, -90.0), // Rotate -90 degrees + transform: AffineTransform::new( + base_size / 2.0, + base_size / 2.0, + (-90f32).to_radians(), + ), // Rotate -90 degreescrates/grida-canvas/src/painter/layer.rs (1)
813-816: Doc param: renamerepo→graphStale doc reference; update for clarity.
Apply this diff:
- /// - `repo`: The node repository containing all nodes + /// - `graph`: The scene graph containing all nodesAlso applies to: 821-824
🧹 Nitpick comments (29)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
9-47: LGTM! Clean SceneGraph migration.The scene construction correctly:
- Creates a
SceneGraphinstance (line 11)- Adds the container as a root node (line 39)
- Adds the circle as a child of the container using the returned ID (line 40)
- Returns a
Scenewith thegraphfield (line 44)This pattern is consistent with the SceneGraph-based approach demonstrated in other examples.
Minor note: The comment on lines 33-36 mentions positioning the circle at "right-bottom" but the actual coordinates
(200.0, 200.0)position it at the same location as the container, which appears to center it. Since this is a golden test for stroke behavior, the positioning is likely intentional.crates/grida-canvas/examples/grida_nested.rs (4)
12-15: Comment/code mismatch: “scale” not applied via transform.You only set translation + rotation; size drives visual downsizing. Either apply transform scale (if supported) or adjust the comment.
If AffineTransform supports scaling (e.g., set_scale or similar), do you intend to use it here?
25-25: Remove redundant cast in powi.i is already i32 here.
- let current_size = base_size * size_reduction.powi(i as i32); + let current_size = base_size * size_reduction.powi(i);
32-38: Rotation pivot/centering may not match intent.AffineTransform::new sets translation then rotation (rotation about origin). If you intend rotation about the container’s center or true centering in parent, extra offset/pivot handling is needed; current 7.5% offset is not “centered.”
Confirm intended pivot and whether additional translate-to-center/translate-back (or a pivot field) is needed. Based on relevant snippet in math2::transform::new.
57-65: Avoid empty font family to reduce platform variance.Passing "" may trigger unpredictable fallback. Prefer a known default family available in your font loader, or document the fallback.
Is there a canonical default (e.g., Inter/Roboto/Noto) in this repo’s font pipeline?
crates/grida-canvas/examples/grida_fills.rs (1)
20-21: Looks good; minor ergonomicsRepeated Parent::NodeId(root_id.clone()) allocations. Consider caching parent once or using append_children like in golden_pdf for brevity and fewer clones.
Also applies to: 41-42, 72-73, 107-108, 155-156, 216-217, 271-272, 334-335, 369-370
crates/grida-canvas/src/io/io_figma.rs (1)
4-5: Remove unused importParent is unused here.
-use crate::node::scene_graph::{Parent, SceneGraph}; +use crate::node::scene_graph::SceneGraph;crates/grida-canvas/examples/grida_vector.rs (1)
3-3: SceneGraph migration looks goodConstruction and parenting via SceneGraph/Parent are correct and consistent with the new model.
Minor nits (optional):
- IDs are hardcoded strings; consider Uuid/NodeFactory to avoid collisions in larger demos.
- Repeating Parent::NodeId(root_id.clone()) is fine; if it grows, a small helper for “append_to(root_id, node)” can reduce noise.
Also applies to: 11-11, 21-21, 54-55, 85-86, 117-118, 148-149, 182-183, 222-223, 254-255, 293-294, 330-331, 337-338
crates/grida-canvas/src/dummy/mod.rs (1)
2-6: LGTM on graph‑based dummy scenesSceneGraph population via append_children/append_child is correct; background color change is fine.
Nit (optional): import math2::transform::AffineTransform at top to avoid repeating fully‑qualified paths.
Also applies to: 39-47, 51-53, 59-60, 81-82, 87-89
crates/grida-canvas/src/runtime/scene.rs (1)
65-87: Skip inactive nodes when collecting fontsConsider ignoring nodes with active == false to avoid requesting unused fonts.
- if let Ok(node) = graph.get_node(id) { + if let Ok(node) = graph.get_node(id) { match node { - Node::TextSpan(n) => { + Node::TextSpan(n) if n.active => { set.insert(n.text_style.font_family.clone()); } _ => {} } }crates/grida-canvas/examples/grida_shapes.rs (2)
26-28: Reduce clone noise by caching parentCreate a reusable
root_parentand use it at append sites.Apply this diff:
// Set up the root container first let root_container_id = graph.append_child(Node::Container(root_container_node), Parent::Root); +let root_parent = Parent::NodeId(root_container_id.clone()); -graph.append_child(Node::Rectangle(rect), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::Rectangle(rect), root_parent.clone()); -graph.append_child(Node::Ellipse(ellipse), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::Ellipse(ellipse), root_parent.clone()); -graph.append_child(Node::Polygon(polygon), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::Polygon(polygon), root_parent.clone()); -graph.append_child(Node::RegularPolygon(regular_polygon), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::RegularPolygon(regular_polygon), root_parent.clone()); -graph.append_child(Node::SVGPath(path), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::SVGPath(path), root_parent.clone()); -graph.append_child(Node::RegularStarPolygon(star), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::RegularStarPolygon(star), root_parent.clone()); -graph.append_child(Node::Ellipse(arc), Parent::NodeId(root_container_id.clone())); +graph.append_child(Node::Ellipse(arc), root_parent.clone());Also applies to: 45-46, 63-64, 91-92, 111-112, 139-139, 159-160, 181-182
38-41: Clamp corner radius to avoid degenerate geometryLarge radii can exceed half the side on an 80×80 rect. Clamp to
base_size * 0.5.Apply this diff:
- rect.corner_radius = RectangularCornerRadius::circular(0.0 + (i as f32 * 8.0)); // 0 to 72 + let max_radius = base_size * 0.5; + rect.corner_radius = + RectangularCornerRadius::circular(((i as f32) * 8.0).min(max_radius));crates/grida-canvas/examples/grida_image.rs (1)
35-37: Remove unnecessary String clonesAvoid cloning
hash_strandurl; move them.Apply this diff:
- let url = format!("res://images/{}", hash_str.clone()); - rect1.set_fill(Paint::Image(ImagePaint { - image: ResourceRef::RID(url.clone()), + let url = format!("res://images/{}", hash_str); + rect1.set_fill(Paint::Image(ImagePaint { + image: ResourceRef::RID(url),crates/grida-canvas/examples/grida_booleans.rs (1)
77-89: Factor repetitive boolean-op assembly into a helperThe union/intersection/difference/XOR blocks repeat the same wiring. Extract a small helper that takes op, shapes, y_offset, and label to reduce duplication.
Example signature:
- fn add_bool_demo(graph: &mut SceneGraph, parent: NodeId, op: BooleanPathOperation, shapes: Vec, text: TextSpanNodeRec)
Also applies to: 144-156, 210-221, 276-287
crates/grida-canvas/examples/grida_paint.rs (1)
42-46: Reduce repeated clones of parent for readability (optional).Bind once and reuse:
let parent = Parent::NodeId(root_container_id.clone());- Use
parent.clone()in each append if needed.Applies to all subsequent
Parent::NodeId(root_container_id.clone())occurrences.crates/grida-canvas/benches/bench_rectangles.rs (1)
13-21: Minor micro-optimization (optional).Avoid a temporary
idbinding since it’s used once:- let id = format!("rect-{}", i); - - Node::Rectangle(RectangleNodeRec { - id: id.clone(), + Node::Rectangle(RectangleNodeRec { + id: format!("rect-{}", i),crates/grida-canvas/tests/hit_test.rs (1)
106-109: Optional: reduce clones for parent id.Bind once as
let parent = Parent::NodeId(container_id.clone());and reuse it across appends in this test.crates/grida-canvas/examples/grida_strokes.rs (1)
53-56: Batch node inserts to reduce clones and callsWithin loops, collect nodes and use append_children once per row. This avoids repeated Parent::NodeId(root_container_id.clone()) and multiple graph mutations.
Also applies to: 77-80, 97-100, 113-116, 130-133, 148-151, 190-193, 223-226, 258-261, 290-293, 326-329, 344-347, 368-371, 403-406, 452-455, 506-509
crates/grida-canvas/examples/grida_shapes_ellipse.rs (1)
42-43: Batch per-row insertsCollect each row’s nodes and call append_children once to reduce clones and graph churn.
Also applies to: 61-62, 81-82, 102-103, 122-123
crates/grida-canvas/examples/grida_images.rs (1)
12-13: Avoid unwrap on network IO in exampleunwrap() will panic if image load fails. Prefer proper error handling or return a Result from demo_images, or at least log and early-return with a placeholder.
crates/grida-canvas/examples/app_grida.rs (1)
39-43: Simplify Option<Option<Vec>> handlingUse cloned().flatten() for clarity.
- let scene_children = links - .get(&scene_id) - .and_then(|c| c.clone()) - .unwrap_or_default(); + let scene_children = links + .get(&scene_id) + .cloned() + .flatten() + .unwrap_or_default();crates/grida-canvas/src/window/application.rs (1)
287-296: Avoid heap allocation on every get_node lookupid.to_string() allocates per call. Consider extending SceneGraph API to accept &str (e.g., get_node_str(&str) or get_node<S: AsRef>), then call without allocation.
Would you like me to propose the SceneGraph API change and update call sites?
crates/grida-canvas/src/painter/geometry.rs (1)
256-279: Avoid Path ↔ PainterShape round-trips in boolean opsYou build PainterShape from Path only to convert back. Store Paths directly to reduce allocations.
-/// A merged `Path` representing the result of all boolean operations. -/// If no shapes are provided, returns an empty path. -pub fn merge_shapes(shapes: &[(PainterShape, BooleanPathOperation)]) -> Path { +/// A merged `Path` representing the result of all boolean operations. +/// If no shapes are provided, returns an empty path. +pub fn merge_shapes(shapes: &[(Path, BooleanPathOperation)]) -> Path { if shapes.is_empty() { return Path::new(); } - let mut result = shapes[0].0.to_path(); + let mut result = shapes[0].0.clone(); for (shape, operation) in shapes.iter().skip(1) { - let shape_path = shape.to_path(); - if let Some(merged) = Path::op(&result, &shape_path, (*operation).into()) { + if let Some(merged) = Path::op(&result, shape, (*operation).into()) { result = merged; } } result }- let mut shapes_with_ops = Vec::new(); + let mut shapes_with_ops: Vec<(Path, BooleanPathOperation)> = Vec::new(); @@ - shapes_with_ops.push((PainterShape::from_path(path), op)); + shapes_with_ops.push((path, op));Also applies to: 208-223
crates/grida-canvas/examples/grida_webfonts.rs (1)
127-146: Good graph assembly; batch the first two appendsUse append_children for heading/description to reduce repetition and clones.
Apply this diff:
- // Add all text nodes to root container - graph.append_child( - Node::TextSpan(heading_node), - Parent::NodeId(root_container_id.clone()), - ); - graph.append_child( - Node::TextSpan(description_node), - Parent::NodeId(root_container_id.clone()), - ); + // Add heading and description to root container + graph.append_children( + vec![ + Node::TextSpan(heading_node), + Node::TextSpan(description_node), + ], + Parent::NodeId(root_container_id.clone()), + );crates/grida-canvas/examples/grida_effects.rs (1)
45-49: Reduce repeated Parent::NodeId clonesOptional: bind parent once to cut repetition and tiny allocations.
Example:
let parent = Parent::NodeId(root_container_id.clone()); // later graph.append_child(Node::Rectangle(rect), parent.clone());Also applies to: 67-71, 91-95, 110-114, 146-149, 167-171, 186-190, 213-216, 235-239, 305-309, 364-368
crates/grida-canvas/examples/grida_blendmode.rs (1)
1-1: Resolve the FIXME before merging or track itThe demo is flagged “broken”. Either fix here or open an issue and downgrade the comment to a TODO with a link.
crates/grida-canvas/src/painter/layer.rs (1)
552-556: Consistently filter invisible strokes for LineOther branches use filter_visible_paints. Do the same here.
Apply this diff:
- strokes: n.strokes.clone(), + strokes: Self::filter_visible_paints(&n.strokes),crates/grida-canvas/src/node/scene_graph.rs (2)
296-323: Ancestor lookup is O(N²); consider a parent mapOptional: maintain parents: HashMap<NodeId, NodeId> to make get_parent/ancestors O(1)/O(h).
87-105: Validate snapshot integrity (optional)Optionally assert that all roots and link targets exist in nodes to prevent dangling IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/grida-canvas/goldens/pdf.pdfis excluded by!**/*.pdfcrates/grida-canvas/goldens/svg.svgis excluded by!**/*.svgcrates/grida-canvas/goldens/type_stroke.pngis excluded by!**/*.png
📒 Files selected for processing (50)
crates/grida-canvas/Cargo.toml(2 hunks)crates/grida-canvas/benches/bench_rectangles.rs(1 hunks)crates/grida-canvas/examples/app_figma.rs(1 hunks)crates/grida-canvas/examples/app_grida.rs(2 hunks)crates/grida-canvas/examples/bench_100k.rs(2 hunks)crates/grida-canvas/examples/golden_container_stroke.rs(2 hunks)crates/grida-canvas/examples/golden_pdf.rs(3 hunks)crates/grida-canvas/examples/golden_svg.rs(4 hunks)crates/grida-canvas/examples/golden_type_stroke.rs(2 hunks)crates/grida-canvas/examples/grida_basic.rs(3 hunks)crates/grida-canvas/examples/grida_blendmode.rs(8 hunks)crates/grida-canvas/examples/grida_booleans.rs(6 hunks)crates/grida-canvas/examples/grida_container.rs(2 hunks)crates/grida-canvas/examples/grida_effects.rs(13 hunks)crates/grida-canvas/examples/grida_fills.rs(10 hunks)crates/grida-canvas/examples/grida_gradients.rs(6 hunks)crates/grida-canvas/examples/grida_image.rs(2 hunks)crates/grida-canvas/examples/grida_images.rs(2 hunks)crates/grida-canvas/examples/grida_lines.rs(3 hunks)crates/grida-canvas/examples/grida_mask.rs(9 hunks)crates/grida-canvas/examples/grida_nested.rs(1 hunks)crates/grida-canvas/examples/grida_paint.rs(8 hunks)crates/grida-canvas/examples/grida_shapes.rs(9 hunks)crates/grida-canvas/examples/grida_shapes_ellipse.rs(7 hunks)crates/grida-canvas/examples/grida_strokes.rs(18 hunks)crates/grida-canvas/examples/grida_texts.rs(2 hunks)crates/grida-canvas/examples/grida_vector.rs(11 hunks)crates/grida-canvas/examples/grida_webfonts.rs(3 hunks)crates/grida-canvas/examples/sys_camera.rs(2 hunks)crates/grida-canvas/examples/wd_animation.rs(2 hunks)crates/grida-canvas/src/cache/geometry.rs(5 hunks)crates/grida-canvas/src/dummy/mod.rs(3 hunks)crates/grida-canvas/src/io/io_figma.rs(10 hunks)crates/grida-canvas/src/io/io_grida.rs(0 hunks)crates/grida-canvas/src/node/factory.rs(0 hunks)crates/grida-canvas/src/node/mod.rs(1 hunks)crates/grida-canvas/src/node/scene_graph.rs(1 hunks)crates/grida-canvas/src/node/schema.rs(2 hunks)crates/grida-canvas/src/painter/geometry.rs(4 hunks)crates/grida-canvas/src/painter/layer.rs(26 hunks)crates/grida-canvas/src/painter/painter_debug_node.rs(8 hunks)crates/grida-canvas/src/resources/mod.rs(1 hunks)crates/grida-canvas/src/runtime/scene.rs(4 hunks)crates/grida-canvas/src/vectornetwork/vn_painter.rs(1 hunks)crates/grida-canvas/src/window/application.rs(3 hunks)crates/grida-canvas/tests/export_as_pdf.rs(3 hunks)crates/grida-canvas/tests/geometry_cache.rs(3 hunks)crates/grida-canvas/tests/hit_test.rs(4 hunks)crates/grida-canvas/tests/render_bounds.rs(6 hunks)crates/grida-canvas/tests/scene_cache.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/grida-canvas/src/node/factory.rs
- crates/grida-canvas/src/io/io_grida.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: Use the skia-safe crate for all painting in the 2D real-time rendering engine
Use the math2 crate for geometry and common math operations
Ensure source is formatted with rustfmt (cargo fmt)
Files:
crates/grida-canvas/src/vectornetwork/vn_painter.rscrates/grida-canvas/examples/grida_vector.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/examples/grida_paint.rscrates/grida-canvas/examples/grida_texts.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/examples/grida_shapes.rscrates/grida-canvas/tests/render_bounds.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/examples/grida_strokes.rscrates/grida-canvas/examples/grida_mask.rscrates/grida-canvas/benches/bench_rectangles.rscrates/grida-canvas/tests/geometry_cache.rscrates/grida-canvas/tests/scene_cache.rscrates/grida-canvas/examples/golden_type_stroke.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/examples/sys_camera.rscrates/grida-canvas/src/dummy/mod.rscrates/grida-canvas/examples/grida_images.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/examples/golden_container_stroke.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/tests/hit_test.rscrates/grida-canvas/src/node/mod.rscrates/grida-canvas/examples/golden_svg.rscrates/grida-canvas/examples/grida_webfonts.rscrates/grida-canvas/tests/export_as_pdf.rscrates/grida-canvas/examples/app_grida.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/examples/grida_blendmode.rscrates/grida-canvas/examples/grida_booleans.rscrates/grida-canvas/examples/grida_gradients.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/examples/grida_nested.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/examples/wd_animation.rscrates/grida-canvas/examples/grida_basic.rscrates/grida-canvas/examples/grida_container.rscrates/grida-canvas/examples/grida_effects.rscrates/grida-canvas/examples/bench_100k.rscrates/grida-canvas/examples/app_figma.rscrates/grida-canvas/examples/golden_pdf.rscrates/grida-canvas/examples/grida_fills.rscrates/grida-canvas/examples/grida_image.rscrates/grida-canvas/examples/grida_shapes_ellipse.rscrates/grida-canvas/examples/grida_lines.rs
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/src/vectornetwork/vn_painter.rscrates/grida-canvas/examples/grida_vector.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/examples/grida_paint.rscrates/grida-canvas/examples/grida_texts.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/Cargo.tomlcrates/grida-canvas/examples/grida_shapes.rscrates/grida-canvas/tests/render_bounds.rscrates/grida-canvas/src/painter/painter_debug_node.rscrates/grida-canvas/examples/grida_strokes.rscrates/grida-canvas/examples/grida_mask.rscrates/grida-canvas/benches/bench_rectangles.rscrates/grida-canvas/tests/geometry_cache.rscrates/grida-canvas/tests/scene_cache.rscrates/grida-canvas/examples/golden_type_stroke.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/examples/sys_camera.rscrates/grida-canvas/src/dummy/mod.rscrates/grida-canvas/examples/grida_images.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/examples/golden_container_stroke.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/tests/hit_test.rscrates/grida-canvas/src/node/mod.rscrates/grida-canvas/examples/golden_svg.rscrates/grida-canvas/examples/grida_webfonts.rscrates/grida-canvas/tests/export_as_pdf.rscrates/grida-canvas/examples/app_grida.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/examples/grida_blendmode.rscrates/grida-canvas/examples/grida_booleans.rscrates/grida-canvas/examples/grida_gradients.rscrates/grida-canvas/src/resources/mod.rscrates/grida-canvas/examples/grida_nested.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/examples/wd_animation.rscrates/grida-canvas/examples/grida_basic.rscrates/grida-canvas/examples/grida_container.rscrates/grida-canvas/examples/grida_effects.rscrates/grida-canvas/examples/bench_100k.rscrates/grida-canvas/examples/app_figma.rscrates/grida-canvas/examples/golden_pdf.rscrates/grida-canvas/examples/grida_fills.rscrates/grida-canvas/examples/grida_image.rscrates/grida-canvas/examples/grida_shapes_ellipse.rscrates/grida-canvas/examples/grida_lines.rs
🧠 Learnings (5)
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/tests/ui_parser_test.rs : Place high-level UI API tests (parse_ui) in tests/ui_parser_test.rs
Applied to files:
crates/grida-canvas/src/vectornetwork/vn_painter.rs
📚 Learning: 2025-09-26T09:30:18.313Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-09-26T09:30:18.313Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use the math2 crate for geometry and common math operations
Applied to files:
crates/grida-canvas/src/cache/geometry.rs
📚 Learning: 2025-09-26T09:30:18.313Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-09-26T09:30:18.313Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use the skia-safe crate for all painting in the 2D real-time rendering engine
Applied to files:
crates/grida-canvas/examples/grida_paint.rs
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Declare ttf-parser = "0.25" as a required dependency for the fonts crate
Applied to files:
crates/grida-canvas/Cargo.toml
📚 Learning: 2025-09-26T09:29:53.155Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-09-26T09:29:53.155Z
Learning: Applies to crates/grida-canvas-wasm/{crates/grida-canvas-wasm/src/main.rs,**/grida-canvas-wasm.d.ts} : When introducing new public APIs in the WASM entrypoint (main.rs), update the TypeScript declarations in grida-canvas-wasm.d.ts to keep bindings in sync
Applied to files:
crates/grida-canvas/src/node/mod.rs
🧬 Code graph analysis (45)
crates/grida-canvas/src/vectornetwork/vn_painter.rs (1)
packages/grida-canvas-cg/lib.ts (6)
BlendMode(138-154)FillRule(177-177)ImagePaint(783-824)Paint(647-653)SolidPaint(685-690)StrokeAlign(218-218)
crates/grida-canvas/examples/grida_vector.rs (3)
packages/grida-canvas-schema/grida.ts (2)
Scene(812-841)Node(1103-1123)crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/repository.rs (1)
new(13-17)
crates/grida-canvas/src/runtime/scene.rs (2)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)
crates/grida-canvas/src/cache/geometry.rs (3)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)crates/grida-canvas/src/node/schema.rs (3)
id(179-179)id(185-202)id(305-319)
crates/grida-canvas/examples/grida_paint.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/grida_texts.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)packages/grida-canvas-schema/grida.ts (2)
Node(1103-1123)Scene(812-841)
crates/grida-canvas/examples/grida_shapes.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/tests/render_bounds.rs (3)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)
crates/grida-canvas/src/painter/painter_debug_node.rs (1)
crates/grida-canvas/src/painter/geometry.rs (1)
boolean_operation_shape(300-306)
crates/grida-canvas/examples/grida_strokes.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/grida_mask.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/factory.rs (1)
new(12-14)
crates/grida-canvas/benches/bench_rectangles.rs (3)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)crates/grida-canvas/src/node/repository.rs (1)
new(13-17)
crates/grida-canvas/tests/geometry_cache.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (2)
new(69-75)node_count(241-243)
crates/grida-canvas/tests/scene_cache.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)
crates/grida-canvas/examples/golden_type_stroke.rs (2)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/src/window/application.rs (2)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/src/node/scene_graph.rs (1)
new_from_snapshot(86-105)
crates/grida-canvas/examples/sys_camera.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/repository.rs (1)
new(13-17)
crates/grida-canvas/src/dummy/mod.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/repository.rs (1)
new(13-17)
crates/grida-canvas/examples/grida_images.rs (3)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)
crates/grida-canvas/src/io/io_figma.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new_from_snapshot(86-105)
crates/grida-canvas/examples/golden_container_stroke.rs (2)
crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/src/painter/layer.rs (1)
crates/grida-canvas/src/painter/geometry.rs (2)
boolean_operation_shape(300-306)boolean_operation_path(244-297)
crates/grida-canvas/tests/hit_test.rs (4)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/repository.rs (1)
new(13-17)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)
crates/grida-canvas/examples/golden_svg.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)packages/grida-canvas-schema/grida.ts (2)
Node(1103-1123)Scene(812-841)
crates/grida-canvas/examples/grida_webfonts.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)packages/grida-canvas-schema/grida.ts (2)
Node(1103-1123)Scene(812-841)
crates/grida-canvas/tests/export_as_pdf.rs (3)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)
crates/grida-canvas/examples/app_grida.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new_from_snapshot(86-105)
crates/grida-canvas/src/node/schema.rs (1)
packages/grida-canvas-schema/grida.ts (1)
Scene(812-841)
crates/grida-canvas/examples/grida_blendmode.rs (2)
packages/grida-canvas-schema/grida.ts (2)
Scene(812-841)Node(1103-1123)crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/grida_booleans.rs (3)
packages/grida-canvas-schema/grida.ts (2)
Scene(812-841)Node(1103-1123)crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/repository.rs (1)
new(13-17)
crates/grida-canvas/examples/grida_gradients.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/src/resources/mod.rs (2)
crates/grida-canvas/src/node/schema.rs (3)
id(179-179)id(185-202)id(305-319)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)
crates/grida-canvas/examples/grida_nested.rs (4)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/node/factory.rs (1)
new(12-14)crates/grida-canvas/src/cg/types.rs (9)
new(16-18)new(803-809)new(1162-1169)circular(386-391)circular(451-453)r(63-65)g(66-68)b(69-71)from_font(968-985)crates/math2/src/transform.rs (2)
new(44-49)rotation(191-193)
crates/grida-canvas/src/node/scene_graph.rs (3)
crates/grida-canvas/src/node/schema.rs (3)
id(179-179)id(185-202)id(305-319)crates/grida-canvas/src/node/factory.rs (2)
id(16-20)new(12-14)crates/grida-canvas/src/io/io_figma.rs (1)
new(260-267)
crates/grida-canvas/examples/wd_animation.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/grida_basic.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)packages/grida-canvas-schema/grida.ts (2)
Node(1103-1123)Scene(812-841)
crates/grida-canvas/examples/grida_container.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/grida_effects.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/bench_100k.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/app_figma.rs (3)
crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)crates/grida-canvas/src/node/scene_graph.rs (2)
roots(206-208)node_count(241-243)
crates/grida-canvas/examples/golden_pdf.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)packages/grida-canvas-schema/grida.ts (2)
Node(1103-1123)Scene(812-841)
crates/grida-canvas/examples/grida_fills.rs (2)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/src/io/io_figma.rs (1)
new(260-267)
crates/grida-canvas/examples/grida_image.rs (3)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)crates/grida-canvas/examples/golden_container_stroke.rs (1)
scene(9-47)crates/grida-canvas/examples/golden_type_stroke.rs (1)
scene(9-99)
crates/grida-canvas/examples/grida_shapes_ellipse.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
crates/grida-canvas/examples/grida_lines.rs (1)
crates/grida-canvas/src/node/scene_graph.rs (1)
new(69-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: test
- GitHub Check: cargo test
| let mut graph = SceneGraph::new(); | ||
|
|
There was a problem hiding this comment.
Guard against divide‑by‑zero (n≈1) in diagonal hue
For n=1, (grid_height + grid_width - 2) becomes 0, producing NaN hues and unexpected colors.
Apply:
- let diagonal_progress = (row + col) as f32 / (grid_height + grid_width - 2) as f32;
+ let denom = (grid_height + grid_width - 2).max(1) as f32;
+ let diagonal_progress = (row + col) as f32 / denom;Optional perf tweak: accumulate nodes in a Vec and use graph.append_children(..., Parent::Root) to reduce per‑append overhead when generating 100k nodes.
Also applies to: 25-55
🤖 Prompt for AI Agents
In crates/grida-canvas/examples/bench_100k.rs around lines 11-12 (and similarly
lines 25-55), the diagonal hue calculation divides by (grid_height + grid_width
- 2) which becomes zero when n==1 and yields NaN; fix by guarding the
denominator (e.g., compute denom = (grid_height + grid_width - 2).max(1) or
branch when n==1) and use denom in the hue division so no divide-by-zero occurs,
and as an optional perf improvement collect the generated nodes into a Vec and
call graph.append_children(..., Parent::Root) once instead of appending each
node individually to reduce per-append overhead when generating many nodes.
| // FIXME: broken demo - make this golden_ not grida_ | ||
|
|
There was a problem hiding this comment.
Resolve FIXME before merging (broken demo)
This example is marked broken. Either fix it now, or move/rename to a non-shipping “golden_” or WIP location to avoid regressions in examples.
Want me to draft a minimal fix or relocate it and add a tracking issue?
| links: std::collections::HashMap<NodeId, Vec<NodeId>>, | ||
| image_urls: std::collections::HashMap<String, String>, |
There was a problem hiding this comment.
Vector children are not linked into the graph
convert_vector creates path children and inserts them into the repository, but never records the parent→children relation in self.links. Those nodes will be unreachable during traversal.
Insert the link before returning the container (use the same id as the container):
fn convert_vector(&mut self, origin: &Box<VectorNode>) -> Result<Node, String> {
let mut children = Vec::new();
let mut path_index = 0;
@@
- // Create a group node containing all the path nodes
- Ok(Node::Container(ContainerNodeRec {
- id: origin.id.clone(),
+ // Record children relationship
+ let node_id = origin.id.clone();
+ if !children.is_empty() {
+ self.links.insert(node_id.clone(), children);
+ }
+ // Create a group node containing all the path nodes
+ Ok(Node::Container(ContainerNodeRec {
+ id: node_id,
name: Some(origin.name.clone()),Also applies to: 1046-1114
🤖 Prompt for AI Agents
In crates/grida-canvas/src/io/io_figma.rs around lines 254-255, convert_vector
creates path child nodes and inserts them into the repository but never records
the parent→children relation in self.links, making those nodes unreachable;
before returning the container node, insert an entry into self.links using the
container's NodeId as the key and a Vec of the child NodeIds as the value (same
id as the container), and apply the same fix in the other convert_vector
location around lines 1046-1114 so both places record the parent→children
relationship.
| pub fn add_child(&mut self, parent: &NodeId, child: NodeId) -> SceneGraphResult<()> { | ||
| let children = self | ||
| .links | ||
| .get_mut(parent) | ||
| .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?; | ||
| children.push(child); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
add_child should initialize children when absent
Currently errors if parent has no existing entry in links, even if parent node exists.
Apply this diff:
pub fn add_child(&mut self, parent: &NodeId, child: NodeId) -> SceneGraphResult<()> {
- let children = self
- .links
- .get_mut(parent)
- .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?;
- children.push(child);
+ if self.links.contains_key(parent) {
+ self.links.get_mut(parent).unwrap().push(child);
+ } else if self.has_node(parent) {
+ self.links.insert(parent.clone(), vec![child]);
+ } else {
+ return Err(SceneGraphError::ParentNotFound(parent.clone()));
+ }
Ok(())
}🤖 Prompt for AI Agents
In crates/grida-canvas/src/node/scene_graph.rs around lines 151–158, the
add_child currently returns an error if the parent has no entry in links even
when the parent node exists; change it to first verify the parent node exists in
self.nodes and return SceneGraphError::ParentNotFound if not, otherwise use
links.entry(parent.clone()).or_insert_with(Vec::new) to create an empty children
vec when absent and then push the child; ensure the function still returns
Ok(()) on success.
| pub fn add_child_at( | ||
| &mut self, | ||
| parent: &NodeId, | ||
| child: NodeId, | ||
| index: usize, | ||
| ) -> SceneGraphResult<()> { | ||
| let children = self | ||
| .links | ||
| .get_mut(parent) | ||
| .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?; | ||
|
|
||
| if index > children.len() { | ||
| return Err(SceneGraphError::IndexOutOfBounds { | ||
| parent: parent.clone(), | ||
| index, | ||
| len: children.len(), | ||
| }); | ||
| } | ||
|
|
||
| children.insert(index, child); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
add_child_at should allow first insertion and validate index
Handle empty-children case when parent exists; keep bounds checks.
Apply this diff:
pub fn add_child_at(
&mut self,
parent: &NodeId,
child: NodeId,
index: usize,
) -> SceneGraphResult<()> {
- let children = self
- .links
- .get_mut(parent)
- .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?;
-
- if index > children.len() {
- return Err(SceneGraphError::IndexOutOfBounds {
- parent: parent.clone(),
- index,
- len: children.len(),
- });
- }
-
- children.insert(index, child);
+ if !self.links.contains_key(parent) {
+ if !self.has_node(parent) {
+ return Err(SceneGraphError::ParentNotFound(parent.clone()));
+ }
+ if index > 0 {
+ return Err(SceneGraphError::IndexOutOfBounds {
+ parent: parent.clone(),
+ index,
+ len: 0,
+ });
+ }
+ self.links.insert(parent.clone(), vec![child]);
+ return Ok(());
+ }
+ let children = self.links.get_mut(parent).unwrap();
+ if index > children.len() {
+ return Err(SceneGraphError::IndexOutOfBounds {
+ parent: parent.clone(),
+ index,
+ len: children.len(),
+ });
+ }
+ children.insert(index, child);
Ok(())
}🤖 Prompt for AI Agents
In crates/grida-canvas/src/node/scene_graph.rs around lines 161 to 182, the
current add_child_at assumes the parent has an existing children vector and
returns ParentNotFound if there is no entry; instead, ensure the parent node's
existence is checked (return ParentNotFound only if the parent node truly
doesn't exist), then obtain or create an empty children Vec for that parent
using the map's entry API (or otherwise insert a default Vec::new() when the
parent exists but has no children), keep the bounds check as index >
children.len() to allow insertion at the end (and allow index == 0 for first
insertion), and finally insert the child at the validated index.
| /// Remove a node from the repository and return it | ||
| pub fn remove_node(&mut self, id: &NodeId) -> SceneGraphResult<Node> { | ||
| self.nodes | ||
| .remove(id) | ||
| .ok_or_else(|| SceneGraphError::NodeNotFound(id.clone())) | ||
| } |
There was a problem hiding this comment.
Removing nodes leaves dangling links/roots
remove_node only touches the repository; links/roots can retain stale IDs.
Add a safe removal API to unlink and remove subtrees:
+ /// Remove a node and its entire subtree; unlink from parents/roots and links.
+ pub fn remove_subtree(&mut self, id: &NodeId) -> SceneGraphResult<usize> {
+ if !self.has_node(id) {
+ return Err(SceneGraphError::NodeNotFound(id.clone()));
+ }
+ // Detach from roots
+ self.roots.retain(|r| r != id);
+ // Detach from any parent lists
+ for children in self.links.values_mut() {
+ children.retain(|c| c != id);
+ }
+ // Recursively remove descendants
+ let mut removed = 1usize;
+ if let Some(children) = self.links.remove(id) {
+ for child in children {
+ removed += self.remove_subtree(&child)?;
+ }
+ }
+ self.nodes.remove(id);
+ Ok(removed)
+ }Keep existing remove_node for low-level use; prefer remove_subtree in higher-level code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Remove a node from the repository and return it | |
| pub fn remove_node(&mut self, id: &NodeId) -> SceneGraphResult<Node> { | |
| self.nodes | |
| .remove(id) | |
| .ok_or_else(|| SceneGraphError::NodeNotFound(id.clone())) | |
| } | |
| /// Remove a node from the repository and return it | |
| pub fn remove_node(&mut self, id: &NodeId) -> SceneGraphResult<Node> { | |
| self.nodes | |
| .remove(id) | |
| .ok_or_else(|| SceneGraphError::NodeNotFound(id.clone())) | |
| } | |
| /// Remove a node and its entire subtree; unlink from parents/roots and links. | |
| pub fn remove_subtree(&mut self, id: &NodeId) -> SceneGraphResult<usize> { | |
| if !self.has_node(id) { | |
| return Err(SceneGraphError::NodeNotFound(id.clone())); | |
| } | |
| // Detach from roots | |
| self.roots.retain(|r| r != id); | |
| // Detach from any parent lists | |
| for children in self.links.values_mut() { | |
| children.retain(|c| c != id); | |
| } | |
| // Recursively remove descendants | |
| let mut removed = 1usize; | |
| if let Some(children) = self.links.remove(id) { | |
| for child in children { | |
| removed += self.remove_subtree(&child)?; | |
| } | |
| } | |
| self.nodes.remove(id); | |
| Ok(removed) | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/node/scene_graph.rs around lines 228 to 233,
remove_node currently only deletes an entry from the nodes map leaving
parent/child links and root lists pointing at stale IDs; add a new safe API
remove_subtree(&mut self, id: &NodeId) -> SceneGraphResult<Vec<Node>> that (1)
iteratively/recursively traverses the subtree rooted at id, collecting all
descendant IDs, (2) for each node unlinks it from its parent and removes
references from its children, (3) removes any occurrences from the roots
collection, (4) deletes each node from the nodes map and returns the removed
nodes (or their IDs) as the result; keep the existing remove_node for low-level
use but update higher-level code to call remove_subtree to avoid dangling links
and ensure tests cover parent/child/link/root invariants after removal.
| for id in scene.graph.roots() { | ||
| let result = Self::flatten_node(&id, &scene.graph, scene_cache, 1.0, &mut list.layers); | ||
| list.commands.extend(result.commands); |
There was a problem hiding this comment.
Fix type mismatch in from_scene (&&NodeId)
Pass the iterator item directly; remove the extra reference.
Apply this diff:
- for id in scene.graph.roots() {
- let result = Self::flatten_node(&id, &scene.graph, scene_cache, 1.0, &mut list.layers);
+ for id in scene.graph.roots() {
+ let result = Self::flatten_node(id, &scene.graph, scene_cache, 1.0, &mut list.layers);
list.commands.extend(result.commands);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for id in scene.graph.roots() { | |
| let result = Self::flatten_node(&id, &scene.graph, scene_cache, 1.0, &mut list.layers); | |
| list.commands.extend(result.commands); | |
| for id in scene.graph.roots() { | |
| let result = Self::flatten_node(id, &scene.graph, scene_cache, 1.0, &mut list.layers); | |
| list.commands.extend(result.commands); | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/painter/layer.rs around lines 219-221, the code is
passing &id to Self::flatten_node which causes a &&NodeId type mismatch; change
the call to pass the iterator item directly (i.e., use Self::flatten_node(id,
&scene.graph, scene_cache, 1.0, &mut list.layers)) so the function receives a
single reference type as expected.
| // Filter links to remove None values | ||
| let filtered_links: std::collections::HashMap<String, Vec<String>> = links | ||
| .into_iter() | ||
| .filter_map(|(parent_id, children_opt)| { | ||
| children_opt.map(|children| (parent_id, children)) | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Build scene graph from snapshot | ||
| let graph = SceneGraph::new_from_snapshot(nodes, filtered_links, scene_children); | ||
|
|
There was a problem hiding this comment.
Filter out scene parent links to avoid dangling edges
Retain only non-scene parents (at least exclude the current scene_id) to keep graph.links consistent with nodes.
- // Filter links to remove None values
- let filtered_links: std::collections::HashMap<String, Vec<String>> = links
- .into_iter()
- .filter_map(|(parent_id, children_opt)| {
- children_opt.map(|children| (parent_id, children))
- })
- .collect();
+ // Filter links to remove None values and scene parent
+ let filtered_links: std::collections::HashMap<String, Vec<String>> = links
+ .into_iter()
+ .filter(|(parent_id, _)| parent_id != &scene_id)
+ .filter_map(|(parent_id, children_opt)| {
+ children_opt.map(|children| (parent_id, children))
+ })
+ .collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Filter links to remove None values | |
| let filtered_links: std::collections::HashMap<String, Vec<String>> = links | |
| .into_iter() | |
| .filter_map(|(parent_id, children_opt)| { | |
| children_opt.map(|children| (parent_id, children)) | |
| }) | |
| .collect(); | |
| // Build scene graph from snapshot | |
| let graph = SceneGraph::new_from_snapshot(nodes, filtered_links, scene_children); | |
| // Filter links to remove None values and scene parent | |
| let filtered_links: std::collections::HashMap<String, Vec<String>> = links | |
| .into_iter() | |
| .filter(|(parent_id, _)| parent_id != &scene_id) | |
| .filter_map(|(parent_id, children_opt)| { | |
| children_opt.map(|children| (parent_id, children)) | |
| }) | |
| .collect(); | |
| // Build scene graph from snapshot | |
| let graph = SceneGraph::new_from_snapshot(nodes, filtered_links, scene_children); |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/window/application.rs around lines 503 to 513, the
links are currently only filtered for None children but still include parent
entries pointing to the scene itself, producing dangling edges; update the
filter_map to also exclude entries whose parent_id equals the current scene_id
(or otherwise represents a scene parent) so only non-scene parents are retained,
i.e., check children_opt.map only when parent_id != scene_id (or perform an
explicit early filter for parent_id != scene_id) before collecting into
filtered_links so graph.links align with existing nodes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-canvas/src/io/io_figma.rs (1)
473-481: Stroke align mapping is broken (string is JSON-quoted).serde_json::to_string produces quoted strings (e.g., ""INSIDE""), which won’t match "INSIDE"/"OUTSIDE"/"CENTER". Result: non-default alignments collapse to Center.
Quick fix: trim quotes and normalize case in convert_stroke_align.
- fn convert_stroke_align(stroke_align: String) -> StrokeAlign { - match stroke_align.as_str() { - "INSIDE" => StrokeAlign::Inside, - "OUTSIDE" => StrokeAlign::Outside, - "CENTER" => StrokeAlign::Center, - _ => StrokeAlign::Center, - } - } + fn convert_stroke_align(stroke_align: String) -> StrokeAlign { + let s = stroke_align.trim_matches('"').to_ascii_uppercase(); + match s.as_str() { + "INSIDE" => StrokeAlign::Inside, + "OUTSIDE" => StrokeAlign::Outside, + "CENTER" => StrokeAlign::Center, + _ => StrokeAlign::Center, + } + }Follow-up (recommended): stop stringifying altogether and map the enum directly. I can provide a refactor if you prefer.
♻️ Duplicate comments (7)
crates/grida-canvas/src/io/io_figma.rs (1)
1093-1111: Vector children are not linked into the graph (unreachable paths).convert_vector inserts path nodes into the repository but never records parent→children in self.links, so the group won’t expose its paths in SceneGraph traversal.
- // Create a group node containing all the path nodes - Ok(Node::Container(ContainerNodeRec { - id: origin.id.clone(), + // Create a group node containing all the path nodes + // Record children relationship + let node_id = origin.id.clone(); + if !children.is_empty() { + self.links.insert(node_id.clone(), children); + } + Ok(Node::Container(ContainerNodeRec { + id: node_id, name: Some(origin.name.clone()), active: origin.visible.unwrap_or(true), opacity: Self::convert_opacity(origin.visible), blend_mode: Self::convert_blend_mode(origin.blend_mode), mask: None, transform: Self::convert_transform(origin.relative_transform.as_ref()), size: Self::convert_size(origin.size.as_ref()), corner_radius: RectangularCornerRadius::zero(), fills: Paints::new([TRANSPARENT]), strokes: Paints::default(), stroke_width: 0.0, stroke_align: StrokeAlign::Inside, stroke_dash_array: None, effects: LayerEffects::default(), clip: false, }))crates/grida-canvas/src/painter/layer.rs (1)
219-221: Fix type mismatch: passing &&NodeId to flatten_node.id from roots() is already &NodeId. Drop the extra &.
- for id in scene.graph.roots() { - let result = Self::flatten_node(&id, &scene.graph, scene_cache, 1.0, &mut list.layers); + for id in scene.graph.roots() { + let result = Self::flatten_node(id, &scene.graph, scene_cache, 1.0, &mut list.layers); list.commands.extend(result.commands); }crates/grida-canvas/src/node/scene_graph.rs (5)
150-158: add_child should initialize children when absent (parent exists).Currently errors if parent has no links entry. Use entry API; keep ParentNotFound only when parent node truly doesn’t exist.
pub fn add_child(&mut self, parent: &NodeId, child: NodeId) -> SceneGraphResult<()> { - let children = self - .links - .get_mut(parent) - .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?; - children.push(child); + if self.links.contains_key(parent) { + self.links.get_mut(parent).unwrap().push(child); + } else if self.has_node(parent) { + self.links.insert(parent.clone(), vec![child]); + } else { + return Err(SceneGraphError::ParentNotFound(parent.clone())); + } Ok(()) }
161-182: add_child_at should allow first insertion and validate index.Handle empty-children case for existing parents; keep bounds checks.
pub fn add_child_at( &mut self, parent: &NodeId, child: NodeId, index: usize, ) -> SceneGraphResult<()> { - let children = self - .links - .get_mut(parent) - .ok_or_else(|| SceneGraphError::ParentNotFound(parent.clone()))?; - - if index > children.len() { - return Err(SceneGraphError::IndexOutOfBounds { - parent: parent.clone(), - index, - len: children.len(), - }); - } - - children.insert(index, child); + if !self.links.contains_key(parent) { + if !self.has_node(parent) { + return Err(SceneGraphError::ParentNotFound(parent.clone())); + } + if index > 0 { + return Err(SceneGraphError::IndexOutOfBounds { parent: parent.clone(), index, len: 0 }); + } + self.links.insert(parent.clone(), vec![child]); + return Ok(()); + } + let children = self.links.get_mut(parent).unwrap(); + if index > children.len() { + return Err(SceneGraphError::IndexOutOfBounds { parent: parent.clone(), index, len: children.len() }); + } + children.insert(index, child); Ok(()) }
296-323: ancestors is O(d×n); add a parent map for O(d).Maintain parents: HashMap<NodeId, NodeId> and walk it in ancestors. Update it in constructors/mutations.
pub struct SceneGraph { /// Root node IDs - direct children of the scene roots: Vec<NodeId>, /// Parent to children adjacency list links: HashMap<NodeId, Vec<NodeId>>, + /// Child to parent map for O(1) ancestor lookup + parents: HashMap<NodeId, NodeId>, /// Node data repository nodes: NodeRepository, } pub fn new() -> Self { Self { roots: Vec::new(), links: HashMap::new(), + parents: HashMap::new(), nodes: NodeRepository::new(), } } pub fn new_from_snapshot( nodes: impl IntoIterator<Item = Node>, links: HashMap<NodeId, Vec<NodeId>>, roots: Vec<NodeId>, ) -> Self { let mut graph = Self::new(); for node in nodes { graph.nodes.insert(node); } - graph.links = links; + graph.links = links; + // Build parents map from links + for (p, cs) in &graph.links { + for c in cs { + graph.parents.insert(c.clone(), p.clone()); + } + } graph.roots = roots; graph } -/// Get all ancestors of a node (path to root) +/// Get all ancestors of a node (path to root) pub fn ancestors(&self, id: &NodeId) -> SceneGraphResult<Vec<NodeId>> { if !self.has_node(id) { return Err(SceneGraphError::NodeNotFound(id.clone())); } - let mut result = Vec::new(); - let mut current = id.clone(); - // Find parent by searching all links - loop { - let mut found_parent = false; - for (parent_id, children) in &self.links { - if children.contains(¤t) { - result.push(parent_id.clone()); - current = parent_id.clone(); - found_parent = true; - break; - } - } - if !found_parent { break; } - } - Ok(result) + let mut result = Vec::new(); + let mut cur = id; + while let Some(parent) = self.parents.get(cur) { + result.push(parent.clone()); + cur = parent; + } + Ok(result) }Don’t forget to maintain parents in append_child/add_child/add_child_at/remove logic. I can provide a full patch if you want.
111-131: Reparenting leaves stale roots/parent links (duplicates on traversal).Appending/adding a child doesn’t detach it from previous parent or from roots. Nodes can appear under multiple parents and be traversed twice.
+ /// Move an existing node under a new parent, detaching from previous parent/roots. + pub fn reparent(&mut self, child: &NodeId, new_parent: Parent) -> SceneGraphResult<()> { + if !self.has_node(child) { + return Err(SceneGraphError::NodeNotFound(child.clone())); + } + // Detach from roots + self.roots.retain(|r| r != child); + // Detach from any current parent list + for children in self.links.values_mut() { + children.retain(|c| c != child); + } + // Attach to new parent + match new_parent { + Parent::Root => {} + Parent::NodeId(pid) => { + self.links.entry(pid.clone()).or_default().push(child.clone()); + #[cfg(any())] { let _ = &pid; } // placeholder to avoid unused warnings if needed + } + } + // Update parent map if present + #[allow(unused)] + { + if let Some(parents) = (&mut self.parents as *mut _ as *mut Option<HashMap<NodeId, NodeId>>).as_mut() { + parents.as_mut().map(|p| p.insert(child.clone(), match new_parent { Parent::NodeId(pid) => pid, Parent::Root => return })); + } + } + Ok(()) + }Alternatively, make add_child/add_child_at perform detachment semantics directly. I can adjust tests accordingly.
Also applies to: 150-158, 161-182
228-233: remove_node leaves dangling links and roots. Provide a safe subtree removal.Removing only from the repository keeps stale IDs in links/roots.
+ /// Remove a node and its entire subtree; unlink from parents/roots and links. + pub fn remove_subtree(&mut self, id: &NodeId) -> SceneGraphResult<usize> { + if !self.has_node(id) { + return Err(SceneGraphError::NodeNotFound(id.clone())); + } + // Detach from roots + self.roots.retain(|r| r != id); + // Detach from any parent lists + for children in self.links.values_mut() { + children.retain(|c| c != id); + } + // Recursively remove descendants + let mut removed = 1usize; + if let Some(children) = self.links.remove(id) { + for child in children { + removed += self.remove_subtree(&child)?; + } + } + self.nodes.remove(id); + Ok(removed) + }
🧹 Nitpick comments (5)
crates/grida-canvas-fonts/examples/ttf_parser_chained_sequence_features.rs (1)
150-154: Consider removing the unused_faceparameter.Both
extract_gsub_features_simplifiedandextract_gpos_features_simplifiedhave an unused_faceparameter. Since the functions extract glyphs directly from the table structures without referencing the face, this parameter can be removed to simplify the function signatures.Apply this diff to remove the unused parameter from both functions:
fn extract_gsub_features_simplified( - _face: &ttf_parser::Face, gsub_table: ttf_parser::opentype_layout::LayoutTable, glyph_map: &std::collections::HashMap<u16, char>, ) -> Vec<ChainedFeature> {fn extract_gpos_features_simplified( - _face: &ttf_parser::Face, gpos_table: ttf_parser::opentype_layout::LayoutTable, glyph_map: &std::collections::HashMap<u16, char>, ) -> Vec<ChainedFeature> {And update the call sites in
extract_features_via_chained_sequences:if let Some(gsub_table) = face.tables().gsub { features.extend(extract_gsub_features_simplified( - face, gsub_table, glyph_map, + gsub_table, glyph_map, )); } if let Some(gpos_table) = face.tables().gpos { features.extend(extract_gpos_features_simplified( - face, gpos_table, glyph_map, + gpos_table, glyph_map, )); }Also applies to: 232-236
crates/grida-canvas/src/node/schema.rs (1)
243-262: mask() may move out of a borrowed field; return a reference or cloneAccessing
n.maskmoves the field unlessOption<LayerMaskType>is Copy. Prefer cloning or returning a reference to avoid potential move-from-borrow errors.Option A (no allocation, preferred): return by reference
-impl Node { - pub fn mask(&self) -> Option<LayerMaskType> { +impl Node { + pub fn mask(&self) -> Option<&LayerMaskType> { match self { - Node::Group(n) => n.mask, + Node::Group(n) => n.mask.as_ref(), // ... same for all variants ... - Node::Error(_) => None, + Node::Error(_) => None, } } }Option B (keep current signature): clone
- pub fn mask(&self) -> Option<LayerMaskType> { + pub fn mask(&self) -> Option<LayerMaskType> { match self { - Node::Group(n) => n.mask, + Node::Group(n) => n.mask.clone(), // ... same for all variants ... - Node::Error(_) => None, + Node::Error(_) => None, } }crates/grida-canvas/src/io/io_figma.rs (2)
851-857: Graph snapshot includes all repository nodes (cross‑canvas contamination).new_from_snapshot uses the entire repository and global links for every canvas. Subsequent canvases will carry nodes/links from earlier ones, bloating scenes and risking unintended references. Build the snapshot from nodes reachable from this canvas’s roots (or reset repository/links per canvas). I can provide a reachability filter if helpful.
86-90: Remove dead_repeator wire it through ImagePaint._repeat is computed but unused (commented-out field). Either remove it or plumb repeat into your Paint/Image runtime.
Also applies to: 352-356
crates/grida-canvas/src/painter/layer.rs (1)
792-804: Update docs: parameter isgraph, notrepo.Minor doc mismatch. Replace “repo” with “graph” for accuracy.
- /// - `repo`: The node repository containing all nodes + /// - `graph`: The scene graph containing all nodes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/grida-canvas/goldens/type_line_height.pngis excluded by!**/*.png
📒 Files selected for processing (13)
crates/grida-canvas-fonts/examples/ttf_parser_chained_sequence_features.rs(2 hunks)crates/grida-canvas/examples/bench_cache_picture.rs(1 hunks)crates/grida-canvas/examples/golden_type_line_height.rs(0 hunks)crates/grida-canvas/examples/grida_fills.rs(10 hunks)crates/grida-canvas/examples/grida_shapes.rs(9 hunks)crates/grida-canvas/examples/grida_shapes_ellipse.rs(7 hunks)crates/grida-canvas/examples/grida_vector.rs(11 hunks)crates/grida-canvas/src/io/io_figma.rs(12 hunks)crates/grida-canvas/src/node/scene_graph.rs(1 hunks)crates/grida-canvas/src/node/schema.rs(3 hunks)crates/grida-canvas/src/painter/layer.rs(26 hunks)crates/grida-canvas/src/runtime/scene.rs(5 hunks)crates/grida-canvas/src/window/application.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/grida-canvas/examples/golden_type_line_height.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/grida-canvas/src/window/application.rs
- crates/grida-canvas/examples/grida_vector.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: Use the skia-safe crate for all painting in the 2D real-time rendering engine
Use the math2 crate for geometry and common math operations
Ensure source is formatted with rustfmt (cargo fmt)
Files:
crates/grida-canvas/examples/grida_shapes_ellipse.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/examples/grida_shapes.rscrates/grida-canvas/examples/grida_fills.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/examples/bench_cache_picture.rs
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/examples/grida_shapes_ellipse.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/examples/grida_shapes.rscrates/grida-canvas-fonts/examples/ttf_parser_chained_sequence_features.rscrates/grida-canvas/examples/grida_fills.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/src/painter/layer.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/examples/bench_cache_picture.rs
🔇 Additional comments (9)
crates/grida-canvas/examples/bench_cache_picture.rs (1)
246-251: LGTM!The refactoring from mutable state to per-frame immutable calculations is clean and correct. The camera motion uses standard circular path logic, and the zoom oscillation properly ranges from 0.5 to 1.5 as documented.
crates/grida-canvas/src/node/schema.rs (1)
4-4: Scene now owns a SceneGraph — LGTMImport and struct update look correct and align with centralizing hierarchy + data.
Also applies to: 83-85, 88-90
crates/grida-canvas/examples/grida_fills.rs (2)
10-10: SceneGraph adoption in example — LGTMGood migration to
SceneGraphwithParent::{Root, NodeId(..)}and finalScene { graph, .. }.Also applies to: 20-21, 41-44, 75-78, 113-116, 164-167, 228-231, 286-289, 352-355, 390-393, 397-397
58-58: Potential typo: AffineTransform::from_rotatationPlease verify the method name. If the API is
from_rotation, these calls will fail to compile.If needed:
- transform: AffineTransform::from_rotatation(45.0), + transform: AffineTransform::from_rotation(45.0),and
- transform: AffineTransform::from_rotatation(90.0), + transform: AffineTransform::from_rotation(90.0),Run to confirm the symbol spelling and usage:
Also applies to: 129-129
crates/grida-canvas/src/runtime/scene.rs (2)
3-3: Import switch to SceneGraph — LGTMChange aligns with graph-based traversal elsewhere in this module.
672-676: Tests updated to SceneGraph — LGTMTests construct scenes via
SceneGraphand exercise caches accordingly.Also applies to: 688-690, 693-693, 754-756, 759-761
crates/grida-canvas/examples/grida_shapes_ellipse.rs (1)
19-22: SceneGraph wiring — LGTMRoot container creation and child attachment via
append_child(..., Parent::NodeId(root_container_id))look correct.Also applies to: 43-45, 65-67, 88-90, 112-114, 135-137, 142-144
crates/grida-canvas/examples/grida_shapes.rs (2)
26-28: SceneGraph migration — LGTMClean switch to
SceneGraphwith consistent parent-child wiring and finalScene { graph, .. }.Also applies to: 45-48, 66-69, 97-101, 121-124, 151-154, 175-178, 200-203, 207-209
303-304: Potential typo: AffineTransform::from_rotatationConfirm the method exists. If not, replace with
from_rotation.Example:
- transform: AffineTransform::from_rotatation(0.0), + transform: AffineTransform::from_rotation(0.0),(and similarly for 60.0 and 120.0)
Use the same script provided in grida_fills.rs to verify across the repo.
Also applies to: 319-320, 335-336
| fn walk(id: &NodeId, graph: &SceneGraph, set: &mut HashSet<String>) { | ||
| if let Ok(node) = graph.get_node(id) { | ||
| match node { | ||
| Node::TextSpan(n) => { | ||
| set.insert(n.text_style.font_family.clone()); | ||
| } | ||
| Node::Group(n) => { | ||
| for child in &n.children { | ||
| walk(child, repo, set); | ||
| } | ||
| } | ||
| Node::Container(n) => { | ||
| for child in &n.children { | ||
| walk(child, repo, set); | ||
| } | ||
| } | ||
| Node::BooleanOperation(n) => { | ||
| for child in &n.children { | ||
| walk(child, repo, set); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| if let Some(children) = graph.get_children(id) { | ||
| for child in children { | ||
| walk(child, graph, set); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: pass reference in DFS; current call moves NodeId into walk
walk expects &NodeId but the loop passes child by value. This likely won’t compile and prevents traversal.
Apply:
- if let Some(children) = graph.get_children(id) {
- for child in children {
- walk(child, graph, set);
- }
- }
+ if let Some(children) = graph.get_children(id) {
+ for child in children {
+ walk(&child, graph, set);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn walk(id: &NodeId, graph: &SceneGraph, set: &mut HashSet<String>) { | |
| if let Ok(node) = graph.get_node(id) { | |
| match node { | |
| Node::TextSpan(n) => { | |
| set.insert(n.text_style.font_family.clone()); | |
| } | |
| Node::Group(n) => { | |
| for child in &n.children { | |
| walk(child, repo, set); | |
| } | |
| } | |
| Node::Container(n) => { | |
| for child in &n.children { | |
| walk(child, repo, set); | |
| } | |
| } | |
| Node::BooleanOperation(n) => { | |
| for child in &n.children { | |
| walk(child, repo, set); | |
| } | |
| } | |
| _ => {} | |
| } | |
| } | |
| if let Some(children) = graph.get_children(id) { | |
| for child in children { | |
| walk(child, graph, set); | |
| } | |
| } | |
| } | |
| fn walk(id: &NodeId, graph: &SceneGraph, set: &mut HashSet<String>) { | |
| if let Ok(node) = graph.get_node(id) { | |
| match node { | |
| Node::TextSpan(n) => { | |
| set.insert(n.text_style.font_family.clone()); | |
| } | |
| _ => {} | |
| } | |
| } | |
| if let Some(children) = graph.get_children(id) { | |
| for child in children { | |
| walk(&child, graph, set); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/runtime/scene.rs around lines 66 to 80, the DFS
helper walk takes &NodeId but the loop calls walk(child, graph, set) moving the
NodeId; change the recursive call to pass a reference: walk(&child, graph, set).
Update the for loop to iterate without moving (e.g., for child in children {
walk(&child, graph, set); }) so NodeId is borrowed rather than moved, preserving
ownership and compiling correctly.
softmarshmallow
changed the title
1 participant
Responsive Layout Model
Prep
Summary by CodeRabbit
New Features
Refactor
Chores