Skip to content

Add elliptical radial gradient support using aspect ratio#4054

Open
afrdbaig7 wants to merge 3 commits intoGraphiteEditor:masterfrom
afrdbaig7:feature/elliptical-radial-clean
Open

Add elliptical radial gradient support using aspect ratio#4054
afrdbaig7 wants to merge 3 commits intoGraphiteEditor:masterfrom
afrdbaig7:feature/elliptical-radial-clean

Conversation

@afrdbaig7
Copy link
Copy Markdown
Contributor

Fixes #3993
This PR adds support for elliptical radial gradients in a straightforward way without overcomplicating the system. Instead of introducing full transform matrices and making the data model harder to manage, it simply adds an aspect value that controls how stretched the gradient is, with a default of 1.0 so all existing gradients continue to work exactly the same. The gradient tool is updated with extra handles so you can easily adjust the shape, and all interactions are handled in document space to keep things stable no matter how you zoom, pan, or rotate the canvas—holding Shift quickly snaps it back to a perfect circle. Since SVG and Vello only support circular gradients, the ellipse is created by applying a scaling transform behind the scenes, using the same approach in both rendering paths to keep everything consistent. Overall, this keeps the implementation simple, avoids unnecessary complexity, and adds the flexibility you need without breaking anything that already works.

… field and minor-axis handles to control shape. Rendering adjusts transforms (SVG/Vello) to simulate ellipses while keeping existing gradients unchanged
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 introduces support for elliptical radial gradients by adding an aspect ratio property to the gradient definition. It includes updates to the gradient tool for interactive manipulation of the minor axis via new handles, as well as rendering logic for both SVG and Vello. Feedback highlights a syntax error in gradient_tool.rs where a function was left unclosed during refactoring and an incorrect transformation multiplication order in the SVG rendering logic.

Comment on lines 345 to 346
return Some(projection);
}
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.

critical

The calculate_insertion function is missing its closing brace and the final None return value. This will cause a compilation error because the function is expected to return Option<f64>, but it currently only returns a value inside the if block and is never closed.

		return Some(projection);
	}

	None
}

* DAffine2::from_angle(-angle)
* DAffine2::from_translation(-start);

squash * gradient_transform_raw
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.

high

The order of transform multiplication is incorrect. squash is defined in world/element space (using the start coordinate), and gradient_transform_raw maps from world/element space to the gradient's object space (0..1). To correctly apply the squash in world space before mapping to object space, squash should be the right-hand operand.

Suggested change
squash * gradient_transform_raw
gradient_transform_raw * squash

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.

4 issues found across 4 files

Confidence score: 2/5

  • There is meaningful regression risk in gradient rendering: node-graph/libraries/rendering/src/render_ext.rs applies squash and gradient_transform_raw in the wrong order, which can produce incorrect gradient positioning/orientation under transforms.
  • node-graph/libraries/rendering/src/renderer.rs has two coordinate/affine composition concerns (elliptical radial gradient order and unchecked aspect ratio), and zero/negative aspect ratios can lead to degenerate or mirrored radial gradients in user-visible output.
  • Given the medium-to-high severities (6–7/10) with high confidence, this is better treated as high merge risk until transform composition and input validation are corrected.
  • Pay close attention to node-graph/libraries/rendering/src/render_ext.rs, node-graph/libraries/rendering/src/renderer.rs - transform order and aspect-ratio handling can cause visible gradient misrendering.
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/libraries/rendering/src/render_ext.rs">

<violation number="1" location="node-graph/libraries/rendering/src/render_ext.rs:56">
P3: Duplicated `gradientTransform` serialization logic in both gradient branches increases maintenance risk and potential behavior drift.</violation>

<violation number="2" location="node-graph/libraries/rendering/src/render_ext.rs:78">
P1: Transform multiplication order is reversed. `squash` is defined in element space (around `start`), so it must be applied before `gradient_transform_raw` (which maps from element space to gradient object space). With `A * B` applying `B` first, the correct composition is `gradient_transform_raw * squash`.</violation>
</file>

<file name="node-graph/libraries/rendering/src/renderer.rs">

<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:1185">
P2: Radial gradient aspect ratio is used as a raw scale factor without validation, allowing zero/negative values to create degenerate or mirrored transforms.</violation>

<violation number="2" location="node-graph/libraries/rendering/src/renderer.rs:1189">
P2: Elliptical radial gradient affine is composed in the wrong order, causing coordinate-space mismatch under transforms.</violation>
</file>

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

* DAffine2::from_angle(-angle)
* DAffine2::from_translation(-start);

squash * gradient_transform_raw
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: Transform multiplication order is reversed. squash is defined in element space (around start), so it must be applied before gradient_transform_raw (which maps from element space to gradient object space). With A * B applying B first, the correct composition is gradient_transform_raw * squash.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/render_ext.rs, line 78:

<comment>Transform multiplication order is reversed. `squash` is defined in element space (around `start`), so it must be applied before `gradient_transform_raw` (which maps from element space to gradient object space). With `A * B` applying `B` first, the correct composition is `gradient_transform_raw * squash`.</comment>

<file context>
@@ -58,14 +52,41 @@ impl RenderExt for Gradient {
+						* DAffine2::from_angle(-angle)
+						* DAffine2::from_translation(-start);
+					
+					squash * gradient_transform_raw
+				} else {
+					gradient_transform_raw
</file context>
Suggested change
squash * gradient_transform_raw
gradient_transform_raw * squash


let ellipse_affine = kurbo::Affine::translate(center)
* kurbo::Affine::rotate(angle)
* kurbo::Affine::scale_non_uniform(1., gradient.aspect)
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.

P2: Radial gradient aspect ratio is used as a raw scale factor without validation, allowing zero/negative values to create degenerate or mirrored transforms.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 1185:

<comment>Radial gradient aspect ratio is used as a raw scale factor without validation, allowing zero/negative values to create degenerate or mirrored transforms.</comment>

<file context>
@@ -1174,7 +1174,21 @@ impl Render for Table<Vector> {
+
+						let ellipse_affine = kurbo::Affine::translate(center)
+							* kurbo::Affine::rotate(angle)
+							* kurbo::Affine::scale_non_uniform(1., gradient.aspect)
+							* kurbo::Affine::rotate(-angle)
+							* kurbo::Affine::translate(-center);
</file context>

* kurbo::Affine::rotate(-angle)
* kurbo::Affine::translate(-center);

brush_transform = ellipse_affine * brush_transform;
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.

P2: Elliptical radial gradient affine is composed in the wrong order, causing coordinate-space mismatch under transforms.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 1189:

<comment>Elliptical radial gradient affine is composed in the wrong order, causing coordinate-space mismatch under transforms.</comment>

<file context>
@@ -1174,7 +1174,21 @@ impl Render for Table<Vector> {
+							* kurbo::Affine::rotate(-angle)
+							* kurbo::Affine::translate(-center);
+						
+						brush_transform = ellipse_affine * brush_transform;
+					}
+
</file context>
Suggested change
brush_transform = ellipse_affine * brush_transform;
brush_transform = brush_transform * ellipse_affine;

@@ -36,17 +36,11 @@ impl RenderExt for Gradient {
let start = transform_points.transform_point2(self.start);
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.

P3: Duplicated gradientTransform serialization logic in both gradient branches increases maintenance risk and potential behavior drift.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/render_ext.rs, line 56:

<comment>Duplicated `gradientTransform` serialization logic in both gradient branches increases maintenance risk and potential behavior drift.</comment>

<file context>
@@ -58,14 +52,41 @@ impl RenderExt for Gradient {
 		match self.gradient_type {
 			GradientType::Linear => {
+				let gradient_transform = format_transform_matrix(gradient_transform_raw);
+				let gradient_transform = if gradient_transform.is_empty() {
+					String::new()
+				} else {
</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.

Gradient tool for setting radial ellipse's minor and major axis

1 participant