-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Svg and icon support #111
Svg and icon support #111
Conversation
I opened an issue about the missing functionality. |
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.
Thanks! I think this is a great start.
The main limitation I see currently is that the rasterization is not aware of DPI scaling nor the actual size of the viewport (the widget size). The great thing about SVGs is that they can be scaled without losing quality.
This makes me think that, instead of trying to reuse the current Image
primitive, we may need a new Primitive
in iced_wgpu
to deal with this and cache it properly.
I'll keep thinking about it.
Yes there are definitely limitations in the current implementation. I'll look into creating a new primitive and maybe use gpu rendering for that. Could take me a while, though. |
I implemented a new |
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.
Hey, thanks for all the effort here!
I think we should keep this PR focused on the SVG primitive. We are not ready to introduce anything related to themes yet (I am not sure if we should even have first-class themes). Anything lazy_static
is also completely out of my comfort zone.
We may be able to use the image
pipeline for the new Svg
primitive, as all it changes is the caching strategy. I would also rename Icon
to Svg
to keep things unambiguous. Is the rasterization aware of the widget size now? I can't find the logic handling that.
Icon is implemented in a way that it does create a |
wgpu/src/svg.rs
Outdated
let mut opt = resvg::Options::default(); | ||
opt.usvg.path = Some(handle.path.clone()); | ||
opt.usvg.dpi = handle.dpi as f64; | ||
opt.usvg.font_size = handle.font_size as f64; |
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.
Could we use DrawTarget::set_transform
to set the DPI scaling in upload
? Everything should scale nicely and we will avoid the weird 96.0
and 20.0
magic constants.
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.
Those constants are for usvg which takes them to create the node tree. I'm not entirely sure how they affect the image creation but I think they influence how <text>
tags in the svg file are processed.
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.
The idea is to leave everything in default settings and simply apply a uniform scaling to the DrawTarget
.
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.
Usvg does not use the dpi setting to create bigger images. It's just for font rendering inside the icon.
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.
Right, I don't think we should alter that setting. Applying a transformation to the DrawTarget
should be enough.
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.
Sure, I can change it to keep the standard settings. I think this is a corner case if one at all anyway.
native/src/widget/icon.rs
Outdated
pub struct Icon { | ||
path: PathBuf, | ||
size: Length, | ||
} |
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.
I would name this widget Svg
and let it be configurable both in width and height.
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.
After thinking a bit about it, it may make sense to merge this with the Image
widget. We will probably want to keep the SVG aspect ratio after all.
We can check the extension as you were doing and have an additional Memory
variant in image
to deal with caching aware of DPI and viewport dimensions.
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.
As I said, that widget can handle svg as well as png or other files since some icons still come in png format. So I think naming it Svg
would be rather misleading about its capabilities.
I also thought that since icons are usually squares, giving it width and height would be unnecessary and a single size attribute simpler.
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.
I was thinking about the image widget too. I would like to give the user more control about aspect ratio of the widget. Maybe an enum with KeepAspectRatio
, Stretch
and KeepAspectRatioCrop
at least. But icons don't need that.
Also having a separate widget makes integrating themed icons easier in the future.
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.
What an icon is depends on each particular application.
To keep things simple, I was proposing that we should instead expose a widget that is able to render only SVG with configurable dimensions. One that can be used to render SVG directly, not just icons.
This way, you could easily create a function icon
in your application that chooses Svg
or an Image
widget based on the path, sets square dimensions, and returns an Element
.
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.
Now the Icon
widget does all that for you. You don't have to create an icon
function yourself.
It's very easy to use.
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.
Convenient, but not simple.
If our renderer supports SVG, we need a widget that can leverage it to its full extent, not just squares.
For instance, I may want to draw a logo using SVG. I won't be able to use Icon
for that unless the logo is a square.
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.
I could easily merge the svg capability into Image
. If simplicity is the main goal here, I could try to merge the two pipelines too. That shouldn't be too hard.
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.
I am not completely sure about it.
I like the idea of having a dedicated Svg
widget. It makes everything more explicit and forces users to keep in mind Svg
and Image
have different performance implications. It may also help us in the long run to feature-gate some particular widgets and reduce binary size. I'd say let's keep it as different widgets for now.
However, I believe we can reuse the image
pipeline by adding a new variant to the Memory
enum to handle the Svg
primitive. This should reduce the scope of the changes too.
For instance:
enum Memory {
Host(HostMemory),
Device {
bind_group: Rc<wgpu::BindGroup>,
width: u32,
height: u32,
},
NotFound,
Invalid,
}
enum HostMemory {
Image(image::ImageBuffer<image::Bgra<u8>, Vec<u8>>),
Svg { /* ... */ },
}
The aspect ratio is now preserved like in the `Image` widget.
This reduces binary size when SVG supoprt is not needed.
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.
Thanks again for the efforts here!
The changes looked great. However, I played with it and noticed that we were not rerasterizing on resize. I have implemented this feature and refactored things a bit.
Specifically, I have split the previous image::Cache
in two: one for raster images and another for vector images. This seems necessary as the caching strategies differ. Although this causes a bit of code duplication, it's mostly wgpu
boilerplate.
Additionally, I have feature-gated the SVG rendering (and the resvg
dependency) under a new svg
feature.
And finally, I have added an svg
example that renders the classic tiger:
Let me know what you think!
This avoids trying to reload the file constantly on every frame.
Good catch with the missing rerasterization. I would have liked the svg feature to be included in the default features but it displaying a placeholder text works too. |
Regarding the rasterization being slow on resize: Maybe we could rasterize the svg to a slightly too large viewport and downscale. Then only rerasterize, if the resized svg becomes larger than the viewport. |
Yeah, I know. However, the It does not display a placeholder text when disabled, that happens because I added some conditional compilation to the
It may be worth a try. Downscaling can make things look wrong though, so we should keep that in mind. The cool thing about the current design is that any change in the caching strategy should only affect the |
Agreed. This is fine for now. |
Thanks again for the patience and the work 🥂 Let's merge this! |
This is the first step for proper icon support.
As you requested, I changed the
resvg
backend toraqoste
. Unforunately this adds some code as theraqoste
crate does not have a function to write a png encoded image to memory. (See the last commit)I'll open a feature request about that with them.