-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add SVG support via resvg #40
Conversation
Nice, still great to have this! Awesome work 👏 |
Co-authored-by: Kleis Auke Wolthuizen <github@kleisauke.nl>
Just a heads up - might be better to merge kleisauke/libvips#2 before this one to reduce complexity of patches in this PR. |
Commit 54bce44 fixes the remaining tests. I just opened PR libvips/libvips#3307 for that. |
I just opened draft PR rust-lang/rust#107221 and rust-lang/libc#3087 for this. |
Nice! Out of curiosity, why do you create those PRs as drafts? They look like should be ready for review straight away? |
The Rust PR depends on the libc PR, so the libc crate needs to be updated first in Rust (i.e. a similar commit as rust-lang/rust@e294640 is needed). The libc PR was marked as draft because it does not meet this check:
But perhaps I could add a new |
I don't think that's necessary a criteria for draft, it just will be part of review comments? Idk, I just almost never use the draft PR feature so was curious about your usage / rationale. |
Ah, sorry I misread your "review straight away" as "merge straight away". I usually open pull requests as a draft if I think it's not ready to merge yet, these PRs can be reviewed but not merged as-is (because it is dependent on another PR or still being worked on). Perhaps I should treat draft PRs as non-reviewable rather than non-mergeable? If so, the draft status of these PRs can be removed. |
Ah that probably is our mismatch - that's how I understood them since they were introduced, yeah, sort of "work in progress, please don't review yet". After all, the button on the draft PRs is called "Ready for review". |
From #40 (comment)
I just wanted to mention that I need this because I want to convert my website's icon into web app manifest icons. The website icon is SVG because that is the best format, but SVG is not supported on all platforms for the manifest, so I need to convert it to PNG. |
Remove `?disableModules` in favor of `?modules=`, for example: `?modules=jxl,heif,svg` or `?modules=`, etc.
It seems that a release of libc with that PR included is taking longer than expected. I'll merge this as-is after the CI passes (the Rust nightly version is pinned, so I don't expect any build failures). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This will be in v0.0.5.
This is still missing text rendering because dealing with fonts is its own problem, but otherwise works well and is enough to pass Sharp tests.
Wanted to submit anyway so that my branch / work isn't lost, but leaving disabled by default because it's both incomplete w/o text, but also because it's a relatively rarely useful feature. More common problem is optimising SVG to SVG, for which people use tools like SVGO, while in libvips SVG can only be rasterised and treated like other raster images.