Merge pull request #5243 from dotCooCoo/fix/svg-round-trip-edge-ellipse

SVG: an exported ellipse imports as the four-anchor ellipse it was written from
This commit is contained in:
Brandon Thomas
2026-11-03 00:18:50 -05:00
committed by GitHub
7 changed files with 253 additions and 6 deletions
+3
View File
@@ -161,6 +161,7 @@ pub(crate) fn import(svg: &str, opts: &ImportOptions, max_nodes: usize) -> Resul
foreign: found.foreign,
sublayers: found.sublayers,
sublayer_groups: HashSet::new(),
ellipses: found.ellipses,
screen: k,
converted_nodes: 1,
max_nodes,
@@ -318,6 +319,8 @@ struct Importer {
sublayers: HashSet<String>,
/// The groups made of them, made sublayers where they sit in a layer ([`promote_sublayers`]).
sublayer_groups: HashSet<NodeId>,
/// Ids of the `<circle>` and `<ellipse>` elements ([`prepass`]).
ellipses: HashSet<String>,
/// Points per screen pixel (the root's user unit before its `viewBox`): what a non-scaling
/// stroke's width is measured in.
screen: f64,
+47 -1
View File
@@ -1,4 +1,6 @@
use super::{Affine, Arc, BezPath, BlendMode, Color, FillRule, GradientStop, HashMap, Node, Point, XNode, roxmltree};
use super::{Affine, Arc, BezPath, BlendMode, Color, FillRule, GradientStop, HashMap, Node, PathData, Point, XNode, roxmltree};
use vectorcraft_geom::path::{Anchor, AnchorKind, SubPath};
use vectorcraft_geom::shapes::KAPPA;
/// `n` with every object id (its descendants' and its mask art's too) zeroed, to compare art.
pub(super) fn without_ids(n: &Node) -> Node {
@@ -135,6 +137,50 @@ pub(super) fn bezpath(p: &usvg::tiny_skia_path::Path, m: Affine) -> BezPath {
bp
}
/// The path usvg makes of a `<circle>` or `<ellipse>` (`p`: one closed outline of arcs from the
/// right end of the horizontal axis round through the bottom) as the ellipse VectorCraft draws: four
/// smooth anchors on the axes in the same order, each quarter a cubic with handles [`KAPPA`] of the
/// radii long, the ellipse export writes as `<ellipse>`. `None` when `p` is not such an outline.
pub(super) fn drawn_ellipse(p: &usvg::tiny_skia_path::Path) -> Option<PathData> {
use usvg::tiny_skia_path::PathSegment as S;
let mut ends = vec![];
let mut closed = false;
for s in p.segments() {
match s {
S::MoveTo(q) if ends.is_empty() => ends.push(Point::new(q.x as f64, q.y as f64)),
S::LineTo(q) | S::QuadTo(_, q) | S::CubicTo(_, _, q) if !ends.is_empty() && !closed => ends.push(Point::new(q.x as f64, q.y as f64)),
S::Close if !closed => closed = true,
_ => return None,
}
}
let (x0, x1) = ends.iter().fold((f64::INFINITY, f64::NEG_INFINITY), |(lo, hi), q| (lo.min(q.x), hi.max(q.x)));
let (y0, y1) = ends.iter().fold((f64::INFINITY, f64::NEG_INFINITY), |(lo, hi), q| (lo.min(q.y), hi.max(q.y)));
let (c, rx, ry) = (Point::new((x0 + x1) / 2.0, (y0 + y1) / 2.0), (x1 - x0) / 2.0, (y1 - y0) / 2.0);
// usvg works in f32: points match within a few of its steps at the size of the coordinates, and
// a radius of fewer than two steps does not survive the conversion.
let step = f64::from(f32::EPSILON) * [x0, x1, y0, y1].iter().fold(1.0f64, |m, v| m.max(v.abs()));
let tol = 16.0 * step;
let axes = [Point::new(x1, c.y), Point::new(c.x, y1), Point::new(x0, c.y), Point::new(c.x, y0)];
let off = 1e-3 + 4.0 * tol / rx.min(ry);
let on_ellipse = |q: &Point| (((q.x - c.x) / rx).powi(2) + ((q.y - c.y) / ry).powi(2) - 1.0).abs() <= off;
if !closed
|| rx < 2.0 * step
|| ry < 2.0 * step
|| ends.first().is_none_or(|s| s.distance(axes[0]) > tol)
|| !axes.iter().all(|a| ends.iter().any(|q| q.distance(*a) <= tol))
|| !ends.iter().all(on_ellipse)
{
return None;
}
let anchors = (0..4)
.map(|i| {
let (p, next, prev) = (axes[i], axes[(i + 1) % 4], axes[(i + 3) % 4]);
Anchor { p, h_in: p + (prev - c) * KAPPA, h_out: p + (next - c) * KAPPA, kind: AnchorKind::Smooth }
})
.collect();
Some(PathData::single(SubPath::new(anchors, true)))
}
/// The shapes of clip path `cp` on an element whose user space `ts` maps to the document.
pub(super) fn clip_shapes(cp: &usvg::ClipPath, ts: Affine) -> Vec<(BezPath, FillRule)> {
fn collect(g: &usvg::Group, m: Affine, out: &mut Vec<(BezPath, FillRule)>) {
+6 -3
View File
@@ -1,4 +1,4 @@
use super::convert::{aff, bezpath, blend, clip_shapes, gradient_stops, is_plain, made_up, rule, uncut, unique_name, without_ids};
use super::convert::{aff, bezpath, blend, clip_shapes, drawn_ellipse, gradient_stops, is_plain, made_up, rule, uncut, unique_name, without_ids};
use super::layers::filter_names;
use super::prepass::link;
use super::{
@@ -601,7 +601,8 @@ impl Importer {
if !p.is_visible() {
return None;
}
let path = PathData::from_bezpath(&bezpath(p.data(), acc));
let ellipse = if self.ellipses.contains(p.id()) { drawn_ellipse(p.data()) } else { None };
let path = ellipse.map_or_else(|| PathData::from_bezpath(&bezpath(p.data(), acc)), |e| e.transformed(acc));
if path.is_empty() {
return None;
}
@@ -683,9 +684,10 @@ impl Importer {
std::mem::take(&mut self.non_scaling),
std::mem::take(&mut self.data),
std::mem::take(&mut self.events),
std::mem::take(&mut self.ellipses),
);
let art = self.group_node(tree.root(), acc);
(self.links, self.hidden, self.uses, self.labels, self.non_scaling, self.data, self.events) = outer;
(self.links, self.hidden, self.uses, self.labels, self.non_scaling, self.data, self.events, self.ellipses) = outer;
art
}
}
@@ -720,6 +722,7 @@ mod tests {
foreign: HashMap::new(),
sublayers: HashSet::new(),
sublayer_groups: HashSet::new(),
ellipses: HashSet::new(),
screen: 1.0,
converted_nodes: 0,
max_nodes: 1000,
+67 -2
View File
@@ -28,6 +28,9 @@ pub(super) struct Found {
pub(super) scripts: Vec<String>,
/// Ids of the `<g>` elements marked as layers ([`marked_layer`]).
pub(super) sublayers: HashSet<String>,
/// Ids of the `<circle>` and `<ellipse>` elements, which come back as the ellipse VectorCraft
/// draws ([`drawn_ellipse`](super::convert::drawn_ellipse)).
pub(super) ellipses: HashSet<String>,
pub(super) warnings: Vec<String>,
}
@@ -37,6 +40,11 @@ pub(crate) struct Edits {
list: Vec<(std::ops::Range<usize>, String)>,
/// The ids made up so far, by element.
ids: HashMap<roxmltree::NodeId, String>,
/// The numbers `n` of the `{MADE_UP_ID}n` the file names (as an element's id, in a style
/// sheet's `#id` selector or in a `#id` link to no element), read when the first id is made up.
taken: Option<HashSet<usize>>,
/// The number the next made-up id is tried with: the numbers below it are made up or taken.
next: usize,
/// Declarations added to elements' `style`s, by element: (where they go, whether the element
/// has a `style` attribute, the declarations, XML-escaped).
styles: HashMap<roxmltree::NodeId, (usize, bool, String)>,
@@ -47,7 +55,8 @@ impl Edits {
self.list.push((at..at, text));
}
/// The id of element `n` (in `svg`), made up and written in when it has none.
/// The id of element `n` (in `svg`), made up and written in when it has none. A made-up id is
/// one the file does not name ([`named_made_up_ids`]).
pub(super) fn id(&mut self, svg: &str, n: XNode) -> String {
if let Some(id) = n.attribute("id").filter(|id| !id.is_empty()) {
return id.to_string();
@@ -55,7 +64,12 @@ impl Edits {
if let Some(id) = self.ids.get(&n.id()) {
return id.clone();
}
let id = format!("{MADE_UP_ID}{}", self.ids.len());
let taken = self.taken.get_or_insert_with(|| named_made_up_ids(n.document()));
while taken.contains(&self.next) {
self.next += 1;
}
let id = format!("{MADE_UP_ID}{}", self.next);
self.next += 1;
self.set(svg, n, "id", &id);
self.ids.insert(n.id(), id.clone());
id
@@ -105,6 +119,24 @@ impl Edits {
}
}
/// The number after each [`MADE_UP_ID`] followed by digits in the attribute values and text of
/// `xml`, read with their character references decoded (numbers too large for a `usize` left out).
fn named_made_up_ids(xml: &roxmltree::Document) -> HashSet<usize> {
fn numbers(s: &str) -> impl Iterator<Item = usize> + '_ {
s.match_indices(MADE_UP_ID).filter_map(|(at, _)| {
let rest = s.get(at + MADE_UP_ID.len()..)?;
let digits = rest.bytes().take_while(u8::is_ascii_digit).count();
rest.get(..digits)?.parse().ok()
})
}
let mut taken = HashSet::new();
for n in xml.descendants() {
taken.extend(n.attributes().flat_map(|a| numbers(a.value())));
taken.extend(n.text().filter(|_| n.is_text()).into_iter().flat_map(numbers));
}
taken
}
/// usvg reads `<a href>` and `<use>` as plain groups, drops what isn't displayed and reads linked
/// files on its own. Before it runs, the SVG is changed so that what we keep can be found:
/// * every `<a href>` and every `<use>` of a `<symbol>` gets an id when it has none
@@ -115,6 +147,9 @@ impl Edits {
/// * objects with event attributes get an id when they have none, recorded with them
/// ([`user_events`]), and the `<script>` links are recorded ([`linked_scripts`]);
/// * so do objects with attributes in other applications' namespaces ([`foreign`]);
/// * so do `<circle>` and `<ellipse>` elements, recorded: usvg makes their arcs into a varying
/// number of curves, and they come back as the ellipse VectorCraft draws (not in a file whose
/// style sheet has an `[id` selector);
/// * linked files are read ([`files`]);
/// * percentage lengths in a `<symbol>` with a `viewBox` become numbers ([`symbol_units`]);
/// * what style sheet rules with a `:not()` set goes in `style` attributes ([`not_rules`]).
@@ -129,10 +164,13 @@ pub(super) fn prepass<'s>(svg: &'s str, markers: &str, xml: &roxmltree::Document
let not = markers.contains(":not(");
let events = SVG_EVENTS.iter().any(|e| markers.contains(e));
let foreign_attrs = markers.contains("xmlns:") && has_foreign_attributes(xml);
// An id would change what a style sheet's `[id]` selector matches: such files keep usvg's curves.
let ellipses = !markers.contains("[id") && ["<circle", ":circle", "<ellipse", ":ellipse"].iter().any(|t| markers.contains(t));
if !css_transform
&& !not
&& !events
&& !foreign_attrs
&& !ellipses
&& !["<a", "display", "<use", ":use", "<image", ":image", vector_effect::NON_SCALING, "data-", "groupmode", "script"]
.iter()
.any(|t| markers.contains(t))
@@ -170,6 +208,11 @@ pub(super) fn prepass<'s>(svg: &'s str, markers: &str, xml: &roxmltree::Document
}
}
}
if ellipses {
for e in elements().filter(|e| matches!(e.tag_name().name(), "circle" | "ellipse")) {
found.ellipses.insert(edits.id(svg, e));
}
}
if markers.contains("script") {
found.scripts = linked_scripts(xml, &mut found.warnings);
}
@@ -389,3 +432,25 @@ pub(super) fn labels(xml: &roxmltree::Document) -> HashMap<String, String> {
})
.collect()
}
#[cfg(test)]
mod tests {
use super::*;
/// A file that already holds the made-up ids does not make each new id try them all again:
/// 5,000 ids made up past 50,000 taken ones take a moment, not 250 million tries.
#[test]
fn made_up_ids_skip_the_taken_ones_once() {
let taken: String = (0..50_000).map(|i| format!("<g id=\"{MADE_UP_ID}{i}\"/>")).collect();
let bare = "<rect/>".repeat(5_000);
let svg = format!("<svg xmlns=\"http://www.w3.org/2000/svg\">{taken}{bare}</svg>");
let doc = roxmltree::Document::parse(&svg).unwrap();
let rects: Vec<XNode> = doc.descendants().filter(|n| n.has_tag_name("rect")).collect();
let mut edits = Edits::default();
let start = std::time::Instant::now();
let ids: Vec<String> = rects.iter().map(|n| edits.id(&svg, *n)).collect();
assert!(start.elapsed() < std::time::Duration::from_secs(1), "{:?}", start.elapsed());
assert_eq!(ids.first().map(String::as_str), Some(format!("{MADE_UP_ID}50000").as_str()));
assert_eq!(ids.iter().collect::<HashSet<_>>().len(), ids.len());
}
}
+101
View File
@@ -134,3 +134,104 @@ fn shape_elements_reimport_with_the_same_geometry() {
assert!(!far, "{a:?} came back as {b:?}");
}
}
/// The paths of the objects in `d`, in order.
fn paths(d: &Document) -> Vec<PathData> {
let mut v = Vec::new();
d.walk(|n| v.extend(n.path_data().cloned()));
v
}
#[test]
fn circles_and_ellipses_import_as_the_ellipse_drawn() {
// What export writes comes back with the anchors and handles it was written from.
let drawn = [shapes::ellipse(Rect::new(10.0, 80.0, 50.0, 120.0)), shapes::ellipse(Rect::new(60.0, 80.0, 140.0, 120.0))];
let d = doc(drawn.iter().map(|p| (p.clone(), false)).collect());
let back = paths(&import(&export(&d, &ExportOptions::default())).unwrap());
assert_eq!(back.len(), 2);
for (want, got) in drawn.iter().zip(&back) {
let [w, g] = [want, got].map(|p| &p.subpaths[0].anchors);
assert!(got.subpaths.len() == 1 && got.subpaths[0].closed && g.len() == 4, "{got:?}");
// The same four anchors, starting on the right as an SVG ellipse does.
for a in g {
let same = w.iter().any(|b| a.p.distance(b.p) < 1e-4 && a.h_in.distance(b.h_in) < 1e-4 && a.h_out.distance(b.h_out) < 1e-4);
assert!(same, "{a:?} is not one of {w:?}");
}
assert!((g[0].p.x - want.bounds().unwrap().x1).abs() < 1e-4, "{g:?}");
}
// From another app, with a transform: four anchors on the axes, transformed.
let svg = r#"<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
<circle cx="50" cy="50" r="40" transform="translate(10 20)"/>
<ellipse cx="100" cy="100" rx="60" ry="20" transform="rotate(30 100 100)"/>
<ellipse cx="36.255" cy="154.118" rx="36.255" ry="5.421"/>
<circle cx="40000000" cy="0" r="400"/>
<ellipse cx="1000000" cy="1000000" rx="36" ry="1"/></svg>"#;
let back = paths(&import(svg).unwrap());
assert_eq!(back.len(), 5);
let circle = &back[0].subpaths[0].anchors;
let want: Vec<Point> = [(100.0, 70.0), (60.0, 110.0), (20.0, 70.0), (60.0, 30.0)].map(|(x, y)| Point::new(x, y)).into();
assert_eq!(circle.len(), 4);
for (a, w) in circle.iter().zip(&want) {
assert!(a.p.distance(*w) < 1e-4, "{circle:?}");
}
let turned = &back[1].subpaths[0].anchors;
let r = Affine::rotate_about(30f64.to_radians(), Point::new(100.0, 100.0));
assert!(turned.len() == 4 && turned[0].p.distance(r * Point::new(160.0, 100.0)) < 1e-3, "{turned:?}");
// However many curves the arcs of a flat ellipse make.
let flat = &back[2].subpaths[0].anchors;
assert!(flat.len() == 4 && flat[0].p.distance(Point::new(72.51, 154.118)) < 1e-3, "{flat:?}");
// A small circle far from the origin too.
let far = &back[3].subpaths[0].anchors;
assert!(far.len() == 4 && far[0].p.distance(Point::new(40_000_400.0, 0.0)) < 8.0, "{far:?}");
// A radius of a few of f32's steps at its coordinates, with handles of the standard length.
let thin = &back[4].subpaths[0].anchors;
let handle = thin.first().map(|a| a.h_out - a.p);
assert!(thin.len() == 4 && thin[0].p.distance(Point::new(1_000_036.0, 1_000_000.0)) < 0.25, "{thin:?}");
assert!(handle.is_some_and(|h| h.x.abs() < 1e-6 && (h.y - shapes::KAPPA).abs() < 1e-6), "{thin:?}");
}
#[test]
fn circles_import_without_changing_ids_or_style_matches() {
// The file's own ids stay unique: the `<use>` shows the rectangle, not the circle.
let svg = r##"<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="200">
<circle cx="30" cy="30" r="20" fill="#0000ff"/>
<defs><rect id="vectorcraft-id-0" width="40" height="40" fill="#ff0000"/></defs>
<use xlink:href="#vectorcraft-id-0" x="120" y="120"/></svg>"##;
let img = render_artboard(&import(svg).unwrap());
assert_eq!(img.over_white(30, 30), [0, 0, 255]);
assert_eq!(img.over_white(140, 140), [255, 0, 0]);
// A style sheet's `[id]` selector still matches only the circle that has an id.
let svg = r##"<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
<style>circle[id] { fill: #ff0000 }</style>
<circle cx="30" cy="30" r="20" fill="#0000ff"/>
<circle id="named" cx="130" cy="30" r="20" fill="#0000ff"/></svg>"##;
let img = render_artboard(&import(svg).unwrap());
assert_eq!(img.over_white(30, 30), [0, 0, 255]);
assert_eq!(img.over_white(130, 30), [255, 0, 0]);
}
#[test]
fn circles_import_without_an_id_the_file_names() {
// A style sheet's rule for an id no element has does not reach the circle.
let svg = r##"<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
<style>#vectorcraft-id-0 { fill: #ff0000 }</style>
<circle cx="30" cy="30" r="20" fill="#000000"/></svg>"##;
let img = render_artboard(&import(svg).unwrap());
assert_eq!(img.over_white(30, 30), [0, 0, 0]);
}
#[test]
fn circles_import_without_an_id_a_use_names() {
// A `<use>` of an id no element has does not show the circle, nor does one that writes the
// id with a character reference.
for href in ["#vectorcraft-id-0", "#vectorcraft-id-&#48;"] {
let svg = format!(
r##"<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="200">
<circle cx="30" cy="30" r="20" fill="#000000"/>
<use xlink:href="{href}" x="100"/></svg>"##
);
let img = render_artboard(&import(&svg).unwrap());
assert_eq!(img.over_white(30, 30), [0, 0, 0], "{href}");
assert_eq!(img.over_white(130, 30), [255, 255, 255], "{href}");
}
}
+28
View File
@@ -92,6 +92,34 @@ proptest! {
}
}
#[test]
fn filled_ellipse_on_the_artboard_edge_survives_visually() {
use vectorcraft_geom::Point;
use vectorcraft_geom::path::{Anchor, AnchorKind, SubPath};
let a = |p: (f64, f64), i: (f64, f64), o: (f64, f64)| Anchor {
p: Point::new(p.0, p.1),
h_in: Point::new(i.0, i.1),
h_out: Point::new(o.0, o.1),
kind: AnchorKind::Smooth,
};
let path = PathData::new(vec![SubPath::new(
vec![
a((0.0, 154.1183786634012), (0.0, 157.11240210742653), (0.0, 151.12435521937584)),
a((36.254761132097215, 148.69721965992943), (16.231809450081727, 148.69721965992943), (56.277712814112704, 148.69721965992943)),
a((72.50952226419443, 154.1183786634012), (72.50952226419443, 151.12435521937584), (72.50952226419443, 157.11240210742653)),
a((36.254761132097215, 159.53953766687295), (56.277712814112704, 159.53953766687295), (16.231809450081727, 159.53953766687295)),
],
true,
)]);
let mut b = DocBuilder::new(200.0, 200.0);
b.path(path, Appearance::basic(Paint::solid(Color::rgb(0.0, 0.0, 0.0)), Paint::None, 0.0), |n| n.opacity = 0.983_042_96);
let d = b.build();
let svg = export(&d, &ExportOptions::default());
assert!(svg.contains("<ellipse "), "{svg}");
let back = import(&svg).unwrap();
assert_similar(&render_artboard(&d), &render_artboard(&back), 6.0, 0.002);
}
#[test]
fn rich_fixture_exports_with_all_options() {
let s = fixtures::rich_session();
+1
View File
@@ -1088,6 +1088,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
SVG import's style cascade, used for hidden objects, non-scaling strokes and type, tests each element only against the `<style>` rules for its id, its classes and its element name, and the rules whose selector ends in none of these (`*`, `[x]`).
A `.svgz` file copied in a file manager pastes as its art, as a copied `.svg` file does.
SVG export (Save As, Export As, Export for Screens, Asset Export and the clipboard share the writer) writes simple shapes as SVG basic shapes, as the reference app does (an ellipse is an `ellipse` tag until an anchor is added, [forum](https://community.adobe.com/t5/illustrator-discussions/trouble-exporting-shape-with-points-to-svg/td-p/10720325); rectangles come out as `<rect>` and `<polygon>`, [forum](https://community.adobe.com/t5/illustrator-discussions/force-svg-export-using-all-lt-rect-gt/td-p/11282378)): an axis-aligned rectangle as `<rect>`, a circle or ellipse drawn with four standard arcs as `<circle>`/`<ellipse>`, a straight line as `<line>`, and other straight-segment paths as `<polygon>`/`<polyline>`, at the written precision (#4652). A rectangle or ellipse with a dashed stroke, whose dashes would start elsewhere, a turned ellipse, curves and compound paths stay `<path>`. `crates/svg/tests/it/basic_shapes.rs` checks each element, that the picture renders as the canvas does and that the geometry reimports.
SVG import reads each `<circle>` and `<ellipse>` as the ellipse VectorCraft draws: four smooth anchors on the axes, starting at the right, with handles of the standard length, so an ellipse exported as `<ellipse>` comes back with the anchors it was written from and draws the same (`circles_and_ellipses_import_as_the_ellipse_drawn` in `crates/svg/tests/it/basic_shapes.rs`, `filled_ellipse_on_the_artboard_edge_survives_visually` in `prop_svg.rs`).
SVG import keeps attributes in other applications' XML namespaces (`plotter:pen="2"`) on the objects and layers they belong to, as it keeps their `data-*` attributes: copies, duplicates and `.vectorcraft` files keep them, and SVG export writes them back with each namespace declared once on the root (the rest of #522; `crates/svg/tests/it/foreign_attributes.rs`, `tests/attributes.rs`).
In a `.ai` or EPS opened from its editing data, type has its text object's opacity and blend mode, whether it is made from the file's text document or taken from the page. Where such type has transparency and comes in with other art (the rest of its look, or page type and strokes that go with it), a group of them has it. Where the page draws its type, or the strokes around it, with transparency that is neither its text object's nor its text object's combined with that of the objects around it, the page's type and strokes keep their own instead (`crates/eps/src/import/tests_native.rs`). Where the page draws type of no text object over text objects made from the text document, with their words in any case, that type is theirs as the page draws it and is left out; the page's other type of no text object goes only to text objects whose type the text document can't make, or else is left out with a warning.
`.ai` editing-data import restores the document's listed graphic styles in panel order and links objects that use a listed style; unsupported appearances keep their drawn look (`crates/eps/src/import/ai/tests.rs`).