Add teardrop and heard shape to the predefined shapes by LennartKDeveloper · Pull Request #3998 · GraphiteEditor/Graphite

LennartKDeveloper · 2026-04-03T12:03:01Z

Closes #3419

Screen.Recording.2026-04-03.at.13.20.45.mov

This PR adds teardrop and heart shapes to the shape tool options. It fully integrates both shapes and implements them in core.rs.
Additionally, the HANDLE_OFFSET_FACTOR constant is extracted and properly documented.

cubic-dev-ai

3 issues found across 10 files

Confidence score: 2/5

There is clear functional risk in shape handling: editor/src/messages/tool/common_functionality/gizmos/gizmo_manager.rs checks get_spiral_id for Teardrop/Heart, so the correct gizmo handlers may not be selected during editing.
editor/src/messages/tool/common_functionality/graph_modification_utils.rs has a concrete node-type mismatch (get_teardrop_id resolving spiral), which can propagate wrong behavior anywhere teardrop detection is expected.
editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rs can divide by zero on zero-sized drags, producing inf/NaN transforms and likely unstable user-facing geometry behavior.
Pay close attention to editor/src/messages/tool/common_functionality/gizmos/gizmo_manager.rs, editor/src/messages/tool/common_functionality/graph_modification_utils.rs, editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rs - identifier mix-ups and zero-drag math need correction before merge.

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/gizmo_manager.rs">

<violation number="1" location="editor/src/messages/tool/common_functionality/gizmos/gizmo_manager.rs:241">
P1: Teardrop and Heart gizmo detection incorrectly checks `get_spiral_id`, so new shape handlers are not selected correctly.</violation>
</file>

<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:139">
P1: Division by zero in teardrop scale computation can generate `inf`/`NaN` transforms for zero-sized drags.</violation>
</file>

<file name="editor/src/messages/tool/common_functionality/graph_modification_utils.rs">

<violation number="1" location="editor/src/messages/tool/common_functionality/graph_modification_utils.rs:381">
P1: `get_teardrop_id` looks up the spiral identifier, so teardrop node detection returns the wrong node type.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

editor/src/messages/tool/common_functionality/gizmos/gizmo_manager.rs

editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rs

editor/src/messages/tool/common_functionality/graph_modification_utils.rs

Keavon · 2026-04-03T12:08:02Z

Thanks for the contribution. As it stands, this will not work because it requires properly parametric controls over all the unique variations on each of these shapes. I see only a radius is implemented, which is insufficient to give full artistic control over the forms of the shapes. Additionally, the tool needs to draw, not just place, the shapes. I am marking this as a draft to give you time to make these updates. Note that this is a research-focused topic as you'll have to find high-quality shape parameters and algorithms to create them. If they do not meet our requirements for intuitive and flexible artistic control, we will unfortunately be unable to merge the feature. Good luck!

gemini-code-assist

Code Review

This pull request introduces Heart and Teardrop shapes to the editor. Several critical issues were identified, including copy-paste errors in the gizmo manager and graph modification utilities where spiral identifiers were incorrectly used for the new shapes. Additionally, the gizmo handlers for both shapes appear to be copied from other tools, leading to inappropriate UI controls, and the generator nodes currently hardcode parameters that should be exposed. I have provided suggestions to fix the identifier lookups and the tool creation calls, and I recommend refactoring the gizmo handlers and node parameters to better suit the new shapes.

gemini-code-assist · 2026-04-03T12:09:43Z

editor/src/messages/tool/common_functionality/gizmos/gizmo_manager.rs

+		if graph_modification_utils::get_spiral_id(layer, &document.network_interface).is_some() {
+			return Some(ShapeGizmoHandlers::Teardrop(TeardropGizmoHandler::default()));
+		}
+		// Heart
+		if graph_modification_utils::get_spiral_id(layer, &document.network_interface).is_some() {
+			return Some(ShapeGizmoHandlers::Heart(HeartGizmoHandler::default()));


There seems to be a copy-paste error here. You are using get_spiral_id to detect Teardrop and Heart shapes. You should use get_teardrop_id and get_heart_id respectively.

Suggested change

if graph_modification_utils::get_spiral_id(layer, &document.network_interface).is_some() {

return Some(ShapeGizmoHandlers::Teardrop(TeardropGizmoHandler::default()));

}

// Heart

if graph_modification_utils::get_spiral_id(layer, &document.network_interface).is_some() {

return Some(ShapeGizmoHandlers::Heart(HeartGizmoHandler::default()));

if graph_modification_utils::get_teardrop_id(layer, &document.network_interface).is_some() {

return Some(ShapeGizmoHandlers::Teardrop(TeardropGizmoHandler::default()));

}

// Heart

if graph_modification_utils::get_heart_id(layer, &document.network_interface).is_some() {

return Some(ShapeGizmoHandlers::Heart(HeartGizmoHandler::default()));

}

gemini-code-assist · 2026-04-03T12:09:43Z

editor/src/messages/tool/common_functionality/graph_modification_utils.rs

+pub fn get_teardrop_id(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {
+	NodeGraphLayer::new(layer, network_interface).upstream_node_id_from_name(&DefinitionIdentifier::ProtoNode(graphene_std::vector_nodes::spiral::IDENTIFIER))
+}
+
+pub fn get_heart_id(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {
+	NodeGraphLayer::new(layer, network_interface).upstream_node_id_from_name(&DefinitionIdentifier::ProtoNode(graphene_std::vector_nodes::spiral::IDENTIFIER))
+}


It looks like there's a copy-paste error in these new functions. Both get_teardrop_id and get_heart_id are checking for the spiral identifier. They should be checking for their respective shape identifiers.

Suggested change

pub fn get_teardrop_id(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {

NodeGraphLayer::new(layer, network_interface).upstream_node_id_from_name(&DefinitionIdentifier::ProtoNode(graphene_std::vector_nodes::spiral::IDENTIFIER))

}

pub fn get_heart_id(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {

NodeGraphLayer::new(layer, network_interface).upstream_node_id_from_name(&DefinitionIdentifier::ProtoNode(graphene_std::vector_nodes::spiral::IDENTIFIER))

}

pub fn get_teardrop_id(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {

NodeGraphLayer::new(layer, network_interface).upstream_node_id_from_name(&DefinitionIdentifier::ProtoNode(graphene_std::vector::generator_nodes::teardrop::IDENTIFIER))

}

pub fn get_heart_id(layer: LayerNodeIdentifier, network_interface: &NodeNetworkInterface) -> Option<NodeId> {

NodeGraphLayer::new(layer, network_interface).upstream_node_id_from_name(&DefinitionIdentifier::ProtoNode(graphene_std::vector::generator_nodes::heart::IDENTIFIER))

}

gemini-code-assist · 2026-04-03T12:09:43Z

editor/src/messages/tool/common_functionality/shapes/heart_shape.rs

+pub struct HeartGizmoHandler {
+	number_of_points_dial: NumberOfPointsDial,
+	point_radius_handle: PointRadiusHandle,


The HeartGizmoHandler seems to be using gizmos for number_of_points_dial and point_radius_handle, which appear to be copied from another shape handler. The number_of_points_dial is not applicable to a heart shape. While point_radius_handle could potentially be used to control a property like the spike length, the heart generator node currently doesn't expose such a parameter. This will lead to incorrect and confusing behavior for the user. Please implement gizmos appropriate for the heart shape.

gemini-code-assist · 2026-04-03T12:09:43Z

editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rs

+pub struct TeardropGizmoHandler {
+	number_of_points_dial: NumberOfPointsDial,
+	point_radius_handle: PointRadiusHandle,


The TeardropGizmoHandler seems to be using gizmos for number_of_points_dial and point_radius_handle, which appear to be copied from another shape handler. The number_of_points_dial is not applicable to a teardrop shape. While point_radius_handle could potentially be used to control a property like the spike length, the teardrop generator node currently doesn't expose such a parameter. This will lead to incorrect and confusing behavior for the user. Please implement gizmos appropriate for the teardrop shape.

gemini-code-assist · 2026-04-03T12:09:43Z

node-graph/nodes/vector/src/generator_nodes.rs

+fn teardrop(
+	_: impl Ctx,
+	_primary: (),
+	#[unit(" px")]
+	#[default(50.)]
+	radius: f64,
+) -> Table<Vector> {
+	let radius = radius.abs();
+	Table::new_from_element(Vector::from_subpath(subpath::Subpath::new_teardrop(DVec2::ZERO, radius, radius * 2.0)))
+}


The teardrop node hardcodes the spike_radius to radius * 2.0. This limits the shape's customizability. It would be better to expose spike_radius as a parameter to this node. This would also allow a gizmo to control it, which seems to be the intention with the point_radius_handle in teardrop_shape.rs.

gemini-code-assist · 2026-04-03T12:09:43Z

node-graph/nodes/vector/src/generator_nodes.rs

+fn heart(
+	_: impl Ctx,
+	_primary: (),
+	#[unit(" px")]
+	#[default(50.)]
+	radius: f64,
+) -> Table<Vector> {
+	let radius = radius.abs();
+	Table::new_from_element(Vector::from_subpath(subpath::Subpath::new_heart(DVec2::ZERO, radius, radius * 2.0)))
+}


The heart node hardcodes the spike_radius to radius * 2.0. This limits the shape's customizability. It would be better to expose spike_radius as a parameter to this node. This would also allow a gizmo to control it, which seems to be the intention with the point_radius_handle in heart_shape.rs.

gemini-code-assist · 2026-04-03T12:09:43Z

editor/src/messages/tool/common_functionality/shapes/heart_shape.rs

+			// TODO: We need to determine how to allow the polygon node to make irregular shapes
+			update_radius_sign(end, start, layer, document, responses);


This TODO comment seems to be a leftover from another file. Also, update_radius_sign is likely not applicable to the heart shape as it's intended for flipping regular polygons and stars. Please remove the comment and verify if the function call is necessary.

gemini-code-assist · 2026-04-03T12:09:44Z

editor/src/messages/tool/common_functionality/shapes/teardrop_shape.rs

+			// TODO: We need to determine how to allow the polygon node to make irregular shapes
+			update_radius_sign(end, start, layer, document, responses);


This TODO comment seems to be a leftover from another file. Also, update_radius_sign is likely not applicable to the teardrop shape as it's intended for flipping regular polygons and stars. Please remove the comment and verify if the function call is necessary.

gemini-code-assist · 2026-04-03T12:09:44Z

editor/src/messages/tool/tool_messages/shape_tool.rs

+					ShapeType::Teardrop => Teardrop::create_node(tool_options.vertices), // FIXME
+					ShapeType::Heart => Heart::create_node(tool_options.vertices),       // FIXME


As noted by your FIXME comments, Teardrop::create_node and Heart::create_node are incorrectly passed tool_options.vertices. The _vertices parameter in those functions is unused and should be removed. The calls here should be updated accordingly.

Suggested change

ShapeType::Teardrop => Teardrop::create_node(tool_options.vertices), // FIXME

ShapeType::Heart => Heart::create_node(tool_options.vertices), // FIXME

ShapeType::Teardrop => Teardrop::create_node(),

ShapeType::Heart => Heart::create_node(),

…pulation

… refined the heart constrains

LennartKDeveloper · 2026-04-03T16:15:37Z

Screen.Recording.2026-04-03.at.18.00.48.mov

@Keavon Thank you very much for the feedback. I fixed the issues Gemini suggested and considered how the teardrop and heart shapes should be mutable.

The teardrop is now defined by the body radius and tail_length (distance from the spike to the center).
The heart is defined by height, width, and the roundness of the two top lobes.

To be honest, I didn’t fully understand what you meant by “drawable” in the context of these shapes. Could you clarify that part for me?

I hope this is moving in the direction you had in mind.
I’ll spend some more time researching how to further refine the shape parameters.

LennartKDeveloper added 2 commits April 3, 2026 13:25

added teardrop and heard shape to the predefined shaped

7c89020

Move HANDLE_OFFSET_FACTOR constant to consts.rs and added description

a9e66ca

cubic-dev-ai bot reviewed Apr 3, 2026

View reviewed changes

Keavon marked this pull request as draft April 3, 2026 12:08

gemini-code-assist bot reviewed Apr 3, 2026

View reviewed changes

LennartKDeveloper added 3 commits April 3, 2026 16:16

Fix incorrect references introduced in the branch initial commit

1230d30

Add width and height to heart, tail_length to teardrop for shape mani…

9cfb347

…pulation

Add top_lobe_roundness to heart for independent lobe manipulation and…

ec4ee3e

… refined the heart constrains

		// TODO: We need to determine how to allow the polygon node to make irregular shapes
		update_radius_sign(end, start, layer, document, responses);

		ShapeType::Teardrop => Teardrop::create_node(tool_options.vertices), // FIXME
		ShapeType::Heart => Heart::create_node(tool_options.vertices), // FIXME

Uh oh!

Conversation

LennartKDeveloper commented Apr 3, 2026

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Keavon commented Apr 3, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

LennartKDeveloper commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants