Conversation
… Skew node, and fix stroke transform interpolation
There was a problem hiding this comment.
Code Review
This pull request refactors the transformation decomposition logic by introducing a ScaleType enum and expanding the Transform trait with methods like decompose_rotation_scale_skew, scale_magnitudes, and decompose_skew. It updates various parts of the codebase, including stroke interpolation and bounding box calculations, to use these new methods. Review feedback identifies an incorrect area calculation in vector_nodes.rs where scale_magnitudes should be replaced by decompose_scale to correctly account for the transformation determinant, and suggests a mathematical simplification in the skew calculation within transform.rs.
There was a problem hiding this comment.
5 issues found across 15 files
Confidence score: 3/5
- There is concrete user-facing correctness risk:
node-graph/nodes/vector/src/vector_nodes.rsusesscale_magnitudes()for area, which can miscompute area under skewed transforms (over/under-scaling). editor/src/messages/portfolio/document/node_graph/node_properties.rsalso usesscale_magnitudes()in transform editing, which can reconstruct mirrored/skewed transforms incorrectly when rotation changes, so regression risk is meaningful.- A couple of findings are lower impact housekeeping (doc wording in
node-graph/nodes/transform/src/transform_nodes.rs, algebra simplification innode-graph/libraries/core-types/src/transform.rs), plus PR-title convention enforcement, so this looks fixable rather than fundamentally unsafe. - Pay close attention to
node-graph/nodes/vector/src/vector_nodes.rsandeditor/src/messages/portfolio/document/node_graph/node_properties.rs- both touch transform math where skew/mirroring behavior can become incorrect.
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/portfolio/document/node_graph/node_properties.rs">
<violation number="1" location="editor/src/messages/portfolio/document/node_graph/node_properties.rs:26">
P1: Custom agent: **PR title enforcement**
PR title violates the title convention for new node additions: it references a node name without single quotes and does not use the required `New node: 'Node Name'` format.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/node_graph/node_properties.rs:571">
P2: Using `scale_magnitudes()` in the transform property editor causes mirrored/skewed transforms to be reconstructed incorrectly when rotation is changed.</violation>
</file>
<file name="node-graph/nodes/transform/src/transform_nodes.rs">
<violation number="1" location="node-graph/nodes/transform/src/transform_nodes.rs:162">
P3: The doc comment is inaccurate: this node returns a skew angle in degrees, not a skew coefficient.
(Based on your team's feedback about using consistent, precise terminology in names and docs.) [FEEDBACK_USED]</violation>
</file>
<file name="node-graph/nodes/vector/src/vector_nodes.rs">
<violation number="1" location="node-graph/nodes/vector/src/vector_nodes.rs:2537">
P1: Using `scale_magnitudes()` in area calculation breaks area correctness for skewed transforms; it over/under-scales area because magnitudes include skew length distortion.</violation>
</file>
<file name="node-graph/libraries/core-types/src/transform.rs">
<violation number="1" location="node-graph/libraries/core-types/src/transform.rs:47">
P3: The denominator `(sin * sin * scale_x + cos * cos * scale_x)` simplifies to `scale_x` via the Pythagorean identity (sin² + cos² = 1). The current form obscures this.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
editor/src/messages/portfolio/document/node_graph/node_properties.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 2 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="node-graph/nodes/vector/src/vector_nodes.rs">
<violation number="1" location="node-graph/nodes/vector/src/vector_nodes.rs:7">
P1: Custom agent: **PR title enforcement**
PR title does not follow the new-node title convention: it introduces a node without single-quoting the node name and does not use the dedicated `New node: 'Node Name'` format required by the guideline.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 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="node-graph/nodes/transform/src/transform_nodes.rs">
<violation number="1" location="node-graph/nodes/transform/src/transform_nodes.rs:162">
P1: Custom agent: **PR title enforcement**
Update the PR title to follow the required new-node format and quote the node name, e.g. `New node: 'Decompose Skew'` (and adjust the rest of the title accordingly).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (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/portfolio/document/graph_operation/transform_utils.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/transform_utils.rs:15">
P1: Custom agent: **PR title enforcement**
PR title must use the required new-node format. The rules specify "New node: 'Node Name'" for PRs that add a node, but the current title is a generic sentence instead of the mandated format.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -3,44 +3,21 @@ use glam::{DAffine2, DVec2}; | |||
| use graph_craft::document::value::TaggedValue; | |||
There was a problem hiding this comment.
P1: Custom agent: PR title enforcement
PR title must use the required new-node format. The rules specify "New node: 'Node Name'" for PRs that add a node, but the current title is a generic sentence instead of the mandated format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/transform_utils.rs, line 15:
<comment>PR title must use the required new-node format. The rules specify "New node: 'Node Name'" for PRs that add a node, but the current title is a generic sentence instead of the mandated format.</comment>
<file context>
@@ -12,12 +12,12 @@ pub fn update_transform(network_interface: &mut NodeNetworkInterface, node_id: &
let rotation = rotation.to_degrees();
- let shear = DVec2::new(skew.atan().to_degrees(), 0.);
+ let skew = DVec2::new(skew.atan().to_degrees(), 0.);
network_interface.set_input(&InputConnector::node(*node_id, 1), NodeInput::value(TaggedValue::DVec2(translation), false), &[]);
</file context>
Also fixes a bug: stroke interpolation (e.g. when morphing between shapes with different transforms) no longer breaks when transforms have opposing rotations like 0° vs. 180°, which previously caused a division by zero in the renderer.