Skip to content

Refactor transform decomposition API with skew support, add 'Decompose Skew' node, and fix stroke transform interpolation#3973

Merged
Keavon merged 5 commits intomasterfrom
refactor-transform-decomposition
Mar 29, 2026
Merged

Refactor transform decomposition API with skew support, add 'Decompose Skew' node, and fix stroke transform interpolation#3973
Keavon merged 5 commits intomasterfrom
refactor-transform-decomposition

Conversation

@Keavon
Copy link
Copy Markdown
Member

@Keavon Keavon commented Mar 29, 2026

  • New node: Decompose Skew, extracts the skew angle from a transform
  • Updated node: Decompose Scale, now has a "Magnitude" vs. "Pure" toggle (magnitude gives the visual axis lengths as before; pure gives the signed scale factors with rotation/skew stripped)

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.

… Skew node, and fix stroke transform interpolation
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot temporarily deployed to graphite-dev (Preview) March 29, 2026 02:27 Inactive
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 15 files

Confidence score: 3/5

  • There is concrete user-facing correctness risk: node-graph/nodes/vector/src/vector_nodes.rs uses scale_magnitudes() for area, which can miscompute area under skewed transforms (over/under-scaling).
  • editor/src/messages/portfolio/document/node_graph/node_properties.rs also uses scale_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 in node-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.rs and editor/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.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot temporarily deployed to graphite-dev (Preview) March 29, 2026 03:17 Inactive
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Keavon Keavon changed the title Refactor transform decomposition API with skew support, add Decompose Skew node, and fix stroke transform interpolation Refactor transform decomposition API with skew support, add 'Decompose Skew' node, and fix stroke transform interpolation Mar 29, 2026
@github-actions github-actions bot temporarily deployed to graphite-dev (Preview) March 29, 2026 03:33 Inactive
@Keavon Keavon merged commit a3ea6ab into master Mar 29, 2026
7 of 9 checks passed
@Keavon Keavon deleted the refactor-transform-decomposition branch March 29, 2026 03:47
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant