All defs children are guarantee to have a unique, non-empty ID.

Closes #699

CHANGELOG.md

@@ -11,6 +11,8 @@ This changelog also contains important changes in dependencies.
 - All types in `usvg` are immutable now. Meaning that `usvg::Tree` cannot be modified
   after creation anymore.
 - All struct fields in `usvg` are private now. Use getters instead.
+- All `defs` children like gradients, patterns, clipPaths, masks and filters are guarantee
+  to have a unique, non-empty ID.
 
 ## [0.39.0] - 2024-02-06
 ### Added

crates/usvg/src/parser/clippath.rs

@@ -8,7 +8,7 @@ use std::str::FromStr;
 
 use super::converter;
 use super::svgtree::{AId, EId, SvgNode};
-use crate::{ClipPath, Group, SharedClipPath, Transform, Units};
+use crate::{ClipPath, Group, NonEmptyString, SharedClipPath, Transform, Units};
 
 pub(crate) fn convert(
     node: SvgNode,
@@ -39,12 +39,14 @@ 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: node.element_id().to_string(),
+        id,
         units,
         transform,
         clip_path,

crates/usvg/src/parser/converter.rs

@@ -2,7 +2,8 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
+use std::hash::{Hash, Hasher};
 use std::rc::Rc;
 use std::str::FromStr;
 
@@ -27,12 +28,85 @@ pub struct State<'a> {
     pub(crate) opt: &'a Options,
 }
 
-#[derive(Default)]
+#[derive(Clone, Default)]
 pub struct Cache {
     pub clip_paths: HashMap<String, SharedClipPath>,
     pub masks: HashMap<String, SharedMask>,
     pub filters: HashMap<String, filter::SharedFilter>,
     pub paint: HashMap<String, Paint>,
+
+    // used for ID generation
+    all_ids: HashSet<u64>,
+    linear_gradient_index: usize,
+    radial_gradient_index: usize,
+    pattern_index: usize,
+    clip_path_index: usize,
+    filter_index: usize,
+}
+
+impl Cache {
+    // TODO: macros?
+    pub fn gen_linear_gradient_id(&mut self) -> NonEmptyString {
+        loop {
+            self.linear_gradient_index += 1;
+            let new_id = format!("linearGradient{}", self.linear_gradient_index);
+            let new_hash = string_hash(&new_id);
+            if !self.all_ids.contains(&new_hash) {
+                return NonEmptyString::new(new_id).unwrap();
+            }
+        }
+    }
+
+    pub fn gen_radial_gradient_id(&mut self) -> NonEmptyString {
+        loop {
+            self.radial_gradient_index += 1;
+            let new_id = format!("radialGradient{}", self.radial_gradient_index);
+            let new_hash = string_hash(&new_id);
+            if !self.all_ids.contains(&new_hash) {
+                return NonEmptyString::new(new_id).unwrap();
+            }
+        }
+    }
+
+    pub fn gen_pattern_id(&mut self) -> NonEmptyString {
+        loop {
+            self.pattern_index += 1;
+            let new_id = format!("pattern{}", self.pattern_index);
+            let new_hash = string_hash(&new_id);
+            if !self.all_ids.contains(&new_hash) {
+                return NonEmptyString::new(new_id).unwrap();
+            }
+        }
+    }
+
+    pub fn gen_clip_path_id(&mut self) -> NonEmptyString {
+        loop {
+            self.clip_path_index += 1;
+            let new_id = format!("clipPath{}", self.clip_path_index);
+            let new_hash = string_hash(&new_id);
+            if !self.all_ids.contains(&new_hash) {
+                return NonEmptyString::new(new_id).unwrap();
+            }
+        }
+    }
+
+    pub fn gen_filter_id(&mut self) -> NonEmptyString {
+        loop {
+            self.filter_index += 1;
+            let new_id = format!("filter{}", self.filter_index);
+            let new_hash = string_hash(&new_id);
+            if !self.all_ids.contains(&new_hash) {
+                return NonEmptyString::new(new_id).unwrap();
+            }
+        }
+    }
+}
+
+// TODO: is there a simpler way?
+fn string_hash(s: &str) -> u64 {
+    let mut h = std::collections::hash_map::DefaultHasher::new();
+    s.hash(&mut h);
+    h.finish()
 }
 
 impl<'a, 'input: 'a> SvgNode<'a, 'input> {
@@ -157,6 +231,7 @@ pub(crate) fn convert_doc(svg_doc: &svgtree::Document, opt: &Options) -> Result<
         size,
         view_box,
         root: Group::empty(),
+        cache: Cache::default(),
     };
 
     if !svg.is_visible_element(opt) {
@@ -172,8 +247,17 @@ pub(crate) fn convert_doc(svg_doc: &svgtree::Document, opt: &Options) -> Result<
         opt,
     };
 
-    let mut cache = Cache::default();
-    convert_children(svg_doc.root(), &state, &mut cache, &mut tree.root);
+    for node in svg_doc.descendants() {
+        if let Some(tag) = node.tag_name() {
+            if matches!(tag, EId::Filter | EId::ClipPath) {
+                if !node.element_id().is_empty() {
+                    tree.cache.all_ids.insert(string_hash(node.element_id()));
+                }
+            }
+        }
+    }
+
+    convert_children(svg_doc.root(), &state, &mut tree.cache, &mut tree.root);
 
     if restore_viewbox {
         tree.calculate_abs_transforms();

crates/usvg/src/parser/filter.rs

@@ -13,7 +13,8 @@ use strict_num::PositiveF32;
 use svgtypes::{Length, LengthUnit as Unit};
 
 use crate::{
-    filter, filter::*, ApproxZeroUlps, Color, Group, Node, NonZeroF32, NonZeroRect, Opacity, Units,
+    filter::{self, *},
+    ApproxZeroUlps, Color, Group, Node, NonEmptyString, NonZeroF32, NonZeroRect, Opacity, Units,
 };
 
 use super::converter::{self, SvgColorExt};
@@ -44,35 +45,36 @@ pub(crate) fn convert(
     let mut has_invalid_urls = false;
     let mut filters = Vec::new();
 
-    let create_base_filter_func = |kind, filters: &mut Vec<SharedFilter>| {
-        // Filter functions, unlike `filter` elements, do not have a filter region.
-        // We're currently do not support an unlimited region, so we simply use a fairly large one.
-        // This if far from ideal, but good for now.
-        // TODO: Should be fixed eventually.
-        let rect = match kind {
-            Kind::DropShadow(_) | Kind::GaussianBlur(_) => {
-                NonZeroRect::from_xywh(-0.5, -0.5, 2.0, 2.0).unwrap()
-            }
-            _ => NonZeroRect::from_xywh(-0.1, -0.1, 1.2, 1.2).unwrap(),
-        };
+    let create_base_filter_func =
+        |kind, filters: &mut Vec<SharedFilter>, cache: &mut converter::Cache| {
+            // Filter functions, unlike `filter` elements, do not have a filter region.
+            // We're currently do not support an unlimited region, so we simply use a fairly large one.
+            // This if far from ideal, but good for now.
+            // TODO: Should be fixed eventually.
+            let rect = match kind {
+                Kind::DropShadow(_) | Kind::GaussianBlur(_) => {
+                    NonZeroRect::from_xywh(-0.5, -0.5, 2.0, 2.0).unwrap()
+                }
+                _ => NonZeroRect::from_xywh(-0.1, -0.1, 1.2, 1.2).unwrap(),
+            };
 
-        filters.push(Rc::new(RefCell::new(Filter {
-            id: String::new(),
-            units: Units::ObjectBoundingBox,
-            primitive_units: Units::UserSpaceOnUse,
-            rect,
-            primitives: vec![Primitive {
-                x: None,
-                y: None,
-                width: None,
-                height: None,
-                // Unlike `filter` elements, filter functions use sRGB colors by default.
-                color_interpolation: ColorInterpolation::SRGB,
-                result: "result".to_string(),
-                kind,
-            }],
-        })));
-    };
+            filters.push(Rc::new(RefCell::new(Filter {
+                id: cache.gen_filter_id(),
+                units: Units::ObjectBoundingBox,
+                primitive_units: Units::UserSpaceOnUse,
+                rect,
+                primitives: vec![Primitive {
+                    x: None,
+                    y: None,
+                    width: None,
+                    height: None,
+                    // Unlike `filter` elements, filter functions use sRGB colors by default.
+                    color_interpolation: ColorInterpolation::SRGB,
+                    result: "result".to_string(),
+                    kind,
+                }],
+            })));
+        };
 
     for func in svgtypes::FilterValueListParser::from(value) {
         let func = match func {
@@ -85,9 +87,11 @@ pub(crate) fn convert(
         };
 
         match func {
-            svgtypes::FilterValue::Blur(std_dev) => {
-                create_base_filter_func(convert_blur_function(node, std_dev, state), &mut filters)
-            }
+            svgtypes::FilterValue::Blur(std_dev) => create_base_filter_func(
+                convert_blur_function(node, std_dev, state),
+                &mut filters,
+                cache,
+            ),
             svgtypes::FilterValue::DropShadow {
                 color,
                 dx,
@@ -96,30 +100,31 @@ pub(crate) fn convert(
             } => create_base_filter_func(
                 convert_drop_shadow_function(node, color, dx, dy, std_dev, state),
                 &mut filters,
+                cache,
             ),
             svgtypes::FilterValue::Brightness(amount) => {
-                create_base_filter_func(convert_brightness_function(amount), &mut filters)
+                create_base_filter_func(convert_brightness_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::Contrast(amount) => {
-                create_base_filter_func(convert_contrast_function(amount), &mut filters)
+                create_base_filter_func(convert_contrast_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::Grayscale(amount) => {
-                create_base_filter_func(convert_grayscale_function(amount), &mut filters)
+                create_base_filter_func(convert_grayscale_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::HueRotate(angle) => {
-                create_base_filter_func(convert_hue_rotate_function(angle), &mut filters)
+                create_base_filter_func(convert_hue_rotate_function(angle), &mut filters, cache)
             }
             svgtypes::FilterValue::Invert(amount) => {
-                create_base_filter_func(convert_invert_function(amount), &mut filters)
+                create_base_filter_func(convert_invert_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::Opacity(amount) => {
-                create_base_filter_func(convert_opacity_function(amount), &mut filters)
+                create_base_filter_func(convert_opacity_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::Sepia(amount) => {
-                create_base_filter_func(convert_sepia_function(amount), &mut filters)
+                create_base_filter_func(convert_sepia_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::Saturate(amount) => {
-                create_base_filter_func(convert_saturate_function(amount), &mut filters)
+                create_base_filter_func(convert_saturate_function(amount), &mut filters, cache)
             }
             svgtypes::FilterValue::Url(url) => {
                 if let Some(link) = node.document().element_by_id(url) {
@@ -208,8 +213,10 @@ fn convert_url(
         return Err(());
     }
 
+    let id = NonEmptyString::new(node.element_id().to_string()).ok_or(())?;
+
     let filter = Rc::new(RefCell::new(Filter {
-        id: node.element_id().to_string(),
+        id,
         units,
         primitive_units,
         rect,

crates/usvg/src/parser/marker.rs

@@ -120,7 +120,7 @@ fn resolve(
             r.size().to_non_zero_rect(0.0, 0.0)
         };
 
-        let mut clip_path = ClipPath::empty();
+        let mut clip_path = ClipPath::empty(cache.gen_clip_path_id());
 
         let mut path = Path::with_path(Rc::new(tiny_skia_path::PathBuilder::from_rect(
             clip_rect.to_rect(),

crates/usvg/src/parser/mask.rs

@@ -9,7 +9,7 @@ use svgtypes::{Length, LengthUnit as Unit};
 
 use super::svgtree::{AId, EId, SvgNode};
 use super::{converter, OptionLog};
-use crate::{Group, Mask, MaskType, NonZeroRect, SharedMask, Units};
+use crate::{Group, Mask, MaskType, NonEmptyString, NonZeroRect, SharedMask, Units};
 
 pub(crate) fn convert(
     node: SvgNode,
@@ -26,6 +26,8 @@ pub(crate) fn convert(
         return Some(mask.clone());
     }
 
+    let id = NonEmptyString::new(node.element_id().to_string())?;
+
     let units = node
         .attribute(AId::MaskUnits)
         .unwrap_or(Units::ObjectBoundingBox);
@@ -61,7 +63,7 @@ pub(crate) fn convert(
     };
 
     let mut mask = Mask {
-        id: node.element_id().to_string(),
+        id,
         units,
         content_units,
         rect,

crates/usvg/src/parser/mod.rs

@@ -18,6 +18,7 @@ mod text;
 mod units;
 mod use_node;
 
+pub(crate) use converter::Cache;
 pub use image::{ImageHrefDataResolverFn, ImageHrefResolver, ImageHrefStringResolverFn};
 pub use options::Options;
 pub(crate) use svgtree::{AId, EId};

crates/usvg/src/parser/paint_server.rs

@@ -48,6 +48,8 @@ pub(crate) fn convert(
 
 #[inline(never)]
 fn convert_linear(node: SvgNode, state: &converter::State) -> Option<ServerOrColor> {
+    let id = NonEmptyString::new(node.element_id().to_string())?;
+
     let stops = convert_stops(find_gradient_with_stops(node)?);
     if stops.len() < 2 {
         return stops_to_color(&stops);
@@ -68,7 +70,7 @@ fn convert_linear(node: SvgNode, state: &converter::State) -> Option<ServerOrCol
         ),
         y2: resolve_number(node, AId::Y2, units, state, Length::zero()),
         base: BaseGradient {
-            id: node.element_id().to_string(),
+            id,
             units,
             transform,
             spread_method: convert_spread_method(node),
@@ -83,6 +85,8 @@ fn convert_linear(node: SvgNode, state: &converter::State) -> Option<ServerOrCol
 
 #[inline(never)]
 fn convert_radial(node: SvgNode, state: &converter::State) -> Option<ServerOrColor> {
+    let id = NonEmptyString::new(node.element_id().to_string())?;
+
     let stops = convert_stops(find_gradient_with_stops(node)?);
     if stops.len() < 2 {
         return stops_to_color(&stops);
@@ -129,7 +133,7 @@ fn convert_radial(node: SvgNode, state: &converter::State) -> Option<ServerOrCol
         fx,
         fy,
         base: BaseGradient {
-            id: node.element_id().to_string(),
+            id,
             units,
             transform,
             spread_method,
@@ -150,6 +154,8 @@ fn convert_pattern(
 ) -> Option<ServerOrColor> {
     let node_with_children = find_pattern_with_children(node)?;
 
+    let id = NonEmptyString::new(node.element_id().to_string())?;
+
     let view_box = {
         let n1 = resolve_attr(node, AId::ViewBox);
         let n2 = resolve_attr(node, AId::PreserveAspectRatio);
@@ -178,7 +184,7 @@ fn convert_pattern(
     })?;
 
     let mut patt = Pattern {
-        id: node.element_id().to_string(),
+        id,
         units,
         content_units,
         transform,