userSpaceOnUse/objectBoundingBox

[jsjgdh]

Request

This feature depends on Yohei's ongoing refactor and should be merged after that work is completed.

[gemini-code-assist]

Code Review

This pull request introduces support for preserving and rendering SVG gradientUnits (userSpaceOnUse and objectBoundingBox) during import and export. It adds a GradientUnits enum, extracts the units from the raw SVG XML during import, propagates the property through the node graph, and utilizes it during rendering to correctly set the gradientUnits attribute and adjust the gradient transform. The feedback recommends optimizing the import process by avoiding redundant XML parsing of the same SVG string across multiple functions, and suggests renaming the _bounds variable to bounds since it is now actively used.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Redundant XML Parsing

Both extract_graphite_gradient_stops and extract_svg_gradient_units parse the same raw SVG XML string independently using usvg::roxmltree::Document::parse. For large SVG files, parsing the XML tree multiple times introduces unnecessary CPU and memory overhead.

Consider refactoring these functions to accept a shared reference to the parsed roxmltree::Document, or combining them into a single pass over the XML descendants to extract both gradient stops and units simultaneously.

[gemini-code-assist]

Used Variable with Underscore Prefix

The _bounds variable is now used in the call to svg_gradient_transform. By Rust convention, variables prefixed with an underscore should remain unused. Since it is now used, consider renaming _bounds to bounds in the function signature of RenderExt::render for List<GradientStops> to improve code clarity and adhere to standard style guidelines.

[jsjgdh]

@cubic-dev

[cubic-dev-ai]

@cubic-dev

@jsjgdh I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

5 issues found across 10 files

Confidence score: 3/5

• In editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, unwrap_or_default() for GradientUnits can silently choose UserSpaceOnUse instead of the SVG-spec default objectBoundingBox, which can change gradient positioning in imported SVGs and cause visible rendering regressions — align the fallback with spec/default parsing before merging.
• node-graph/libraries/graphic-types/src/graphic.rs writes ATTR_GRADIENT_UNITS unconditionally while legacy bounding-box transform behavior is guarded elsewhere, creating a cross-file contract mismatch that can make gradients render differently across backends — gate or normalize this write to match renderer expectations (as in create_peniko_gradient_brush) before merge.
• In node-graph/libraries/vector-types/src/gradient.rs, GradientUnits lacks the node_macro::ChoiceType derive and #[widget(Radio)], so users may not be able to edit this setting in the properties UI and could be stuck with incorrect defaults — add the same enum metadata used by sibling gradient enums.
• editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs parses the same SVG XML twice (extract_graphite_gradient_stops and extract_svg_gradient_units), which can add avoidable overhead on large imports; this is a performance risk rather than correctness — consider parsing once and reusing the document as a follow-up if release timing is tight.

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:136">
P3: The `_bounds` parameter is now used in the call to `svg_gradient_transform`, but the underscore prefix indicates an intentionally-unused binding. Rename it to `bounds` in the function signature to follow Rust naming conventions.</violation>
</file>

<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:455">
P2: Both `extract_graphite_gradient_stops` and `extract_svg_gradient_units` independently parse the same raw SVG XML string with `usvg::roxmltree::Document::parse`. For large SVG files this doubles the parsing cost. Consider refactoring to share a single parsed `roxmltree::Document` between the two extraction passes, or combining them into one function.</violation>

<violation number="2" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:870">
P2: Using `unwrap_or_default()` for `GradientUnits` falls back to `UserSpaceOnUse` because that variant is marked `#[default]`, but the SVG spec default is `objectBoundingBox` and the same file's `extract_svg_gradient_units` already uses `ObjectBoundingBox` as its explicit fallback. This creates an inconsistent, hidden cross-file dependency: missing/unmapped gradient IDs silently get the wrong units, which will corrupt re-exported SVGs.

A similar `unwrap_or_default()` bug exists on the radial gradient path a few lines below.</violation>
</file>

<file name="node-graph/libraries/vector-types/src/gradient.rs">

<violation number="1" location="node-graph/libraries/vector-types/src/gradient.rs:18">
P2: `GradientUnits` enum is missing `node_macro::ChoiceType` derive and `#[widget(Radio)]` attribute used by sibling enums (`GradientType`, `GradientSpreadMethod`), likely preventing it from being rendered in the property panel.</violation>
</file>

<file name="node-graph/libraries/graphic-types/src/graphic.rs">

<violation number="1" location="node-graph/libraries/graphic-types/src/graphic.rs:216">
P2: Writing `ATTR_GRADIENT_UNITS` unconditionally alongside the legacy bounding-box transform path creates a cross-file rendering contract mismatch. The Vello renderer (`create_peniko_gradient_brush`) correctly guards legacy gradients by checking `ATTR_GRADIENT_LEGACY`, but the SVG renderers (`render_ext.rs` and `renderer.rs`) read `ATTR_GRADIENT_UNITS` directly without checking the legacy flag. For `ObjectBoundingBox` units, SVG applies `bounds.inverse()` on top of an already-baked bbox transform, double-applying the coordinate conversion and producing incorrect gradient placement in SVG exports.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: Using unwrap_or_default() for GradientUnits falls back to UserSpaceOnUse because that variant is marked #[default], but the SVG spec default is objectBoundingBox and the same file's extract_svg_gradient_units already uses ObjectBoundingBox as its explicit fallback. This creates an inconsistent, hidden cross-file dependency: missing/unmapped gradient IDs silently get the wrong units, which will corrupt re-exported SVGs.

A similar unwrap_or_default() bug exists on the radial gradient path a few lines below.

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/graph_operation_message_handler.rs, line 870:

<comment>Using `unwrap_or_default()` for `GradientUnits` falls back to `UserSpaceOnUse` because that variant is marked `#[default]`, but the SVG spec default is `objectBoundingBox` and the same file's `extract_svg_gradient_units` already uses `ObjectBoundingBox` as its explicit fallback. This creates an inconsistent, hidden cross-file dependency: missing/unmapped gradient IDs silently get the wrong units, which will corrupt re-exported SVGs.

A similar `unwrap_or_default()` bug exists on the radial gradient path a few lines below.</comment>

<file context>
@@ -803,6 +866,8 @@ fn apply_usvg_fill(fill: &usvg::Fill, modify_inputs: &mut ModifyInputsContext, g
 
 			let gradient_type = GradientType::Linear;
+			// Look up the original gradientUnits from the XML pass (usvg hides it after conversion).
+			let units = svg_gradient_units.get(linear.id()).copied().unwrap_or_default();
 
 			let stops = match graphite_gradient_stops.get(linear.id()) {
</file context>

Suggested change

	let units = svg_gradient_units.get(linear.id()).copied().unwrap_or_default();
	let units = svg_gradient_units.get(linear.id()).copied().unwrap_or(GradientUnits::ObjectBoundingBox);

[cubic-dev-ai]

P2: GradientUnits enum is missing node_macro::ChoiceType derive and #[widget(Radio)] attribute used by sibling enums (GradientType, GradientSpreadMethod), likely preventing it from being rendered in the property panel.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/gradient.rs, line 18:

<comment>`GradientUnits` enum is missing `node_macro::ChoiceType` derive and `#[widget(Radio)]` attribute used by sibling enums (`GradientType`, `GradientSpreadMethod`), likely preventing it from being rendered in the property panel.</comment>

<file context>
@@ -14,6 +14,24 @@ pub enum GradientType {
 }
 
+#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
+#[derive(Default, PartialEq, Eq, Clone, Copy, Debug, Hash, graphene_hash::CacheHash, DynAny)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+pub enum GradientUnits {
</file context>

[cubic-dev-ai]

P2: Writing ATTR_GRADIENT_UNITS unconditionally alongside the legacy bounding-box transform path creates a cross-file rendering contract mismatch. The Vello renderer (create_peniko_gradient_brush) correctly guards legacy gradients by checking ATTR_GRADIENT_LEGACY, but the SVG renderers (render_ext.rs and renderer.rs) read ATTR_GRADIENT_UNITS directly without checking the legacy flag. For ObjectBoundingBox units, SVG applies bounds.inverse() on top of an already-baked bbox transform, double-applying the coordinate conversion and producing incorrect gradient placement in SVG exports.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/graphic-types/src/graphic.rs, line 216:

<comment>Writing `ATTR_GRADIENT_UNITS` unconditionally alongside the legacy bounding-box transform path creates a cross-file rendering contract mismatch. The Vello renderer (`create_peniko_gradient_brush`) correctly guards legacy gradients by checking `ATTR_GRADIENT_LEGACY`, but the SVG renderers (`render_ext.rs` and `renderer.rs`) read `ATTR_GRADIENT_UNITS` directly without checking the legacy flag. For `ObjectBoundingBox` units, SVG applies `bounds.inverse()` on top of an already-baked bbox transform, double-applying the coordinate conversion and producing incorrect gradient placement in SVG exports.</comment>

<file context>
@@ -211,6 +213,7 @@ pub fn fill_to_graphic_list(fill: &Fill, bounding_box_transform: DAffine2) -> Op
 			let gradient_item = Item::new_from_element(gradient.stops.clone())
 				.with_attribute(ATTR_TRANSFORM, transform)
 				.with_attribute(ATTR_GRADIENT_TYPE, gradient.gradient_type)
+				.with_attribute(ATTR_GRADIENT_UNITS, gradient.units)
 				.with_attribute(ATTR_SPREAD_METHOD, gradient.spread_method)
 				.with_attribute(ATTR_GRADIENT_LEGACY, legacy);
</file context>

[cubic-dev-ai]

P2: Both extract_graphite_gradient_stops and extract_svg_gradient_units independently parse the same raw SVG XML string with usvg::roxmltree::Document::parse. For large SVG files this doubles the parsing cost. Consider refactoring to share a single parsed roxmltree::Document between the two extraction passes, or combining them into one function.

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/graph_operation_message_handler.rs, line 455:

<comment>Both `extract_graphite_gradient_stops` and `extract_svg_gradient_units` independently parse the same raw SVG XML string with `usvg::roxmltree::Document::parse`. For large SVG files this doubles the parsing cost. Consider refactoring to share a single parsed `roxmltree::Document` between the two extraction passes, or combining them into one function.</comment>

<file context>
@@ -452,6 +452,7 @@ impl MessageHandler<GraphOperationMessage, GraphOperationMessageContext<'_>> for
 				placement_transform.translation = placement_transform.translation.round();
 
 				let graphite_gradient_stops = extract_graphite_gradient_stops(&svg);
+				let svg_gradient_units = extract_svg_gradient_units(&svg);
 
 				// Pass identity so each leaf layer receives only its SVG-native transform from `abs_transform`.
</file context>

[cubic-dev-ai]

P3: The _bounds parameter is now used in the call to svg_gradient_transform, but the underscore prefix indicates an intentionally-unused binding. Rename it to bounds in the function signature to follow Rust naming conventions.

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 136:

<comment>The `_bounds` parameter is now used in the call to `svg_gradient_transform`, but the underscore prefix indicates an intentionally-unused binding. Rename it to `bounds` in the function signature to follow Rust naming conventions.</comment>

<file context>
@@ -122,7 +133,7 @@ impl RenderExt for List<GradientStops> {
 
 		let placement = gradient_placement(document_transform, gradient_type);
-		let gradient_transform = format_transform_matrix(element_transform_inverse * placement);
+		let (gradient_units, gradient_transform) = svg_gradient_transform(element_transform_inverse * placement, _bounds, gradient_units);
 		let gradient_transform = if gradient_transform.is_empty() {
 			String::new()
</file context>