Repository navigation
Conversation
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Code Review
This pull request introduces an "Attach Markers" node to place marker artwork at the start, middle, and end vertices of a path following SVG marker placement semantics. It adds helper functions and structures to compute tangents and transform markers. The reviewer suggested using the idiomatic .to_radians() method instead of manual conversion for the angle offset.
725ba9e to
f307f18
Compare
e15b572 to
70c6796
Compare
There was a problem hiding this comment.
2 issues found across 1 file
Confidence score: 3/5
- There is a concrete functional risk in
node-graph/nodes/vector/src/vector_nodes.rs: flattening vertices across subpaths drops intermediate subpath boundaries, which can misclassify start/end vertices and produce incorrect marker behavior. - Given the high severity/confidence on boundary handling (8/10, 10/10), this is more than a housekeeping concern and introduces real regression risk if merged as-is.
- The PR-title formatting issue in
node-graph/nodes/vector/src/vector_nodes.rscontext is process-only (5/10) and easy to fix, but it does not materially change runtime behavior. - Pay close attention to
node-graph/nodes/vector/src/vector_nodes.rs- subpath boundary handling needs validation so intermediate subpaths keep correct start/end classification.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
f8e340b to
de74278
Compare
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 4/5
- In
node-graph/nodes/vector/src/vector_nodes.rs,marker_vertices_for_bezpathmay render markers at the wrong size on scaled or sheared paths; account for the path transform when computing marker size.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/nodes/vector/src/vector_nodes.rs">
<violation number="1" location="node-graph/nodes/vector/src/vector_nodes.rs:599">
P2: Marker size ignores the path's scale/shear. `marker_vertices_for_bezpath` bakes `path_transform` into the vertices, so positions and orientations are in the path's applied (world) space, but the per-marker transform only contains uniform `scale`, rotation, and translation — the path transform's scale component is dropped. A path scaled 2x therefore renders markers at half their relative size, and under a non-uniform/sheared path transform the marker geometry cannot match the path's coordinate system. In SVG the marker inherits the CTM, which the node's doc comment ('following SVG marker placement semantics') implies; if markers are meant to stay at constant size, that should be documented as a deviation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let tangent = vertex.tangent(index == 0, index == last_index); | ||
| let angle = if self.auto_orient { tangent.y.atan2(tangent.x) } else { 0. }; | ||
|
|
||
| DAffine2::from_scale_angle_translation(DVec2::splat(self.scale), angle + self.angle_offset.to_radians(), vertex.position) |
There was a problem hiding this comment.
P2: Marker size ignores the path's scale/shear. marker_vertices_for_bezpath bakes path_transform into the vertices, so positions and orientations are in the path's applied (world) space, but the per-marker transform only contains uniform scale, rotation, and translation — the path transform's scale component is dropped. A path scaled 2x therefore renders markers at half their relative size, and under a non-uniform/sheared path transform the marker geometry cannot match the path's coordinate system. In SVG the marker inherits the CTM, which the node's doc comment ('following SVG marker placement semantics') implies; if markers are meant to stay at constant size, that should be documented as a deviation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At node-graph/nodes/vector/src/vector_nodes.rs, line 599:
<comment>Marker size ignores the path's scale/shear. `marker_vertices_for_bezpath` bakes `path_transform` into the vertices, so positions and orientations are in the path's applied (world) space, but the per-marker transform only contains uniform `scale`, rotation, and translation — the path transform's scale component is dropped. A path scaled 2x therefore renders markers at half their relative size, and under a non-uniform/sheared path transform the marker geometry cannot match the path's coordinate system. In SVG the marker inherits the CTM, which the node's doc comment ('following SVG marker placement semantics') implies; if markers are meant to stay at constant size, that should be documented as a deviation.</comment>
<file context>
@@ -515,6 +517,175 @@ async fn copy_to_points<I: 'n + Send + Clone>(
+ let tangent = vertex.tangent(index == 0, index == last_index);
+ let angle = if self.auto_orient { tangent.y.atan2(tangent.x) } else { 0. };
+
+ DAffine2::from_scale_angle_translation(DVec2::splat(self.scale), angle + self.angle_offset.to_radians(), vertex.position)
+ }
+}
</file context>
6aea789 to
a8c7430
Compare
A handle sitting on its anchor has a zero derivative, which `atan2` reads as due east, so an arrowhead drawn from a clicked point came out sideways. The endpoint tangent now skips to the first control point that actually differs, in either direction, and every endpoint the markers read uses it. That retires the re-checking fallback and its inverted finiteness test.
a8c7430 to
abc794d
Compare
No description provided.