Add new Teardrop shape - #4466
Conversation
There was a problem hiding this comment.
4 issues found across 8 files
Confidence score: 2/5
editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rscan panic increate_nodewhen the Teardrop definition is unregistered, crashing the editor when drawing begins; handle the missing definition locally or propagate the failure instead of callingexpect.node-graph/nodes/vector/src/generator_nodes.rsallows zero or negativevelocitythrough its soft range, soteardrop_bezpathcan divide by an invalid value and generate broken geometry; add a hard lower bound such as#[hard(1..)].node-graph/libraries/vector-types/src/vector/algorithms/shapes.rsindependently divides by non-positivevelocityincircle_centerandvertical_handle_offset, producing non-finite path coordinates for typed or graph-provided inputs; validate the value before these calculations.editor/src/messages/tool/tool_messages/shape_tool.rsomits the Teardropvelocitycontrol and selection synchronization, preventing users from editing the parameter through the Shape tool; add the control, tool state/update variant, and synchronization.
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/shapes/teardrop_shape.rs">
<violation number="1" location="editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rs:17">
P1: When the Teardrop node is not registered, `create_node` panics while starting a draw and brings down the editor. Handle the missing definition locally or propagate the failure instead of calling `expect`.</violation>
</file>
<file name="node-graph/nodes/vector/src/generator_nodes.rs">
<violation number="1" location="node-graph/nodes/vector/src/generator_nodes.rs:93">
P1: When a node input supplies `velocity` at or below zero, the soft range does not prevent invalid geometry: `teardrop_bezpath` divides by this value. Add a hard lower bound such as `#[hard(1..)]` before the soft range.</violation>
</file>
<file name="node-graph/libraries/vector-types/src/vector/algorithms/shapes.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/vector/algorithms/shapes.rs:268">
P2: When `velocity` is zero or negative, this function divides by it in both `circle_center` and `vertical_handle_offset`, emitting non-finite path coordinates. The node declares only a soft range, so typed or graph-provided values are not hard-clamped; validate a finite positive velocity before these calculations.</violation>
</file>
<file name="editor/src/messages/tool/tool_messages/shape_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/shape_tool.rs:418">
P2: The Shape tool does not expose the teardrop's `velocity` parameter. Add a velocity control, tool state/update variant, and selection synchronization instead of treating Teardrop as a parameterless shape.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
|
|
||
| impl Teardrop { | ||
| pub fn create_node() -> NodeTemplate { | ||
| let node_type = resolve_proto_node_type(graphene_std::vector::generator_nodes::teardrop::IDENTIFIER).expect("Teardrop node can't be found"); |
There was a problem hiding this comment.
P1: When the Teardrop node is not registered, create_node panics while starting a draw and brings down the editor. Handle the missing definition locally or propagate the failure instead of calling expect.
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/shapes/teardrop_shape.rs, line 17:
<comment>When the Teardrop node is not registered, `create_node` panics while starting a draw and brings down the editor. Handle the missing definition locally or propagate the failure instead of calling `expect`.</comment>
<file context>
@@ -0,0 +1,50 @@
+
+impl Teardrop {
+ pub fn create_node() -> NodeTemplate {
+ let node_type = resolve_proto_node_type(graphene_std::vector::generator_nodes::teardrop::IDENTIFIER).expect("Teardrop node can't be found");
+ node_type.node_template_input_override([None, Some(NodeInput::value(TaggedValue::F64(0.5), false)), Some(NodeInput::value(TaggedValue::F64(0.5), false))])
+ }
</file context>
| height: Item<f64>, | ||
| #[default(1.7)] | ||
| #[range] | ||
| #[soft(1.4..3.8)] |
There was a problem hiding this comment.
P1: When a node input supplies velocity at or below zero, the soft range does not prevent invalid geometry: teardrop_bezpath divides by this value. Add a hard lower bound such as #[hard(1..)] before the soft range.
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 93:
<comment>When a node input supplies `velocity` at or below zero, the soft range does not prevent invalid geometry: `teardrop_bezpath` divides by this value. Add a hard lower bound such as `#[hard(1..)]` before the soft range.</comment>
<file context>
@@ -77,6 +77,39 @@ fn spiral(
+ height: Item<f64>,
+ #[default(1.7)]
+ #[range]
+ #[soft(1.4..3.8)]
+ velocity: Item<f64>,
+) -> Item<Vector> {
</file context>
| #[soft(1.4..3.8)] | |
| #[hard(1..)] | |
| #[soft(1.4..3.8)] |
| let size = (corner1 - corner2).abs(); | ||
|
|
||
| // the bottom half of the teardrop is a circle, upon which these calculations are heavily based | ||
| let circle_center = DVec2::new((corner1.x + corner2.x) / 2., (corner1.y + (2. * velocity - 1.) * corner2.y) / (2. * velocity)); |
There was a problem hiding this comment.
P2: When velocity is zero or negative, this function divides by it in both circle_center and vertical_handle_offset, emitting non-finite path coordinates. The node declares only a soft range, so typed or graph-provided values are not hard-clamped; validate a finite positive velocity before these calculations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/vector/algorithms/shapes.rs, line 268:
<comment>When `velocity` is zero or negative, this function divides by it in both `circle_center` and `vertical_handle_offset`, emitting non-finite path coordinates. The node declares only a soft range, so typed or graph-provided values are not hard-clamped; validate a finite positive velocity before these calculations.</comment>
<file context>
@@ -260,6 +260,41 @@ pub fn spiral_bezpath(a: f64, outer_radius: f64, turns: f64, start_angle: f64, d
+ let size = (corner1 - corner2).abs();
+
+ // the bottom half of the teardrop is a circle, upon which these calculations are heavily based
+ let circle_center = DVec2::new((corner1.x + corner2.x) / 2., (corner1.y + (2. * velocity - 1.) * corner2.y) / (2. * velocity));
+
+ let top = DVec2::new(circle_center.x, corner1.y);
</file context>
| let circle_center = DVec2::new((corner1.x + corner2.x) / 2., (corner1.y + (2. * velocity - 1.) * corner2.y) / (2. * velocity)); | |
| \tlet velocity = if velocity.is_finite() && velocity > 0. { velocity } else { 1.7 };\n\tlet circle_center = DVec2::new((corner1.x + corner2.x) / 2., (corner1.y + (2. * velocity - 1.) * corner2.y) / (2. * velocity)); |
| } | ||
| } | ||
| ShapeType::Ellipse | ShapeType::Rectangle | ShapeType::Line | ShapeType::Circle => {} | ||
| ShapeType::Ellipse | ShapeType::Rectangle | ShapeType::Line | ShapeType::Circle | ShapeType::Teardrop => {} |
There was a problem hiding this comment.
P2: The Shape tool does not expose the teardrop's velocity parameter. Add a velocity control, tool state/update variant, and selection synchronization instead of treating Teardrop as a parameterless shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/shape_tool.rs, line 418:
<comment>The Shape tool does not expose the teardrop's `velocity` parameter. Add a velocity control, tool state/update variant, and selection synchronization instead of treating Teardrop as a parameterless shape.</comment>
<file context>
@@ -407,7 +415,7 @@ fn sync_shape_options_from_selection(options: &mut ShapeToolOptions, tool_data:
}
}
- ShapeType::Ellipse | ShapeType::Rectangle | ShapeType::Line | ShapeType::Circle => {}
+ ShapeType::Ellipse | ShapeType::Rectangle | ShapeType::Line | ShapeType::Circle | ShapeType::Teardrop => {}
}
</file context>
This PR adds Teardrop as an option within the Shape tool. Partially closes #3419
In addition to the parameters for width and height, a slider has been added to adjust the velocity of the shape. This is because, during testing, I found that a variety of aspect ratios/steepnesses/velocities, looked good and thought it would be worth exposing for the user to decide.
Three teardrops, with varying stroke styles, widths, and velocities, the center being the default:

^ I originally recorded a video, but it's just me drawing these shapes slowly so I figured it'd be better to just screenshot the end result.
Potential future work
velocity, as well as location and direction for the top pointOther notes
No AI was used whatsoever throughout this PR.
This code is a modified copy of the implementations for the spiral and ellipse tools, in case that's important.
This is basically my first real pull request, so a little help in making sure I'm going about this the right way / not forgetting anything would be greatly appreciated.
When I started work on this, I forgot to check for other PRs on the same issue. I realize now that other people have also worked on this, and I understand if this PR doesn't get merged because of one of them.
Sorry about splitting some stuff onto multiple lines. My IDE does it automatically, and I was planning on reverting it but I saw one of the other PRs also doing it and it didn't seem to be an issue there, so I ended up leaving it. I'm happy to undo it if that'd be preferable.