-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make usvg::Tree
read-only
#710
Comments
@laurmaedje IIRC Typst also manually pushes fonts to the SVG tree, no? |
Typst starts out with an empty fontdb, traverses the tree and inspects the |
I see, so if the tree were to become read-only, we could probably still traverse it to find all fonts, but we can't define our own fallback fonts anymore for each text span. |
@laurmaedje this feature would be removed as well... The problem is that currently parsing and text-to-path conversion are two separate steps which leads to a lot of complexity and issues as well. For one, to do a lot of things with SVG we must know the layer bbox, which we don't know before text layout is finished. A chicken and egg problem. Will a callback approach work for you? The way we handle images resolving right now. usvg will call a callback for each font not present in fontdb and you could either add it to it or ignore it. I haven't tried implementing it yet, but that's the idea. In general, the issue here is that usvg is too flexible for me to maintain. Therefore I plan to remove all the features not used by resvg. Ideally, there should be only one way an SVG can be parsed. |
So the idea is that text is still preserved in the usvg tree, but you already need to supply a |
So for |
As far as font selection goes, this could work. What's also relevant to note here is how the family names are resolved. Typst and fontdb parse family metadata somewhat differently and currently we parse the SVG family names in the Typst way and then, after loading the font into fontdb, patch the name in the SVG to the fontdb variant. I'd like to bring up one other thing, too: A current problem with Typst's SVG stack is that text isn't selectable. In the current usvg design, I think it would be possible to layout the text with Typst's text stack in the common, simple case (no crazy SVG features on the text), which would make it selectable. Forcing text-to-path in the parsing stage would remove this capability as far as I can tell, which would be unfortunate. I really liked that usvg offered more control over the text in recent versions. Of course, I can't judge the complexity this forces upon usvg, but I wonder whether it's possible to have some middle ground where consumers can interact with the text beyond just paths. |
As far as I understood, text will still be preserved, but the used font will be decided on at parsing stage so that the bbox is known in advance? If I'm wrong and text would also be converted automatically into paths again, that would be very unfortunate indeed. :( |
These changes may be more impactful, so I also cc @zimond |
Yes. Ideally, all you have to do is too call Yes, it will simplify |
It's not possible, because we have to know all parent transforms. And you would not be able to create individual Nodes anyway. Just a tree. That's the problem. To properly parse SVG I need access to the whole tree, not just part of it. |
There are plans to preserve text layout as well. So a
The
This would not be possible anymore. And I'm not sure how can we handle this.
The complexity comes from the fact that the whole reason The fact that To put it bluntly, any modifications to |
Hm okay... This might mean that it might break stuff for me then. :/ But it's hard to say, I would have to see the concrete implementation. |
Why do you create filters yourself? To rasterize them? Also, I will sound like a broken record, but you should not be creating filters by yourself as well. |
Then, it should be fine I think.
I think I'm not alone in saying that it is very useful on its own and that's actually a great feature! I just remembered this message from you on reddit and I wholeheartedly agree with it. |
@laurmaedje |
I'm not creating filters myself. Basically, in general, I traverse the And it works really well, there are a few edge cases that don't work yet, but all other test cases pass successfully. |
@LaurenzV couldn't you simply render the node itself? |
EDIT: Nevermind, looks like the method on the group is public... Maybe I missed something, I will look into it later. 😅 |
Okay, I rewrote it using the node render method, and except for two edge cases that break, it works fine otherwise. So I think read-only tree would be fine in my case, as long as you can still create nodes manually and work with those! |
This wouldn't be allowed. That's the whole point. Why do you want this? |
Okay, nevermind, I guess I don't strictly need to be able to construct a image node by myself. But I can still at least read the attributes of an image node (such as I think for me it's just a bit difficult to visualize what the exact API you had in mind would look like, so I guess it's just best to try it out once it's ready. So feel free to write here once it happens and I'll try it out (although I presume that this will probably a long time). :p |
No worries, I will make a pull request soon. |
Created #712 to illustrate the idea. |
That was fast. 😄 Will experiment with this. |
Okay, I guess I understand now. so basically make all fields private (as they are implementation details) and expose everything necessary as methods. It does break two things for me where I manually edited the Node, which should be fixable from my side. The thing with rendering filters seems trickier, I'll have to spend some more time trying it out. Not sure how easy it is going to be. But I guess in general, the change is fine for me. |
We can add necessary tweaks if needed, but the idea of making struct fields private will probably here to stay. It just makes everything way easier. |
Out of context question but can/should the My idea the other day was to like have a flat custom usvg like tree to apply changes from the Bevy ECS too and then construct the real I know my stack is kind off weird and I’m learning by doing, so I’m open for feedback & ideas & roasts :)
Thanks 🙏 https://github.com/dyndotart/monorepo/tree/develop/crates/svg_render/src/resources/svg_composition |
@bennoinbeta If I understood you correctly, then no. The only |
ok thanks for the quick reply. Makes sense, so I'll stick with my current approach where I construct the SVG and then render it via resvg & tiny-skia when required instead of directly using the Just out of curiosity: Is the But never the less resvg & usvg & tiny-skia & fontdb & rustybuzz are really helpful libraries 🙏 Thanks :) |
|
@RazrFalcon I tried porting the newest main and I can manage to get nearly everything to work (and parts of the code got much simpler!), except for one edge case: I'm using the following method to render groups with filters into a pixmap: Lines 75 to 98 in 96eec97
I can get it to work for all SVGs with filters except for the ones where the group doesn't have a bounding box, such as for example: <svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
<title>On an empty group (1)</title>
<filter id="filter1" filterUnits="userSpaceOnUse" x="20" y="20" width="160" height="160">
<feFlood flood-color="seagreen"/>
</filter>
<g id="g1" filter="url(#filter1)"/>
<!-- image frame -->
<rect id="frame" x="1" y="1" width="198" height="198" fill="none" stroke="black"/>
</svg> Becaues the call to |
Thanks for the testing! Yes, a lot of things become way more straight-forward. As for |
|
Yup now all tests pass, thanks a lot!! |
Just as another piece of feedback on this change, it negatively impacts an implementation I have. The two use cases we have are:
I understand this use cases were not intentionally supported, but this change will either require us to:
I appreciate the intention of this change. It does make the API much easier to use. I'm not sure if you have recommendations on how we might support these use cases given the vision you have with this library. |
The problem is quite simple. To render SVG fast I need to know all nodes bboxes. And it's ridiculously hard to do. Not to mention that In the end, if you find PS: if you care about performance, you shouldn't be using SVG to begin with. It's horrible, horrible file format. |
I did attempt to port Typst to usvg 40.0 in the meantime. Because fonts may be loaded lazily from the network, I can't provide a populated fontdb. What I tried is parsing the SVG without fonts and traversing it to find all text nodes and then loading the specified fonts plus any fonts that may be required for font fallback. Then, I call has_text_nodes and if that's the case reparse with a populated fontdb. The unfortunate problem is that usvg does not generate any Alternatively, as discussed above, it'd be great if there was an API for on-demand font loading. Basically just getting access to the text properties and the ability to populate the fontdb. Ideally, this would also provide access to the text itself so that callers can provide extra fonts that usvg's font fallback can make use of if the primary family isn't available. |
@laurmaedje This can be done only via some sort of callback. We can add one to On the other hand, if we want to add embedded fonts support in the future, we need a mutable fontdb anyway, external or internal. As always, there is no such thing as "simple" in SVG... Will see what can be done here. We might end up with internally cloning the
This is by design. If a node doesn't have a valid bbox - it should be removed. |
@RazrFalcon What concrete design did you have in mind for the text callback? I think one where the callback is called with a mutable fontdb would work well enough, but one where the callee has control over the font selection itself would be even more powerful and flexible. Either way, it would be great to get the ball rolling here, so that maybe this could be part of the next release. Right now, Typst is still on usvg 0.38 because otherwise it can't display any text. If there's anything I can help with to move this forward, let me know. |
As I've mentioned above, Also, I'm not really sure what exactly do you need. Do you need a callback that would be called for each font or for all fonts at once? What do you plan to do in this callback? Simply populate the fontdb with your fonts or explicitly return the fn font_callback(font: &FontStyle, db: &mut fontdb::Database) -> Option<fontdb::ID> {} ? I haven't looked much into it myself. I will try looking into it on the weekend. |
Either one is fine for me.
Find a suitable font, load it via Typst's lazy font loading mechanism and then provide it to usvg.
The latter version, where we get a
Thank you! |
So you do not plan to modify the What should happen when the callback returns |
I do need some way to provide the font data to usvg. So the fontdb would need to be modified. Basically, I can't construct the fontdb upfront because I only want to lazily load the font once I know that usvg will need it.
Ideally, we could also influence the selection of the fallback font. There could be a second callback that can request a fallback font for a specific text run that we could serve in the same way. Or it's all one callback and the callback also provides the text itself so that we can do the fallback in there. Then, |
Right now,
usvg::Tree
can be modified in pretty much any way and can be even constructed from scratch. It might have some benefits, but also introduces a lot of issues:resvg
basically assumes thatusvg::Tree
was parsed from SVG. There are no guarantee it would work with a manually created tree. By having a mutable tree the user assumes that everything can be changed, but in reality almost nothing can be safely changed.id
values, which we need during writing. Or even introduce recursive references - we cannot check for that either.@LaurenzV @yisibl I hope this change would not break your use case. Or anyone else.
The text was updated successfully, but these errors were encountered: