Skip to content
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

Merged
merged 11 commits into from
Dec 16, 2019
Merged

Svg and icon support #111

merged 11 commits into from
Dec 16, 2019

Conversation

Maldela
Copy link
Contributor

@Maldela Maldela commented Dec 6, 2019

This is the first step for proper icon support.
As you requested, I changed the resvg backend to raqoste. Unforunately this adds some code as the raqoste 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.

@Maldela
Copy link
Contributor Author

Maldela commented Dec 6, 2019

I opened an issue about the missing functionality.

@hecrj hecrj added the feature New feature or request label Dec 7, 2019
@hecrj hecrj added this to the 0.1.0 milestone Dec 7, 2019
Copy link
Member

@hecrj hecrj left a 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.

@Maldela
Copy link
Contributor Author

Maldela commented Dec 8, 2019

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.

@Maldela
Copy link
Contributor Author

Maldela commented Dec 11, 2019

I implemented a new Svg primitive that handles svg files exclusively. It shares a lot of code with the Image primitive. It uses resvg and raqote to create a texture with the size of the viewport and draws that texture like the Image primitive does.
I also implemented a theme icon loader and made it a global static with lazy_static. Let me know what you think about that.

Copy link
Member

@hecrj hecrj left a 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.

@Maldela
Copy link
Contributor Author

Maldela commented Dec 11, 2019

Icon is implemented in a way that it does create a Svg primitive, if its file path ends in .svg or .svgz. Otherwise it tries to create an Image primitive. I wanted the Icon widget to be able to handle svg and png files.
The Svg primitive is now fully viewport aware. The viewport size is handled in its draw and upload functions.
The icon loader being a lazy_static was quick and dirty way of getting it working. I can scrap that for now.

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;
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@hecrj hecrj Dec 11, 2019

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

pub struct Icon {
path: PathBuf,
size: Length,
}
Copy link
Member

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.

Copy link
Member

@hecrj hecrj Dec 11, 2019

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@hecrj hecrj Dec 11, 2019

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.

Copy link
Contributor Author

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.

Copy link
Member

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 { /* ... */ },
}

@Maldela Maldela requested a review from hecrj December 12, 2019 16:08
This reduces binary size when SVG supoprt is not needed.
Copy link
Member

@hecrj hecrj left a 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:

image

Let me know what you think!

This avoids trying to reload the file constantly on every frame.
@Maldela
Copy link
Contributor Author

Maldela commented Dec 15, 2019

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.
I think this can be merged.

@Maldela
Copy link
Contributor Author

Maldela commented Dec 15, 2019

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.

@hecrj
Copy link
Member

hecrj commented Dec 15, 2019

I would have liked the svg feature to be included in the default features but it displaying a placeholder text works too.

Yeah, I know. However, the resvg dependency was increasing around 1MB the binary size in release mode. I think it's fine behind a feature gate. It can be easily enabled if you need it.

It does not display a placeholder text when disabled, that happens because I added some conditional compilation to the svg example. If you try to use an Svg widget without the svg feature enabled, you will get a compiler error telling you that the wgpu renderer does not implement svg::Renderer.

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.

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 image::vector module. In any case, I think we should explore/tackle this in another PR.

@Maldela
Copy link
Contributor Author

Maldela commented Dec 15, 2019

In any case, I think we should explore/tackle this in another PR.

Agreed. This is fine for now.
We could also try to rasterize in another thread and display a blank texture or a texture that finished for another viewport size as long as the thread is running.

@hecrj
Copy link
Member

hecrj commented Dec 16, 2019

Thanks again for the patience and the work 🥂

Let's merge this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants