Pen/Freehand/Splines tool set its origin to the first drawn point#3795
Pen/Freehand/Splines tool set its origin to the first drawn point#3795jsjgdh wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello @jsjgdh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where vector drawing tools were not correctly establishing the origin for newly created layers. By modifying the layer creation process to explicitly include and configure a Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request addresses issue #2776 by modifying the origin of Pen/Freehand/Splines tools to the first drawn point. This involves adding a first_anchor variable to capture the initial point and then using it to set the ORIGIN_OFFSET_INDEX of the Transform node. The changes are consistent across utility_types.rs, graph_modification_utils.rs, freehand_tool.rs, pen_tool.rs, shape_tool.rs, and spline_tool.rs to ensure the new behavior is applied to all relevant tools. The modifications appear to correctly implement the desired functionality.
| } | ||
|
|
||
| pub fn insert_vector(&mut self, subpaths: Vec<Subpath<PointId>>, layer: LayerNodeIdentifier, include_transform: bool, include_fill: bool, include_stroke: bool) { | ||
| let first_anchor = subpaths.first().and_then(|subpath| subpath.manipulator_groups().first().map(|group| group.anchor)); |
There was a problem hiding this comment.
This line correctly captures the first anchor point from the subpaths, which is then used to set the origin offset for the transform node. This is a good approach to ensure the tool's origin is anchored to the initial drawing point.
| let mut transform = resolve_network_node_type("Transform").expect("Transform node does not exist").default_node_template(); | ||
| if let Some(anchor) = first_anchor { | ||
| const ORIGIN_OFFSET_INDEX: usize = 5; | ||
| if let Some(origin_offset) = transform.document_node.inputs.get_mut(ORIGIN_OFFSET_INDEX) { | ||
| *origin_offset = NodeInput::value(TaggedValue::DVec2(anchor), false); | ||
| } |
There was a problem hiding this comment.
The logic here correctly applies the first_anchor to the ORIGIN_OFFSET_INDEX of the Transform node. This ensures that the vector layer's transform is correctly initialized with the starting point of the drawn path.
| let transform_node_id = NodeId::new(); | ||
| let mut nodes = nodes; | ||
|
|
||
| // Insert a Transform node | ||
| let transform_node = crate::messages::portfolio::document::node_graph::document_node_definitions::resolve_network_node_type("Transform") | ||
| .expect("Transform node does not exist") | ||
| .default_node_template(); | ||
| nodes.push((transform_node_id, transform_node)); |
There was a problem hiding this comment.
This new section introduces a Transform node into the custom layer creation. While this is necessary for the new origin functionality, it might be beneficial to pass the transform_node_id and transform_node as arguments to new_custom if they are always going to be the same, to avoid redundant code in the calling sites. However, given the context of setting the origin, this approach is acceptable for now.
| use crate::messages::tool::common_functionality::utility_functions::should_extend; | ||
| use glam::DVec2; | ||
| use graph_craft::document::NodeId; | ||
| use graph_craft::document::{NodeId, NodeInput}; |
There was a problem hiding this comment.
Adding NodeInput to the use statement is necessary for the new Transform node logic. This is a correct and required change.
| let mut transform_node = resolve_network_node_type("Transform").expect("Transform node does not exist").default_node_template(); | ||
| if let Some(input) = transform_node.document_node.inputs.get_mut(0) { | ||
| *input = NodeInput::node(NodeId(1), 0); | ||
| } | ||
| const ORIGIN_OFFSET_INDEX: usize = 5; | ||
| if let Some(origin_offset) = transform_node.document_node.inputs.get_mut(ORIGIN_OFFSET_INDEX) { | ||
| *origin_offset = NodeInput::value(graph_craft::document::value::TaggedValue::DVec2(document_space_position), false); | ||
| } |
There was a problem hiding this comment.
This block correctly initializes a Transform node and sets its ORIGIN_OFFSET_INDEX to the document_space_position. This ensures that the freehand tool's origin is set to the first drawn point, aligning with the PR's objective.
| let mut transform_node = crate::messages::portfolio::document::node_graph::document_node_definitions::resolve_network_node_type("Transform") | ||
| .expect("Transform node does not exist") | ||
| .default_node_template(); | ||
| if let Some(input) = transform_node.document_node.inputs.get_mut(0) { | ||
| *input = NodeInput::node(NodeId(1), 0); | ||
| } | ||
| const ORIGIN_OFFSET_INDEX: usize = 5; | ||
| if let Some(origin_offset) = transform_node.document_node.inputs.get_mut(ORIGIN_OFFSET_INDEX) { | ||
| *origin_offset = NodeInput::value(TaggedValue::DVec2(tool_data.data.drag_start), false); | ||
| } |
There was a problem hiding this comment.
This block correctly initializes a Transform node and sets its ORIGIN_OFFSET_INDEX to the tool_data.data.drag_start. This ensures that the shape tool's origin is set to the first drawn point, aligning with the PR's objective.
| if let Some(origin_offset) = transform_node.document_node.inputs.get_mut(ORIGIN_OFFSET_INDEX) { | ||
| *origin_offset = NodeInput::value(TaggedValue::DVec2(tool_data.data.drag_start), false); | ||
| } | ||
| let nodes = vec![(NodeId(0), transform_node), (NodeId(1), node)]; |
There was a problem hiding this comment.
The nodes vector is updated to include both the Transform node and the shape node, with NodeId(0) assigned to the Transform node and NodeId(1) to the shape node. This order is crucial for the graph's data flow, as the shape node's input is set to NodeId(1) (the Transform node) in the previous lines.
| use crate::messages::tool::common_functionality::graph_modification_utils::{self, find_spline, merge_layers, merge_points}; | ||
| use crate::messages::tool::common_functionality::snapping::{SnapCandidatePoint, SnapData, SnapManager, SnapTypeConfiguration, SnappedPoint}; | ||
| use crate::messages::tool::common_functionality::utility_functions::{closest_point, should_extend}; | ||
| use graph_craft::document::value::TaggedValue; |
There was a problem hiding this comment.
Adding TaggedValue to the use statement is necessary for the new Transform node logic. This is a correct and required change.
| let mut transform_node = resolve_network_node_type("Transform").expect("Transform node does not exist").default_node_template(); | ||
| if let Some(input) = transform_node.document_node.inputs.get_mut(0) { | ||
| *input = NodeInput::node(NodeId(1), 0); | ||
| } | ||
| const ORIGIN_OFFSET_INDEX: usize = 5; | ||
| if let Some(origin_offset) = transform_node.document_node.inputs.get_mut(ORIGIN_OFFSET_INDEX) { | ||
| *origin_offset = NodeInput::value(TaggedValue::DVec2(snapped.snapped_point_document), false); | ||
| } |
There was a problem hiding this comment.
This block correctly initializes a Transform node and sets its ORIGIN_OFFSET_INDEX to the snapped.snapped_point_document. This ensures that the spline tool's origin is set to the first drawn point, aligning with the PR's objective.
| *origin_offset = NodeInput::value(TaggedValue::DVec2(snapped.snapped_point_document), false); | ||
| } | ||
|
|
||
| let nodes = vec![(NodeId(2), path_node), (NodeId(1), spline_node), (NodeId(0), transform_node)]; |
There was a problem hiding this comment.
The nodes vector is updated to include the Path node, Spline node, and Transform node, with NodeId(0) assigned to the Transform node, NodeId(1) to the Spline node, and NodeId(2) to the Path node. This order is crucial for the graph's data flow, as the Spline node's input is set to NodeId(2) (the Path node) and the Transform node's input is set to NodeId(1) (the Spline node) in the previous lines.
1 participant
closes #2776