Add focal point, focal radius, and gradient units support to gradients#4139
Add focal point, focal radius, and gradient units support to gradients#4139jsjgdh wants to merge 2 commits into
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 enhances gradient support by introducing focal points, focal radii, and gradient units to the Gradient structure. It improves SVG import capabilities by adding support for percentage-based offsets, 3-digit hex colors, and named CSS colors via the svgtypes crate. Additionally, it implements a mechanism for gradient stops to inherit from other gradients using href. Feedback focuses on making the href resolution more robust to handle multi-level or out-of-order references and simplifying the color parsing logic to better utilize the svgtypes library and correctly handle alpha multiplication.
There was a problem hiding this comment.
1 issue found across 10 files
Confidence score: 4/5
- This PR looks generally safe to merge, but there is a moderate, user-visible rendering issue in
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rswhere parsed color alpha is dropped. - The most significant impact is that stop-colors using embedded alpha (for example
rgba(...),hsla(...), or 8-digit hex) may render with incorrect opacity, which can change gradient appearance. - Given the single mid-severity finding (5/10) with good confidence, this is a limited-risk fix area rather than a likely broad regression.
- Pay close attention to
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs- preserve and propagatec.alphawhen converting parsed colors.
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/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:625">
P2: `svgtypes::Color::from_str` already parses alpha for formats like `rgba(...)`, `hsla(...)`, and 8-digit hex, but the alpha component (`c.alpha`) is discarded here. This means any stop-color with an embedded alpha channel will lose it. Furthermore, per SVG2, the final opacity should be `(c.alpha / 255) * stop_opacity` — a product of the color's alpha and the `stop-opacity` attribute — but `parse_stop_color` currently uses `alpha.unwrap_or(opacity)`, choosing one or the other instead of multiplying them.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 11 files
Confidence score: 3/5
- There is concrete user-facing rendering risk: in
node-graph/libraries/rendering/src/render_ext.rs, thegradientUnitscondition is inverted, so non-defaultUserSpaceOnUsemay be omitted and SVG viewers can interpret gradient coordinates incorrectly. - In
node-graph/libraries/rendering/src/renderer.rs, radial gradient focal radius is not transformed with the rest of the gradient, which can produce visibly wrong inner-radius behavior under scaling/transforms. - Given the high confidence and medium-to-high severities (7/10 and 6/10), this looks mergeable only with caution rather than low risk.
- Pay close attention to
node-graph/libraries/rendering/src/render_ext.rsandnode-graph/libraries/rendering/src/renderer.rs- gradient coordinate-space and focal-radius transform handling can cause incorrect SVG output.
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:57">
P1: `gradientUnits` emission condition is inverted: `UserSpaceOnUse` (non-default) is omitted, causing SVG consumers to misinterpret the gradient coordinate space</violation>
</file>
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:1208">
P2: Radial gradient focal radius is not transformed with the rest of the gradient, so scaled/transformed gradients can render with the wrong inner radius.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| format!(r#" spreadMethod="{}""#, self.spread_method.svg_name()) | ||
| }; | ||
|
|
||
| let gradient_units = if self.gradient_units == GradientUnits::UserSpaceOnUse { |
There was a problem hiding this comment.
P1: gradientUnits emission condition is inverted: UserSpaceOnUse (non-default) is omitted, causing SVG consumers to misinterpret the gradient coordinate space
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 57:
<comment>`gradientUnits` emission condition is inverted: `UserSpaceOnUse` (non-default) is omitted, causing SVG consumers to misinterpret the gradient coordinate space</comment>
<file context>
@@ -54,21 +54,39 @@ impl RenderExt for Gradient {
format!(r#" spreadMethod="{}""#, self.spread_method.svg_name())
};
+ let gradient_units = if self.gradient_units == GradientUnits::UserSpaceOnUse {
+ String::new()
+ } else {
</file context>
| start_center: to_point(start), | ||
| start_radius: 0., | ||
| start_center: to_point(focal), | ||
| start_radius: gradient.focal_radius as f32, |
There was a problem hiding this comment.
P2: Radial gradient focal radius is not transformed with the rest of the gradient, so scaled/transformed gradients can render with the wrong inner radius.
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 1208:
<comment>Radial gradient focal radius is not transformed with the rest of the gradient, so scaled/transformed gradients can render with the wrong inner radius.</comment>
<file context>
@@ -1202,9 +1202,10 @@ impl Render for Table<Vector> {
- start_center: to_point(start),
- start_radius: 0.,
+ start_center: to_point(focal),
+ start_radius: gradient.focal_radius as f32,
end_center: to_point(start),
end_radius: radius as f32,
</file context>
Enhance gradient functionality with three new features:
fx/fy/frattributesUserSpaceOnUse/ObjectBoundingBox) — togglable via the properties panel, matching SVG'sgradientUnitsattributehref-based stop inheritance is handledAlso includes rendering improvements (peniko radial positioning) and a fix to
Fill::color()to evaluate the gradient at a meaningful position instead of always returning the first stop. (description written by Cubic)