userSpaceOnUse/objectBoundingBox#4283
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| fn extract_svg_gradient_units(svg: &str) -> HashMap<String, GradientUnits> { | ||
| let mut result = HashMap::new(); | ||
| let mut gradient_nodes = HashMap::new(); | ||
|
|
||
| let doc = match usvg::roxmltree::Document::parse(svg) { | ||
| Ok(doc) => doc, | ||
| Err(_) => return result, | ||
| }; |
There was a problem hiding this comment.
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.
|
|
||
| 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); |
There was a problem hiding this comment.
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 I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jsjgdh
force-pushed
the
svg-gradient-units-import-export
branch
from
3a673d1 to
3ae1a44
Compare
jsjgdh
force-pushed
the
svg-gradient-units-import-export
branch
from
3ae1a44 to
e4ab060
Compare
There was a problem hiding this comment.
Review completed against the latest diff
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
jsjgdh
force-pushed
the
svg-gradient-units-import-export
branch
from
e4ab060 to
d27f647
Compare
jsjgdh
force-pushed
the
svg-gradient-units-import-export
branch
from
d27f647 to
754f5e9
Compare
1 participant
Request
This feature depends on Yohei's ongoing refactor and should be merged after that work is completed.