Grida Canvas - skia-safe rust backend - standalone performance testing#383
Grida Canvas - skia-safe rust backend - standalone performance testing#383softmarshmallow merged 280 commits intomainfrom
Conversation
[Core Graphics] Scene Graph
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 40
🧹 Nitpick comments (83)
crates/grida-math2/src/measurement.rs (1)
151-197: Consider edge case handling in auxiliary line generation.The function handles the basic cases well, but there's a potential issue in the conditional logic for horizontal/vertical positioning.
In lines 177-183, when the side is Top/Bottom but the point is horizontally outside the rectangle, the logic assumes the point is either left or right of the rectangle. However, this doesn't account for the case where
px >= x && px <= rect_rightbut the point is still vertically outside. The same issue exists for Left/Right sides in lines 186-192.Consider adding explicit bounds checks:
RectangleSide::Top | RectangleSide::Bottom => { - if px < x { + if px < x && (py < y || py > rect_bottom) { let length_v = (x - px) * zoom; (x2, y2, length, rotation) = (x1 + length_v, y1, length_v, -90.0); - } else { + } else if px > rect_right && (py < y || py > rect_bottom) { let length_v = (px - rect_right) * zoom; (x2, y2, length, rotation) = (x1 - length_v, y1, length_v, 90.0); + } else { + // Point is horizontally within bounds but vertically outside + let target_y = if side == RectangleSide::Top { y } else { rect_bottom }; + let length_v = (py - target_y).abs() * zoom; + let rotation_angle = if py < target_y { 180.0 } else { 0.0 }; + (x2, y2, length, rotation) = (x1, target_y * zoom, length_v, rotation_angle); + }.ref/figma/README.md (1)
3-4: Avoid bare URLs in markdown
Markdownlint rule MD034 flags bare URLs. Please convert these to link syntax for clarity and accessibility.Proposed diff:
- https://github.com/figma/plugin-typings/blob/master/plugin-api.d.ts - https://github.com/figma/rest-api-spec/blob/main/dist/api_types.ts + [Figma Plugin Typings](https://github.com/figma/plugin-typings/blob/master/plugin-api.d.ts) + [Figma REST API Types](https://github.com/figma/rest-api-spec/blob/main/dist/api_types.ts)crates/grida-canvas/package.json (1)
2-4: Clarify package name and description
Thename(@crates/grida-canvas) anddescription(“turbo ci rust wrapper”) are generic. Consider using your org’s namespace (e.g.,@gridaco/grida-canvas) and a more descriptive summary (e.g., “High-performance 2D rendering engine (skia-safe) wrapper”).crates/grida-canvas/src/runtime/input.rs (1)
8-16: Suggest derivingDefaultonInputState
You can remove the manualDefaultimpl by deriving it directly, since[f32; 2]already implementsDefault:#[derive(Debug, Clone, Copy)] -pub struct InputState { +#[derive(Debug, Clone, Copy, Default)] +pub struct InputState { /// Cursor position in logical screen coordinates. pub cursor: Vector2, } - -impl Default for InputState { - fn default() -> Self { - Self { cursor: [0.0, 0.0] } - } -}crates/grida-canvas/AGENTS.md (1)
1-1: Fix the grammar: missing article.The sentence should start with an article for better readability.
-safe, high-performance, 2D real-time rendering engine. +A safe, high-performance, 2D real-time rendering engine.crates/grida-canvas/Makefile (1)
4-10: Consider adding standard Makefile targets.The Makefile would benefit from adding standard targets for better usability and consistency with Makefile conventions.
Add the following standard targets:
+.PHONY: all clean test +all: build + +clean: + cargo clean + +test: + cargo test + .PHONY: build build: $(BUILD)crates/grida-canvas/tests/resource_loader.rs (3)
7-13: Consider adding error handling for path construction.The
resource_pathfunction should handle potential errors in path construction more gracefully.fn resource_path(name: &str) -> String { PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("resources") .join(name) .to_string_lossy() .to_string() } + +fn resource_path(name: &str) -> Result<String, std::io::Error> { + let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("resources") + .join(name); + + if !path.exists() { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("Resource file not found: {}", path.display()) + )); + } + + Ok(path.to_string_lossy().to_string()) +}
15-22: Enhance test assertions for better coverage.The test only verifies that
data.is_some()but doesn't validate the actual loaded content or proper resource management.#[tokio::test] async fn font_loader_simple_load_unload() { let mut loader = FontLoader::new_simple(); let path = resource_path("Caveat-VariableFont_wght.ttf"); let data = loader.load("Caveat", &path).await; assert!(data.is_some()); + + // Verify the font data is actually usable + let font_data = data.unwrap(); + assert!(!font_data.is_empty(), "Font data should not be empty"); + + // Test loading the same resource again (should work) + let data2 = loader.load("Caveat", &path).await; + assert!(data2.is_some()); + loader.unload("Caveat").await; + + // Test that unloaded resource needs to be reloaded + let data3 = loader.load("Caveat", &path).await; + assert!(data3.is_some()); }
24-31: Add error case testing for robustness.Consider adding tests for error conditions like invalid paths or corrupted resources.
#[tokio::test] async fn image_loader_simple_load_unload() { let mut loader = ImageLoader::new_simple(); let path = resource_path("4k.jpg"); let data = loader.load(&path, &path).await; assert!(data.is_some()); loader.unload(&path).await; } + +#[tokio::test] +async fn image_loader_handles_invalid_path() { + let mut loader = ImageLoader::new_simple(); + let invalid_path = resource_path("nonexistent.jpg"); + let data = loader.load(&invalid_path, &invalid_path).await; + assert!(data.is_none(), "Should return None for invalid path"); +}crates/grida-canvas/src/resource_loader.rs (1)
1-13: Well-designed trait interface.The
ResourceLoadertrait provides a clean, async interface for resource management. The design is solid with:
- Proper use of
async_traitfor async methods in traits- Generic
Outputtype for flexibility- Clear separation between load and unload operations
- Good documentation
Consider adding optional methods for more advanced use cases:
#[async_trait] pub trait ResourceLoader { type Output; /// Load a resource identified by `key` from `src`. async fn load(&mut self, key: &str, src: &str) -> Option<Self::Output>; /// Unload or remove a cached resource identified by `key`. async fn unload(&mut self, key: &str); + + /// Check if a resource is already loaded. + fn is_loaded(&self, key: &str) -> bool { false } + + /// Clear all cached resources. + async fn clear(&mut self) {} }crates/grida-canvas/tests/camera.rs (1)
4-18: Solid test for zoom anchoring behavior.The test correctly verifies that
set_zoom_atpreserves the canvas coordinates of the anchor point. Good practices include:
- Proper floating-point comparison using
EPSILON- Clear test setup and assertion logic
- Focused test scope
Consider adding edge case tests:
#[test] fn zoom_at_preserves_anchor() { let mut cam = Camera2D::new(Size { width: 200.0, height: 200.0, }); cam.set_position(0.0, 0.0); cam.set_zoom(1.0); let anchor = [50.0, 50.0]; let before = cam.screen_to_canvas_point(anchor); cam.set_zoom_at(2.0, anchor); let after = cam.screen_to_canvas_point(anchor); assert!((before[0] - after[0]).abs() < f32::EPSILON); assert!((before[1] - after[1]).abs() < f32::EPSILON); } + +#[test] +fn zoom_at_edge_cases() { + let mut cam = Camera2D::new(Size { width: 200.0, height: 200.0 }); + + // Test zoom at corner + let corner = [0.0, 0.0]; + let before = cam.screen_to_canvas_point(corner); + cam.set_zoom_at(0.5, corner); + let after = cam.screen_to_canvas_point(corner); + assert!((before[0] - after[0]).abs() < f32::EPSILON); + assert!((before[1] - after[1]).abs() < f32::EPSILON); +}crates/grida-canvas/examples/image.rs (2)
14-18: Consider validating image dimensions dynamically.The hardcoded image dimensions (8070x5196) assume the exact size of the 8K image. If the actual image file has different dimensions, this could cause rendering issues or incorrect aspect ratios.
Consider loading the image metadata to get actual dimensions:
- let image8k = "resources/8k.jpg".to_string(); - let image8ksize = Size { - width: 8070.0, - height: 5196.0, - }; + let image8k = "resources/8k.jpg".to_string(); + // TODO: Load actual image dimensions dynamically + let image8ksize = Size { + width: 7680.0, // Standard 8K width + height: 4320.0, // Standard 8K height + };
9-13: Clean up commented code or document its purpose.The commented 4K image code should either be removed if unused or documented if it serves as an alternative configuration.
crates/grida-canvas/examples/container.rs (1)
28-33: Consider documenting the drop shadow configuration.The drop shadow with
dx: 0.0, dy: 0.0creates a glow effect rather than a directional shadow. This might be intentional for aesthetic purposes, but could be documented for clarity.crates/Makefile (2)
13-19: Optimize Docker usage in extract target.The extract target rebuilds the Docker image even though the build target already does this. This is inefficient when running
make all.Consider making extract depend on build to avoid redundant builds:
extract: - docker build -f Dockerfile -t $(IMAGE_NAME) . -@docker rm -f $(CONTAINER_NAME) 2>/dev/null || true mkdir -p $(OUT_DIR) docker create --name $(CONTAINER_NAME) $(IMAGE_NAME) docker cp $(CONTAINER_NAME):/work/grida/$(WASM_TARGET)/ $(OUT_DIR)/ docker rm -f $(CONTAINER_NAME) + +extract: build + -@docker rm -f $(CONTAINER_NAME) 2>/dev/null || true + mkdir -p $(OUT_DIR) + docker create --name $(CONTAINER_NAME) $(IMAGE_NAME) + docker cp $(CONTAINER_NAME):/work/grida/$(WASM_TARGET)/ $(OUT_DIR)/ + docker rm -f $(CONTAINER_NAME)
6-6: Consider adding test target for completeness.Static analysis suggests adding a test target to the .PHONY declaration. This would be useful for running tests in the containerized environment.
-.PHONY: all build extract clean +.PHONY: all build extract clean test + +test: + docker run --rm $(IMAGE_NAME) cargo testcrates/grida-canvas/src/window/fps.rs (2)
20-30: Consider adding error handling for font loading failures.The font loading logic could fail silently if both Arial and default font matching fail. Consider adding logging or fallback strategies.
static FONT: Font = { let font_mgr = FontMgr::new(); let typeface = font_mgr .match_family_style("Arial", skia_safe::FontStyle::default()) .or_else(|| font_mgr.match_family_style("", skia_safe::FontStyle::default())); match typeface { Some(tf) => Font::new(tf, 36.0), - None => Font::default(), + None => { + eprintln!("Warning: Failed to load font, using default"); + Font::default() + }, } };
33-51: Consider making the FPS meter position and size configurable.The fixed positioning and sizing (rect at 10,10 with 180x60 size) may not work well with different window sizes or UI layouts.
Consider adding parameters to the
drawmethod for position and size:-pub fn draw(surface: &mut Surface, fps: f32) { +pub fn draw(surface: &mut Surface, fps: f32, position: Option<Point>, size: Option<(f32, f32)>) { let canvas = surface.canvas(); - let rect = Rect::from_xywh(10.0, 10.0, 180.0, 60.0); + let pos = position.unwrap_or(Point::new(10.0, 10.0)); + let (width, height) = size.unwrap_or((180.0, 60.0)); + let rect = Rect::from_xywh(pos.x, pos.y, width, height);crates/grida-canvas/examples/nested.rs (1)
26-83: Consider simplifying the nesting logic and adding configuration options.The alternating group/container logic is complex and uses hard-coded values. Consider making it more configurable and easier to understand.
+// Configuration for the demo +const NESTING_LEVELS: usize = 5; +const BASE_OFFSET: f32 = 50.0; +const CONTAINER_OFFSET: f32 = -30.0; +const RECT_SIZE: f32 = 100.0; // Create nested structure -for i in 0..n { +for i in 0..NESTING_LEVELS { if i % 2 == 0 { // Create group with rotation transform let mut group = nf.create_group_node(); group.base.name = format!("Group {}", i); group.transform = AffineTransform::new( - 50.0 * (i as f32 + 1.0), // x offset - 50.0 * (i as f32 + 1.0), // y offset + BASE_OFFSET * (i as f32 + 1.0), // x offset + BASE_OFFSET * (i as f32 + 1.0), // y offset 0.0, );This makes the example more maintainable and easier to modify.
crates/grida-canvas/src/window/hit_overlay.rs (1)
3-37: Consider extracting common overlay utilities to reduce code duplication.The thread-local statics pattern is repeated from the FPS meter with slight variations (different font sizes). Consider creating a shared overlay utility module.
Create a shared
overlay_utils.rsmodule:// In overlay_utils.rs thread_local! { pub static OVERLAY_BG_PAINT: Paint = { let mut p = Paint::default(); p.set_color(Color::from_argb(160, 0, 0, 0)); p.set_anti_alias(true); p }; pub static OVERLAY_TEXT_PAINT: Paint = { let mut p = Paint::default(); p.set_color(Color::WHITE); p.set_anti_alias(true); p }; } pub fn create_overlay_font(size: f32) -> Font { let font_mgr = FontMgr::new(); let typeface = font_mgr .match_family_style("Arial", skia_safe::FontStyle::default()) .or_else(|| font_mgr.match_family_style("", skia_safe::FontStyle::default())); match typeface { Some(tf) => Font::new(tf, size), None => Font::default(), } }AGENTS.md (1)
83-83: Fix the spelling of Next.js framework name.The official spelling is "Next.js" with proper capitalization.
-The editor is a monorepo nextjs project that contains the codebase for the editor. +The editor is a monorepo Next.js project that contains the codebase for the editor.crates/grida-canvas/examples/fonts.rs (1)
119-119: Consider parameterizing the output filename.The hardcoded output filename
"text_output.png"could be made configurable for better reusability.+ let output_filename = std::env::args().nth(1).unwrap_or_else(|| "text_output.png".to_string()); - std::fs::write("text_output.png", bytes).unwrap(); + std::fs::write(&output_filename, bytes).unwrap();crates/grida-canvas/examples/json.rs (1)
36-36: Consider making background color configurable.The hardcoded background color
Color(230, 230, 230, 255)could be made configurable or derived from the parsed scene data.- background_color: Some(Color(230, 230, 230, 255)), + background_color: scene.background_color.or(Some(Color(230, 230, 230, 255))),crates/grida-canvas/src/window/stats_overlay.rs (1)
41-45: Consider making overlay dimensions configurable.The hardcoded values for positioning and sizing could be made configurable for better reusability across different window sizes.
+pub struct StatsOverlayConfig { + pub x: f32, + pub y: f32, + pub width: f32, + pub line_height: f32, + pub padding: f32, +} + +impl Default for StatsOverlayConfig { + fn default() -> Self { + Self { + x: 10.0, + y: 130.0, + width: 600.0, + line_height: 20.0, + padding: 10.0, + } + } +} impl StatsOverlay { - pub fn draw(surface: &mut Surface, stats: &str) { + pub fn draw(surface: &mut Surface, stats: &str) { + Self::draw_with_config(surface, stats, &StatsOverlayConfig::default()) + } + + pub fn draw_with_config(surface: &mut Surface, stats: &str, config: &StatsOverlayConfig) {crates/grida-canvas/src/cache/README.md (1)
9-9: Fix grammatical error.The phrase "independent from" should be "independent of" according to standard English usage.
-The Geometry Cache exists to store and maintain computed geometric properties of nodes in a scene, independent from the user-authored structure. It enables fast rendering, culling, and future hit-testing by treating transform and bounds as first-class cached data. +The Geometry Cache exists to store and maintain computed geometric properties of nodes in a scene, independent of the user-authored structure. It enables fast rendering, culling, and future hit-testing by treating transform and bounds as first-class cached data.crates/grida-canvas/src/window/ruler.rs (1)
49-58: Consider extracting constants for better maintainability.The hardcoded step values and minimum pixel distance could be extracted as module-level constants for easier maintenance and potential future configurability.
+const TICK_STEPS: [f32; 12] = [ + 1.0, 2.0, 5.0, 10.0, 25.0, 50.0, 100.0, 250.0, 500.0, 1000.0, 2500.0, 5000.0, +]; +const MIN_PIXELS_PER_TICK: f32 = 50.0; + impl Ruler { pub fn draw(surface: &mut Surface, camera: &Camera2D) { - const STEPS: [f32; 12] = [ - 1.0, 2.0, 5.0, 10.0, 25.0, 50.0, 100.0, 250.0, 500.0, 1000.0, 2500.0, 5000.0, - ]; - const MIN_PX_PER_TICK: f32 = 50.0; let zoom = camera.get_zoom(); - let step = STEPS + let step = TICK_STEPS .iter() - .find(|s| **s * zoom >= MIN_PX_PER_TICK) + .find(|s| **s * zoom >= MIN_PIXELS_PER_TICK) .copied() - .unwrap_or_else(|| STEPS[STEPS.len() - 1]); + .unwrap_or_else(|| TICK_STEPS[TICK_STEPS.len() - 1]);crates/grida-canvas/src/hit_test.rs (2)
37-50: Complete the TODO for precise path hit testing.While the current bounding box approach works for basic hit testing, the TODO comment indicates missing path-level precision testing. This could lead to false positives for complex shapes with transparent areas.
Consider implementing path-based hit testing for more accurate results, especially for shapes with corner radius, strokes, or complex geometries.
Would you like me to help implement path-level hit testing or create an issue to track this enhancement?
38-39: Consider optimizing repeated spatial queries.Both
hit_firstandhitsmethods callintersects_pointandsort()with similar logic. For scenarios with frequent hit testing, consider caching or combining these operations.Consider extracting the common spatial query logic:
+ fn get_sorted_intersecting_indices(&self, point: Vector2) -> Vec<usize> { + let mut indices = self.cache.intersects_point(point); + indices.sort(); + indices + } + pub fn hit_first(&self, point: Vector2) -> Option<NodeId> { - let mut indices = self.cache.intersects_point(point); - indices.sort(); + let indices = self.get_sorted_intersecting_indices(point); for idx in indices.into_iter().rev() {Also applies to: 57-58
crates/grida-canvas/examples/paint.rs (1)
20-23: Extract magic numbers to named constants.The spacing, positioning, and sizing values are hardcoded throughout the function, making it difficult to maintain and adjust the layout.
+const SPACING: f32 = 100.0; +const START_X: f32 = 50.0; +const BASE_SIZE: f32 = 80.0; +const ITEMS_PER_ROW: usize = 10; + async fn demo_paints() -> Scene { // ... - let spacing = 100.0; - let start_x = 50.0; - let base_size = 80.0; - let items_per_row = 10;crates/grida-canvas/tests/render_bounds.rs (1)
1-90: Consider adding tests for combined effects.The current tests cover individual effects well. Consider adding test cases for nodes with multiple effects applied simultaneously to ensure bounds calculation handles effect composition correctly.
Example additional test:
#[test] fn combined_effects_render_bounds() { // Test rectangle with both stroke and drop shadow // Test rectangle with blur and stroke // Verify bounds account for all effects }crates/grida-canvas/examples/tiling.rs (2)
21-21: Consider graceful error handling instead of unwrap().The
unwrap()call could cause the program to panic if picture recording fails. Consider using proper error handling or providing more context.- let picture = recorder.finish_recording_as_picture(None).unwrap(); + let picture = recorder.finish_recording_as_picture(None) + .expect("Failed to create picture from recorder");
37-40: Consider adding bounds checking for color values.The color calculations could potentially overflow for larger grid sizes, though it's safe for the current 10x10 grid.
- let r = (i * 25) as u8; - let g = (j * 25) as u8; - let b = ((i + j) * 12) as u8; + let r = ((i * 25) % 256) as u8; + let g = ((j * 25) % 256) as u8; + let b = (((i + j) * 12) % 256) as u8;crates/grida-canvas/examples/texts.rs (3)
34-34: Address TODO comments for italic text style support.Multiple TODO comments indicate missing italic support in the text style. This should be tracked and implemented to complete the text styling features.
Would you like me to create an issue to track the implementation of italic text style support?
Also applies to: 63-63, 85-85, 107-107
166-167: Consider using environment variables or configuration for font paths.Hardcoded font paths make the example less portable and harder to maintain.
- let caveat_font_path = "resources/Caveat-VariableFont_wght.ttf".to_string(); - let vt323_font_path = "resources/VT323-Regular.ttf".to_string(); + let caveat_font_path = std::env::var("CAVEAT_FONT_PATH") + .unwrap_or_else(|_| "resources/Caveat-VariableFont_wght.ttf".to_string()); + let vt323_font_path = std::env::var("VT323_FONT_PATH") + .unwrap_or_else(|_| "resources/VT323-Regular.ttf".to_string());
30-46: Consider extracting text style creation into helper functions.The text style creation is repetitive. Consider creating helper functions to reduce code duplication and improve maintainability.
fn create_text_style( font_family: &str, font_size: f32, font_weight: u16, decoration: TextDecoration, transform: TextTransform, ) -> TextStyle { TextStyle { text_decoration: decoration, font_family: font_family.to_string(), font_size, italic: false, font_weight: FontWeight::new(font_weight), letter_spacing: None, line_height: None, text_transform: transform, } }Also applies to: 59-70, 81-92, 103-118
crates/grida-canvas/examples/images.rs (2)
9-9: Consider making the image URL configurable.The hardcoded image URL makes the example less flexible and could fail if the URL becomes unavailable.
- let image_url = "https://grida.co/images/abstract-placeholder.jpg".to_string(); + let image_url = std::env::var("DEMO_IMAGE_URL") + .unwrap_or_else(|_| "https://grida.co/images/abstract-placeholder.jpg".to_string());
27-32: Consider extracting common ImagePaint configuration.The ImagePaint configuration is duplicated across multiple rectangles. Consider creating a helper function.
fn create_image_paint(image_url: &str, fit: BoxFit, transform: AffineTransform) -> Paint { Paint::Image(ImagePaint { _ref: image_url.to_string(), opacity: 1.0, transform, fit, }) }Also applies to: 47-52
crates/grida-canvas/examples/gradients.rs (2)
19-23: Consider extracting layout constants.The hardcoded layout values make the code less maintainable. Consider defining them as constants or making them configurable.
+ const SPACING: f32 = 160.0; + const START_X: f32 = 60.0; + const RECT_SIZE: f32 = 120.0; + const GRADIENT_COUNT: usize = 5; + - let spacing = 160.0; - let start_x = 60.0; - let base = 120.0;
25-51: Consider extracting gradient creation logic into helper functions.The four loops have very similar structure with only minor variations. Consider creating helper functions to reduce code duplication.
fn create_gradient_rectangles( nf: &NodeFactory, repository: &mut NodeRepository, base_name: &str, y_position: f32, gradient_type: GradientType, paint_target: PaintTarget, ) -> Vec<String> { // Common logic for creating gradient rectangles } enum GradientType { Linear, Radial, } enum PaintTarget { Fill, Stroke, }Also applies to: 53-82, 84-116, 118-152
crates/grida-canvas/examples/cache_text_picture.rs (1)
85-90: Consider extracting warmup iterations as a constant.For consistency with the
FRAMESconstant, consider extracting the warmup iteration count.+const WARMUP_FRAMES: usize = 20; + // Warmup phase -for _ in 0..20 { +for _ in 0..WARMUP_FRAMES { - // Increased warmup iterations run(true, &mut surface, &fc, &pic, &texts); run(false, &mut surface, &fc, &pic, &texts); }crates/grida-canvas/examples/100k.rs (1)
14-15: Consider making shape parameters configurable.For better reusability, consider making shape size and spacing configurable parameters.
-async fn demo_n_shapes(n: usize) -> Scene { +async fn demo_n_shapes(n: usize, shape_size: f32, spacing: f32) -> Scene { let nf = NodeFactory::new(); let mut repository = NodeRepository::new(); let mut all_shape_ids = Vec::new(); - // Grid parameters - let shape_size = 100.0; // Fixed size of 100x100 per shape - let spacing = 10.0; // Space between shapesThen update the call in main:
- let scene = demo_n_shapes(100000).await; + let scene = demo_n_shapes(100000, 100.0, 10.0).await;crates/grida-math2/src/align.rs (1)
32-44: Consider early termination optimization for exact matches.For performance optimization, consider early termination when an exact match is found (distance = 0).
for (i, &t) in targets.iter().enumerate() { let signed = point - t; let abs = signed.abs(); + if abs == 0.0 { + return (t, 0.0, vec![i]); + } if abs < min_abs { min_abs = abs; best_value = t; best_signed = signed; indices.clear(); indices.push(i); } else if abs == min_abs { indices.push(i); } }crates/grida-canvas/src/cache/picture.rs (1)
52-54: Consider addingis_empty()method for completeness.For API consistency and usability, consider adding an
is_empty()method alongside the existinglen()method.pub fn len(&self) -> usize { self.node_pictures.len() } + pub fn is_empty(&self) -> bool { + self.node_pictures.is_empty() + }crates/grida-canvas/tests/geometry_cache.rs (1)
10-13: Consider extracting common test setup.Both tests have similar setup code for creating rectangles and repositories. Consider extracting this into a helper function to reduce duplication.
fn create_test_rectangle(nf: &NodeFactory, x: f32, y: f32, width: f32, height: f32) -> (Node, NodeId) { let mut rect = nf.create_rectangle_node(); rect.transform = AffineTransform::new(x, y, 0.0); rect.size = Size { width, height }; let id = rect.base.id.clone(); (Node::Rectangle(rect), id) }crates/grida-canvas/examples/effects.rs (3)
85-87: Fix incorrect color comment.The comment says "White" but the color value
Color(200, 200, 200, 255)is actually light gray, not white.- color: Color(200, 200, 200, 255), // White + color: Color(200, 200, 200, 255), // Light gray
104-106: Fix incorrect color comment.The comment says "White" but the color value
Color(200, 200, 200, 255)is actually light gray, not white.- color: Color(200, 200, 200, 255), // White + color: Color(200, 200, 200, 255), // Light gray
25-71: Consider refactoring to reduce code duplication.The three effect sections (drop shadow, gaussian blur, backdrop blur) have very similar structure with only minor variations. Consider extracting a helper function to reduce duplication and improve maintainability.
+fn create_effect_row<F>( + nf: &NodeFactory, + repository: &mut NodeRepository, + effect_name: &str, + y_position: f32, + start_x: f32, + spacing: f32, + base_size: f32, + base_color: Color, + effect_factory: F, +) -> Vec<String> +where + F: Fn(usize) -> FilterEffect, +{ + let mut effect_ids = Vec::new(); + for i in 0..6 { + // Common shape creation logic... + effect_ids.push(shape.base.id.clone()); + repository.insert(shape); + } + effect_ids +}Also applies to: 73-114, 146-186
crates/grida-canvas/examples/shapes.rs (1)
167-167: Fix incorrect comment about inner radius range.The calculation
0.7 - (i as f32 * 0.05)with i from 0 to 9 produces values from 0.7 to 0.25, not "0.3 to 0.75" as stated in the comment.- star.inner_radius = 0.7 - (i as f32 * 0.05); // 0.3 to 0.75 inner radius + star.inner_radius = 0.7 - (i as f32 * 0.05); // 0.7 to 0.25 inner radiuscrates/grida-canvas/examples/webfonts.rs (1)
63-81: Consider extracting font variants as a constant.The variants array is quite large and could be extracted as a module-level constant for better readability and reusability.
+const ALBERT_SANS_VARIANTS: &[(&str, u16, bool)] = &[ + ("Regular", 400, false), + ("Thin", 100, false), + // ... rest of variants +]; - let variants = [ - ("Regular", 400, false), - // ... variants array - ]; + let variants = ALBERT_SANS_VARIANTS;crates/grida-canvas/src/node/repository.rs (1)
22-36: Consider extracting node ID retrieval into a helper method.The match statement is repetitive. Since all node variants have a
base.idfield, consider adding a method to theNodeenum.Add a method to the
Nodeenum in the schema module:impl Node { pub fn id(&self) -> &NodeId { match self { Node::Error(n) => &n.base.id, Node::Group(n) => &n.base.id, Node::Container(n) => &n.base.id, Node::Rectangle(n) => &n.base.id, Node::Ellipse(n) => &n.base.id, Node::Polygon(n) => &n.base.id, Node::RegularPolygon(n) => &n.base.id, Node::RegularStarPolygon(n) => &n.base.id, Node::Line(n) => &n.base.id, Node::TextSpan(n) => &n.base.id, Node::Path(n) => &n.base.id, Node::BooleanOperation(n) => &n.base.id, Node::Image(n) => &n.base.id, } } }Then simplify the insert method:
- let id = match &node { - Node::Error(n) => n.base.id.clone(), - Node::Group(n) => n.base.id.clone(), - Node::Container(n) => n.base.id.clone(), - Node::Rectangle(n) => n.base.id.clone(), - Node::Ellipse(n) => n.base.id.clone(), - Node::Polygon(n) => n.base.id.clone(), - Node::RegularPolygon(n) => n.base.id.clone(), - Node::RegularStarPolygon(n) => n.base.id.clone(), - Node::Line(n) => n.base.id.clone(), - Node::TextSpan(n) => n.base.id.clone(), - Node::Path(n) => n.base.id.clone(), - Node::BooleanOperation(n) => n.base.id.clone(), - Node::Image(n) => n.base.id.clone(), - }; + let id = node.id().clone();crates/grida-math2/src/color.rs (1)
82-82: Fix typo in function name.The function name contains a typo: "unit8" should be "uint8".
-pub fn rgba_to_unit8_chunk(rgba: RGBA8888) -> Vector4 { +pub fn rgba_to_uint8_chunk(rgba: RGBA8888) -> Vector4 {crates/grida-canvas/src/painter/cvt.rs (1)
80-100: Remove commented code.The
sk_paint_with_strokefunction is commented out and appears to be unused.Consider removing this commented code to improve code cleanliness, or uncomment it if it's intended for future use.
crates/grida-math2/src/range.rs (1)
56-112: Consider the exponential complexity for large inputs.The implementation correctly uses powerset search and documents the exponential complexity. While this is acceptable for small inputs, it could become problematic for larger datasets.
For production use with potentially large inputs, consider:
- Adding an upper bound check on the input size
- Implementing a heuristic or approximation algorithm for large inputs
- Adding a timeout mechanism to prevent excessive computation time
crates/grida-math2/src/bezier.rs (1)
79-94: Simplify min/max calculations using iterator methods.The manual min/max tracking can be simplified for better readability.
-let mut min_x = f32::INFINITY; -let mut min_y = f32::INFINITY; -let mut max_x = f32::NEG_INFINITY; -let mut max_y = f32::NEG_INFINITY; - -for t in candidates { - let x = cubic_eval(a[0], c1[0], c2[0], b[0], t); - let y = cubic_eval(a[1], c1[1], c2[1], b[1], t); - if x < min_x { - min_x = x; - } - if x > max_x { - max_x = x; - } - if y < min_y { - min_y = y; - } - if y > max_y { - max_y = y; - } -} +let points: Vec<(f32, f32)> = candidates + .into_iter() + .map(|t| (cubic_eval(a[0], c1[0], c2[0], b[0], t), cubic_eval(a[1], c1[1], c2[1], b[1], t))) + .collect(); + +let min_x = points.iter().map(|(x, _)| *x).fold(f32::INFINITY, f32::min); +let max_x = points.iter().map(|(x, _)| *x).fold(f32::NEG_INFINITY, f32::max); +let min_y = points.iter().map(|(_, y)| *y).fold(f32::INFINITY, f32::min); +let max_y = points.iter().map(|(_, y)| *y).fold(f32::NEG_INFINITY, f32::max);crates/grida-canvas/src/cache/geometry.rs (2)
366-383: Improve error handling for SVG path parsing.The function returns a zero-sized rectangle when SVG parsing fails, which could hide parsing errors.
Consider logging failed SVG parsing for debugging:
fn path_bounds(data: &str) -> Rectangle { if let Some(path) = skia_safe::path::Path::from_svg(data) { let b = path.compute_tight_bounds(); Rectangle { x: b.left(), y: b.top(), width: b.width(), height: b.height(), } } else { + eprintln!("Warning: Failed to parse SVG path data"); Rectangle { x: 0.0, y: 0.0, width: 0.0, height: 0.0, } } }
338-364: Simplify polygon bounds calculation using iterator methods.The manual min/max tracking can be simplified for better readability and performance.
fn polygon_bounds(points: &[crate::node::schema::Point]) -> Rectangle { - let mut min_x = f32::INFINITY; - let mut min_y = f32::INFINITY; - let mut max_x = f32::NEG_INFINITY; - let mut max_y = f32::NEG_INFINITY; - for p in points { - min_x = min_x.min(p.x); - min_y = min_y.min(p.y); - max_x = max_x.max(p.x); - max_y = max_y.max(p.y); - } if points.is_empty() { Rectangle { x: 0.0, y: 0.0, width: 0.0, height: 0.0, } } else { + let (min_x, max_x) = points.iter() + .map(|p| p.x) + .fold((f32::INFINITY, f32::NEG_INFINITY), |(min, max), x| (min.min(x), max.max(x))); + let (min_y, max_y) = points.iter() + .map(|p| p.y) + .fold((f32::INFINITY, f32::NEG_INFINITY), |(min, max), y| (min.min(y), max.max(y))); Rectangle { x: min_x, y: min_y, width: max_x - min_x, height: max_y - min_y, } } }crates/grida-canvas/examples/cache_picture.rs (2)
300-305: Remove duplicate GPU flush call.The code calls
flush_and_submit()twice in succession, which is unnecessary.// Flush GPU commands if let Some(mut gr_ctx) = surface.recording_context() { if let Some(mut direct_ctx) = gr_ctx.as_direct_context() { direct_ctx.flush_and_submit(); - // Wait for GPU to finish using flush_and_submit - direct_ctx.flush_and_submit(); } }
273-279: Improve surface pointer management during resize.The surface pointer management during resize could be clearer to avoid confusion.
Consider updating the original surface_ptr instead of creating a shadowed local variable:
-.expect("Could not create skia surface"); -unsafe { _ = Box::from_raw(surface_ptr) }; -let new_surface_ptr = Box::into_raw(Box::new(new_surface)); -// SAFETY: update surface pointer -let surface = unsafe { &mut *new_surface_ptr }; -// Optionally, re-record the scene if you want to match new size -// scene = CachedScene::new(size.width as f32, size.height as f32); +.expect("Could not create skia surface"); +// Drop the old surface and update the pointer +unsafe { + _ = Box::from_raw(surface_ptr); + surface_ptr = Box::into_raw(Box::new(new_surface)); +} +// Optionally, re-record the scene if you want to match new size +// scene = CachedScene::new(size.width as f32, size.height as f32);Note: This requires making
surface_ptrmutable in the destructuring assignment.crates/grida-canvas/src/runtime/camera.rs (1)
37-39: Remove duplicate comment.Line 38 duplicates the comment from line 37.
/// Create with identity transform + no zoom (1:1). - /// Create with identity transform + no zoom (1:1) using default zoom limits. pub fn new(viewport_size: Size) -> Self {crates/docs/optimization.md (2)
63-77: Fix loose punctuation formatting.The property list has inconsistent punctuation that affects readability.
- - Properties include: - - - `depth`: + - Properties include: + - **depth**: - `0` → Entire scene is one cache. - `1` → Cache per top-level container. - `n` → Cache at depth `n`, chunking deeper layers. - - - `mode`: `AlwaysPicture`, `Hybrid`, `AlwaysTile` - - - `tile_size`, `tile_padding` - - - `zoom_threshold_for_tiles` - - - `frame_budget_threshold_ms` - - - `use_bbh`, `enable_lod`, etc. + - **mode**: `AlwaysPicture`, `Hybrid`, `AlwaysTile` + - **tile_size**, **tile_padding** + - **zoom_threshold_for_tiles** + - **frame_budget_threshold_ms** + - **use_bbh**, **enable_lod**, etc.
158-161: Track the mipmap selection TODO.The TODO indicates that the current mipmap selection strategy is temporary and should be improved with proper cache invalidation based on zoom.
Would you like me to create an issue to track implementing the proper zoom-based cache invalidation mechanism for mipmap selection?
crates/grida-canvas/src/webfont_helper.rs (3)
64-76: Consider making the metadata URL configurable.The GitHub URL is hard-coded. Consider making it configurable through an environment variable or configuration parameter for flexibility.
+const DEFAULT_WEBFONTS_METADATA_URL: &str = "https://raw.githubusercontent.com/gridaco/google.fonts/refs/heads/main/metadata/webfonts.metadata.json"; + pub async fn load_webfonts_metadata() -> Result<Value, String> { - let url = "https://raw.githubusercontent.com/gridaco/google.fonts/refs/heads/main/metadata/webfonts.metadata.json"; + let url = std::env::var("WEBFONTS_METADATA_URL") + .unwrap_or_else(|_| DEFAULT_WEBFONTS_METADATA_URL.to_string()); let response = reqwest::get(url)
78-82: Document WASM limitations.The WASM implementations return errors without attempting any functionality. If this is intentional (e.g., WASM builds handle fonts differently), it should be documented in the module-level documentation.
Add module-level documentation explaining the platform differences:
//! Web font helper module for loading font metadata and files. //! //! Note: WASM targets currently do not support direct HTTP fetching //! of font resources. Applications targeting WASM should use alternative //! methods for font loading, such as bundling fonts or using browser APIs.Also applies to: 165-169
88-125: Optimize repeated case-insensitive string comparisons.The function performs case-insensitive comparisons in nested loops. Consider pre-processing the metadata keys to lowercase for more efficient lookups.
For better performance with large font collections, consider creating a case-insensitive lookup map when loading the metadata, rather than performing linear searches with case conversion on each lookup.
crates/grida-canvas/src/io/io_figma.rs (2)
1068-1070: Extract hardcoded default values as named constants.The hardcoded values for star and polygon nodes should be defined as constants for better maintainability and discoverability.
Add these constants at the top of the file:
+/// Default number of points for star shapes when not provided by Figma API +const DEFAULT_STAR_POINT_COUNT: u32 = 5; +/// Default inner radius ratio for star shapes when not provided by Figma API +const DEFAULT_STAR_INNER_RADIUS: f32 = 0.4; +/// Default number of points for regular polygons when not provided by Figma API +const DEFAULT_POLYGON_POINT_COUNT: u32 = 3; const TRANSPARENT: Paint = Paint::Solid(SolidPaint { color: Color(0, 0, 0, 0), opacity: 0.0, });Then update the usages:
- point_count: 5, // Default to 5 points for a star - inner_radius: 0.4, // Default inner radius to 0.4 (40% of outer radius) + point_count: DEFAULT_STAR_POINT_COUNT, + inner_radius: DEFAULT_STAR_INNER_RADIUS,- // No count in api ? - point_count: 3, + point_count: DEFAULT_POLYGON_POINT_COUNT,Also applies to: 1183-1184
1290-1293: Add tests for the Figma converter.The test function is empty. Given the complexity of the conversion logic, comprehensive tests are essential.
Would you like me to generate unit tests for the key conversion functions, including edge cases for transforms, paints, and node types?
crates/grida-canvas/src/window/mod.rs (2)
551-551: Usedrop()for clarity when deallocating the Box.The current pattern works but using
drop()makes the intent clearer.- unsafe { _ = Box::from_raw(self.surface_ptr) }; + unsafe { drop(Box::from_raw(self.surface_ptr)) };
496-514: Improve frame statistics formatting for readability.The statistics string is very long and difficult to read. Consider using a multi-line format.
- let stat_string = format!( - "fps*: {:.0} | t: {:.2}ms | render: {:.1}ms | flush: {:.1}ms | overlays: {:.1}ms | frame: {:.1}ms | list: {:.1}ms ({:?}) | draw: {:.1}ms | $:pic: {:?} ({:?} use) | $:geo: {:?} | tiles: {:?} ({:?} use) | q: {:?} | z: {:?}", + let stat_string = format!( + "FPS: {:.0} | Frame: {:.2}ms\n\ + ├─ Render: {:.1}ms | Flush: {:.1}ms | Overlays: {:.1}ms\n\ + ├─ Display List: {:.1}ms (size: {:?}) | Draw: {:.1}ms\n\ + ├─ Cache - Pictures: {:?} (used: {:?}) | Geometry: {:?}\n\ + └─ Tiles: {:?} (used: {:?}) | Queue: {:?} | Sleep: {:?}",crates/grida-canvas/src/painter/painter.rs (1)
532-563: Remove commented code block.This large block of commented code should be removed. If needed for reference, it should be in version control history.
Remove lines 532-563 entirely.
crates/grida-canvas/src/painter/geometry.rs (2)
77-84: Log warning when dash pattern application fails.Dash pattern application can fail silently, leading to unexpected visual results.
if let Some(dashes) = stroke_dash_array { if let Some(pe) = PathEffect::dash(dashes, 0.0) { if let Some((dashed, _)) = pe.filter_path(source_path, &stroke_rec, source_path.bounds()) { path_to_stroke = dashed; + } else { + eprintln!("Warning: Failed to apply dash pattern to path"); } + } else { + eprintln!("Warning: Failed to create dash path effect"); } }
93-97: Add debug logging for failed path operations.Boolean path operations can fail but currently fall back silently, making debugging difficult.
Add debug logging when operations fail:
if let Some(result) = Path::op(&stroked_path, source_path, PathOp::Intersect) { result } else { + eprintln!("Debug: Path intersection failed for inside stroke"); stroked_path }if let Some(result) = Path::op(&stroked_path, source_path, PathOp::Difference) { result } else { + eprintln!("Debug: Path difference failed for outside stroke"); stroked_path }if let Some(merged) = Path::op(&result, &shape_path, (*operation).into()) { result = merged; + } else { + eprintln!("Debug: Boolean operation {:?} failed", operation); }Also applies to: 101-105, 332-334
crates/grida-canvas/src/painter/layer.rs (1)
573-573: Remove unnecessary empty match arm comment.The comment adds no value and should be removed for cleaner code.
- _ => {} // Skip other node types + _ => {}crates/grida-canvas/src/font_loader.rs (2)
119-130: Improve error handling consistency.The
fetch_font_datamethod returns errors differently between native and wasm builds. Consider making the error message more informative for wasm builds.#[cfg(target_arch = "wasm32")] - Err("Font loading not supported in wasm".into()) + Err(format!("Font loading not supported in wasm for path: {}", path).into())
157-167: Consider consolidating duplicate font loading functions.The standalone
load_fontfunctions duplicate logic from theFontLoadermethods. Consider removing these or having them use a temporaryFontLoaderinstance internally to avoid code duplication.#[cfg(not(target_arch = "wasm32"))] pub async fn load_font(path: &str) -> Result<Vec<u8>, Box<dyn std::error::Error>> { let mut loader = FontLoader::new_simple(); loader.fetch_font_data(path).await }crates/grida-canvas/src/runtime/scene.rs (2)
563-564: Avoid using string literals as node IDs in tests.Using raw strings as node IDs in tests could be fragile. Consider using a constant or the actual node factory to create proper IDs.
- let pic = renderer.with_recording_cached(&"missing".to_string(), |_| {}); + let missing_id = NodeId::from("test_missing_node"); + let pic = renderer.with_recording_cached(&missing_id, |_| {});
373-374: Add comment explaining why sorting is expensive.The TODO comment mentions sorting is expensive but doesn't explain why it's necessary or suggest alternatives.
- // TODO: sort is expensive + // TODO: sort is expensive - O(n log n) operation performed per region + // Consider using a pre-sorted spatial index or z-order curve for better performance indices.sort();crates/grida-canvas/src/cache/tile.rs (1)
334-340: Consider extracting zoom threshold check to reduce duplication.The zoom > max_zoom_for_cache check is duplicated in both
update_zoom(line 309) and here. Consider extracting this into a helper method likeis_caching_enabled(zoom: f32) -> bool.crates/grida-canvas/src/io/io_json.rs (1)
489-503: Consider adding more comprehensive test assertions.While the current test verifies basic parsing, consider adding assertions for specific node types, properties, and edge cases like the corner radius deserialization.
Would you like me to generate additional test cases to improve coverage?
crates/grida-canvas/src/node/schema.rs (1)
887-896: Consider documenting valid range for inner_radius.While the documentation explains the effect of inner_radius values, it doesn't specify the valid range. Consider documenting that typical values are between 0.0 and 1.0, where values > 1.0 may produce unexpected shapes.
crates/grida-canvas/examples/stroke.rs (2)
26-90: Consider refactoring repetitive shape creation code.The stroke alignment and width demo sections contain significant code duplication. Each shape creation follows the same pattern with only minor property variations.
Consider extracting a helper function to reduce duplication:
+fn create_demo_rectangle( + nf: &NodeFactory, + name: String, + transform: AffineTransform, + base_size: f32, + stroke_width: f32, + stroke_align: StrokeAlign, +) -> RectangleNode { + let mut rect = nf.create_rectangle_node(); + rect.base.name = name; + rect.transform = transform; + rect.size = Size { width: base_size, height: base_size }; + rect.corner_radius = RectangularCornerRadius::all(8.0); + rect.fill = Paint::Solid(SolidPaint { color: Color(0, 0, 0, 0), opacity: 1.0 }); + rect.stroke = Paint::Solid(SolidPaint { color: Color(0, 0, 0, 255), opacity: 1.0 }); + rect.stroke_width = stroke_width; + rect.stroke_align = stroke_align; + rect +}This would reduce the repetitive code in the alignment and width demo sections.
355-376: Misleading naming for solid stroke example.The section is titled "Multi-color Solid Stroke" but uses a single solid color (orange) with a dash pattern. This is not truly multi-color.
Consider updating the name to be more accurate:
- rect.base.name = "Multi-color Stroke".to_string(); + rect.base.name = "Dashed Solid Stroke".to_string();Alternatively, implement an actual multi-color stroke using gradients if that was the intended demonstration.
crates/grida-canvas/examples/srcset.rs (2)
14-16: Consider more graceful error handling.Using
expect()will cause the program to panic on file read or image decode failures. For a benchmark tool, more informative error messages might be helpful.Consider using proper error handling:
- let data = std::fs::read(image_path).expect("Failed to read image file"); - let original = - Image::from_encoded(skia_safe::Data::new_copy(&data)).expect("Failed to decode image"); + let data = std::fs::read(image_path) + .map_err(|e| format!("Failed to read image file '{}': {}", image_path, e))?; + let original = Image::from_encoded(skia_safe::Data::new_copy(&data)) + .ok_or_else(|| format!("Failed to decode image from '{}'", image_path))?;This would require changing the return type to
Result<Self, String>and handling the error inmain().
91-92: Consider extracting paint creation to reduce duplication.The paint creation with anti-aliasing is duplicated in both constraint test loops.
Consider extracting paint creation:
+ let mut paint = Paint::default(); + paint.set_anti_alias(true); + for _ in 0..iterations { // Test with Fast constraint canvas.clear(Color::WHITE); let start = Instant::now(); for i in 0..images_per_iteration { // ... grid calculation ... - let mut paint = Paint::default(); - paint.set_anti_alias(true);Apply the same change to the Strict constraint loop.
Also applies to: 115-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockcrates/fixtures/test-figma/766822741396935685.zipis excluded by!**/*.zipcrates/fixtures/test-figma/767108022358877208.zipis excluded by!**/*.zipcrates/fixtures/test-figma/767127152320102433.zipis excluded by!**/*.zipcrates/fixtures/test-figma/767135333966182437.zipis excluded by!**/*.zipcrates/fixtures/test-figma/767544357629075510.zipis excluded by!**/*.zipcrates/fixtures/test-figma/768233500885703748.zipis excluded by!**/*.zipcrates/fixtures/test-figma/768273489419236430.zipis excluded by!**/*.zipcrates/grida-canvas/Cargo.lockis excluded by!**/*.lockcrates/grida-canvas/cover.pngis excluded by!**/*.pngcrates/grida-canvas/resources/4k.jpgis excluded by!**/*.jpgcrates/grida-canvas/resources/8k.jpgis excluded by!**/*.jpgcrates/grida-canvas/resources/Bungee-Regular.ttfis excluded by!**/*.ttfcrates/grida-canvas/resources/Caveat-VariableFont_wght.ttfis excluded by!**/*.ttfcrates/grida-canvas/resources/Fruktur-Regular.ttfis excluded by!**/*.ttfcrates/grida-canvas/resources/VT323-Regular.ttfis excluded by!**/*.ttfpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (105)
.github/workflows/test.yml(2 hunks).gitignore(1 hunks).ref/figma/README.md(1 hunks).ref/figma/figma-rest-api.min.d.ts(1 hunks)AGENTS.md(6 hunks)Cargo.toml(1 hunks)crates/.gitignore(1 hunks)crates/Dockerfile(1 hunks)crates/Makefile(1 hunks)crates/docs/optimization.md(1 hunks)crates/grida-canvas/AGENTS.md(1 hunks)crates/grida-canvas/Cargo.toml(1 hunks)crates/grida-canvas/Makefile(1 hunks)crates/grida-canvas/README.md(1 hunks)crates/grida-canvas/benches/bench_mipmap.rs(1 hunks)crates/grida-canvas/benches/bench_rectangles.rs(1 hunks)crates/grida-canvas/examples/100k.rs(1 hunks)crates/grida-canvas/examples/basic.rs(1 hunks)crates/grida-canvas/examples/booleans.rs(1 hunks)crates/grida-canvas/examples/cache_image.rs(1 hunks)crates/grida-canvas/examples/cache_picture.rs(1 hunks)crates/grida-canvas/examples/cache_text_picture.rs(1 hunks)crates/grida-canvas/examples/camera.rs(1 hunks)crates/grida-canvas/examples/container.rs(1 hunks)crates/grida-canvas/examples/effects.rs(1 hunks)crates/grida-canvas/examples/figma.rs(1 hunks)crates/grida-canvas/examples/fonts.rs(1 hunks)crates/grida-canvas/examples/gradients.rs(1 hunks)crates/grida-canvas/examples/image.rs(1 hunks)crates/grida-canvas/examples/images.rs(1 hunks)crates/grida-canvas/examples/json.rs(1 hunks)crates/grida-canvas/examples/nested.rs(1 hunks)crates/grida-canvas/examples/paint.rs(1 hunks)crates/grida-canvas/examples/shapes.rs(1 hunks)crates/grida-canvas/examples/srcset.rs(1 hunks)crates/grida-canvas/examples/stroke.rs(1 hunks)crates/grida-canvas/examples/texts.rs(1 hunks)crates/grida-canvas/examples/tiling.rs(1 hunks)crates/grida-canvas/examples/webfonts.rs(1 hunks)crates/grida-canvas/examples/winit_raf.rs(1 hunks)crates/grida-canvas/package.json(1 hunks)crates/grida-canvas/resources/.gitignore(1 hunks)crates/grida-canvas/src/cache/README.md(1 hunks)crates/grida-canvas/src/cache/geometry.rs(1 hunks)crates/grida-canvas/src/cache/mod.rs(1 hunks)crates/grida-canvas/src/cache/picture.rs(1 hunks)crates/grida-canvas/src/cache/scene.rs(1 hunks)crates/grida-canvas/src/cache/tile.rs(1 hunks)crates/grida-canvas/src/font_loader.rs(1 hunks)crates/grida-canvas/src/hit_test.rs(1 hunks)crates/grida-canvas/src/image_loader.rs(1 hunks)crates/grida-canvas/src/io/io_figma.rs(1 hunks)crates/grida-canvas/src/io/io_json.rs(1 hunks)crates/grida-canvas/src/io/mod.rs(1 hunks)crates/grida-canvas/src/lib.rs(1 hunks)crates/grida-canvas/src/mipmap.rs(1 hunks)crates/grida-canvas/src/node/factory.rs(1 hunks)crates/grida-canvas/src/node/mod.rs(1 hunks)crates/grida-canvas/src/node/repository.rs(1 hunks)crates/grida-canvas/src/node/schema.rs(1 hunks)crates/grida-canvas/src/painter/cvt.rs(1 hunks)crates/grida-canvas/src/painter/geometry.rs(1 hunks)crates/grida-canvas/src/painter/layer.rs(1 hunks)crates/grida-canvas/src/painter/mod.rs(1 hunks)crates/grida-canvas/src/painter/painter.rs(1 hunks)crates/grida-canvas/src/repository.rs(1 hunks)crates/grida-canvas/src/resource_loader.rs(1 hunks)crates/grida-canvas/src/runtime/camera.rs(1 hunks)crates/grida-canvas/src/runtime/input.rs(1 hunks)crates/grida-canvas/src/runtime/mod.rs(1 hunks)crates/grida-canvas/src/runtime/scene.rs(1 hunks)crates/grida-canvas/src/text/mod.rs(1 hunks)crates/grida-canvas/src/text/text_transform.rs(1 hunks)crates/grida-canvas/src/webfont_helper.rs(1 hunks)crates/grida-canvas/src/window/fps.rs(1 hunks)crates/grida-canvas/src/window/hit_overlay.rs(1 hunks)crates/grida-canvas/src/window/mod.rs(1 hunks)crates/grida-canvas/src/window/ruler.rs(1 hunks)crates/grida-canvas/src/window/scheduler.rs(1 hunks)crates/grida-canvas/src/window/stats_overlay.rs(1 hunks)crates/grida-canvas/src/window/tile_overlay.rs(1 hunks)crates/grida-canvas/tests/camera.rs(1 hunks)crates/grida-canvas/tests/dashed_stroke.rs(1 hunks)crates/grida-canvas/tests/geometry_cache.rs(1 hunks)crates/grida-canvas/tests/hit_test.rs(1 hunks)crates/grida-canvas/tests/mipmap.rs(1 hunks)crates/grida-canvas/tests/render_bounds.rs(1 hunks)crates/grida-canvas/tests/resource_loader.rs(1 hunks)crates/grida-canvas/tests/scene_cache.rs(1 hunks)crates/grida-math2/Cargo.toml(1 hunks)crates/grida-math2/LICENSE(1 hunks)crates/grida-math2/README.md(1 hunks)crates/grida-math2/package.json(1 hunks)crates/grida-math2/src/align.rs(1 hunks)crates/grida-math2/src/bezier.rs(1 hunks)crates/grida-math2/src/box_fit.rs(1 hunks)crates/grida-math2/src/color.rs(1 hunks)crates/grida-math2/src/compass.rs(1 hunks)crates/grida-math2/src/delta.rs(1 hunks)crates/grida-math2/src/layout.rs(1 hunks)crates/grida-math2/src/lib.rs(1 hunks)crates/grida-math2/src/measurement.rs(1 hunks)crates/grida-math2/src/packing.rs(1 hunks)crates/grida-math2/src/range.rs(1 hunks)crates/grida-math2/src/raster.rs(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
.ref/figma/README.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
4-4: Bare URL used
null
(MD034, no-bare-urls)
🪛 checkmake (0.2.2)
crates/grida-canvas/Makefile
[warning] 8-8: Missing required phony target "all"
(minphony)
[warning] 8-8: Missing required phony target "clean"
(minphony)
[warning] 8-8: Missing required phony target "test"
(minphony)
crates/Makefile
[warning] 13-13: Target body for "extract" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 6-6: Missing required phony target "test"
(minphony)
🪛 LanguageTool
crates/grida-canvas/src/cache/README.md
[grammar] ~9-~9: The usual collocation for “independent” is “of”, not “from”. Did you mean “independent of”?
Context: ...ometric properties of nodes in a scene, independent from the user-authored structure. It enables...
(INDEPENDENTLY_FROM_OF)
AGENTS.md
[uncategorized] ~83-~83: The official spelling of this programming framework is “Next.js”.
Context: ...Very high The editor is a monorepo nextjs project that contains the codebase for ...
(NODE_JS)
crates/docs/optimization.md
[style] ~40-~40: Consider an alternative to strengthen your wording.
Context: ...ort, take snapshot. debounced (after no more changes. e.g. 150ms) - Divide the snapshot i...
(CHANGES_ADJUSTMENTS)
[uncategorized] ~63-~63: Loose punctuation mark.
Context: ... - Properties include: - depth: - 0 → Entire scene is one cac...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~69-~69: Loose punctuation mark.
Context: ..., chunking deeper layers. - mode: AlwaysPicture, Hybrid, AlwaysTile...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~71-~71: Loose punctuation mark.
Context: ...ybrid, AlwaysTile -tile_size, tile_padding -zoom_threshold_...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~77-~77: Loose punctuation mark.
Context: ...e_budget_threshold_ms -use_bbh, enable_lod`, etc. - Cache accessor...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~158-~158: Possible missing comma found.
Context: ...on projected screen size. - TODO: currently we select mipmap levels solely by the s...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~160-~160: The name of this brand needs to be capitalized.
Context: ...e invalidation mechanism based on zoom is introduced. --- ## Text & Glyph Op...
(ON_SKYPE)
[uncategorized] ~168-~168: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short).
Context: ...graph Caching)** - Cache rasterized or vector glyphs used across the document....
(COMMA_COMPOUND_SENTENCE_2)
🔇 Additional comments (130)
crates/grida-canvas/resources/.gitignore (1)
1-1: LGTM! Appropriate ignore rule for local development files.The addition of "local" to the .gitignore file is a sensible practice for excluding local development files from version control in the resources directory.
.gitignore (1)
202-205: LGTM! Essential ignore rules for Rust and Node.js.The addition of
/targetfor Rust build artifacts andnode-compile-cache/for Node.js compilation cache are standard and necessary ignore patterns for a hybrid project.crates/grida-math2/src/measurement.rs (6)
9-16: LGTM! Well-structured measurement result type.The
Measurementstruct is clearly documented and appropriately structured with input rectangles, reference rectangle, and distance array for the four directional measurements.
22-65: LGTM! Comprehensive rectangle relationship handling.The
measurefunction correctly handles all rectangle relationship scenarios:
- Identical rectangles (early return)
- Non-intersecting rectangles
- Container relationships (one contains the other)
- Partial intersections
The logic flow is clear and covers all edge cases appropriately.
67-74: LGTM! Correct intersecting spacing calculation.The calculation properly computes distances from the intersection rectangle to the outer bounds of both input rectangles using min/max operations for each direction.
76-92: LGTM! Proper non-intersecting spacing logic.The function correctly handles non-intersecting rectangles by checking horizontal and vertical separation independently, calculating the gap distances only for the relevant directions.
94-101: LGTM! Accurate container spacing calculation.The function properly calculates spacing from the inner rectangle to each edge of the outer container rectangle using absolute differences.
110-144: LGTM! Well-implemented guide line generation.The function correctly:
- Calculates rectangle center points
- Determines anchor points based on the specified side
- Applies zoom scaling consistently
- Returns appropriate rotation angles for each direction
The coordinate calculations and scaling are mathematically sound.
Cargo.toml (1)
1-7: LGTM! Proper Rust workspace configuration.The workspace setup is clean and appropriate:
- Resolver "3" is correct for modern Rust versions
- The three member crates align with the project's modular structure
- Configuration follows Rust workspace best practices
crates/grida-canvas/src/text/mod.rs (1)
1-1: LGTM! Proper module declaration.The addition of the
text_transformmodule declaration correctly exposes the text transformation functionality as a public module within the text hierarchy..github/workflows/test.yml (2)
1-1: Workflow renaming is clear
Renaming “Test” to “test packages” accurately reflects that this job runs package tests.
25-25: Exclude Rust crates from JS tests
Adding--filter='!./crates/*'cleanly separates Node.js package tests from Rust crate tests.crates/grida-canvas/src/node/mod.rs (1)
1-3: Public module exports look good
Exposingfactory,repository, andschemamodules aligns with crate architecture.crates/grida-math2/LICENSE (1)
1-22: MIT License inclusion is correct
The license text matches standard MIT terms and includes the proper copyright.crates/grida-canvas/src/cache/mod.rs (1)
1-4: Cache module exports are appropriate
Publicly exposinggeometry,picture,scene, andtilecaches fits the engine’s modular design.crates/grida-canvas/src/runtime/mod.rs (1)
1-3: Module declarations look good
Publicly exposingcamera,input, andscenealigns with the crate structure and Rust conventions.crates/grida-canvas/src/io/mod.rs (1)
1-2: IO module exports are correct
io_figmaandio_jsonare properly declared; no issues spotted.crates/grida-math2/Cargo.toml (1)
3-5: Verify crate naming consistency
The manifest setsname = "math2"while the directory isgrida-math2. Ensure this matches your workspace root and avoids collisions on crates.io.crates/grida-canvas/src/runtime/input.rs (1)
1-2: Import usage approved
Re-exportingVector2frommath2::vector2is clear and matches the dependency.crates/grida-canvas/AGENTS.md (2)
3-4: Good dependency documentation.Clear references to the key dependencies with proper links.
8-20: Standard development commands documented well.The testing and development section provides clear, standard Cargo commands for common development tasks.
crates/grida-canvas/src/painter/mod.rs (1)
1-5: Well-structured module organization.The module structure follows Rust conventions with a clear separation of concerns:
painter: Core painting functionality (re-exported)cvt: Conversion utilitiesgeometry: Geometric operationslayer: Layer managementThis organization supports maintainability and logical code separation.
crates/grida-canvas/README.md (3)
1-4: Good project branding and visual documentation.The README starts with a clear project name and includes a cover image to showcase the rendering capabilities.
7-10: Clear build instructions.Simple make commands for both debug and release builds make it easy for developers to get started.
16-58: Comprehensive feature roadmap.The checklist format clearly shows the scope and current implementation status of the rendering engine. This helps set expectations and provides a clear development roadmap covering:
- 2D node types (shapes, text, images)
- Visual effects (gradients, shadows, blend modes)
- API features (camera, resource loading)
- Interactivity (hit testing)
crates/.gitignore (1)
1-21: Comprehensive Rust project gitignore configuration.The gitignore file includes all the standard patterns for Rust projects:
- Build artifacts (
debug/,target/)- Backup files (
*.rs.bk)- Platform-specific files (
*.pdb)- Testing artifacts (
**/mutants.out*/)- IDE configuration (with helpful comments)
The comments are particularly helpful in explaining the purpose of each section.
crates/grida-canvas/benches/bench_mipmap.rs (4)
1-4: Proper benchmark dependencies and imports.The imports are clean and include all necessary components for the mipmap benchmark.
5-8: Good test data setup.Creating a 2048x2048 surface provides a realistic test case for mipmap generation performance, and using
image_snapshot()is the correct way to get the image data.
9-32: Well-structured benchmark comparison.The benchmark properly compares two mipmap generation strategies:
- Chained: Uses
chained: truefor sequential mipmap generation- Direct: Uses
chained: falsefor independent generationBoth use
MipmapLevels::FullChainfor consistency, and the benchmark correctly usesblack_box()to prevent compiler optimizations from affecting the results.
34-35: Correct Criterion macro usage.The
criterion_group!andcriterion_main!macros are used correctly to set up the benchmark runner.crates/grida-canvas/tests/dashed_stroke.rs (1)
5-15: Well-designed test for dashed stroke functionality.The test correctly verifies that dashed strokes produce more path segments than solid strokes. The test setup is minimal but sufficient, using a simple horizontal line to isolate the dashing behavior. The assertion logic is sound and will catch regressions in the stroke geometry generation.
crates/Dockerfile (2)
8-9: Comprehensive Emscripten configuration for WebAssembly builds.The EMCC_CFLAGS are well-configured for WebAssembly deployment with appropriate settings for error handling, WebGL 2 support, and modularization. The flags ensure proper WebAssembly module creation with the correct export name and runtime methods.
14-16: Standard WebAssembly build process.The build process correctly sources the Emscripten environment and builds the project in release mode targeting WebAssembly. The workflow is appropriate for containerized builds.
crates/grida-math2/README.md (2)
27-36: Well-crafted usage example.The example code demonstrates the crate's functionality clearly with proper Rust syntax. The example shows practical usage of rectangle transformation with affine transforms, which is a common use case for 2D graphics libraries.
38-44: Thoughtful API design considerations.The future improvements section shows good awareness of Rust idioms and potential API enhancements. The suggestions for implementing standard traits and adding methods to types align with Rust best practices.
crates/grida-canvas/tests/hit_test.rs (1)
6-53: Comprehensive hit test validation.The test properly validates hit testing functionality with a well-structured scene graph. It correctly:
- Sets up a scene with nested container and rectangle nodes
- Updates both geometry and layer caches as required
- Tests hit detection at multiple points including edge cases
- Verifies both positive hits (rectangle, container) and negative hits (outside bounds)
The test design ensures the hit tester correctly identifies the topmost node under various coordinates.
crates/grida-canvas/Makefile (1)
1-10: Well-configured WebAssembly build automation.The Makefile provides good build automation with comprehensive Emscripten flags and proper SDK management via asdf. The build configuration matches the Dockerfile setup and supports both debug and release builds.
crates/grida-math2/src/compass.rs (1)
1-27: LGTM! Clean and correct implementation.The compass utility functions are well-implemented with:
- Exhaustive pattern matching covering all 8 cardinal directions
- Clear documentation explaining the behavior
- Correct logic for direction inversion and rectangle side mapping
- Proper handling of diagonal directions in
to_rectangle_sidecrates/grida-math2/src/delta.rs (1)
16-23: Verify the delta transformation logic.The current implementation applies both scaling and translation (
row[i] * offset + row[2]), but delta transformations typically should only apply scaling and rotation, not translation.For a delta (offset) transformation, you usually want to transform the direction and magnitude of the change, not apply the translation component. Consider if this should be:
pub fn transform(offset: f32, axis: Axis, transform: &AffineTransform) -> f32 { let i = match axis { Axis::X => 0, Axis::Y => 1, }; let row = transform.matrix[i]; - row[i] * offset + row[2] + row[i] * offset // Only apply scaling, not translation }Please verify if the translation component should be included for delta transformations in your use case.
crates/grida-canvas/src/text/text_transform.rs (3)
26-48: Excellent implementation with comprehensive logic.The text transformation function handles all CSS text-transform cases correctly with efficient string handling. The capitalize implementation properly treats non-alphanumeric characters as word boundaries, which aligns with CSS behavior.
37-37: Consider edge case handling for Unicode normalization.While unlikely, if
to_uppercase()returns an empty iterator for certain Unicode characters, theunwrap_or(c)will fall back to the original character, which is correct behavior. The implementation is robust.
50-122: Comprehensive test coverage with excellent edge cases.The test suite covers all transformation modes and includes important edge cases like multiple spaces, punctuation separators, and mixed case inputs. This ensures the capitalize logic works correctly across different word boundary scenarios.
crates/grida-canvas/examples/image.rs (1)
30-35: Good use of BoxFit::Cover for image scaling.The Cover fit mode ensures the image maintains its aspect ratio while filling the container, which is the appropriate choice for this full-size image display example.
crates/grida-canvas/examples/container.rs (2)
14-35: Well-designed container with comprehensive styling.The container setup demonstrates multiple features effectively: positioning, rounded corners, fill/stroke styling, drop shadow effects, and clipping. The styling choices create a visually appealing red container.
40-53: Excellent positioning for clipping demonstration.The ellipse is positioned at (100, 150) with height 200, extending to y=350, which exceeds the container's bounds (y=100 to y=400). This positioning effectively demonstrates the clipping behavior when
container.clip = true.crates/Makefile (1)
1-4: Well-organized variable definitions.The variable definitions are clear and make the Makefile easily configurable for different environments or target configurations.
crates/grida-canvas/src/window/tile_overlay.rs (3)
26-38: Correct coordinate transformation and stroke handling.The implementation properly transforms tile coordinates from canvas space to screen space using the camera's view matrix. The stroke width adjustment (half-width offset and full-width subtraction) ensures the green outline renders within the tile boundaries without visual artifacts.
15-17: Good early optimization for empty tile sets.The early return when tiles are empty prevents unnecessary canvas and paint setup, improving performance when no tiles need visualization.
20-25: Appropriate Skia paint configuration for debug overlay.The paint setup uses green color (0x00FF00) with stroke style and anti-aliasing enabled, which provides clear visual distinction for tile boundaries during debugging without interfering with the main rendered content.
crates/grida-canvas/src/lib.rs (1)
1-16: Excellent modular organization with appropriate conditional compilation.The module structure is well-organized and the conditional compilation for the
windowmodule on non-WASM targets is a smart approach for a graphics library that needs to support different deployment environments.crates/grida-canvas/examples/nested.rs (1)
7-25: Good example structure with proper async patterns.The async function properly demonstrates scene graph construction with the repository pattern. The initial rectangle setup is clear and well-structured.
crates/grida-canvas/src/window/hit_overlay.rs (1)
41-64: Good use of early return pattern for optional rendering.The Option pattern with early return is clean and efficient. The overlay rendering logic is straightforward and well-structured.
AGENTS.md (2)
19-19: Excellent documentation of the new Rust crates integration.The addition of the
/cratesdirectory documentation clearly explains its purpose and importance in the project architecture. The high importance rating and description of its role as the new rendering backend is well-documented.Also applies to: 71-78
168-193: Comprehensive testing and development commands documentation.The updated commands properly cover both the existing Node.js/TypeScript toolchain and the new Rust crates, with clear separation and appropriate filters for different parts of the monorepo.
crates/grida-canvas/tests/scene_cache.rs (1)
7-66: Test implementation looks correct and well-structured.The test properly validates spatial intersection queries for partially visible nested nodes. The geometry setup and assertions align correctly:
- Rectangle at (50,50) with size 100x100 spans (50,50) to (150,150)
- First query (140,40) to (160,60) correctly expects 1 layer (rectangle only)
- Second query (50,0) to (150,100) correctly expects 2 layers (both rectangle and container)
The test effectively validates the
SceneCachespatial indexing functionality.crates/grida-canvas/src/window/stats_overlay.rs (1)
22-28: Font fallback logic is well-implemented.The font fallback approach trying "Arial" first and then falling back to the default font manager is a good defensive programming practice.
crates/grida-canvas/src/cache/README.md (1)
1-76: Excellent documentation quality.The documentation clearly explains the geometry cache design philosophy, data structures, and operations. The philosophy section with principles like "Authoring is relative. Rendering is absolute" and "Flat is fast" provides valuable insight into the design decisions. The code examples and operation descriptions make the API clear and approachable.
crates/grida-math2/src/box_fit.rs (3)
1-16: Well-designed BoxFit enum with comprehensive documentation.The enum design follows established patterns from CSS
object-fitand Flutter'sBoxFit, providing clear semantic meaning for each variant. The documentation references are helpful for understanding the intended behavior.
21-55: Correct mathematical implementation of box fitting transforms.The transform calculations are mathematically sound:
Containcorrectly uses minimum scale to ensure content fits within containerCoveruses maximum scale to fill container completelyNonemaintains original size with proper centering- Translation calculations properly center the scaled content
The affine transform matrix construction is correct with scaling and translation components.
58-108: Comprehensive test coverage validates implementation correctness.The test suite covers all BoxFit modes with both square and non-square aspect ratios, verifying exact matrix values. This ensures the mathematical calculations are precise and handles edge cases like differing aspect ratios.
crates/grida-canvas/src/window/ruler.rs (2)
16-45: Excellent use of thread-local storage for performance optimization.The thread-local static variables for paints and font objects are a smart optimization, avoiding repeated allocation and configuration of Skia objects. The font fallback logic handles cases where Arial isn't available gracefully.
48-75: Efficient caching strategy with proper invalidation logic.The cache invalidation logic correctly triggers updates when viewport rectangle or zoom level changes, using appropriate floating-point comparison with epsilon. This ensures the ruler adapts to camera changes while avoiding unnecessary recalculations.
crates/grida-canvas/examples/basic.rs (3)
7-31: Well-structured example demonstrating image node creation.The image node setup showcases proper configuration of transforms, sizing, corner radius, stroke, and drop shadow effects. This serves as an excellent reference for image node usage.
54-81: Excellent demonstration of gradient fills and blend modes.The ellipse node effectively showcases radial gradients with multiple color stops and blend mode usage. The gradient configuration demonstrates the flexibility of the paint system.
192-233: Proper scene graph assembly and node management.The scene construction follows best practices:
- Collecting node IDs before insertion prevents borrow checker issues
- Hierarchical structure with groups and containers is well-organized
- Repository pattern for node management is properly utilized
- Scene metadata (background color, transform) is appropriately set
crates/grida-canvas/examples/booleans.rs (3)
24-105: Excellent demonstration of Union operation with clear documentation.The Union example effectively shows how to combine two shapes while providing descriptive text that explains the operation's purpose. The visual layout with proper spacing makes the example easy to understand.
107-188: Clear Intersection operation example with overlapping shapes.The positioned circles demonstrate the intersection concept effectively, and the explanatory text helps users understand the expected behavior. The consistent styling maintains visual coherence across examples.
356-369: Proper scene assembly following established patterns.The scene construction mirrors the basic example's approach with appropriate metadata and proper node repository management. The consistent pattern across examples aids in learning.
crates/grida-canvas/Cargo.toml (4)
9-25: Well-organized dependency structure with proper feature flags.The dependency organization is excellent:
- Core dependencies are clearly identified
- WASM-specific dependencies are properly marked as optional
- Version specifications are appropriate and up-to-date
- UUID features for both native and JS environments are correctly configured
26-36: Excellent platform-specific dependency management.The conditional compilation approach for native-only dependencies is clean and prevents unnecessary dependencies in WASM builds. The selection of libraries (tokio, reqwest, glutin, winit) covers the full stack needed for native windowing and async operations.
44-50: Proper benchmark configuration with disabled harness.The benchmark setup correctly disables the default harness for Criterion benchmarks, allowing for custom benchmark implementations. The separation of rectangle and mipmap benchmarks provides focused performance testing.
1-4: Verify Rust edition 2024 compatibility.Edition 2024 is very recent and may not be stable or widely supported yet. Consider using edition 2021 for better compatibility unless specific 2024 features are required.
What is the current stable Rust edition and is edition 2024 stable?crates/grida-canvas/src/hit_test.rs (1)
7-21: Excellent documentation and clear algorithm explanation.The documentation clearly explains the three-step hit testing approach, which aligns with standard DOM hit testing behavior. This makes the code highly maintainable and understandable.
crates/grida-canvas/benches/bench_rectangles.rs (2)
99-106: Review unsafe pointer dereferencing for safety.The benchmark uses unsafe dereferencing of surface pointers in multiple locations. While this may be necessary for Skia integration, ensure proper safety guarantees.
Please verify:
- The
surface_ptris guaranteed to be valid throughout its usage- No other code can mutate the surface concurrently
- The pointer lifetime matches the expected usage pattern
Consider documenting the safety invariants or exploring safer alternatives if available.
Also applies to: 125-132, 150-157, 176-183, 201-208
85-87: Well-configured benchmark parameters.The sample size of 100 and measurement time of 10 seconds provide good statistical confidence for performance measurements while keeping benchmark runtime reasonable.
crates/grida-canvas/tests/render_bounds.rs (3)
5-31: Excellent test coverage for stroke effects on render bounds.The test correctly verifies that outside stroke alignment expands render bounds by the stroke width on all sides. The assertions match the expected behavior: original 100x100 rectangle with 10px outside stroke becomes 120x120 rectangle positioned at (-10, -10).
33-58: Accurate gaussian blur bounds calculation test.The test properly verifies that gaussian blur with radius 5 expands render bounds by the blur radius in all directions, resulting in the expected 110x110 bounds at (-5, -5).
60-90: Correct drop shadow bounds verification.The test accurately checks that drop shadow with offset (5,5) and blur 10 expands bounds appropriately. The final bounds of 120x120 at (-5, -5) correctly account for both shadow offset and blur radius.
crates/grida-canvas/examples/winit_raf.rs (4)
19-26: Appropriate simulation approach for demo purposes.Using random sleep duration to simulate variable rendering work is a good approach for demonstrating frame pacing behavior under realistic conditions where frame times vary.
84-84: Well-configured frame scheduling parameters.The configuration with 120 FPS target and 144 FPS maximum provides a good demonstration of frame pacing with headroom for performance spikes while maintaining smooth animation.
43-58: Effective FPS measurement and logging.The frame counting and per-second logging provides clear feedback on the frame scheduler's effectiveness. The reset logic correctly handles the measurement window.
65-89: Clean winit integration and application structure.The application follows winit best practices with proper event loop setup, window creation, and graceful shutdown handling. The overall structure is clean and maintainable.
crates/grida-canvas/examples/tiling.rs (1)
58-59: Improve error handling for surface creation.The
expectmessage is good, but consider if the program should handle this more gracefully in a production environment.crates/grida-canvas/tests/mipmap.rs (3)
4-22: LGTM - Comprehensive test for mipmap level calculation.The test correctly validates that the full chain generates the expected number of levels and terminates at 1x1 or smaller. The calculation using
log2().ceil()is mathematically sound.
24-38: LGTM - Good test for mipmap level selection.The test validates that the correct mipmap level is selected based on requested size. The assertion values (150x100) are mathematically correct for the input (300x200) and requested size (100x100).
40-53: Excellent edge case testing for NaN handling.This test ensures the system is robust against invalid floating-point values in the mipmap configuration. The comment "should not panic" clearly indicates the expected behavior.
crates/grida-canvas/examples/gradients.rs (1)
36-36: Potential typo in method name.The method
from_rotatationappears to have a typo - should it befrom_rotation?#!/bin/bash # Check if this is a typo or the correct method name rg -A 2 "from_rotatation" --type rustAlso applies to: 100-100
crates/grida-canvas/examples/cache_text_picture.rs (3)
1-18: Well-structured paragraph creation function.The
make_paragraphfunction is clean and follows Skia's text layout API correctly. Good separation of concerns with paragraph style and text style configuration.
20-32: Effective picture caching implementation.The
make_picturefunction correctly records multiple paragraphs into a single Picture for caching. The spacing calculation and y-offset management are appropriate.
34-58: Clean benchmark function with proper timing.The
runfunction effectively demonstrates the performance difference between cached and uncached rendering paths. The timing placement correctly measures only the rendering operations.crates/grida-canvas/examples/100k.rs (4)
17-19: Efficient grid dimension calculation.The grid calculation creates a nearly square layout, which is optimal for visual presentation and memory access patterns.
26-60: Well-implemented shape generation with rainbow effect.The grid positioning logic is correct, and the diagonal rainbow effect calculation is mathematically sound. Good use of the node factory pattern for consistent object creation.
72-98: Correct HSV to RGB conversion implementation.The HSV to RGB conversion follows the standard algorithm correctly, with proper handling of the hue sectors and color component calculations.
100-104: Appropriate use of async for large-scale operations.Using async/await for generating 100k shapes is good practice as it prevents blocking the thread during the potentially lengthy operation.
crates/grida-math2/src/align.rs (3)
3-22: Excellent documentation with clear examples.The function documentation is comprehensive with clear parameter descriptions and practical examples. The panic conditions are well documented.
23-51: Correct implementation of scalar alignment algorithm.The algorithm correctly finds the closest target within threshold, handles ties by collecting all equally close indices, and properly handles the case where no target is within threshold.
53-95: Consistent vector2 alignment implementation.The vector2 version maintains consistency with the scalar version while properly using Euclidean distance for 2D points. Good reuse of the existing
distancefunction.crates/grida-canvas/src/cache/picture.rs (3)
5-19: Well-designed cache strategy with clear documentation.The strategy configuration is well-documented and the default implementation is appropriate. The depth-based caching approach provides good flexibility.
21-33: Clean cache implementation with proper initialization.The cache structure is well-designed with clear separation between strategy and data. The constructor properly initializes both components.
39-42: Good design choice to invalidate on strategy change.Automatically invalidating the cache when the strategy changes prevents stale data and ensures consistency.
crates/grida-canvas/tests/geometry_cache.rs (2)
5-53: Comprehensive test for recursive geometry cache building.This test effectively validates the hierarchical transform calculation by creating a realistic node structure with multiple levels of nesting. The expected world transform calculation (21.0, 34.0) correctly accounts for the cumulative transforms.
55-95: Thorough test for container bounds calculation.This test properly validates that container bounds are calculated correctly when they include child elements. The test setup creates a realistic scenario with a container and child rectangle.
crates/grida-canvas/examples/effects.rs (1)
36-38: Fix incorrect color comment.The comment indicates "White" but the actual color value
Color(255, 255, 255, 255)is indeed white. However, the comment on line 86 says "White" but usesColor(200, 200, 200, 255)which is light gray.crates/grida-canvas/src/window/scheduler.rs (3)
15-25: LGTM! Clean constructor implementation.The constructor properly initializes all fields and uses sensible defaults for the rolling buffer size.
42-50: LGTM! Robust FPS calculation.The FPS calculation handles edge cases properly and uses appropriate averaging over recent frames.
64-67: Fix critical logic error in max FPS enforcement.The current logic uses
max()which would select the longer duration (lower FPS), but for max FPS enforcement, we need the shorter duration (higher FPS). This bug would prevent the max FPS cap from working correctly.- let target = match self.max_frame_time { - Some(max_time) => self.target_frame_time.max(max_time), - None => self.target_frame_time, - }; + let target = match self.max_frame_time { + Some(max_time) => self.target_frame_time.min(max_time), + None => self.target_frame_time, + };Likely an incorrect or invalid review comment.
crates/grida-canvas/examples/shapes.rs (1)
26-47: LGTM! Comprehensive shape demonstrations.Each shape row effectively demonstrates different properties and variations. The consistent spacing, positioning, and color progression create a well-organized visual demonstration.
Also applies to: 49-69, 71-101, 103-124, 126-155, 157-179
crates/grida-canvas/examples/webfonts.rs (1)
189-216: LGTM! Excellent async font loading implementation.The concurrent font loading with proper error handling and progress logging is well implemented. Good use of
join_allfor concurrent loading and proper resource management with cloning.crates/grida-math2/src/packing.rs (3)
16-43: LGTM! Clean MaxRects implementation.The core fitting algorithm is well-implemented with proper space subdivision and lexicographical sorting for consistent placement.
54-78: LGTM! Robust fallback algorithm.The walk_to_fit extension provides a good fallback when the primary algorithm fails. The step calculation ensures a minimum of 1.0, preventing potential issues with zero step sizes.
59-59: Verify step calculation edge cases.While the current calculation
(agent.0.min(agent.1) / 2.0).max(1.0)should always produce a positive value ≥ 1.0, consider adding explicit validation to ensure robustness against potential floating-point edge cases.#!/bin/bash # Search for similar step calculation patterns in the codebase rg -A 3 -B 3 "step_by.*as usize"crates/grida-canvas/src/node/repository.rs (1)
92-93: Document or validate that the iterator's NodeId matches the node's internal ID.The
FromIteratorimplementation ignores theNodeIdfrom the iterator and uses the node's internal ID instead. This could be confusing and lead to bugs if they don't match.Consider either:
- Validating that the IDs match:
- for (_, node) in iter { - repo.insert(node); + for (id, node) in iter { + let node_id = repo.insert(node); + debug_assert_eq!(id, node_id, "Iterator NodeId doesn't match node's internal ID");
- Or document this behavior clearly in the trait implementation.
crates/grida-math2/src/layout.rs (1)
122-127: Potential division by zero in direction vector calculation.The normalization of the direction vector could result in division by zero if two consecutive points are identical (when the denominator equals zero).
Apply this pattern to handle the edge case:
- let dir_a = Point { - x: (first.x - last.x) / ((first.x - last.x).powi(2) + (first.y - last.y).powi(2)).sqrt(), - y: (first.y - last.y) / ((first.x - last.x).powi(2) + (first.y - last.y).powi(2)).sqrt(), - }; + let dx = first.x - last.x; + let dy = first.y - last.y; + let len = (dx.powi(2) + dy.powi(2)).sqrt(); + let dir_a = if len > f32::EPSILON { + Point { x: dx / len, y: dy / len } + } else { + Point { x: 0.0, y: 0.0 } + };The same issue appears in lines 139-142 and 146-149.
Likely an incorrect or invalid review comment.
crates/grida-canvas/src/node/factory.rs (1)
1-251: Well-structured factory implementation.The factory provides a clean and consistent way to create nodes with sensible defaults. Good use of constants and helper methods to avoid duplication.
crates/grida-canvas/examples/cache_image.rs (1)
1-314: Excellent benchmarking example.The example provides comprehensive performance testing across various device specifications with proper warm-up, statistics collection, and clear output formatting. The complex drawing function effectively demonstrates the benefits of image caching.
crates/grida-canvas/src/repository.rs (1)
9-38: Well-designed generic repository trait!The trait provides a clean abstraction for resource storage with all essential operations. The use of associated types and lifetime bounds is correct.
crates/grida-canvas/src/cache/scene.rs (1)
29-166: Excellent scene cache implementation!The design effectively consolidates multiple cache types and uses R-tree spatial indexing for efficient queries. The separation between intersection, containment, and point queries is well thought out.
.ref/figma/figma-rest-api.min.d.ts (1)
1-2081: LGTM!The TypeScript type definitions for the Figma REST API are comprehensive and well-structured. The types properly model the Figma document hierarchy with appropriate traits and node types.
crates/grida-canvas/src/cache/tile.rs (3)
11-24: LGTM! Good type choices for tile coordinates.The use of
i32for coordinates (allowing negative positions) andu32for dimensions (ensuring non-negative sizes) is appropriate for a canvas coordinate system.
54-99: Well-implemented progressive quality rendering!The blur calculation logic with adaptive radius based on zoom difference and the sorting strategy to draw higher quality tiles on top are both excellent design choices for smooth zoom transitions.
263-276: Smart cache management with fallback preservation.The
clear()method's retention of lowest zoom tiles while clearing others is an excellent optimization that balances memory usage with zoom-out performance.crates/grida-canvas/src/io/io_json.rs (1)
92-126: Excellent flexible deserialization for corner radius!The custom deserializer elegantly handles both single value (uniform radius) and array (per-corner radius) formats, making the API user-friendly.
crates/grida-math2/src/lib.rs (1)
1-80: Clean module organization and API surface!The library provides a well-organized facade with clear re-exports, making it easy for consumers to use the math utilities without navigating the internal module structure.
crates/grida-canvas/src/node/schema.rs (1)
827-863: Correct implementation of regular polygon generation!The algorithm properly handles the orientation difference between even and odd-sided polygons, ensuring triangles point up and squares have flat tops.
crates/grida-canvas/examples/stroke.rs (2)
7-24: Well-structured demo function setup.The async function structure and initial setup with NodeFactory and NodeRepository is clean and follows good practices. The layout constants provide clear organization for the demo grid.
393-397: Clean and appropriate async main function.The main function properly uses the Tokio runtime and calls the demo function, following the pattern established in other examples.
crates/grida-canvas/examples/srcset.rs (4)
6-56: Well-designed benchmark structure with good image scaling logic.The
ImageBenchmarkstruct and its initialization are well-organized. The scaling logic correctly creates different resolution versions of the source image using proper Skia drawing operations.
58-136: Solid benchmarking methodology with accurate timing.The benchmark method correctly implements a grid-based rendering test with separate timing for Fast and Strict constraints. The performance measurement approach is sound and provides meaningful metrics.
139-177: Excellent result formatting and user experience.The main function provides clear feedback about the benchmark progress and formats results in a readable table format. The file existence check prevents common user errors.
141-148: Good user experience with clear error message.The file existence check and informative error message help users understand what's needed to run the benchmark.
| { | ||
| "name": "@crates/math2", | ||
| "description": "turbo ci rust wrapper", | ||
| "private": true, |
There was a problem hiding this comment.
Missing required ‘version’ field and metadata enhancements
The package.json lacks a version property (required for npm/Yarn workspaces) and could benefit from standard metadata like license and repository. Also confirm that the scope (@crates/math2) matches your organization’s naming convention (e.g., @grida/math2 if preferred).
Example diff:
{
"name": "@crates/math2",
+ "version": "0.1.0",
"description": "turbo ci rust wrapper",
"private": true,
+ "license": "MIT",
+ "repository": {
+ "type": "git",
+ "url": "https://github.com/gridaco/grida.git",
+ "directory": "crates/grida-math2"
+ },
"scripts": {📝 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.
| { | |
| "name": "@crates/math2", | |
| "description": "turbo ci rust wrapper", | |
| "private": true, | |
| { | |
| "name": "@crates/math2", | |
| "version": "0.1.0", | |
| "description": "turbo ci rust wrapper", | |
| "private": true, | |
| "license": "MIT", | |
| "repository": { | |
| "type": "git", | |
| "url": "https://github.com/gridaco/grida.git", | |
| "directory": "crates/grida-math2" | |
| }, | |
| "scripts": { |
🤖 Prompt for AI Agents
In crates/grida-math2/package.json at the beginning of the file, add a required
"version" field with an appropriate semantic version string. Also include
standard metadata fields such as "license" with the correct license identifier
and "repository" with the URL to the source repository. Verify and update the
"name" field scope to match your organization's naming convention, for example
changing "@crates/math2" to "@grida/math2" if that is the preferred scope.
| paint.set_color(Color::BLACK); | ||
|
|
||
| // Load the Caveat font from local resources | ||
| let font_data = fs::read("resources/Caveat-VariableFont_wght.ttf").unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for resource file loading.
The hardcoded path "resources/Caveat-VariableFont_wght.ttf" will cause a panic if the file doesn't exist. Consider using graceful error handling similar to the other fonts.
- let font_data = fs::read("resources/Caveat-VariableFont_wght.ttf").unwrap();
+ let font_data = fs::read("resources/Caveat-VariableFont_wght.ttf")
+ .expect("Caveat font file not found in resources/");Or better yet, use conditional loading like the other fonts:
- let font_data = fs::read("resources/Caveat-VariableFont_wght.ttf").unwrap();
- let font_mgr = FontMgr::new();
- let typeface = font_mgr.new_from_data(&font_data, None).unwrap();
- let font = Font::new(typeface, 24.0);
+ let font_mgr = FontMgr::new();
+ let (font, fallback_message) = if let Ok(font_data) = fs::read("resources/Caveat-VariableFont_wght.ttf") {
+ if let Some(typeface) = font_mgr.new_from_data(&font_data, None) {
+ (Font::new(typeface, 24.0), None)
+ } else {
+ (Font::default(), Some("Failed to create Caveat typeface"))
+ }
+ } else {
+ (Font::default(), Some("Caveat font not found"))
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/grida-canvas/examples/fonts.rs at line 23, the code reads the font
file with unwrap(), which will panic if the file is missing. Replace unwrap()
with proper error handling by matching the result of fs::read and handling the
Err case gracefully, such as logging an error or returning a Result.
Alternatively, implement conditional loading similar to other fonts in the file
to avoid panics when the resource is unavailable.
| let mut font_collection = FontCollection::new(); | ||
| let mut provider = TypefaceFontProvider::new(); | ||
| provider.register_typeface( | ||
| font_mgr.new_from_data(&vt323_data, None).unwrap(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate redundant font data loading.
The VT323 font data is loaded again unnecessarily. You already have vt323_typeface from line 63, so reuse it instead of reloading the font data.
- provider.register_typeface(
- font_mgr.new_from_data(&vt323_data, None).unwrap(),
- Some("VT323"),
- );
+ provider.register_typeface(vt323_typeface.clone(), Some("VT323"));📝 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.
| font_mgr.new_from_data(&vt323_data, None).unwrap(), | |
| provider.register_typeface(vt323_typeface.clone(), Some("VT323")); |
🤖 Prompt for AI Agents
In crates/grida-canvas/examples/fonts.rs at line 82, the VT323 font data is
being loaded again redundantly. Instead of calling
font_mgr.new_from_data(&vt323_data, None).unwrap() again, reuse the existing
vt323_typeface variable declared around line 63. Replace the redundant loading
with a reference to vt323_typeface to avoid unnecessary data loading.
| let file: String = fs::read_to_string(file_path).expect("failed to read file"); | ||
| let canvas_file = parse(&file).expect("failed to parse file"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling to avoid panics.
Using expect() will cause the program to panic on file I/O or parsing errors. Consider returning Result types for better error handling.
-async fn load_scene_from_file(file_path: &str) -> Scene {
- let file: String = fs::read_to_string(file_path).expect("failed to read file");
- let canvas_file = parse(&file).expect("failed to parse file");
+async fn load_scene_from_file(file_path: &str) -> Result<Scene, Box<dyn std::error::Error>> {
+ let file: String = fs::read_to_string(file_path)?;
+ let canvas_file = parse(&file)?;And update the main function accordingly:
- let scene = load_scene_from_file(&cli.path).await;
+ let scene = load_scene_from_file(&cli.path).await
+ .unwrap_or_else(|e| {
+ eprintln!("Error loading scene: {}", e);
+ std::process::exit(1);
+ });🤖 Prompt for AI Agents
In crates/grida-canvas/examples/json.rs at lines 16 to 17, replace the use of
expect() with proper error handling by returning Result types from the function.
Change the main function signature to return a Result and use the ? operator on
fs::read_to_string and parse calls to propagate errors instead of panicking.
This will improve error handling and avoid abrupt program termination.
| async fn demo_paints() -> Scene { | ||
| let nf = NodeFactory::new(); | ||
| let mut repository = NodeRepository::new(); | ||
|
|
||
| // Create a root container node | ||
| let mut root_container_node = nf.create_container_node(); | ||
| root_container_node.base.name = "Root Container".to_string(); | ||
| root_container_node.size = Size { | ||
| width: 1080.0, | ||
| height: 1080.0, | ||
| }; | ||
|
|
||
| let mut all_shape_ids = Vec::new(); | ||
| let spacing = 100.0; | ||
| let start_x = 50.0; | ||
| let base_size = 80.0; | ||
| let items_per_row = 10; | ||
|
|
||
| // Solid Colors Row | ||
| for i in 0..items_per_row { | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = format!("Solid Color {}", i + 1); | ||
| rect.transform = AffineTransform::new(start_x + spacing * i as f32, 100.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
| rect.fill = Paint::Solid(SolidPaint { | ||
| color: Color( | ||
| 255 - (i * 25) as u8, | ||
| 100 + (i * 15) as u8, | ||
| 50 + (i * 20) as u8, | ||
| 255, | ||
| ), | ||
| opacity: 1.0, | ||
| }); | ||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
| } | ||
|
|
||
| // Linear Gradient Row | ||
| for i in 0..items_per_row { | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = format!("Linear Gradient {}", i + 1); | ||
| rect.transform = AffineTransform::new(start_x + spacing * i as f32, 200.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
|
|
||
| // Create a linear gradient that changes angle based on index | ||
| let angle = (i as f32 * 36.0) * std::f32::consts::PI / 180.0; // 0 to 360 degrees | ||
| let transform = AffineTransform::new(0.0, 0.0, angle); | ||
|
|
||
| rect.fill = Paint::LinearGradient(LinearGradientPaint { | ||
| transform, | ||
| stops: vec![ | ||
| GradientStop { | ||
| offset: 0.0, | ||
| color: Color(255, 100, 100, 255), | ||
| }, | ||
| GradientStop { | ||
| offset: 1.0, | ||
| color: Color(100, 100, 255, 255), | ||
| }, | ||
| ], | ||
| opacity: 1.0, | ||
| }); | ||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
| } | ||
|
|
||
| // Radial Gradient Row | ||
| for i in 0..items_per_row { | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = format!("Radial Gradient {}", i + 1); | ||
| rect.transform = AffineTransform::new(start_x + spacing * i as f32, 300.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
|
|
||
| // Create a radial gradient with varying center positions | ||
| let center_x = 0.2 + (i as f32 * 0.06); // 0.2 to 0.8 | ||
| let center_y = 0.2 + (i as f32 * 0.06); // 0.2 to 0.8 | ||
| let transform = AffineTransform::new(center_x * base_size, center_y * base_size, 0.0); | ||
|
|
||
| rect.fill = Paint::RadialGradient(RadialGradientPaint { | ||
| transform, | ||
| stops: vec![ | ||
| GradientStop { | ||
| offset: 0.0, | ||
| color: Color(255, 255, 100, 255), | ||
| }, | ||
| GradientStop { | ||
| offset: 1.0, | ||
| color: Color(100, 255, 100, 255), | ||
| }, | ||
| ], | ||
| opacity: 1.0, | ||
| }); | ||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
| } | ||
|
|
||
| // Stroke Solid Colors Row | ||
| for i in 0..items_per_row { | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = format!("Stroke Solid Color {}", i + 1); | ||
| rect.transform = AffineTransform::new(start_x + spacing * i as f32, 400.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
|
|
||
| // No fill | ||
| rect.fill = Paint::Solid(SolidPaint { | ||
| color: Color(0, 0, 0, 0), // Transparent | ||
| opacity: 1.0, | ||
| }); | ||
|
|
||
| // Solid color stroke with varying colors | ||
| rect.stroke = Paint::Solid(SolidPaint { | ||
| color: Color( | ||
| 255 - (i * 25) as u8, | ||
| 100 + (i * 15) as u8, | ||
| 50 + (i * 20) as u8, | ||
| 255, | ||
| ), | ||
| opacity: 1.0, | ||
| }); | ||
| rect.stroke_width = 4.0; // Consistent stroke width | ||
|
|
||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
| } | ||
|
|
||
| // Stroke Linear Gradient Row | ||
| for i in 0..items_per_row { | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = format!("Stroke Linear Gradient {}", i + 1); | ||
| rect.transform = AffineTransform::new(start_x + spacing * i as f32, 500.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
|
|
||
| // No fill | ||
| rect.fill = Paint::Solid(SolidPaint { | ||
| color: Color(0, 0, 0, 0), // Transparent | ||
| opacity: 1.0, | ||
| }); | ||
|
|
||
| // Create a linear gradient that changes angle based on index | ||
| let angle = (i as f32 * 36.0) * std::f32::consts::PI / 180.0; // 0 to 360 degrees | ||
| let transform = AffineTransform::new(0.0, 0.0, angle); | ||
|
|
||
| rect.stroke = Paint::LinearGradient(LinearGradientPaint { | ||
| transform, | ||
| stops: vec![ | ||
| GradientStop { | ||
| offset: 0.0, | ||
| color: Color(255, 100, 100, 255), | ||
| }, | ||
| GradientStop { | ||
| offset: 1.0, | ||
| color: Color(100, 100, 255, 255), | ||
| }, | ||
| ], | ||
| opacity: 1.0, | ||
| }); | ||
| rect.stroke_width = 4.0; // Consistent stroke width | ||
|
|
||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
| } | ||
|
|
||
| // Stroke Radial Gradient Row | ||
| for i in 0..items_per_row { | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = format!("Stroke Radial Gradient {}", i + 1); | ||
| rect.transform = AffineTransform::new(start_x + spacing * i as f32, 600.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
|
|
||
| // No fill | ||
| rect.fill = Paint::Solid(SolidPaint { | ||
| color: Color(0, 0, 0, 0), // Transparent | ||
| opacity: 1.0, | ||
| }); | ||
|
|
||
| // Create a radial gradient with varying center positions | ||
| let center_x = 0.2 + (i as f32 * 0.06); // 0.2 to 0.8 | ||
| let center_y = 0.2 + (i as f32 * 0.06); // 0.2 to 0.8 | ||
| let transform = AffineTransform::new(center_x * base_size, center_y * base_size, 0.0); | ||
|
|
||
| rect.stroke = Paint::RadialGradient(RadialGradientPaint { | ||
| transform, | ||
| stops: vec![ | ||
| GradientStop { | ||
| offset: 0.0, | ||
| color: Color(255, 255, 100, 255), | ||
| }, | ||
| GradientStop { | ||
| offset: 1.0, | ||
| color: Color(100, 255, 100, 255), | ||
| }, | ||
| ], | ||
| opacity: 1.0, | ||
| }); | ||
| rect.stroke_width = 4.0; // Consistent stroke width | ||
|
|
||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
| } | ||
|
|
||
| // Set up the root container | ||
| root_container_node.children.extend(all_shape_ids); | ||
| let root_container_id = root_container_node.base.id.clone(); | ||
| repository.insert(Node::Container(root_container_node)); | ||
|
|
||
| Scene { | ||
| id: "scene".to_string(), | ||
| name: "Paints Demo".to_string(), | ||
| transform: AffineTransform::identity(), | ||
| children: vec![root_container_id], | ||
| nodes: repository, | ||
| background_color: Some(Color(250, 250, 250, 255)), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication by extracting common rectangle creation logic.
The function has significant code duplication across the six paint style sections. Each section follows nearly identical patterns for rectangle creation, positioning, and repository insertion.
Consider extracting a helper function:
+fn create_rectangle_row(
+ nf: &NodeFactory,
+ repository: &mut NodeRepository,
+ row_name: &str,
+ y_position: f32,
+ paint_fn: impl Fn(usize) -> Paint,
+ stroke_fn: Option<impl Fn(usize) -> Paint>,
+) -> Vec<String> {
+ let mut ids = Vec::new();
+ let spacing = 100.0;
+ let start_x = 50.0;
+ let base_size = 80.0;
+ let items_per_row = 10;
+
+ for i in 0..items_per_row {
+ let mut rect = nf.create_rectangle_node();
+ rect.base.name = format!("{} {}", row_name, i + 1);
+ rect.transform = AffineTransform::new(start_x + spacing * i as f32, y_position, 0.0);
+ rect.size = Size { width: base_size, height: base_size };
+ rect.corner_radius = RectangularCornerRadius::all(8.0);
+ rect.fill = paint_fn(i);
+ if let Some(stroke_fn) = &stroke_fn {
+ rect.stroke = stroke_fn(i);
+ rect.stroke_width = 4.0;
+ }
+ ids.push(rect.base.id.clone());
+ repository.insert(Node::Rectangle(rect));
+ }
+ ids
+}Then use it for each row:
- // Solid Colors Row
- for i in 0..items_per_row {
- // ... repetitive code
- }
+ all_shape_ids.extend(create_rectangle_row(
+ &nf, &mut repository, "Solid Color", 100.0,
+ |i| Paint::Solid(SolidPaint {
+ color: Color(255 - (i * 25) as u8, 100 + (i * 15) as u8, 50 + (i * 20) as u8, 255),
+ opacity: 1.0,
+ }),
+ None,
+ ));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/grida-canvas/examples/paint.rs from lines 7 to 244, the demo_paints
function has repetitive code blocks creating rectangles with similar setup steps
for different paint styles. To fix this, extract a helper function that takes
parameters like name, position, fill paint, stroke paint, and stroke width, and
returns a configured rectangle node. Replace each paint style loop by calling
this helper with appropriate arguments, reducing duplication and improving
maintainability.
| if let Some(scene_ptr) = self.scene.as_ref().map(|s| s as *const Scene) { | ||
| // SAFETY: the pointer is only used for the duration of this call | ||
| // and the scene is not mutated while borrowed. | ||
| let Some(backend) = self.backend.as_ref() else { | ||
| return None; | ||
| }; | ||
| let surface = unsafe { &mut *backend.get_surface() }; | ||
| let scene = unsafe { &*scene_ptr }; |
There was a problem hiding this comment.
Document the safety justification for unsafe pointer usage.
The unsafe block uses a raw pointer to the scene but the safety comment is incomplete. Please provide a more detailed safety justification.
if let Some(scene_ptr) = self.scene.as_ref().map(|s| s as *const Scene) {
- // SAFETY: the pointer is only used for the duration of this call
- // and the scene is not mutated while borrowed.
+ // SAFETY:
+ // 1. The pointer is derived from a valid reference that outlives this function call
+ // 2. The scene is not mutated during the rendering process
+ // 3. No other mutable references to the scene exist during this call
+ // 4. The pointer is only dereferenced within this function's scope
let Some(backend) = self.backend.as_ref() else {
return None;
};📝 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.
| if let Some(scene_ptr) = self.scene.as_ref().map(|s| s as *const Scene) { | |
| // SAFETY: the pointer is only used for the duration of this call | |
| // and the scene is not mutated while borrowed. | |
| let Some(backend) = self.backend.as_ref() else { | |
| return None; | |
| }; | |
| let surface = unsafe { &mut *backend.get_surface() }; | |
| let scene = unsafe { &*scene_ptr }; | |
| if let Some(scene_ptr) = self.scene.as_ref().map(|s| s as *const Scene) { | |
| // SAFETY: | |
| // 1. The pointer is derived from a valid reference that outlives this function call | |
| // 2. The scene is not mutated during the rendering process | |
| // 3. No other mutable references to the scene exist during this call | |
| // 4. The pointer is only dereferenced within this function's scope | |
| let Some(backend) = self.backend.as_ref() else { | |
| return None; | |
| }; | |
| let surface = unsafe { &mut *backend.get_surface() }; | |
| let scene = unsafe { &*scene_ptr }; | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/runtime/scene.rs around lines 249 to 256, the unsafe
block uses a raw pointer to the scene but the safety comment is incomplete.
Expand the safety comment to clearly explain why dereferencing the raw pointer
is safe, including that the pointer is valid for the duration of the call, the
scene is not mutated or dropped during this time, and no other mutable
references exist concurrently.
| impl From<RGBA> for Color { | ||
| fn from(color: RGBA) -> Self { | ||
| Color(color.r, color.g, color.b, (color.a * 255.0) as u8) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add clamping to prevent alpha overflow.
The alpha conversion could overflow if the input value is outside the 0-1 range. Consider clamping before conversion.
impl From<RGBA> for Color {
fn from(color: RGBA) -> Self {
- Color(color.r, color.g, color.b, (color.a * 255.0) as u8)
+ Color(color.r, color.g, color.b, (color.a.clamp(0.0, 1.0) * 255.0) as u8)
}
}📝 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.
| impl From<RGBA> for Color { | |
| fn from(color: RGBA) -> Self { | |
| Color(color.r, color.g, color.b, (color.a * 255.0) as u8) | |
| } | |
| } | |
| impl From<RGBA> for Color { | |
| fn from(color: RGBA) -> Self { | |
| Color( | |
| color.r, | |
| color.g, | |
| color.b, | |
| (color.a.clamp(0.0, 1.0) * 255.0) as u8, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/io/io_json.rs around lines 286 to 290, the alpha
value in the RGBA to Color conversion is multiplied by 255.0 and cast to u8
without clamping, which can cause overflow if alpha is outside 0-1. Fix this by
clamping the alpha value to the 0.0 to 1.0 range before multiplying and casting,
ensuring the resulting u8 value stays within valid bounds.
| "solid" => { | ||
| if let Some(color) = fill.color { | ||
| Paint::Solid(SolidPaint { | ||
| color: Color(color.r, color.g, color.b, (color.a * 255.0) as u8), | ||
| opacity: 1.0, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse Color conversion to avoid duplication and ensure consistency.
The alpha conversion logic is duplicated from the Color implementation. Consider reusing it.
"solid" => {
if let Some(color) = fill.color {
Paint::Solid(SolidPaint {
- color: Color(color.r, color.g, color.b, (color.a * 255.0) as u8),
+ color: color.into(),
opacity: 1.0,
})📝 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.
| "solid" => { | |
| if let Some(color) = fill.color { | |
| Paint::Solid(SolidPaint { | |
| color: Color(color.r, color.g, color.b, (color.a * 255.0) as u8), | |
| opacity: 1.0, | |
| }) | |
| "solid" => { | |
| if let Some(color) = fill.color { | |
| Paint::Solid(SolidPaint { | |
| color: color.into(), | |
| opacity: 1.0, | |
| }) |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/io/io_json.rs around lines 296 to 301, the alpha
conversion for Color is duplicated. Refactor the code to reuse the existing
Color conversion logic instead of manually converting alpha by multiplying by
255 and casting. This will avoid duplication and ensure consistency by calling
the appropriate Color constructor or conversion method directly.
| /// Panics if the value is not between 1 and 1000. | ||
| pub fn new(value: u32) -> Self { | ||
| assert!( | ||
| value >= 1 && value <= 1000, | ||
| "Font weight must be between 1 and 1000" | ||
| ); | ||
| Self(value) | ||
| } |
There was a problem hiding this comment.
Replace panic with Result for better error handling.
Using assert! in a constructor will cause panics on invalid input. Consider returning a Result instead.
- pub fn new(value: u32) -> Self {
- assert!(
- value >= 1 && value <= 1000,
- "Font weight must be between 1 and 1000"
- );
- Self(value)
+ pub fn new(value: u32) -> Result<Self, &'static str> {
+ if value >= 1 && value <= 1000 {
+ Ok(Self(value))
+ } else {
+ Err("Font weight must be between 1 and 1000")
+ }
}📝 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.
| /// Panics if the value is not between 1 and 1000. | |
| pub fn new(value: u32) -> Self { | |
| assert!( | |
| value >= 1 && value <= 1000, | |
| "Font weight must be between 1 and 1000" | |
| ); | |
| Self(value) | |
| } | |
| /// Panics if the value is not between 1 and 1000. | |
| pub fn new(value: u32) -> Result<Self, &'static str> { | |
| if value >= 1 && value <= 1000 { | |
| Ok(Self(value)) | |
| } else { | |
| Err("Font weight must be between 1 and 1000") | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/node/schema.rs around lines 295 to 302, the
constructor uses assert! to validate the input, which causes a panic on invalid
values. Refactor the new function to return a Result<Self, ErrorType> instead of
panicking. Replace the assert! with a conditional check that returns an Err with
a descriptive error if the value is out of range, and return Ok(Self(value)) if
valid. Define or use an appropriate error type for the Result.
| // Conic Gradient Stroke | ||
| let mut rect = nf.create_rectangle_node(); | ||
| rect.base.name = "Conic Gradient Stroke".to_string(); | ||
| rect.transform = AffineTransform::new(start_x + spacing * 2.0, 850.0, 0.0); | ||
| rect.size = Size { | ||
| width: base_size, | ||
| height: base_size, | ||
| }; | ||
| rect.corner_radius = RectangularCornerRadius::all(8.0); | ||
| rect.fill = Paint::Solid(SolidPaint { | ||
| color: Color(0, 0, 0, 0), | ||
| opacity: 1.0, | ||
| }); | ||
| rect.stroke = Paint::RadialGradient(RadialGradientPaint { | ||
| transform: AffineTransform::new(base_size / 2.0, base_size / 2.0, 0.0), | ||
| stops: vec![ | ||
| GradientStop { | ||
| offset: 0.0, | ||
| color: Color(0, 255, 0, 255), // Green | ||
| }, | ||
| GradientStop { | ||
| offset: 0.5, | ||
| color: Color(0, 255, 255, 255), // Cyan | ||
| }, | ||
| GradientStop { | ||
| offset: 1.0, | ||
| color: Color(0, 255, 0, 255), // Green | ||
| }, | ||
| ], | ||
| opacity: 1.0, | ||
| }); | ||
| rect.stroke_width = 8.0; | ||
| all_shape_ids.push(rect.base.id.clone()); | ||
| repository.insert(Node::Rectangle(rect)); | ||
|
|
There was a problem hiding this comment.
Fix incorrect comment and paint type.
The comment indicates "Conic Gradient Stroke" but the implementation uses RadialGradient instead of a conic gradient.
Apply this fix to use the correct gradient type or update the comment:
- // Conic Gradient Stroke
+ // Radial Gradient Stroke (Alternative)
let mut rect = nf.create_rectangle_node();
- rect.base.name = "Conic Gradient Stroke".to_string();
+ rect.base.name = "Radial Gradient Stroke Alt".to_string();Or if conic gradients are supported, implement the correct gradient type instead of using RadialGradient.
📝 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.
| // Conic Gradient Stroke | |
| let mut rect = nf.create_rectangle_node(); | |
| rect.base.name = "Conic Gradient Stroke".to_string(); | |
| rect.transform = AffineTransform::new(start_x + spacing * 2.0, 850.0, 0.0); | |
| rect.size = Size { | |
| width: base_size, | |
| height: base_size, | |
| }; | |
| rect.corner_radius = RectangularCornerRadius::all(8.0); | |
| rect.fill = Paint::Solid(SolidPaint { | |
| color: Color(0, 0, 0, 0), | |
| opacity: 1.0, | |
| }); | |
| rect.stroke = Paint::RadialGradient(RadialGradientPaint { | |
| transform: AffineTransform::new(base_size / 2.0, base_size / 2.0, 0.0), | |
| stops: vec![ | |
| GradientStop { | |
| offset: 0.0, | |
| color: Color(0, 255, 0, 255), // Green | |
| }, | |
| GradientStop { | |
| offset: 0.5, | |
| color: Color(0, 255, 255, 255), // Cyan | |
| }, | |
| GradientStop { | |
| offset: 1.0, | |
| color: Color(0, 255, 0, 255), // Green | |
| }, | |
| ], | |
| opacity: 1.0, | |
| }); | |
| rect.stroke_width = 8.0; | |
| all_shape_ids.push(rect.base.id.clone()); | |
| repository.insert(Node::Rectangle(rect)); | |
| // Radial Gradient Stroke (Alternative) | |
| let mut rect = nf.create_rectangle_node(); | |
| rect.base.name = "Radial Gradient Stroke Alt".to_string(); | |
| rect.transform = AffineTransform::new(start_x + spacing * 2.0, 850.0, 0.0); | |
| rect.size = Size { | |
| width: base_size, | |
| height: base_size, | |
| }; | |
| rect.corner_radius = RectangularCornerRadius::all(8.0); | |
| rect.fill = Paint::Solid(SolidPaint { | |
| color: Color(0, 0, 0, 0), | |
| opacity: 1.0, | |
| }); | |
| rect.stroke = Paint::RadialGradient(RadialGradientPaint { | |
| transform: AffineTransform::new(base_size / 2.0, base_size / 2.0, 0.0), | |
| stops: vec![ | |
| GradientStop { | |
| offset: 0.0, | |
| color: Color(0, 255, 0, 255), // Green | |
| }, | |
| GradientStop { | |
| offset: 0.5, | |
| color: Color(0, 255, 255, 255), // Cyan | |
| }, | |
| GradientStop { | |
| offset: 1.0, | |
| color: Color(0, 255, 0, 255), // Green | |
| }, | |
| ], | |
| opacity: 1.0, | |
| }); | |
| rect.stroke_width = 8.0; | |
| all_shape_ids.push(rect.base.id.clone()); | |
| repository.insert(Node::Rectangle(rect)); |
🤖 Prompt for AI Agents
In crates/grida-canvas/examples/stroke.rs around lines 320 to 354, the comment
states "Conic Gradient Stroke" but the code uses a RadialGradientPaint for the
stroke. To fix this, either update the comment to accurately reflect that the
stroke uses a radial gradient or replace the RadialGradientPaint with the
correct conic gradient paint type if supported. Ensure the comment and paint
type are consistent to avoid confusion.
1 participant
packages/grida-cmathto rustmath2lib #382Related:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores