Try replacing `objectBoundingBox` with `userSpaceOnUse` during postprocessing.

[RazrFalcon]

Since we know all bounding boxes already we can resolve object units as well. At least when possible.

[LaurenzV]

Do you think there are cases where this might not be possible at all? Because it would allow to simplify a lot of things, for example, when resolving a pattern, we might be able to remove the viewBox, contentUnits and units attributes and instead resolve the transform necessary for rendering those at parse time, unless I'm missing something? Having those removed might make implementing context fill a lot easier as well.

[RazrFalcon]

Do you think there are cases where this might not be possible at all?

It's hard to be certain when it comes to SVG. In theory, it should be lossless. Will try soon.

[LaurenzV]

Nice, looking forward to that! Feel free to let me know if you need someone testing the changes (I'll try them out in svg2pdf).

[RazrFalcon]

I was able to implement objectBoundingBox resolving for clipPaths, masks and filters. Went relatively smooth. The only non-lossless part is this, because I have no idea what to do with it. Previously, it was located in resvg, which was fine, but now we have to keep in usvg.

As for gradients and patterns - I have no idea what to do. Because of text, as usual. The problem is that I have to know node's object bbox before parsing its styles (fill/stroke). It's fine for paths, but basically impossible for text. I have to parse text first, with dummy styles, calculate the bbox and then parse it again with actual styles. Which is absurd, because text parsing is one of the most complex and expensive parts in usvg.

Editing gradients and patterns afterward is no go either, because they are inside Arc, which is immutable. Yes, you can mutate it when it has a single owner, but we don't, because text clones it a billion times.

The only option left is too simply replace/recreate all gradients and patterns with objects units afterwards, which might work. We would still have to store units internally, but they would not be visible in the public API.

[LaurenzV]

The only option left is too simply replace/recreate all gradients and patterns with objects units afterwards, which might work. We would still have to store units internally, but they would not be visible in the public API.

Yes, this is the idea I originally had in mind. We basically split parsing in two stages, in the first stage we built the tree as usual (including preserving the object bounding box attribute in patterns) and in the second stage, we make another traversal through the tree where we replace stuff as needed.

This was also what I would have planned for context fill: When parsing the first time, if a shape has fill/stroke context we basically store this as well as the reference to the context object (should be possible using weak Rc I think? Although I've never used this). And in the meanwhile, we store some dummy fill there.

And then we do a second walk through the already parsed tree where we resolve the remaining parts that simply cannot be resolved in a first walk. When doing so, we basically walk through each node again and apply the necessary postprocessing steps for that specific node, if necessary. Yes, it's gonna clutter the structs a bit, but since they are private now, I don't think it's a big problem, since the only interface are the public methods now. It's an implementation detail.

If we had many edge cases like that I would say it's not worth it as it might make the internal code harder to reason about, but if I'm not wrong implementing objectBoundingBox and contextFill are the only really tricky features that would require this, so I think it should be manageable, as long as we document what's going on.

If you don't want to invest more time in this though, I can also give it a try. I'll probably be much slower than you since I don't have as much familiarity with the codebase, but I can still try! :D

[RazrFalcon]

Thanks, but I will try to finish it myself. This change is a low-level as it gets. Even I struggle with it.

The issue with mutli-pass parsing is that our "defs" are shared. But right now everything is read-only shared, because working with mutable shared state in Rust is pain (aka Rc<RefCell<T>> and Arc<Mutex<T>>). And by having mutably shared defs we would not be able to return just &T. Making the public API worse. It's one issue after another...

We either have to drop shared state completely, which is fine by resvg, but will bloat usvg output and potentially increase memory usage a bit. Or recreate objects entirely when needed, aka copy-on-write.

I haven't looked into contextFill yet, but the current goal is too make usvg more like a streaming parser, which is very hard with SVG.

[LaurenzV]

Okay! No worries, even if it doesn't end up working, it's already a lot simpler now.

[RazrFalcon]

Done. 🎉

It took a while, but it seems to be working just fine. At least from the rendering perspective. Writing is questionable. Will have to add a bunch of rendering and svg writing tests before releasing. And it made everything way easier. Specifically text_bbox is gone from the renderer, which was very confusing. It's handled automatically now.

This is one the biggest changes to usvg in a while, so a lot of testing is still required.

[LaurenzV]

Ob wow, I will test it in a second! :D

[LaurenzV]

Yup, my test cases seem to be passing as well!

I'm wondering, could we go even further and remove the viewBox attribute of patterns as well and merge it with the pattern transform? Afaik the only thing we need viewBox for is to then merge it with the transform, right?:

let mut transform = tiny_skia::Transform::from_scale(sx, sy);
if let Some(vbox) = pattern.view_box() {
    let ts = usvg::utils::view_box_to_transform(vbox.rect, vbox.aspect, rect.size());
    transform = transform.pre_concat(ts);
}

EDIT: Ah, damn, but then it might affect the calculation of sx and sy above... So not sure if this is possible.

[RazrFalcon]

Yes, patterns rendering is absurdly complicated in SVG. I will take a look if viewBox could be avoided.

[LaurenzV]

Don't worry though, I think what you've managed so far is already very incredible, thanks a lot again!