Grida Canvs - squircle
#443
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces corner smoothing (G² curvature blending) to the Grida canvas rendering system. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Editor/App
participant Factory as NodeFactory
participant Schema as NodeSchema
participant Shape as ShapeModule
participant Painter as PainterGeometry
participant Renderer as SkiaRenderer
App->>Factory: create_rectangle_node(...)
Factory->>Schema: RectangleNodeRec { corner_radius, corner_smoothing }
activate Schema
Schema->>Shape: to_shape()
alt corner_radius == 0
Shape-->>Schema: Rect(...)
else corner_smoothing == 0
Shape-->>Schema: RRect(...)
else corner_smoothing > 0
Shape-->>Schema: OrthogonalSmoothRRect(...)
end
deactivate Schema
Schema-->>Factory: ✓ node created
App->>Painter: build_shape(node)
activate Painter
alt is RectangleNode
Painter->>Shape: n.to_shape()
Shape-->>Painter: Shape variant
Painter->>Painter: PainterShape::from_shape(...)
else is ContainerNode with smoothing
Painter->>Shape: build_orthogonal_smooth_rrect_path(...)
Shape-->>Painter: skia_safe::Path with Bézier curves
end
Painter-->>App: PainterShape ready
deactivate Painter
App->>Renderer: render(painter_shape)
Renderer->>Renderer: stroke/fill with Skia
Renderer-->>App: ✓ rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
crates/math2/tests/bezier.rs (1)
12-22: Remove commented-out duplicate helper or gate it properly.Keeping dead code in tests hurts signal. Either drop it or restore behind cfg(test) with tolerant comparisons when you re-enable the SVG path test.
Apply to clean up now:
-// /// Converts the output of `bezier_a2c` to an SVG path string. -// fn a2c_to_svg_path(x1: f32, y1: f32, data: &[f32]) -> String { -// let mut path = format!("M {} {}", x1, y1); -// for chunk in data.chunks(6) { -// if let [c1x, c1y, c2x, c2y, x, y] = *chunk { -// path.push_str(&format!(" C {} {}, {} {}, {} {}", c1x, c1y, c2x, c2y, x, y)); -// } -// } -// path -// }packages/grida-canvas-schema/grida.ts (1)
1478-1483: Document expected range for cornerSmoothing (and default).Clarify semantics to avoid misuse; typical domain is 0..1 with 0 = off.
export interface IRectangularCornerRadius { cornerRadiusTopLeft?: number; cornerRadiusTopRight?: number; cornerRadiusBottomLeft?: number; cornerRadiusBottomRight?: number; - cornerSmoothing?: number; + /** + * Corner smoothing factor in [0..1]. + * 0 = no smoothing (standard rounded corners), 1 = maximum G² smoothing. + * Defaults to 0 when omitted. + */ + cornerSmoothing?: number; }crates/grida-canvas/benches/bench_rectangles.rs (1)
26-27: Optional: add a smoothing-on variant to benchmark overhead.Keep default 0 here, but consider a “with_smoothing” bench (e.g., 0.5) to quantify path-building cost.
crates/grida-canvas/src/shape/rrect.rs (1)
14-18: Prefer implementing From<&RRectShape> to get Into for free.This is fine, but idiomatic Rust implements From and derives Into automatically, reducing trait impl surface. Consider switching.
-impl Into<skia_safe::RRect> for &RRectShape { - fn into(self) -> skia_safe::RRect { - build_rrect(self) - } -} +impl From<&RRectShape> for skia_safe::RRect { + fn from(shape: &RRectShape) -> Self { + build_rrect(shape) + } +}crates/grida-canvas/src/shape/rect.rs (1)
25-50: Vector network builder looks correct.Vertices and segments form a closed clockwise rectangle; suitable for downstream consumers. Optionally, document winding and populate regions later if your consumers rely on it.
Also applies to: 37-44
editor/grida-canvas/editor.ts (1)
1665-1675: Clamp corner smoothing to [0,1] at the API edge.Prevents invalid values entering state; keeps UI and WASM in sync.
changeNodePropertyCornerSmoothing( node_id: editor.NodeID, cornerSmoothing: number ): void { - this.dispatch({ + const v = Math.max(0, Math.min(1, cornerSmoothing)); + this.dispatch({ type: "node/change/*", node_id: node_id, - cornerSmoothing, + cornerSmoothing: v, }); }crates/grida-canvas/src/cg/types.rs (1)
983-1007: Type is solid; consider From/Into ergonomics (and serde if needed).Add From/Into to ease conversions; optionally #[serde(transparent)] if you plan to serialize this internally (IO already maps via f32).
#[derive(Debug, Clone, Copy, PartialEq, PartialOrd)] pub struct CornerSmoothing(pub f32); +impl From<f32> for CornerSmoothing { + fn from(v: f32) -> Self { CornerSmoothing::new(v) } +} + +impl From<CornerSmoothing> for f32 { + fn from(cs: CornerSmoothing) -> Self { cs.0 } +}crates/grida-canvas/src/painter/geometry.rs (2)
200-216: Unify smoothing checks and micro‑nits.
- Prefer
!n.corner_smoothing.is_zero()overvalue() > 0.0for consistency withRectangle::to_shape()and to avoid magic thresholds.- Optional: extract the Container RRect construction into a helper to reduce duplication with the Image branch.
225-237: Reduce duplication by reusing shape conversion.Mirror Rectangle by introducing
ImageNodeRec::to_shape()(Rect/RRect/OrthogonalSmoothRRect) and then callPainterShape::from_shape(&shape). This centralizes branching and keeps painter lean.docs/math/g2-curve-blending.md.md (2)
5-5: Hyphenate compound adjective.Use “Computer‑Aided Geometric Design”.
-This document ... from CAGD (Computer Aided Geometric Design) ... +This document ... from CAGD (Computer‑Aided Geometric Design) ...
53-57: Consistent term: parameterization vs parametrization.Pick one form and use consistently; suggest “parameterization” to match typical CG literatures.
-**Parametrization (first quadrant):** +**Parameterization (first quadrant):**Also applies to: 148-149
crates/grida-canvas/src/io/io_grida.rs (1)
1238-1239: Propagation OK; add clamp tests and NaN guard.
- Using
CornerSmoothing::new(node.base.corner_smoothing.unwrap_or(0.0))is fine and clamps [0,1]. Add tests for inputs<0,>1, andNaN(treat NaN as 0.0).Apply tests via this diff:
@@ #[cfg(test)] mod tests { @@ + #[test] + fn deserialize_corner_smoothing_out_of_range_is_clamped() { + let json_hi = r#"{ + "id":"rect-hi","type":"rectangle","width":100,"height":100, + "cornerRadius":10,"cornerSmoothing": 1.7 + }"#; + let node_hi: JSONNode = serde_json::from_str(json_hi).unwrap(); + if let JSONNode::Rectangle(r) = node_hi { + let n: Node = r.into(); + if let Node::Rectangle(rec) = n { + assert_eq!(rec.corner_smoothing.value(), 1.0); + } + } + + let json_lo = r#"{ + "id":"rect-lo","type":"rectangle","width":100,"height":100, + "cornerRadius":10,"cornerSmoothing": -0.3 + }"#; + let node_lo: JSONNode = serde_json::from_str(json_lo).unwrap(); + if let JSONNode::Rectangle(r) = node_lo { + let n: Node = r.into(); + if let Node::Rectangle(rec) = n { + assert_eq!(rec.corner_smoothing.value(), 0.0); + } + } + } + + #[test] + fn deserialize_corner_smoothing_nan_falls_back_to_zero() { + // serde rejects non-numeric tokens, so simulate by constructing directly: + let s = CornerSmoothing::new(f32::NAN); + // If new() doesn't sanitize NaN, we should ensure downstream treats it as 0.0. + // For now, assert it's not finite and flag follow-up: + assert!(s.value().is_nan()); + }If you prefer to sanitize NaN in
CornerSmoothing::new, I can open a follow-up patch.Also applies to: 1462-1463, 1560-1561 </blockquote></details> <details> <summary>crates/grida-canvas/src/shape/srrect_orthogonal.rs (2)</summary><blockquote> `43-84`: **Numerical safety for Bézier spans.** `b = ((p - l) - (1+d_to_c)*c)/3` may go negative near extreme aspect ratios; consider clamping `a,b,c,d ≥ 0` to avoid inverted control handles. Happy to provide a follow-up with clamps and visual tests across angle/radius grids. --- `243-253`: **Strengthen tests.** Also assert closure; this catches future regressions. ```diff - let path = build_orthogonal_smooth_rrect_path(&shape); - assert!(!path.is_empty()); + let path = build_orthogonal_smooth_rrect_path(&shape); + assert!(!path.is_empty()); + assert!(path.is_last_contour_closed());crates/grida-canvas/src/node/schema.rs (1)
1016-1023: Consistency nit: useis_zero()idiomatically.Minor: replace
if self.corner_smoothing.is_zero()with a match on smoothing state across all call sites for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/grida-canvas/goldens/corner_smoothing.pngis excluded by!**/*.png
📒 Files selected for processing (23)
crates/grida-canvas/benches/bench_rectangles.rs(1 hunks)crates/grida-canvas/examples/golden_corner_smoothing.rs(1 hunks)crates/grida-canvas/examples/golden_layout_flex.rs(1 hunks)crates/grida-canvas/examples/golden_layout_flex_alignment.rs(1 hunks)crates/grida-canvas/examples/golden_layout_flex_padding.rs(1 hunks)crates/grida-canvas/examples/golden_layout_padding.rs(1 hunks)crates/grida-canvas/src/cg/types.rs(1 hunks)crates/grida-canvas/src/io/io_figma.rs(6 hunks)crates/grida-canvas/src/io/io_grida.rs(5 hunks)crates/grida-canvas/src/node/factory.rs(3 hunks)crates/grida-canvas/src/node/schema.rs(5 hunks)crates/grida-canvas/src/painter/geometry.rs(4 hunks)crates/grida-canvas/src/shape/mod.rs(4 hunks)crates/grida-canvas/src/shape/rect.rs(2 hunks)crates/grida-canvas/src/shape/rrect.rs(1 hunks)crates/grida-canvas/src/shape/srrect_orthogonal.rs(1 hunks)crates/math2/tests/bezier.rs(1 hunks)docs/math/g2-curve-blending.md.md(1 hunks)docs/math/superellipse.md(1 hunks)editor/grida-canvas/editor.i.ts(1 hunks)editor/grida-canvas/editor.ts(2 hunks)editor/grida-canvas/reducers/node.reducer.ts(1 hunks)packages/grida-canvas-schema/grida.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
Author and maintain all documentation under the root docs directory; it is the source of truth
Files:
docs/math/superellipse.mddocs/math/g2-curve-blending.md.md
packages/grida-canvas-*/**
📄 CodeRabbit inference engine (AGENTS.md)
Name canvas-related packages with the grida-canvas-* prefix under /packages
Files:
packages/grida-canvas-schema/grida.ts
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/benches/bench_rectangles.rscrates/grida-canvas/src/cg/types.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/src/shape/srrect_orthogonal.rscrates/grida-canvas/examples/golden_corner_smoothing.rscrates/grida-canvas/src/shape/rrect.rscrates/grida-canvas/src/node/factory.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/src/shape/rect.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/src/node/schema.rscrates/math2/tests/bezier.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/shape/mod.rscrates/grida-canvas/src/io/io_figma.rs
crates/grida-canvas/src/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/src/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers and not serialize them
Public APIs must accept and return UserNodeId (String) for stability and .grida serialization
Files:
crates/grida-canvas/src/cg/types.rscrates/grida-canvas/src/shape/srrect_orthogonal.rscrates/grida-canvas/src/shape/rrect.rscrates/grida-canvas/src/node/factory.rscrates/grida-canvas/src/shape/rect.rscrates/grida-canvas/src/painter/geometry.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/shape/mod.rscrates/grida-canvas/src/io/io_figma.rs
editor/grida-*/**
📄 CodeRabbit inference engine (AGENTS.md)
Use editor/grida-* directories to isolate domain-specific modules pending promotion to /packages
Files:
editor/grida-canvas/editor.i.tseditor/grida-canvas/editor.tseditor/grida-canvas/reducers/node.reducer.ts
🧠 Learnings (2)
📚 Learning: 2025-10-27T06:28:21.863Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-27T06:28:21.863Z
Learning: Applies to crates/grida-canvas/src/**/*.rs : All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers and not serialize them
Applied to files:
crates/grida-canvas/src/node/schema.rs
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/tests/serde_test.rs : Put JSON serialization tests in tests/serde_test.rs
Applied to files:
crates/grida-canvas/src/io/io_grida.rs
🧬 Code graph analysis (11)
editor/grida-canvas/editor.i.ts (1)
packages/grida-canvas-schema/grida.ts (1)
NodeID(1100-1100)
editor/grida-canvas/editor.ts (2)
editor/grida-canvas/editor.i.ts (1)
NodeID(134-134)packages/grida-canvas-schema/grida.ts (1)
NodeID(1100-1100)
crates/grida-canvas/src/shape/srrect_orthogonal.rs (1)
crates/grida-canvas/src/cg/types.rs (9)
b(79-81)a(82-84)new(20-22)new(823-828)new(987-989)new(1330-1336)new(1689-1696)circular(868-873)circular(933-935)
crates/grida-canvas/examples/golden_corner_smoothing.rs (6)
packages/grida-canvas-cg/lib.ts (1)
AffineTransform(65-68)crates/grida-canvas/src/cg/types.rs (32)
new(20-22)new(823-828)new(987-989)new(1330-1336)new(1689-1696)circular(868-873)circular(933-935)default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(356-358)default(376-378)default(392-394)default(422-424)default(446-448)default(548-550)default(560-562)default(572-574)default(605-607)default(653-658)default(729-731)from(88-94)from(263-265)from(1083-1085)from(1740-1742)from(1746-1748)from(1776-1781)from(1786-1791)from(1919-1921)from_rgb(69-71)crates/grida-canvas/src/node/factory.rs (1)
new(14-16)crates/grida-canvas/src/runtime/scene.rs (1)
new_from_raster(141-144)crates/grida-canvas/src/runtime/camera.rs (1)
new_from_bounds(56-73)crates/math2/src/rect.rs (1)
from_xywh(39-46)
crates/grida-canvas/src/shape/rrect.rs (2)
crates/grida-canvas/src/shape/mod.rs (2)
into(47-60)into(64-83)crates/grida-canvas/src/shape/rect.rs (2)
into(11-13)into(17-22)
crates/grida-canvas/src/shape/rect.rs (2)
crates/grida-canvas-wasm/lib/index.ts (2)
VectorNetwork(65-68)VectorNetworkSegment(59-64)crates/grida-canvas/src/vectornetwork/vn.rs (2)
vertices(448-448)ab(15-22)
crates/grida-canvas/src/painter/geometry.rs (3)
crates/math2/src/rect.rs (1)
from_xywh(39-46)crates/grida-canvas/src/shape/srrect_orthogonal.rs (1)
build_orthogonal_smooth_rrect_path(86-210)crates/grida-canvas/src/shape/rrect.rs (1)
build_rrect(20-36)
crates/grida-canvas/src/node/schema.rs (2)
crates/grida-canvas/src/painter/geometry.rs (1)
to_path(121-138)crates/grida-canvas/src/window/application.rs (2)
to_vector_network(46-46)to_vector_network(313-364)
crates/grida-canvas/src/io/io_grida.rs (1)
crates/grida-canvas/src/cg/types.rs (7)
new(20-22)new(823-828)new(987-989)new(1330-1336)new(1689-1696)value(993-995)value(1339-1341)
crates/grida-canvas/src/shape/mod.rs (3)
crates/grida-canvas/src/shape/rrect.rs (3)
build_rrect_path(38-42)into(15-17)build_rrect_vector_network(50-156)crates/grida-canvas/src/shape/srrect_orthogonal.rs (2)
build_orthogonal_smooth_rrect_path(86-210)build_orthogonal_smooth_rrect_vector_network(212-221)crates/grida-canvas/src/shape/rect.rs (3)
into(11-13)into(17-22)build_rect_vector_network(25-50)
crates/grida-canvas/src/io/io_figma.rs (1)
crates/grida-canvas/src/cg/types.rs (5)
new(20-22)new(823-828)new(987-989)new(1330-1336)new(1689-1696)
🪛 LanguageTool
docs/math/g2-curve-blending.md.md
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...dates established techniques from CAGD (Computer Aided Geometric Design) and relates them to p...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~53-~53: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...produces Apple‑style “icon” corners. Parametrization (first quadrant): $$ x(t)=a,|\cos t...
(EN_WORD_COHERENCY)
[uncategorized] ~148-~148: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e. - Hoschek & Lasser, Fundamentals of Computer Aided Geometric Design — blending and fairne...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (17)
crates/grida-canvas/examples/golden_layout_flex_alignment.rs (1)
27-27: LGTM: default corner_smoothing initialized.Consistent with factory defaults; no behavioral change for this example.
crates/grida-canvas/examples/golden_layout_padding.rs (1)
33-33: LGTM: corner_smoothing default added.Keeps examples aligned with schema without altering rendering.
crates/grida-canvas/examples/golden_layout_flex_padding.rs (1)
126-127: LGTM: default corner_smoothing set.Consistent with other initializations; no change to layout demo.
crates/grida-canvas/src/node/factory.rs (1)
62-63: All verification points confirmed. Corner smoothing wiring is correct.
- IO layer properly maps numeric
cornerSmoothingvalues toCornerSmoothingruntime type via deserialization (unwrap_or default to 0.0).- Painter correctly applies smoothing only when both
corner_smoothing.value() > 0.0AND corner radius is non-zero.- Tests comprehensively verify deserialization for rectangle, container, and image nodes.
- Reducer layer properly exposes the field.
crates/grida-canvas/examples/golden_layout_flex.rs (1)
31-31: Good addition — default-initialize new field.corner_smoothing: Default::default() aligns with the new field and maintains backward compatibility in the example constructor.
editor/grida-canvas/editor.ts (1)
4294-4294: No-op formatting change.Terminating semicolon on changeRotation is fine.
crates/grida-canvas/examples/golden_corner_smoothing.rs (1)
68-69: Remove unnecessary async/tokio – but note tokio is already available as a dev-dependency.The example performs no async work;
create_scene()andmain()are sync functions wrapped unnecessarily with async. Remove the async/tokio machinery to keep the example simple. Note: this won't cause a build failure since tokio is already declared in dev-dependencies; however, removing it improves code clarity.-async fn create_scene() -> Scene { +fn create_scene() -> Scene { let nf = NodeFactory::new(); let mut graph = SceneGraph::new(); … - } -} + } +} -#[tokio::main] -async fn main() { +fn main() { println!("=== Corner Smoothing Visual Test ===\n"); - // Render scene - let scene = create_scene().await; + // Render scene + let scene = create_scene(); … }Also applies to: 14-16
Likely an incorrect or invalid review comment.
crates/grida-canvas/src/painter/geometry.rs (2)
88-92: Nice extension of PainterShape::from_shape.Direct handling for Rect and RRect reduces path conversions and keeps bounds accurate. LGTM.
190-192: Good delegation to Rectangle::to_shape().Keeps painter logic thin and centralizes shape branching in schema. Approved.
crates/grida-canvas/src/io/io_grida.rs (1)
763-765: JSON API surface looks right.
cornerSmoothingadded with#[serde(rename = "cornerSmoothing", default)]is compatible and non‑breaking. Approved.crates/grida-canvas/src/node/schema.rs (3)
851-853: Corner smoothing fields added to nodes: good.Consistent modeling across Container/Rectangle/Image. Approved.
Also applies to: 949-952, 1087-1089
1009-1029: Shape branching reads well.
- Rect when radius=0, RRect when smoothing=0, otherwise OrthogonalSmoothRRect. Clear and extensible. Approved.
1031-1037: Centralizing to_path/to_vector_network on Shape is the right call.Note: Rectangle VN now relies on OrthogonalSmoothRRect VN; please ensure it’s implemented (currently returns empty). See suggested fix in srrect_orthogonal.rs.
Also applies to: 1251-1253
crates/grida-canvas/src/shape/mod.rs (3)
11-11: Re-export wiring LGTM.Module added and publicly re-exported cleanly.
Also applies to: 26-26
34-34: Enum extensions are coherent.New variants integrate cleanly with downstream matches.
Also applies to: 36-36
49-49: No issue found; theimpl Into<skia_safe::Path> for &RectShapeexists.The verification confirms that
impl Into<skia_safe::Path> for &RectShapeis defined atcrates/grida-canvas/src/shape/rect.rs:16. The codeShape::Rect(shape) => shape.into()will compile and work correctly with no changes needed.crates/grida-canvas/src/io/io_figma.rs (1)
766-766: Corner smoothing mapping is correct.Figma's
cornerSmoothingfield uses a 0.0–1.0 range, so the current implementation usingCornerSmoothing::new(component.corner_smoothing.unwrap_or(0.0) as f32)is appropriate. No scaling is needed. The safe [0,1] clamping viaCornerSmoothing::new()and sensible defaults for unsupported nodes are correct.Also applies to: 884-884, 956-956, 1105-1105, 1345-1345, 1633-1633
| impl Into<VectorNetwork> for &Shape { | ||
| fn into(self) -> VectorNetwork { | ||
| match self { | ||
| Shape::Rect(shape) => build_rect_vector_network(shape), | ||
| Shape::RRect(shape) => build_rrect_vector_network(shape), | ||
| Shape::OrthogonalSmoothRRect(shape) => { | ||
| build_orthogonal_smooth_rrect_vector_network(shape) | ||
| } | ||
| Shape::SimplePolygon(shape) => build_simple_polygon_vector_network(shape), |
There was a problem hiding this comment.
Vector network for smooth rrect: avoid returning an empty network; add safe fallback.
Current builder returns an empty network, which can break consumers expecting non-empty geometry. Guard with a fallback to regular RRect when smoothing is zero; keep the smooth version for non-zero smoothing.
Apply this diff to make the match safer:
- Shape::OrthogonalSmoothRRect(shape) => {
- build_orthogonal_smooth_rrect_vector_network(shape)
- }
+ Shape::OrthogonalSmoothRRect(shape) => {
+ if shape.corner_smoothing.value() == 0.0 {
+ // Fallback: ignore smoothing and emit standard RRect network
+ build_rrect_vector_network(&RRectShape {
+ width: shape.width,
+ height: shape.height,
+ corner_radius: shape.corner_radius,
+ })
+ } else {
+ build_orthogonal_smooth_rrect_vector_network(shape)
+ }
+ }Optionally, implement the fallback directly in srrect_orthogonal::build_orthogonal_smooth_rrect_vector_network so callers don’t need to special-case.
📝 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 Into<VectorNetwork> for &Shape { | |
| fn into(self) -> VectorNetwork { | |
| match self { | |
| Shape::Rect(shape) => build_rect_vector_network(shape), | |
| Shape::RRect(shape) => build_rrect_vector_network(shape), | |
| Shape::OrthogonalSmoothRRect(shape) => { | |
| build_orthogonal_smooth_rrect_vector_network(shape) | |
| } | |
| Shape::SimplePolygon(shape) => build_simple_polygon_vector_network(shape), | |
| impl Into<VectorNetwork> for &Shape { | |
| fn into(self) -> VectorNetwork { | |
| match self { | |
| Shape::Rect(shape) => build_rect_vector_network(shape), | |
| Shape::RRect(shape) => build_rrect_vector_network(shape), | |
| Shape::OrthogonalSmoothRRect(shape) => { | |
| if shape.corner_smoothing.value() == 0.0 { | |
| // Fallback: ignore smoothing and emit standard RRect network | |
| build_rrect_vector_network(&RRectShape { | |
| width: shape.width, | |
| height: shape.height, | |
| corner_radius: shape.corner_radius, | |
| }) | |
| } else { | |
| build_orthogonal_smooth_rrect_vector_network(shape) | |
| } | |
| } | |
| Shape::SimplePolygon(shape) => build_simple_polygon_vector_network(shape), |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/shape/mod.rs around lines 63 to 71, the
OrthogonalSmoothRRect arm can return an empty VectorNetwork when smoothing is
zero; change the match arm to check the shape's smoothing value and when
smoothing is zero call build_rrect_vector_network(shape) as a safe fallback,
otherwise call build_orthogonal_smooth_rrect_vector_network(shape);
alternatively implement this zero-smoothing guard inside
srrect_orthogonal::build_orthogonal_smooth_rrect_vector_network so callers need
not special-case it.
| // Circular arc | ||
| let arc_rect = | ||
| skia_safe::Rect::from_xywh(w - tr.radius * 2.0, 0.0, tr.radius * 2.0, tr.radius * 2.0); | ||
| let start_angle = 270.0 + tr.angle_bezier; | ||
| let sweep_angle = 90.0 - 2.0 * tr.angle_bezier; | ||
| path.arc_to(arc_rect, start_angle, sweep_angle, false); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid degenerate Skia arcs when sweep=0.
At full smoothing s=1, sweep_angle = 0. Guard arc emission to skip zero sweeps.
- let start_angle = 270.0 + tr.angle_bezier;
- let sweep_angle = 90.0 - 2.0 * tr.angle_bezier;
- path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ let start_angle = 270.0 + tr.angle_bezier;
+ let sweep_angle = 90.0 - 2.0 * tr.angle_bezier;
+ if sweep_angle > 0.0 {
+ path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ }
@@
- let start_angle = br.angle_bezier;
- let sweep_angle = 90.0 - 2.0 * br.angle_bezier;
- path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ let start_angle = br.angle_bezier;
+ let sweep_angle = 90.0 - 2.0 * br.angle_bezier;
+ if sweep_angle > 0.0 {
+ path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ }
@@
- let start_angle = 90.0 + bl.angle_bezier;
- let sweep_angle = 90.0 - 2.0 * bl.angle_bezier;
- path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ let start_angle = 90.0 + bl.angle_bezier;
+ let sweep_angle = 90.0 - 2.0 * bl.angle_bezier;
+ if sweep_angle > 0.0 {
+ path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ }
@@
- let start_angle = 180.0 + tl.angle_bezier;
- let sweep_angle = 90.0 - 2.0 * tl.angle_bezier;
- path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ let start_angle = 180.0 + tl.angle_bezier;
+ let sweep_angle = 90.0 - 2.0 * tl.angle_bezier;
+ if sweep_angle > 0.0 {
+ path.arc_to(arc_rect, start_angle, sweep_angle, false);
+ }Also applies to: 152-155, 173-178, 196-199
🤖 Prompt for AI Agents
In crates/grida-canvas/src/shape/srrect_orthogonal.rs around lines 121-127 (and
similarly at 152-155, 173-178, 196-199), the code emits Skia arcs even when
sweep_angle becomes zero at full smoothing, which produces degenerate arcs;
detect when the computed sweep_angle is zero (or effectively zero within a small
epsilon) and skip calling path.arc_to in that case (i.e., only call path.arc_to
when sweep_angle.abs() > epsilon), ensuring any related start_angle logic
remains consistent when the arc is skipped.
| For $n ≥ 2$: | ||
|
|
||
| - **C¹ continuous** at all points (smooth tangents, no kinks) | ||
| - **C^∞ continuous** (infinitely differentiable) everywhere | ||
| - Curvature transitions are smooth and continuous | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Revisit differentiability claim; likely too strong.
“C^∞ for n ≥ 2” is generally not true for non-integer exponents; smoothness at the axes depends on n. Suggest softening and citing a source.
Proposed wording:
-For $n ≥ 2$:
-- **C¹ continuous** at all points (smooth tangents, no kinks)
-- **C^∞ continuous** (infinitely differentiable) everywhere
-- Curvature transitions are smooth and continuous
+For $n \ge 2$:
+- **C¹ continuous** everywhere (smooth tangents)
+- Higher-order smoothness near the axes depends on the exponent (integer vs non‑integer n).
+- Curvature transitions are smooth; for larger n the shape approaches rectilinear corners.Please verify with a reputable math reference.
🌐 Web query:
Is the superellipse |x/a|^n + |y/b|^n = 1 C-infinity for n >= 2? How does differentiability at axes depend on n (integer vs non-integer)?
💡 Result:
Short answer: No — the superellipse is C^∞ only in the special n = 2 case (the ordinary ellipse). For every other n > 0 the boundary fails to be C^∞ at the four axis points (±a,0) and (0,±b); in fact for n > 2 and for 1 < n < 2 the curve is not C^1 at those axis points.
Reason (sketch): use the first-quadrant parametrisation
x(t)=a|cos t|^{2/n} sgn(cos t), y(t)=b|sin t|^{2/n} sgn(sin t), t→0+
so y(t) ~ b t^{2/n}. If 2/n = 1 (i.e. n=2) this is smooth; if 2/n < 1 (n>2) or 2/n < 1 (also true when 1<n<2) then y(t) has a non‑integer power <1 near the axis and its derivative with respect to t (hence the tangent vector) blows up or is not finite at t=0. Thus the curve is not C^1 at the axis points unless n=2. (Parametric form and these regularity facts are standard; see e.g. the Superellipse entry and discussions of the parametric form.). [1][2]
References:
[1] Wikipedia: “Superellipse” (Lamé curve) — equation and parametric form, comments on behaviour at the extreme points.
[2] Math StackExchange discussion of the superellipse parametric equation (parametrisation x=a|cos t|^{2/n}, y=b|sin t|^{2/n}).
Remove overstated differentiability claims; distinguish n=2 from n>2.
The superellipse is C^∞ only when n=2 (the ordinary ellipse); for every other n>0 the curve fails to be C^∞ at the four axis points. In fact, for n>2 and 1<n<2 the curve is not C^1 at those axis points. The reason: near the axis the parametric form behaves as y(t)~b·t^{2/n}, and when 2/n<1 the derivative is not finite at the axis.
Proposed wording:
-For $n ≥ 2$:
-- **C¹ continuous** at all points (smooth tangents, no kinks)
-- **C^∞ continuous** (infinitely differentiable) everywhere
-- Curvature transitions are smooth and continuous
+For $n = 2$ (ordinary ellipse): **C^∞ continuous** everywhere.
+
+For $n > 2$: **C¹ continuous** except at the four axis points (±a, 0) and (0, ±b), where the curve has a cusp (no smooth tangent).🤖 Prompt for AI Agents
In docs/math/superellipse.md around lines 70 to 75, remove the blanket claims
that superellipses are C¹ and C^∞ for n≥2 and instead distinguish cases by
exponent n: state that only n=2 gives C^∞ (ellipse), for 1<n<2 and n>2 the curve
fails to be C¹ or C^∞ at the four axis points respectively, and add a short
justification noting the parametric/local behavior y(t) ~ b·t^{2/n} which makes
the derivative non-finite when 2/n<1; update the three bullet lines accordingly
to reflect these corrected differentiability classes and include the brief
explanatory phrase about the 2/n exponent condition.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/grida-canvas/src/shape/srrect_orthogonal.rs (1)
121-127: Guard against degenerate arcs when sweep_angle is zero.At full smoothing (s=1.0),
sweep_angle = 90.0 - 2.0 * 45.0 = 0.0, which produces a degenerate arc in Skia. This issue was flagged in a previous review and remains unresolved. Skip thearc_tocall when the sweep is effectively zero.Apply this diff to all four corners (also at lines 152-155, 173-178, 196-199):
let start_angle = 270.0 + tr.angle_bezier; let sweep_angle = 90.0 - 2.0 * tr.angle_bezier; - path.arc_to(arc_rect, start_angle, sweep_angle, false); + if sweep_angle.abs() > f32::EPSILON { + path.arc_to(arc_rect, start_angle, sweep_angle, false); + }
🧹 Nitpick comments (9)
editor/grida-canvas/reducers/node.reducer.ts (1)
332-336: LGTM! Past review feedback correctly implemented.The clamping logic mirrors the
innerRadiuspattern (lines 343-347) and ensures state integrity by keeping values in the normalized [0, 1] range.Optional: Consider adding type assertion for consistency.
For consistency with similar properties like
innerRadius(line 344), you could optionally add a type assertion:cornerSmoothing: defineNodeProperty<"cornerSmoothing">({ + assert: (node) => typeof node.cornerSmoothing === "number", apply: (draft, value, prev) => { (draft as UN).cornerSmoothing = cmath.clamp(value, 0, 1); }, }),docs/math/g2-curve-blending.md (2)
5-5: Hyphenate compound adjectives.Lines 5 and 148 use "Computer Aided Geometric Design" as a compound adjective modifying a noun. English grammar requires hyphenation when compound adjectives precede the noun they modify.
Apply this diff:
- This document formalizes the problem of replacing a sharp join between two curve segments with a **curvature‑continuous (G²)** transition. It consolidates established techniques from CAGD (Computer Aided Geometric Design) and relates them to practical implementations used in modern editors. + This document formalizes the problem of replacing a sharp join between two curve segments with a **curvature‑continuous (G²)** transition. It consolidates established techniques from CAGD (Computer-Aided Geometric Design) and relates them to practical implementations used in modern editors.And:
- - Hoschek & Lasser, _Fundamentals of Computer Aided Geometric Design_ — blending and fairness. + - Hoschek & Lasser, _Fundamentals of Computer-Aided Geometric Design_ — blending and fairness.Also applies to: 148-148
53-53: Standardize spelling: use "parameterization" throughout.The document mixes "Parametrization" (line 53) with "parameterization" (lines 101, 103, 106). For consistency and professional presentation, use the American English spelling "parameterization" everywhere.
Apply this diff:
- **Parametrization (first quadrant):** + **Parameterization (first quadrant):**Also applies to: 101-101, 103-103, 106-106
crates/grida-canvas/src/shape/srrect_orthogonal.rs (6)
37-38: Consider removing unusedangle_circlefield.The
#[allow(dead_code)]attribute indicates this field is never read. If it's not needed for future functionality, remove both the field and its computation to simplify the code.
48-48: Use method syntax for consistency.Prefer
.min()method syntax overf32::minto align with Rust conventions and match the style used elsewhere (e.g., line 45).Apply this diff:
- let p = f32::min(shortest_side / 2.0, (1.0 + smoothness) * radius); + let p = (shortest_side / 2.0).min((1.0 + smoothness) * radius);
51-59: Clarify theshortest_side / 4.0threshold logic.The conditional angle computation uses
shortest_side / 4.0as a threshold with achange_percentagecalculation, but the rationale isn't documented. This makes the code harder to maintain and verify.Add a comment explaining why this threshold exists and how
change_percentageaffects the angle interpolation.
95-98: Use method syntax for min operations.Replace
f32::minwith.min()method calls for consistency with Rust idioms.Apply this diff:
- let tl_r = f32::min(shape.corner_radius.tl.rx, shape.corner_radius.tl.ry).max(0.0); - let tr_r = f32::min(shape.corner_radius.tr.rx, shape.corner_radius.tr.ry).max(0.0); - let br_r = f32::min(shape.corner_radius.br.rx, shape.corner_radius.br.ry).max(0.0); - let bl_r = f32::min(shape.corner_radius.bl.rx, shape.corner_radius.bl.ry).max(0.0); + let tl_r = shape.corner_radius.tl.rx.min(shape.corner_radius.tl.ry).max(0.0); + let tr_r = shape.corner_radius.tr.rx.min(shape.corner_radius.tr.ry).max(0.0); + let br_r = shape.corner_radius.br.rx.min(shape.corner_radius.br.ry).max(0.0); + let bl_r = shape.corner_radius.bl.rx.min(shape.corner_radius.bl.ry).max(0.0);
111-204: Prefer method syntax throughout path construction.Multiple lines use
f32::minandf32::maxinstead of method syntax (lines 111, 132, 137, 159, 164, 182, 187, 204). For consistency, use.min()and.max().Example for line 111:
- path.line_to((f32::max(w / 2.0, w - tr.p), 0.0)); + path.line_to(((w / 2.0).max(w - tr.p), 0.0));Apply similar changes to lines 132, 137, 159, 164, 182, 187, 204.
221-252: Consider expanding test coverage.The current tests cover basic smoothing parameters and verify the path is non-empty. Consider adding tests for:
- Intermediate smoothing values (e.g., 0.5)
- Edge cases (zero radius, very small dimensions)
- Asymmetric corner radii
- Vector network conversion output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/grida-canvas/src/shape/srrect_orthogonal.rs(1 hunks)docs/math/g2-curve-blending.md(1 hunks)editor/grida-canvas/reducers/node.reducer.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
editor/grida-*/**
📄 CodeRabbit inference engine (AGENTS.md)
Use editor/grida-* directories to isolate domain-specific modules pending promotion to /packages
Files:
editor/grida-canvas/reducers/node.reducer.ts
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/src/shape/srrect_orthogonal.rs
crates/grida-canvas/src/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/src/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers and not serialize them
Public APIs must accept and return UserNodeId (String) for stability and .grida serialization
Files:
crates/grida-canvas/src/shape/srrect_orthogonal.rs
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
Author and maintain all documentation under the root docs directory; it is the source of truth
Files:
docs/math/g2-curve-blending.md
🧬 Code graph analysis (1)
crates/grida-canvas/src/shape/srrect_orthogonal.rs (1)
crates/grida-canvas/src/cg/types.rs (9)
b(79-81)a(82-84)new(20-22)new(823-828)new(987-989)new(1330-1336)new(1689-1696)circular(868-873)circular(933-935)
🪛 LanguageTool
docs/math/g2-curve-blending.md
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...dates established techniques from CAGD (Computer Aided Geometric Design) and relates them to p...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~53-~53: Do not mix variants of the same word (‘parametrization’ and ‘parameterization’) within a single text.
Context: ...produces Apple‑style “icon” corners. Parametrization (first quadrant): $$ x(t)=a,|\cos t...
(EN_WORD_COHERENCY)
[uncategorized] ~148-~148: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e. - Hoschek & Lasser, Fundamentals of Computer Aided Geometric Design — blending and fairne...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: cargo test
🔇 Additional comments (2)
docs/math/g2-curve-blending.md (1)
1-151: Comprehensive and well-structured mathematical documentation.This document provides excellent formalization of G² curve blending suitable as a reference for implementation. The progression from problem statement → curvature formulas → canonical constructions → parameterization → robustness → terminology is logical and thorough. The Rust enum at §7 is appropriately positioned to bridge theory and code. References are curated and relevant to the topic.
crates/grida-canvas/src/shape/srrect_orthogonal.rs (1)
212-219: Fallback implementation resolves previous review concern.The vector network conversion now uses a fallback that converts the generated path to a VectorNetwork, addressing the previous review comment about returning empty geometry. This ensures editor and export functionality remain operational while a more structured implementation can be added later.
1 participant
core
editor
Update
The core shape feature is implemented, but we could not figure out a reasonably simple pipeline to make corner smoothing / superellipse to work with all types of shapes, thus we are not including this on editor's feature.
(yet the shape is accessible via api.)
Summary by CodeRabbit
New Features
Documentation