-
Notifications
You must be signed in to change notification settings - Fork 139
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 gradient support to SVG integration #357
Conversation
Co-authored-by: Sebastian J. Hamel <[email protected]>
integrations/vello_svg/src/lib.rs
Outdated
}) | ||
.collect(); | ||
let center: vello::kurbo::Point = gr.transform.apply(gr.cx, gr.cy).into(); | ||
// TODO: SVG has a scaleX and scaleY - But peniko::Gradient::new_radial only takes a radius. |
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.
Special attention to this - Maybe worth filing an issue for Vello to introduce radiusX and radiusY for radial gradients? SVG has rx and ry.
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.
Based on the SVG spec (https://developer.mozilla.org/en-US/docs/Web/SVG/Element/radialGradient#attributes), there are actually two centers, start and end, which are represented by (fx, fy) and (cx, cy) respectively. In peniko/vello, this type of gradient can be created with the Gradient::new_two_point_radial
constructor which does take two radii. Unfortunately, it doesn't look like usvg exposes the fr
attribute (the focal point, or start circle radius) so I'd just pass the r
attribute for both.
This would look something like Gradient::new_two_point_radial((gr.fx, gr.fy), gr.r, (gr.cx, gr.cy), gr.r)
.
For the transform, we can't apply it directly to the gradient properties because non-uniform scales or skews will change the geometry of the gradient. The vello rendering functions take an optional brush_transform
parameter to handle this. We probably want to change the paint_to_brush
function to return Option<(Brush, Option<Affine>)>
so that gradient transforms can be passed along.
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 for tackling this!
Our gradient API doesn't match SVG (maybe it should?) so mapping these properties is a bit tricky. I've added some comments inline that will hopefully clarify how this might be done.
integrations/vello_svg/src/lib.rs
Outdated
usvg::Paint::LinearGradient(_) => None, | ||
usvg::Paint::RadialGradient(_) => None, | ||
usvg::Paint::LinearGradient(gr) => { | ||
let stops: Vec<Color> = gr |
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'd suggest converting these to peniko::ColorStop
so we can also capture the offset
field. As is, this only works properly if the stops are evenly spaced.
integrations/vello_svg/src/lib.rs
Outdated
Some(Brush::Gradient(gradient)) | ||
} | ||
usvg::Paint::RadialGradient(gr) => { | ||
let stops: Vec<Color> = gr |
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.
Same as above :)
integrations/vello_svg/src/lib.rs
Outdated
}) | ||
.collect(); | ||
let center: vello::kurbo::Point = gr.transform.apply(gr.cx, gr.cy).into(); | ||
// TODO: SVG has a scaleX and scaleY - But peniko::Gradient::new_radial only takes a radius. |
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.
Based on the SVG spec (https://developer.mozilla.org/en-US/docs/Web/SVG/Element/radialGradient#attributes), there are actually two centers, start and end, which are represented by (fx, fy) and (cx, cy) respectively. In peniko/vello, this type of gradient can be created with the Gradient::new_two_point_radial
constructor which does take two radii. Unfortunately, it doesn't look like usvg exposes the fr
attribute (the focal point, or start circle radius) so I'd just pass the r
attribute for both.
This would look something like Gradient::new_two_point_radial((gr.fx, gr.fy), gr.r, (gr.cx, gr.cy), gr.r)
.
For the transform, we can't apply it directly to the gradient properties because non-uniform scales or skews will change the geometry of the gradient. The vello rendering functions take an optional brush_transform
parameter to handle this. We probably want to change the paint_to_brush
function to return Option<(Brush, Option<Affine>)>
so that gradient transforms can be passed along.
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.
Apologies for the delay. I’m currently out of the country on a work trip. Nice job catching the opacity issue. I noticed it but haven’t had time to review.
This PR looks good to me now. Thanks!
It’s part of the SVG spec and is mentioned in the Mozilla docs linked above. I’m not sure how common it is in practice but we implemented it to handle COLRv1 gradients which also require it. I suspect it’s not exposed in usvg because tinyskia isn’t capable of rendering full two point conical gradients. |
Adds radial and linear gradient support.