Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,7 @@ fn apply_usvg_fill(fill: &usvg::Fill, modify_inputs: &mut ModifyInputsContext, b
gradient_type,
stops,
spread_method,
aspect: 1.,
})
}
usvg::Paint::RadialGradient(radial) => {
Expand Down Expand Up @@ -828,6 +829,7 @@ fn apply_usvg_fill(fill: &usvg::Fill, modify_inputs: &mut ModifyInputsContext, b
gradient_type,
stops,
spread_method,
aspect: 1.,
})
}
usvg::Paint::Pattern(_) => {
Expand Down
120 changes: 118 additions & 2 deletions editor/src/messages/tool/tool_messages/gradient_tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,10 @@
Stop(usize),
Midpoint(usize),
New,
/// Drag the +minor-axis handle (perpendicular to major axis, toward +perp direction)
RadialMinorPos,
/// Drag the −minor-axis handle (perpendicular to major axis, toward −perp direction)
RadialMinorNeg,
}

/// Contains information about the selected gradient handle
Expand Down Expand Up @@ -340,10 +344,22 @@

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


None
}

/// Compute minor-axis handle positions in document space for a radial gradient.
fn radial_minor_handles(gradient: &Gradient) -> Option<(DVec2, DVec2)> {
let major_vec = gradient.end - gradient.start;
let major_len = major_vec.length();
if major_len < f64::EPSILON {
return None;
}
let minor_len = major_len * gradient.aspect;
let minor_dir = (major_vec / major_len).perp();
let center = gradient.start;
Some((center + minor_dir * minor_len, center - minor_dir * minor_len))
}

impl SelectedGradient {
pub fn new(gradient: Gradient, layer: LayerNodeIdentifier, document: &DocumentMessageHandler) -> Self {
let transform = gradient_space_transform(layer, document);
Expand Down Expand Up @@ -561,6 +577,28 @@
self.gradient.stops.midpoint[midpoint_index] = midpoint_ratio;
}
}
GradientDragTarget::RadialMinorPos | GradientDragTarget::RadialMinorNeg => {
let document_to_viewport = snap_data.document.metadata().document_to_viewport;
let mouse_doc = document_to_viewport.inverse().transform_point2(mouse);

let center_doc = self.gradient.start;
let major_vec = self.gradient.end - center_doc;
let major_len = major_vec.length();

if major_len < f64::EPSILON {
self.render_gradient(responses);
return;
}

let minor_dir = (major_vec / major_len).perp();
let minor_dist = (mouse_doc - center_doc).dot(minor_dir).abs();

if snap_rotate {
self.gradient.aspect = 1.;
} else {
self.gradient.aspect = (minor_dist / major_len).clamp(0.01, 10.);
}
}
}
self.render_gradient(responses);
}
Expand Down Expand Up @@ -808,6 +846,33 @@
Some(1.),
);
}
if gradient.gradient_type == GradientType::Radial {
let major_vec = end - start;
let major_len = major_vec.length();
if major_len > f64::EPSILON {
let minor_len = major_len * gradient.aspect;
let major_dir = major_vec / major_len;
let minor_dir = major_dir.perp();
let center = start;

let minor_pos_vp = center + minor_dir * minor_len;
let minor_neg_vp = center - minor_dir * minor_len;

let angle = major_dir.y.atan2(major_dir.x);
overlay_context.dashed_ellipse(center, major_len, minor_len, Some(angle), None, None, None, None, Some(COLOR_OVERLAY_BLUE), Some(4.), Some(4.), None);

overlay_context.line(center, minor_pos_vp, Some(COLOR_OVERLAY_BLUE), None);
overlay_context.line(center, minor_neg_vp, Some(COLOR_OVERLAY_BLUE), None);

let minor_tol_sq = (MANIPULATOR_GROUP_MARKER_SIZE * 2.).powi(2);
let pos_active = dragging == Some(GradientDragTarget::RadialMinorPos);
let neg_active = dragging == Some(GradientDragTarget::RadialMinorNeg);
let pos_hovered = !pos_active && !matches!(self, GradientToolFsmState::Drawing { .. }) && minor_pos_vp.distance_squared(mouse) < minor_tol_sq;
let neg_hovered = !neg_active && !matches!(self, GradientToolFsmState::Drawing { .. }) && minor_neg_vp.distance_squared(mouse) < minor_tol_sq;
overlay_context.manipulator_handle(minor_pos_vp, pos_active || pos_hovered, None);
overlay_context.manipulator_handle(minor_neg_vp, neg_active || neg_hovered, None);
}
}
}

let snap_data = SnapData::new(document, input, viewport);
Expand Down Expand Up @@ -914,7 +979,7 @@
responses.add(DocumentMessage::StartTransaction);

// Remove the selected point
match selected_gradient.dragging {

Check failure on line 982 in editor/src/messages/tool/tool_messages/gradient_tool.rs

View workflow job for this annotation

GitHub Actions / test

non-exhaustive patterns: `GradientDragTarget::RadialMinorPos` and `GradientDragTarget::RadialMinorNeg` not covered

Check failure on line 982 in editor/src/messages/tool/tool_messages/gradient_tool.rs

View workflow job for this annotation

GitHub Actions / build / web

non-exhaustive patterns: `GradientDragTarget::RadialMinorPos` and `GradientDragTarget::RadialMinorNeg` not covered
GradientDragTarget::Start => {
// Only delete if there's a real color stop at position ~0 (not the endpoint of the line which isn't itself a color stop)
if selected_gradient.gradient.stops.position.first().is_some_and(|&p| p.abs() < f64::EPSILON * 1000.) {
Expand Down Expand Up @@ -1048,6 +1113,33 @@
let Some(gradient) = get_gradient(layer, &document.network_interface) else { continue };
let transform = gradient_space_transform(layer, document);

if drag_hint.is_none() && gradient.gradient_type == GradientType::Radial {
if let Some((minor_pos_doc, minor_neg_doc)) = radial_minor_handles(&gradient) {
let minor_pos_vp = transform.transform_point2(minor_pos_doc);
let minor_neg_vp = transform.transform_point2(minor_neg_doc);
let minor_tolerance = (MANIPULATOR_GROUP_MARKER_SIZE * 2.).powi(2);

let minor_drag_target = if minor_pos_vp.distance_squared(mouse) < minor_tolerance {
Some(GradientDragTarget::RadialMinorPos)
} else if minor_neg_vp.distance_squared(mouse) < minor_tolerance {
Some(GradientDragTarget::RadialMinorNeg)
} else {
None
};

if let Some(drag_target) = minor_drag_target {
drag_hint = Some(GradientDragHintState::RadialMinor);
tool_data.selected_gradient = Some(SelectedGradient {
layer: Some(layer),
transform,
gradient: gradient.clone(),
dragging: drag_target,
initial_gradient: gradient.clone(),
});
}
}
}

// Check for dragging a midpoint diamond
if drag_hint.is_none() {
let (start, end) = (transform.transform_point2(gradient.start), transform.transform_point2(gradient.end));
Expand Down Expand Up @@ -1380,6 +1472,12 @@
groups.push(HintGroup(vec![HintInfo::mouse(MouseMotion::LmbDouble, "Reset Midpoint")]));
}
}
GradientHoverTarget::RadialMinor => {
groups.push(HintGroup(vec![
HintInfo::mouse(MouseMotion::LmbDrag, "Adjust Ellipse"),
HintInfo::keys([Key::Shift], "Snap to Circle").prepend_plus(),
]));
}
}

// Delete/reset hint based on selection
Expand Down Expand Up @@ -1414,6 +1512,9 @@
GradientDragHintState::Midpoint { resettable: true } => {
groups.push(HintGroup(vec![HintInfo::keys([Key::Backspace], "Reset Midpoint")]));
}
GradientDragHintState::RadialMinor => {
groups.push(HintGroup(vec![HintInfo::keys([Key::Shift], "Snap to Circle")]));
}
_ => {}
}

Expand Down Expand Up @@ -1449,6 +1550,17 @@
let (start, end) = (transform.transform_point2(gradient.start), transform.transform_point2(gradient.end));
let line_length = start.distance(end);

if gradient.gradient_type == GradientType::Radial {
if let Some((minor_pos_doc, minor_neg_doc)) = radial_minor_handles(&gradient) {
let minor_pos_vp = transform.transform_point2(minor_pos_doc);
let minor_neg_vp = transform.transform_point2(minor_neg_doc);
let minor_tolerance = (MANIPULATOR_GROUP_MARKER_SIZE * 2.).powi(2);
if minor_pos_vp.distance_squared(mouse) < minor_tolerance || minor_neg_vp.distance_squared(mouse) < minor_tolerance {
return GradientHoverTarget::RadialMinor;
}
}
}

// Check midpoint diamonds first (smaller hit area, higher priority)
for i in 0..gradient.stops.position.len().saturating_sub(1) {
let left = gradient.stops.position[i];
Expand Down Expand Up @@ -1506,7 +1618,7 @@
let resettable = selected_gradient.gradient.stops.midpoint.get(i).is_some_and(|&midpoint_value| midpoint_is_resettable(midpoint_value));
GradientSelectedTarget::Midpoint { resettable }
}
GradientDragTarget::New => GradientSelectedTarget::None,
GradientDragTarget::New | GradientDragTarget::RadialMinorPos | GradientDragTarget::RadialMinorNeg => GradientSelectedTarget::None,
}
}

Expand Down Expand Up @@ -1593,6 +1705,8 @@
Midpoint {
resettable: bool,
},
/// Hovering over a radial minor-axis handle
RadialMinor,
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, Default)]
Expand All @@ -1615,6 +1729,8 @@
Midpoint {
resettable: bool,
},
/// Dragging a radial minor-axis handle to reshape the ellipse
RadialMinor,
}

#[cfg(test)]
Expand Down
40 changes: 31 additions & 9 deletions node-graph/libraries/rendering/src/render_ext.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::renderer::{RenderParams, format_transform_matrix};
use core_types::uuid::generate_uuid;
use glam::DAffine2;
use glam::{DAffine2, DVec2};
use graphic_types::vector_types::gradient::{Gradient, GradientType};
use graphic_types::vector_types::vector::style::{Fill, PaintOrder, PathStyle, Stroke, StrokeAlign, StrokeCap, StrokeJoin};
use std::fmt::Write;
Expand Down Expand Up @@ -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>

let end = transform_points.transform_point2(self.end);

let gradient_transform = if transformed_bounds.matrix2.determinant() != 0. {
let gradient_transform_raw = if transformed_bounds.matrix2.determinant() != 0. {
transformed_bounds.inverse()
} else {
DAffine2::IDENTITY // Ignore if the transform cannot be inverted (the bounds are zero). See issue #1944.
};
let gradient_transform = format_transform_matrix(gradient_transform);
let gradient_transform = if gradient_transform.is_empty() {
String::new()
} else {
format!(r#" gradientTransform="{gradient_transform}""#)
};

let spread_method = if self.spread_method == GradientSpreadMethod::Pad {
String::new()
Expand All @@ -58,14 +52,42 @@ 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 {
format!(r#" gradientTransform="{gradient_transform}""#)
};
let _ = write!(
svg_defs,
r#"<linearGradient id="{}" x1="{}" y1="{}" x2="{}" y2="{}"{spread_method}{gradient_transform}>{}</linearGradient>"#,
gradient_id, start.x, start.y, end.x, end.y, stop
);
}
GradientType::Radial => {
let radius = (f64::powi(start.x - end.x, 2) + f64::powi(start.y - end.y, 2)).sqrt();
let radius = start.distance(end);

let ellipse_transform = if (self.aspect - 1.).abs() > f64::EPSILON {
let major_vec = end - start;
let angle = major_vec.y.atan2(major_vec.x);
let squash = DAffine2::from_translation(start)
* DAffine2::from_angle(angle)
* DAffine2::from_scale(DVec2::new(1., self.aspect))
* 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

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

} else {
gradient_transform_raw
};

let gradient_transform = format_transform_matrix(ellipse_transform);
let gradient_transform = if gradient_transform.is_empty() {
String::new()
} else {
format!(r#" gradientTransform="{gradient_transform}""#)
};

let _ = write!(
svg_defs,
r#"<radialGradient id="{}" cx="{}" cy="{}" r="{}"{spread_method}{gradient_transform}>{}</radialGradient>"#,
Expand Down
17 changes: 16 additions & 1 deletion node-graph/libraries/rendering/src/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1174,7 +1174,22 @@ impl Render for Table<Vector> {
} else {
Default::default()
};
let brush_transform = kurbo::Affine::new((inverse_element_transform * parent_transform).to_cols_array());
let mut brush_transform = kurbo::Affine::new((inverse_element_transform * parent_transform).to_cols_array());

if gradient.gradient_type == GradientType::Radial && (gradient.aspect - 1.).abs() > f64::EPSILON {
let major_vec = end - start;
let angle = major_vec.y.atan2(major_vec.x);
let center = kurbo::Vec2::new(start.x, start.y);

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;

}

scene.fill(fill_rule, kurbo::Affine::new(element_transform.to_cols_array()), &fill, Some(brush_transform), path);
}
Fill::None => {}
Expand Down
13 changes: 13 additions & 0 deletions node-graph/libraries/vector-types/src/gradient.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,14 @@ pub struct Gradient {
pub end: DVec2,
#[serde(default)]
pub spread_method: GradientSpreadMethod,
/// Ratio of the minor radius to the major radius for radial gradients (1.0 = circle).
/// Ignored for linear gradients. Defaults to 1.0 so old documents deserialize as circles.
#[serde(default = "default_aspect")]
pub aspect: f64,
}

fn default_aspect() -> f64 {
1.
}

impl Default for Gradient {
Expand All @@ -378,6 +386,7 @@ impl Default for Gradient {
start: DVec2::new(0., 0.5),
end: DVec2::new(1., 0.5),
spread_method: GradientSpreadMethod::Pad,
aspect: 1.,
}
}
}
Expand All @@ -390,6 +399,7 @@ impl std::hash::Hash for Gradient {
.chain(self.end.to_array().iter())
.chain(self.stops.position.iter())
.chain(self.stops.midpoint.iter())
.chain(std::iter::once(&self.aspect))
.for_each(|x| x.to_bits().hash(state));
self.stops.color.iter().for_each(|color| color.hash(state));
self.gradient_type.hash(state);
Expand Down Expand Up @@ -432,6 +442,7 @@ impl Gradient {
stops,
gradient_type,
spread_method,
aspect: 1.,
}
}

Expand All @@ -446,13 +457,15 @@ impl Gradient {
let stops = GradientStops::new(stops);
let gradient_type = if time < 0.5 { self.gradient_type } else { other.gradient_type };
let spread_method = if time < 0.5 { self.spread_method } else { other.spread_method };
let aspect = self.aspect + (other.aspect - self.aspect) * time;

Self {
start,
end,
stops,
gradient_type,
spread_method,
aspect,
}
}

Expand Down
Loading