Add vectorization support for raster images#4261
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new vectorize node in the raster node-graph, integrating vtracer and visioncortex to convert raster images into vector subpaths. It also refactors SVG and usvg import utilities by moving them from the editor's graph operation handler into a reusable usvg_utils module within the rendering library. The review feedback highlights critical stability concerns, specifically potential editor crashes due to panics in error handling, and a compilation failure on Wasm targets due to the conditional compilation of the vectorize node. Additionally, the feedback suggests several code quality improvements, including avoiding unnecessary cloning of owned data, replacing println! statements with proper logging, removing commented-out legacy code, and making ParsedSvgText fields public for external access.
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.
| path_precision: Some(6), | ||
| }; | ||
|
|
||
| let vectorized_image = convert(color_image, config).expect("failed to obtain an SvgFile from vtracer."); |
There was a problem hiding this comment.
Using .expect on convert will panic and crash the entire editor if vectorization fails (e.g., due to an empty image or memory allocation failure). It is much safer to log the error and return an empty vector.
let vectorized_image = match convert(color_image, config) {
Ok(img) => img,
Err(e) => {
log::error!("Failed to obtain an SvgFile from vtracer: {e}");
return Item::from_parts(Vector::from_subpaths(vec![], true), row.attributes().clone());
}
};| let svg_tree = match usvg::Tree::from_str(&image_svg, &usvg::Options::default()) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| // TODO: use proper error statements | ||
| panic!("Failed to create a usvg tree:\n{e}"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Panicking on failed SVG parsing will crash the editor. Handle the error gracefully by logging it and returning an empty vector.
| let svg_tree = match usvg::Tree::from_str(&image_svg, &usvg::Options::default()) { | |
| Ok(t) => t, | |
| Err(e) => { | |
| // TODO: use proper error statements | |
| panic!("Failed to create a usvg tree:\n{e}"); | |
| } | |
| }; | |
| let svg_tree = match usvg::Tree::from_str(&image_svg, &usvg::Options::default()) { | |
| Ok(t) => t, | |
| Err(e) => { | |
| log::error!("Failed to create a usvg tree: {e}"); | |
| return Item::from_parts(Vector::from_subpaths(vec![], true), row.attributes().clone()); | |
| } | |
| }; |
| let parsed_group = if let ParsedSvgNode::Group(group) = parsed_node { | ||
| group | ||
| } else { | ||
| panic!("Must return a SVG group."); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Avoid panicking if extract_usvg_node returns something other than a Group. Log an error and return gracefully instead.
let parsed_node = extract_usvg_node(child, &graphite_gradient_stops);
let parsed_group = if let ParsedSvgNode::Group(group) = parsed_node {
group
} else {
log::error!("Expected a SVG group from extract_usvg_node.");
return Item::from_parts(Vector::from_subpaths(vec![], true), row.attributes().clone());
};| #[cfg(not(target_family = "wasm"))] | ||
| #[node_macro::node(category("Raster"), path(core_types::vector))] | ||
| pub fn vectorize(_ctx: impl Ctx, image: List<Raster<CPU>>) -> List<Vector> { |
There was a problem hiding this comment.
Since Graphite is primarily compiled for Wasm, defining the vectorize node only for non-Wasm targets will cause compilation failures in the node registry or serialization code when compiling for Wasm. Providing a fallback implementation that logs a warning and returns an empty list ensures the codebase remains clean and compiles successfully across all targets.
#[cfg(target_family = "wasm")]
#[node_macro::node(category("Raster"), path(core_types::vector))]
pub fn vectorize(_ctx: impl Ctx, _image: List<Raster<CPU>>) -> List<Vector> {
log::warn!("Vectorization is not supported on Wasm targets.");
List::new()
}
#[cfg(not(target_family = "wasm"))]
#[node_macro::node(category("Raster"), path(core_types::vector))]
pub fn vectorize(_ctx: impl Ctx, image: List<Raster<CPU>>) -> List<Vector> {| let parsed_path = extract_usvg_path(child, path, &graphite_gradient_stops); | ||
| let mut child_subpaths = parsed_path.subpaths.clone(); | ||
| child_subpaths.iter_mut().for_each(|s| s.apply_transform(parsed_path.transform)); | ||
| subpaths.extend(child_subpaths); |
There was a problem hiding this comment.
Since parsed_path is owned, we can directly consume and modify its subpaths instead of cloning them, which avoids unnecessary allocations.
| let parsed_path = extract_usvg_path(child, path, &graphite_gradient_stops); | |
| let mut child_subpaths = parsed_path.subpaths.clone(); | |
| child_subpaths.iter_mut().for_each(|s| s.apply_transform(parsed_path.transform)); | |
| subpaths.extend(child_subpaths); | |
| let mut parsed_path = extract_usvg_path(child, path, &graphite_gradient_stops); | |
| parsed_path.subpaths.iter_mut().for_each(|s| s.apply_transform(parsed_path.transform)); | |
| subpaths.extend(parsed_path.subpaths); |
| for child in parsed_group.children { | ||
| if let ParsedSvgNode::Path(path) = child { | ||
| let mut child_subpaths = path.subpaths.clone(); | ||
| child_subpaths.iter_mut().for_each(|s| s.apply_transform(path.transform)); | ||
| subpaths.extend(child_subpaths); | ||
|
|
||
| println!("Reading path (in a group) {} from a total of {}.", i, svg_tree.root().children().len()); | ||
| i += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Since parsed_group.children is owned, we can iterate over it by value and move path.subpaths directly without cloning. Also, replace println! with log::debug!.
for child in parsed_group.children {
if let ParsedSvgNode::Path(mut path) = child {
path.subpaths.iter_mut().for_each(|s| s.apply_transform(path.transform));
subpaths.extend(path.subpaths);
log::debug!("Reading path (in a group) {} from a total of {}.", i, svg_tree.root().children().len());
i += 1;
}
}| /* | ||
| let mut path = BezPath::new(); | ||
| for subpath in vectorized_image.paths.iter() { | ||
| let svg_path = subpath.to_string(); | ||
| println!("svg_path: {}", svg_path); | ||
| // TODO: use usvg instead to read from the svg path | ||
| let bezpath: BezPath = kurbo::BezPath::from_svg(svg_path.as_str()).expect("failed to convert SVG into BezPath."); | ||
|
|
||
| for curve in bezpath.segments() { | ||
| match curve { | ||
| PathSeg::Line(line) => { | ||
| println!("Inserting line: {:?}", line); | ||
| let a = transform.transform_point2(point_to_dvec2(line.p1)); | ||
| path.line_to(kurbo::Point::new(a.x, a.y)); | ||
| } | ||
| PathSeg::Quad(quad_bez) => { | ||
| println!("Inserting quad_bez: {:?}", quad_bez); | ||
| let a = transform.transform_point2(point_to_dvec2(quad_bez.p1)); | ||
| let b = transform.transform_point2(point_to_dvec2(quad_bez.p2)); | ||
| path.quad_to(kurbo::Point::new(a.x, a.y), kurbo::Point::new(b.x, b.y)); | ||
| } | ||
| PathSeg::Cubic(cubic_bez) => { | ||
| println!("Inserting cubic_bez: {:?}", cubic_bez); | ||
| let a = transform.transform_point2(point_to_dvec2(cubic_bez.p1)); | ||
| let b = transform.transform_point2(point_to_dvec2(cubic_bez.p2)); | ||
| let c = transform.transform_point2(point_to_dvec2(cubic_bez.p3)); | ||
| path.curve_to(kurbo::Point::new(a.x, a.y), kurbo::Point::new(b.x, b.y), kurbo::Point::new(c.x, c.y)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| */ |
| pub struct ParsedSvgText { | ||
| text: String, | ||
| transform: DAffine2, | ||
| } |
There was a problem hiding this comment.
The fields of ParsedSvgText are currently private, which prevents them from being accessed outside of usvg_utils.rs. Making them pub ensures consistency with ParsedSvgGroup and ParsedSvgPath.
| pub struct ParsedSvgText { | |
| text: String, | |
| transform: DAffine2, | |
| } | |
| pub struct ParsedSvgText { | |
| pub text: String, | |
| pub transform: DAffine2, | |
| } |
| // let mut child_extents_svg_order: Vec<u32> = Vec::new(); | ||
| // let mut group_extents_map: HashMap<LayerNodeIdentifier, Vec<u32>> = HashMap::new(); | ||
|
|
||
| // let get_child_extents = |group: &Box<Group>, group_extents_map: HashMap<LayerNodeIdentifier, Vec<u32>>| { | ||
| // let mut child_extents: Vec<u32> = Vec::new(); | ||
| // for child in group.children() { | ||
| // let extent = get_child_extend(); | ||
| // child_extents.push(extent); | ||
| // } | ||
|
|
||
| // let n = child_extents.len(); | ||
| // let total_extent = if n == 0 { | ||
| // 0 | ||
| // } else { | ||
| // (2 * STACK_VERTICAL_GAP as u32) * n as u32 - STACK_VERTICAL_GAP as u32 + child_extents.iter().sum::<u32>() | ||
| // }; | ||
| // group_extents_map.insert(layer, child_extents); | ||
| // total_extent | ||
| // }; |
| // match child { | ||
| // ParsedSvgNode::Group(parsed_group) => { | ||
| // parsed_group.extent = get_child_extents(group, group_extents_map); | ||
| // child_extents_svg_order.push(parsed_group.extent); | ||
| // } | ||
| // _ => {} | ||
| // } |
As requested in #2332.