Feature/heart shape tool integration#4167
Conversation
c8f4faa to
125a795
Compare
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 3/5
- There is a concrete medium-severity risk in
node-graph/nodes/vector/src/generator_nodes.rs: whencleavage_angleis 0, the handles collapse to the same side of the anchor, creating a cusp (tangent reversal) instead of the documented smooth join behavior. - Because this is high-confidence (9/10) and affects generated curve geometry, it could produce visible user-facing shape artifacts if merged as-is.
- This looks contained to a specific parameter/behavior rather than a broad architectural problem, so the PR may still be mergeable with a targeted fix or follow-up.
- Pay close attention to
node-graph/nodes/vector/src/generator_nodes.rs- verifycleavage_angle=0handling matches the doc contract and avoids cusp formation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/nodes/vector/src/generator_nodes.rs">
<violation number="1" location="node-graph/nodes/vector/src/generator_nodes.rs:196">
P2: Doc comment for `cleavage_angle` claims zero produces a smooth join, but when angle is zero both in and out handles collapse to the same point on the same side of the anchor, producing a cusp (180° tangent reversal) instead of a smooth curve</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| #[hard_min(0.)] | ||
| #[hard_max(0.6)] | ||
| cleavage_depth: f64, | ||
| /// Half-angle of the top V. Zero collapses the V into a smooth join. |
There was a problem hiding this comment.
P2: Doc comment for cleavage_angle claims zero produces a smooth join, but when angle is zero both in and out handles collapse to the same point on the same side of the anchor, producing a cusp (180° tangent reversal) instead of a smooth curve
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/vector/src/generator_nodes.rs, line 196:
<comment>Doc comment for `cleavage_angle` claims zero produces a smooth join, but when angle is zero both in and out handles collapse to the same point on the same side of the anchor, producing a cusp (180° tangent reversal) instead of a smooth curve</comment>
<file context>
@@ -179,6 +179,110 @@ fn regular_polygon<T: AsU64>(
+ #[hard_min(0.)]
+ #[hard_max(0.6)]
+ cleavage_depth: f64,
+ /// Half-angle of the top V. Zero collapses the V into a smooth join.
+ #[default(45.)]
+ #[range((0., 89.))]
</file context>
There was a problem hiding this comment.
Code Review
This pull request introduces a new parametric Heart shape to the vector toolset. The implementation includes a new heart generator node with extensive parametric controls for cleavage, lobes, shoulders, and taper, alongside the necessary integration into the shape tool UI, FSM, and gizmo management system. Review feedback highlights a potential division-by-zero error when drawing shapes with zero dimensions and suggests a vertical offset adjustment in the generator node to ensure the heart is correctly centered within its local coordinate space.
| let mut scale = DVec2::ONE; | ||
| let radius: f64; | ||
| if dimensions.x > dimensions.y { | ||
| scale.x = dimensions.x / dimensions.y; | ||
| radius = dimensions.y / 2.; | ||
| } else { | ||
| scale.y = dimensions.y / dimensions.x; | ||
| radius = dimensions.x / 2.; | ||
| } |
There was a problem hiding this comment.
Potential division by zero or NaN result if dimensions.x or dimensions.y is zero (e.g., if the user clicks without dragging). This can lead to invalid transforms and rendering issues. A check should be added to ensure dimensions are non-zero before calculating the scale.
if dimensions.x < 1e-6 || dimensions.y < 1e-6 {
return;
}
let mut scale = DVec2::ONE;
let radius: f64;
if dimensions.x > dimensions.y {
scale.x = dimensions.x / dimensions.y;
radius = dimensions.y / 2.;
} else {
scale.y = dimensions.y / dimensions.x;
radius = dimensions.x / 2.;
}| // Anchor points for the right half plus the y-axis cusps, in normalized coordinates (y points downward). | ||
| let top = DVec2::new(0., -1. + cleavage_depth); | ||
| let shoulder = DVec2::new(shoulder_width, -shoulder_height); | ||
| let bottom = DVec2::new(0., 1.); |
There was a problem hiding this comment.
The heart shape is not centered at (0, 0) in its local coordinate space. The vertical range is from -1 + cleavage_depth to 1, meaning the midpoint is at cleavage_depth / 2. This causes the heart to appear shifted when drawn from the center or within a bounding box. It's better to offset the Y coordinates by cleavage_depth / 2 to center the shape.
| // Anchor points for the right half plus the y-axis cusps, in normalized coordinates (y points downward). | |
| let top = DVec2::new(0., -1. + cleavage_depth); | |
| let shoulder = DVec2::new(shoulder_width, -shoulder_height); | |
| let bottom = DVec2::new(0., 1.); | |
| // Anchor points for the right half plus the y-axis cusps, in normalized coordinates (y points downward). | |
| let y_offset = cleavage_depth / 2.; | |
| let top = DVec2::new(0., -1. + cleavage_depth - y_offset); | |
| let shoulder = DVec2::new(shoulder_width, -shoulder_height - y_offset); | |
| let bottom = DVec2::new(0., 1. - y_offset); |
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/common_functionality/gizmos/generic_gizmos/generic_dial_gizmo.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/gizmos/generic_gizmos/generic_dial_gizmo.rs:100">
P2: Dial remains interactive while hidden due to mismatched visibility thresholds</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/gizmos/generic_gizmos/generic_slider_gizmo.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/gizmos/generic_gizmos/generic_slider_gizmo.rs:136">
P2: Drag math in `handle_update()` can flip parameter sign when the mouse crosses the local origin, contradicting the comment that claims sign is preserved. The handle is always rendered at `value.abs()` on the +X axis, so the update logic should derive magnitude with `.abs()` before re-applying the original sign.</violation>
</file>
<file name="editor/src/messages/tool/common_functionality/gizmos/gizmo_registry.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/gizmos/gizmo_registry.rs:1">
P2: Custom agent: **PR title enforcement**
PR title violates PR title enforcement rules: uses a conventional commit-like prefix ('Feature/'), lacks an imperative leading verb, and uses lowercase after the subsystem prefix</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| // Hide the dial when the shape is too small on screen. | ||
| let extent = viewport.transform_point2(DVec2::new(1., 0.)).distance(center); | ||
| if extent < f64::EPSILON { |
There was a problem hiding this comment.
P2: Dial remains interactive while hidden due to mismatched visibility thresholds
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/common_functionality/gizmos/generic_gizmos/generic_dial_gizmo.rs, line 100:
<comment>Dial remains interactive while hidden due to mismatched visibility thresholds</comment>
<file context>
@@ -0,0 +1,175 @@
+
+ // Hide the dial when the shape is too small on screen.
+ let extent = viewport.transform_point2(DVec2::new(1., 0.)).distance(center);
+ if extent < f64::EPSILON {
+ self.state = GenericDialState::Inactive;
+ return;
</file context>
| let viewport = document.metadata().transform_to_viewport(self.layer); | ||
| let local_mouse = viewport.inverse().transform_point2(input.mouse.position); | ||
|
|
||
| let mut value = local_mouse.x; |
There was a problem hiding this comment.
P2: Drag math in handle_update() can flip parameter sign when the mouse crosses the local origin, contradicting the comment that claims sign is preserved. The handle is always rendered at value.abs() on the +X axis, so the update logic should derive magnitude with .abs() before re-applying the original sign.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/common_functionality/gizmos/generic_gizmos/generic_slider_gizmo.rs, line 136:
<comment>Drag math in `handle_update()` can flip parameter sign when the mouse crosses the local origin, contradicting the comment that claims sign is preserved. The handle is always rendered at `value.abs()` on the +X axis, so the update logic should derive magnitude with `.abs()` before re-applying the original sign.</comment>
<file context>
@@ -0,0 +1,188 @@
+ let viewport = document.metadata().transform_to_viewport(self.layer);
+ let local_mouse = viewport.inverse().transform_point2(input.mouse.position);
+
+ let mut value = local_mouse.x;
+
+ // Preserve the sign of the original value for parameters (like radius) that can be negative.
</file context>
| @@ -0,0 +1,279 @@ | |||
| //! # Gizmo Registry | |||
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title violates PR title enforcement rules: uses a conventional commit-like prefix ('Feature/'), lacks an imperative leading verb, and uses lowercase after the subsystem prefix
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/common_functionality/gizmos/gizmo_registry.rs:
<comment>PR title violates PR title enforcement rules: uses a conventional commit-like prefix ('Feature/'), lacks an imperative leading verb, and uses lowercase after the subsystem prefix</comment>
<file context>
@@ -0,0 +1,279 @@
+//! # Gizmo Registry
+//!
+//! A data-driven lookup that maps node types to the parameters that should be exposed as
+//! interactive canvas gizmos. This is the foundation of the *generic* gizmo system: instead of
+//! writing a bespoke handler for every shape (see the `shape_gizmos` module for the legacy,
+//! hand-written handlers), a node simply declares which of its inputs are gizmo-enabled here and
+//! the generic gizmo manager builds the appropriate interactive handles automatically.
+//!
+//! To add gizmos to a new node:
</file context>
Add Heart drawing mode to the Shape tool with gizmo registration
Screen.Recording.2026-05-22.at.1.35.34.PM.mov