usvg::ClipPath is always in userSpaceOnUse units now.

CHANGELOG.md

@@ -19,6 +19,7 @@ This changelog also contains important changes in dependencies.
 - All `usvg::Tree` parsing methods require the `fontdb` argument now.
 - All `defs` children like gradients, patterns, clipPaths, masks and filters are guarantee
   to have a unique, non-empty ID.
+- `usvg::ClipPath` is always in `userSpaceOnUse` units now.
 - Text nodes will not be parsed when the `text` build feature isn't enabled.
 - `usvg::Tree::clip_paths`, `usvg::Tree::masks`, `usvg::Tree::filters` returns
   a pre-collected slice of unique nodes now.
@@ -37,6 +38,7 @@ This changelog also contains important changes in dependencies.
 
 ### Removed
 - `usvg::Tree::postprocess()` and `usvg::PostProcessingSteps`. No longer needed.
+- `usvg::ClipPath::units()`. It's always `userSpaceOnUse` now.
 
 ### Fixed
 - Text bounding box is accounted during SVG size resolving.

crates/resvg/src/clip.rs

@@ -10,28 +10,14 @@ pub fn apply(
     transform: tiny_skia::Transform,
     pixmap: &mut tiny_skia::Pixmap,
 ) {
-    let mut clip_transform = clip.transform();
-    if clip.units() == usvg::Units::ObjectBoundingBox {
-        let object_bbox = match object_bbox.to_non_zero_rect() {
-            Some(v) => v,
-            None => {
-                log::warn!("Clipping of zero-sized shapes is not allowed.");
-                return;
-            }
-        };
-
-        let ts = usvg::Transform::from_bbox(object_bbox);
-        clip_transform = clip_transform.pre_concat(ts);
-    }
-
     let mut clip_pixmap = tiny_skia::Pixmap::new(pixmap.width(), pixmap.height()).unwrap();
     clip_pixmap.fill(tiny_skia::Color::BLACK);
 
     draw_children(
         clip.root(),
         tiny_skia::BlendMode::Clear,
         object_bbox,
-        transform.pre_concat(clip_transform),
+        transform.pre_concat(clip.transform()),
         &mut clip_pixmap.as_mut(),
     );
 

crates/usvg/docs/spec.adoc

@@ -183,8 +183,6 @@ Doesn't have a `xlink:href` attribute because all attributes and children will b
 * `clip-path` = <<func-iri-type,<FuncIRI> >>? +
   An optional reference to a supplemental `clipPath`. +
   Default: none
-* `clipPathUnits` = `objectBoundingBox`? +
-  Default: userSpaceOnUse
 * `transform` = <<transform-type,<transform> >>?
 
 [[mask-element]]

crates/usvg/src/parser/clippath.rs

@@ -7,11 +7,12 @@ use std::sync::Arc;
 
 use super::converter;
 use super::svgtree::{AId, EId, SvgNode};
-use crate::{ClipPath, Group, NonEmptyString, Transform, Units};
+use crate::{ClipPath, Group, NonEmptyString, Rect, Transform, Units};
 
 pub(crate) fn convert(
     node: SvgNode,
     state: &converter::State,
+    object_bbox: Option<Rect>,
     cache: &mut converter::Cache,
 ) -> Option<Arc<ClipPath>> {
     // A `clip-path` attribute must reference a `clipPath` element.
@@ -20,17 +21,40 @@ pub(crate) fn convert(
     }
 
     // The whole clip path should be ignored when a transform is invalid.
-    let transform = resolve_clip_path_transform(node, state)?;
+    let mut transform = resolve_clip_path_transform(node, state)?;
+
+    let units = node
+        .attribute(AId::ClipPathUnits)
+        .unwrap_or(Units::UserSpaceOnUse);
 
     // Check if this element was already converted.
-    if let Some(clip) = cache.clip_paths.get(node.element_id()) {
-        return Some(clip.clone());
+    //
+    // Only `userSpaceOnUse` clipPaths can be shared,
+    // because `objectBoundingBox` one will be converted into user one
+    // and will become node-specific.
+    if units == Units::UserSpaceOnUse {
+        if let Some(clip) = cache.clip_paths.get(node.element_id()) {
+            return Some(clip.clone());
+        }
+    }
+
+    if units == Units::ObjectBoundingBox {
+        let object_bbox = match object_bbox?.to_non_zero_rect() {
+            Some(v) => v,
+            None => {
+                log::warn!("Clipping of zero-sized shapes is not allowed.");
+                return None;
+            }
+        };
+
+        let ts = Transform::from_bbox(object_bbox);
+        transform = transform.pre_concat(ts);
     }
 
     // Resolve linked clip path.
     let mut clip_path = None;
     if let Some(link) = node.attribute::<SvgNode>(AId::ClipPath) {
-        clip_path = convert(link, state, cache);
+        clip_path = convert(link, state, object_bbox, cache);
 
         // Linked `clipPath` must be valid.
         if clip_path.is_none() {
@@ -40,13 +64,8 @@ pub(crate) fn convert(
 
     let id = NonEmptyString::new(node.element_id().to_string())?;
 
-    let units = node
-        .attribute(AId::ClipPathUnits)
-        .unwrap_or(Units::UserSpaceOnUse);
-
     let mut clip = ClipPath {
         id,
-        units,
         transform,
         clip_path,
         root: Group::empty(),

crates/usvg/src/parser/converter.rs

@@ -574,34 +574,64 @@ pub(crate) fn convert_group(
         Opacity::ONE
     };
 
-    // TODO: remove macro
-    macro_rules! resolve_link {
-        ($aid:expr, $f:expr) => {{
-            let mut v = None;
+    let transform = node.resolve_transform(AId::Transform, state);
+    let blend_mode: BlendMode = node.attribute(AId::MixBlendMode).unwrap_or_default();
+    let isolation: Isolation = node.attribute(AId::Isolation).unwrap_or_default();
+    let isolate = isolation == Isolation::Isolate;
 
-            if let Some(link) = node.attribute::<SvgNode>($aid) {
-                v = $f(link, state, cache);
+    // Nodes generated by markers must not have an ID. Otherwise we would have duplicates.
+    let is_g_or_use = matches!(node.tag_name(), Some(EId::G) | Some(EId::Use));
+    let id = if is_g_or_use && state.parent_markers.is_empty() {
+        node.element_id().to_string()
+    } else {
+        String::new()
+    };
 
-                // If `$aid` is linked to an invalid element - skip this group completely.
-                if v.is_none() {
-                    return None;
-                }
-            }
+    let abs_transform = parent.abs_transform.pre_concat(transform);
+    let dummy = Rect::from_xywh(0.0, 0.0, 0.0, 0.0).unwrap();
+    let mut g = Group {
+        id,
+        transform,
+        abs_transform,
+        opacity,
+        blend_mode,
+        isolate,
+        clip_path: None,
+        mask: None,
+        filters: Vec::new(),
+        bounding_box: dummy,
+        abs_bounding_box: dummy,
+        stroke_bounding_box: dummy,
+        abs_stroke_bounding_box: dummy,
+        layer_bounding_box: NonZeroRect::from_xywh(0.0, 0.0, 1.0, 1.0).unwrap(),
+        children: Vec::new(),
+    };
+    collect_children(cache, &mut g);
 
-            v
-        }};
-    }
+    // We need to know group's bounding box before converting
+    // clipPaths, masks and filters.
+    let object_bbox = g.calculate_object_bbox();
 
     // `mask` and `filter` cannot be set on `clipPath` children.
     // But `clip-path` can.
 
-    let clip_path = resolve_link!(AId::ClipPath, super::clippath::convert);
+    let mut clip_path = None;
+    if let Some(link) = node.attribute::<SvgNode>(AId::ClipPath) {
+        clip_path = super::clippath::convert(link, state, object_bbox, cache);
+        if clip_path.is_none() {
+            return None;
+        }
+    }
 
-    let mask = if state.parent_clip_path.is_none() {
-        resolve_link!(AId::Mask, super::mask::convert)
-    } else {
-        None
-    };
+    let mut mask = None;
+    if state.parent_clip_path.is_none() {
+        if let Some(link) = node.attribute::<SvgNode>(AId::Mask) {
+            mask = (super::mask::convert)(link, state, cache);
+            if mask.is_none() {
+                return None;
+            }
+        }
+    }
 
     let filters = {
         let mut filters = Vec::new();
@@ -632,13 +662,6 @@ pub(crate) fn convert_group(
         filters
     };
 
-    let transform = node.resolve_transform(AId::Transform, state);
-    let blend_mode: BlendMode = node.attribute(AId::MixBlendMode).unwrap_or_default();
-    let isolation: Isolation = node.attribute(AId::Isolation).unwrap_or_default();
-    let isolate = isolation == Isolation::Isolate;
-
-    // TODO: ignore just transform
-    let is_g_or_use = matches!(node.tag_name(), Some(EId::G) | Some(EId::Use));
     let required = opacity.get().approx_ne_ulps(&1.0, 4)
         || clip_path.is_some()
         || mask.is_some()
@@ -650,37 +673,15 @@ pub(crate) fn convert_group(
         || force;
 
     if !required {
-        collect_children(cache, parent);
+        parent.children.append(&mut g.children);
         return None;
     }
 
-    // Nodes generated by markers must not have an ID. Otherwise we would have duplicates.
-    let id = if is_g_or_use && state.parent_markers.is_empty() {
-        node.element_id().to_string()
-    } else {
-        String::new()
-    };
+    g.clip_path = clip_path;
+    g.mask = mask;
+    g.filters = filters;
 
-    let abs_transform = parent.abs_transform.pre_concat(transform);
-    let dummy = Rect::from_xywh(0.0, 0.0, 0.0, 0.0).unwrap();
-    let mut g = Group {
-        id,
-        transform,
-        abs_transform,
-        opacity,
-        blend_mode,
-        isolate,
-        clip_path,
-        mask,
-        filters,
-        bounding_box: dummy,
-        abs_bounding_box: dummy,
-        stroke_bounding_box: dummy,
-        abs_stroke_bounding_box: dummy,
-        layer_bounding_box: NonZeroRect::from_xywh(0.0, 0.0, 1.0, 1.0).unwrap(),
-        children: Vec::new(),
-    };
-    collect_children(cache, &mut g);
+    // Must be called after we set Group::filters
     g.calculate_bounding_boxes();
 
     Some(g)

crates/usvg/src/tree/mod.rs

@@ -762,7 +762,6 @@ impl PartialEq for Paint {
 #[derive(Debug)]
 pub struct ClipPath {
     pub(crate) id: NonEmptyString,
-    pub(crate) units: Units,
     pub(crate) transform: Transform,
     pub(crate) clip_path: Option<Arc<ClipPath>>,
     pub(crate) root: Group,
@@ -772,7 +771,6 @@ impl ClipPath {
     pub(crate) fn empty(id: NonEmptyString) -> Self {
         ClipPath {
             id,
-            units: Units::UserSpaceOnUse,
             transform: Transform::default(),
             clip_path: None,
             root: Group::empty(),
@@ -787,13 +785,6 @@ impl ClipPath {
         self.id.get()
     }
 
-    /// Coordinate system units.
-    ///
-    /// `clipPathUnits` in SVG.
-    pub fn units(&self) -> Units {
-        self.units
-    }
-
     /// Clip path transform.
     ///
     /// `transform` in SVG.
@@ -1805,7 +1796,22 @@ impl Group {
         }
     }
 
-    /// Calculates bounding boxes for all children of this group.
+    pub(crate) fn calculate_object_bbox(&mut self) -> Option<Rect> {
+        let mut bbox = BBox::default();
+        for child in &self.children {
+            let mut c_bbox = child.bounding_box();
+            if let Node::Group(ref group) = child {
+                if let Some(r) = c_bbox.transform(group.transform) {
+                    c_bbox = r;
+                }
+            }
+
+            bbox = bbox.expand(c_bbox);
+        }
+
+        bbox.to_rect()
+    }
+
     pub(crate) fn calculate_bounding_boxes(&mut self) -> Option<()> {
         let mut bbox = BBox::default();
         let mut abs_bbox = BBox::default();

crates/usvg/src/writer.rs

@@ -511,7 +511,6 @@ fn write_defs(tree: &Tree, opt: &XmlOptions, xml: &mut XmlWriter) {
     for clip in tree.clip_paths() {
         xml.start_svg_element(EId::ClipPath);
         xml.write_id_attribute(clip.id(), opt);
-        xml.write_units(AId::ClipPathUnits, clip.units, Units::UserSpaceOnUse);
         xml.write_transform(AId::Transform, clip.transform, opt);
 
         if let Some(ref clip) = clip.clip_path {