Add support for `context-fill` and `context-stroke` #665

LaurenzV · 2023-10-11T21:05:48Z

Here is the corresponding specification.

This is still a WIP, in particular, the following I still haven't looked into more deeply (as I'm still unfamiliar with the codebase) and I think might be a bit tricky to implement:

When the context paint layers include paint server references, then the coordinate space and the bounding box used to scale the paint server elements and content are those of the context element. In other words, any gradients and patterns referenced with these keywords should be continuous from the main shape to the markers, or from one element within a use-element shadow tree to another.

But I'm opening this as a draft PR so that people know that someone is working on it. 😄

I also had to temporarily switch to my own version of svgtypes since it's not included in this repository.

crates/usvg-parser/Cargo.toml

crates/usvg-parser/src/converter.rs

RazrFalcon · 2023-10-12T08:17:08Z

For marker tests, let's use triangle and not a line for the base shape. This way we would be able to test marker-mid as well.
And let's remove opacity one markers as well.

RazrFalcon · 2023-10-12T09:24:42Z

Looks ok overall. The feature itself is somewhat convoluted and niche. Even Chrome and Safari doesn't support it yet.

Nested use and marker as well as paint servers bbox preserving seems like the only tricky parts.
We should have tests for all of them. I can help with tests if needed.

LaurenzV · 2023-10-12T11:55:00Z

No worries, I can do the tests! Might need a bit help with figuring out how to properly deal with paint servers, but I'll have a look at it myself later / in the next few days.

LaurenzV · 2023-10-12T13:17:10Z

For marker tests, let's use triangle and not a line for the base shape. This way we would be able to test marker-mid as well.
And let's remove opacity one markers as well.

I used the star instead to make it more consistent with other tests, I hope that is okay.

RazrFalcon · 2023-10-12T13:45:07Z

Sure, it's fine.

LaurenzV · 2023-10-14T08:39:11Z

I looked a bit more into the source code, and I just had two (somewhat) related questions:

Is there a reason why we need to keep the transform attribute in usvg::Path? Because as far as I can tell, if a path element in SVG has a transform attribute, it will automatically be converted into a Group with a corresponding transform, so the transform of the path itself always stays the default. At least I just tried what happened when I remove it and apart from the fact that the writer might need to be changed a bit, all test cases seem to pass. So I guess this would be another nice simplification of usvg:Tree, if possible. But I'm not sure if I'm missing something.
This one might be a bit more difficult to implement, but currently, usvg:Tree distinguishes between the units ObjectBoundingBox and UserSpaceOnUse. But whenever there is ObjectBoundingBox it mainly seems to be about adding another transform to the transform chain to account for the bbox of the object. So I was wondering whether this also is something that could be simplified at the usvg:Tree level to make it even simpler? Or is there a good reason to preserve this semantic difference at that stage?

RazrFalcon · 2023-10-14T13:22:38Z

Is there a reason why we need to keep the transform attribute in usvg::Path?

Probably not. usvg was rewritten a billion times. Probably just a left-over. Let's leave to a separate patch.

Or is there a good reason to preserve this semantic difference at that stage?

Ugh... were do I even begin. I plan to write a whole chapter for Notes on SVG parsing about ObjectBoundingBox. Maybe one day.
In short - no, because of text. Text doesn't have bbox until we outline it, which we cannot do during parsing.

LaurenzV · 2023-10-14T16:27:47Z

Probably not. usvg was rewritten a billion times. Probably just a left-over. Let's leave to a separate patch.

Okay, I'll open a separate PR for this!

In short - no, because of text. Text doesn't have bbox until we outline it, which we cannot do during parsing.

Ah, that's unfortunate. :(

RazrFalcon · 2023-10-14T16:44:35Z

Ah, that's unfortunate.

It is... objectBoundingBox is one of the worst SVG features. It's nice on paper, but it's ridiculously hard to implement. Not only a text bbox is a very complicated thing in SVG (no, it's not an outlined path bbox), but even without text there are tons of edge cases.

I've tried multiple times to get rid of objectBoundingBox, but it's not possible.
Same with transforms. It's technically impossible to flatten transforms in SVG, but people keep asking me to implement this. Because they view SVG a list of paths, which is like 1% of the spec.

LaurenzV · 2023-10-16T21:00:38Z

So after doing some more digging into the source code, I'm honestly a bit clueless about how to implement support for properly resolved gradients/patterns without introducing too much "ugly" code. Two approaches I could think of are:

In the usvg:Tree, all we change is that for patterns and gradients, we store some kind of additional flag that indicates whether the paint should be resolved relative to the bbox and transform of the current path, or whether the context element should be chosen for that. This would reduce the complexity in the tree itself (since we are only adding a simple flag) but leave it upon resvg or other renderers to deal with how exactly to implement this.

However, in addition to that, we also could add another flag to groups to indicate whether this group is a context element. By doing this, the renderer can find the corresponding context element of a path simply by traversing its ancestors until it hits the first group that is a context element. Then, it simply needs to resolve the bbox and transform of that group and it can use this apply the appropriate bounding boxes and transforms to the paint.
Instead of doing the above, we try to somehow embed the bounding box and transform information directly into the paint. This way, we don't have to leave it to the renderer to do that. However, as far as I can tell while parsing we can't even really determine the bounding boxes, so I'm not even sure if this would work, let alone the fact that this would introduce a lot more complexity just to support a simple feature...

So yeah, I think for now I will attempt to implement the first approach and see how far I can get.

RazrFalcon · 2023-10-17T09:58:00Z

Ideally, usvg should do all of this. Like we already have text_bbox in Path. And don't forget this monstrosity. Therefore I would suggest doing something similar.

just to support a simple feature

Welcome to SVG, where everything is 10x more complicated than it should be.

The goal of usvg is to reduce SVG complexity. A renderer, like resvg, shouldn't know any of this.
If I would have time I will try to look into this issue to give a more concrete advice.

LaurenzV · 2023-10-17T10:32:37Z

No worries, I'll also give it some more thought!

LaurenzV · 2023-11-01T21:45:01Z

So I think implementing proper support for gradients/patterns will be really tricky and probably require some bigger changes. Would it be okay for you if we merge this now (so that it at least works properly for solid colors) and create an issue so that we don't forget about adding support for gradients/patterns as well later on? I think for now I will try working on some other "low-hanging fruits" (i.e. some other SVG2 features) before getting back to this.

But if not, I it's fine as well, but not sure when I will revisit this PR. I need to gain some more familiarity with the library by working on some other easier issues before proceeding with this.

It would also fix typst/typst#2359, which is the main reason why I made this PR.

RazrFalcon · 2023-11-02T10:16:28Z

I would be able to work on it maybe next month. I don't want to merge a half-backed solution. It's not very resvg-like.

As for typst - never, ever under any circumstances use SVG for anything. In SVG, you cannot even renderer paths reliably. The format is a joke.
But if you have to, try using only the most primitive SVG features possible. And markers are definitely not one of them. They are essentially unsupported by most libraries. You should not write them to begin with.
And obviously forget about SVG 2.

LaurenzV · 2023-11-02T21:53:27Z

I would be able to work on it maybe next month. I don't want to merge a half-backed solution. It's not very resvg-like.

I totally understand, let's leave it open then. :)

yisibl · 2024-03-05T03:30:19Z

I asked here if Chrome has any plans to support gradients.

LaurenzV · 2024-03-05T06:56:37Z

Ah, then we definitely should look into that... Sorry for the delay from my side, I will try to make some time for it in the next few days.

LaurenzV · 2024-03-05T14:53:38Z

Making progress! :D

LaurenzV · 2024-03-05T15:54:19Z

@RazrFalcon context in use seems to work fine now (Although I still need to clean up the code), but no idea how to deal with markers... Because for use node I could just go through all ancestors to find the corresponding context group, while for markers this doesn't work because markers aren't children of their corresponding paths.

RazrFalcon · 2024-03-05T18:01:33Z

Aren't marker context is the current path fill/stroke? And we already resolved it.

LaurenzV · 2024-03-05T20:00:53Z

I think I figured it out. :D Will clean up a bit and mark the PR as ready to review once I'm done.

LaurenzV · 2024-03-05T20:34:08Z

I hope it's okay!

crates/usvg/src/parser/converter.rs

RazrFalcon · 2024-03-05T20:59:17Z

crates/usvg/src/tree/mod.rs

 /// A fill style.
 #[derive(Clone, Debug)]
 pub struct Fill {
    pub(crate) paint: Paint,
    pub(crate) opacity: Opacity,
    pub(crate) rule: FillRule,
+    // Whether the current fill needs to be resolved relative
+    // to a context element.
+    pub(crate) context_element: Option<ContextElement>,


The only reason we have to store it is because of paint bbox resolver?
I think we can avoid it for markers, since we already know the bbox beforehand, unlike with text.
But I guess I will try figuring it out by myself, since it would be a bit tricky.

Yes, we mainly need it as a "marker" so that when iterating over all paint servers we know which strokes/fills need to be converted.

RazrFalcon · 2024-03-05T21:03:48Z

Paint resolver code looks a bit too complicated. Will see if I would be able to simplify it.
Otherwise, tests look good. And I assume the output matches the expected one (chrome fails to render correctly).
Not much else from me. Almost ready to merge.

LaurenzV · 2024-03-05T21:12:18Z

And I assume the output matches the expected one.

Yes, it does! All of them.

Paint resolver code looks a bit too complicated.

You mean convert_paint? Yes, the fact that we are returning a tuple here is kind of ugly. The only reason we need this is because we are storing the context element inside of the fill/stroke and not the paint. Since convert_paint only returns a paint, we need to preserve that information somehow so that we can insert it again once we construct the new fill/stroke. Maybe it's possible to store it inside the paint directly, not sure if this would require many changes.

I know the code isn't great. :( But it's also a tricky feature to implement...

RazrFalcon · 2024-03-05T21:28:08Z

Thanks again for looking into this! I will look if it can be simplified a bit and will publish a new release probably on weekends.

LaurenzV · 2024-03-05T21:28:52Z

Thanks, I'm glad this feature is implemented now!

LaurenzV mentioned this pull request Oct 11, 2023

Add context-fill and context-stroke RazrFalcon/svgtypes#17

Merged

RazrFalcon reviewed Oct 12, 2023

View reviewed changes

crates/usvg-parser/Cargo.toml Outdated Show resolved Hide resolved

RazrFalcon reviewed Oct 12, 2023

View reviewed changes

crates/usvg-parser/src/converter.rs Outdated Show resolved Hide resolved

RazrFalcon reviewed Oct 12, 2023

View reviewed changes

crates/usvg-parser/src/converter.rs Outdated Show resolved Hide resolved

LaurenzV and others added 10 commits October 18, 2023 15:19

Add warning for unparseable strokes

fc7f19f

patch svg types

b2d4e82

Support context-stroke and context-fill in marker

ee9d0a4

Add support in use

4155792

Undo accidental change

cdf46f4

Use git dependency

549b742

Update lock file

21df6df

Swap fill and stroke

aa534c8

Update test cases

3873028

Add ref images

123feaa

LaurenzV force-pushed the context branch from 27ee0e4 to 123feaa Compare October 18, 2023 13:19

LaurenzV added 2 commits March 5, 2024 10:14

Make some progress

2746ce8

Fix obj bounding box

eb64e7c

LaurenzV added 5 commits March 5, 2024 16:01

Updated test cases

1cb9e94

Update test

224e7c3

Fix tests

9ca38ba

Move context tests into different folder

2cac978

Rename tests and add text tests

98ce836

Try refactor

bcd3c86

LaurenzV added 2 commits March 5, 2024 21:02

Add support for markers

e38ac2e

Cleanup

632fb1f

LaurenzV marked this pull request as ready for review March 5, 2024 20:34

RazrFalcon reviewed Mar 5, 2024

View reviewed changes

crates/usvg/src/parser/converter.rs Outdated Show resolved Hide resolved

RazrFalcon reviewed Mar 5, 2024

View reviewed changes

Avoid clones

f4e444c

LaurenzV added 2 commits March 5, 2024 22:08

Remove is context element parameter

941435f

Update svg2 changelog

dc00dd2

RazrFalcon merged commit 25fd250 into RazrFalcon:master Mar 5, 2024
3 checks passed

LaurenzV deleted the context branch March 5, 2024 21:28

LaurenzV mentioned this pull request Mar 18, 2024

SVG arrowheads missing since 0.11 typst/typst#3715

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `context-fill` and `context-stroke` #665

Add support for `context-fill` and `context-stroke` #665

LaurenzV commented Oct 11, 2023 •

edited

RazrFalcon commented Oct 12, 2023

RazrFalcon commented Oct 12, 2023

LaurenzV commented Oct 12, 2023

LaurenzV commented Oct 12, 2023 •

edited

RazrFalcon commented Oct 12, 2023

LaurenzV commented Oct 14, 2023 •

edited

RazrFalcon commented Oct 14, 2023

LaurenzV commented Oct 14, 2023 •

edited

RazrFalcon commented Oct 14, 2023

LaurenzV commented Oct 16, 2023 •

edited

RazrFalcon commented Oct 17, 2023 •

edited

LaurenzV commented Oct 17, 2023

LaurenzV commented Nov 1, 2023 •

edited

RazrFalcon commented Nov 2, 2023

LaurenzV commented Nov 2, 2023

yisibl commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

LaurenzV commented Mar 5, 2024 •

edited

RazrFalcon commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

RazrFalcon Mar 5, 2024

LaurenzV Mar 5, 2024

RazrFalcon commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

RazrFalcon commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

Add support for context-fill and context-stroke #665

Add support for context-fill and context-stroke #665

Conversation

LaurenzV commented Oct 11, 2023 • edited

RazrFalcon commented Oct 12, 2023

RazrFalcon commented Oct 12, 2023

LaurenzV commented Oct 12, 2023

LaurenzV commented Oct 12, 2023 • edited

RazrFalcon commented Oct 12, 2023

LaurenzV commented Oct 14, 2023 • edited

RazrFalcon commented Oct 14, 2023

LaurenzV commented Oct 14, 2023 • edited

RazrFalcon commented Oct 14, 2023

LaurenzV commented Oct 16, 2023 • edited

RazrFalcon commented Oct 17, 2023 • edited

LaurenzV commented Oct 17, 2023

LaurenzV commented Nov 1, 2023 • edited

RazrFalcon commented Nov 2, 2023

LaurenzV commented Nov 2, 2023

yisibl commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

LaurenzV commented Mar 5, 2024 • edited

RazrFalcon commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

RazrFalcon Mar 5, 2024

Choose a reason for hiding this comment

LaurenzV Mar 5, 2024

Choose a reason for hiding this comment

RazrFalcon commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

RazrFalcon commented Mar 5, 2024

LaurenzV commented Mar 5, 2024

Add support for `context-fill` and `context-stroke` #665

Add support for `context-fill` and `context-stroke` #665

LaurenzV commented Oct 11, 2023 •

edited

LaurenzV commented Oct 12, 2023 •

edited

LaurenzV commented Oct 14, 2023 •

edited

LaurenzV commented Oct 14, 2023 •

edited

LaurenzV commented Oct 16, 2023 •

edited

RazrFalcon commented Oct 17, 2023 •

edited

LaurenzV commented Nov 1, 2023 •

edited

LaurenzV commented Mar 5, 2024 •

edited