feat: update vello_svg to usvg 0.37, fix some artifacts. #422
Conversation
simbleau
commented
Jan 19, 2024
•
simbleau
commented
- Ignores DS_Store
- Updates to usvg 0.37
- Update the docs on integrations
integrations/vello_svg/src/lib.rs
Outdated
| gr.transform.e, | ||
| gr.transform.f, | ||
| ]); | ||
| let start: vello::kurbo::Point = (gr.x1 as f64, gr.y1 as f64).into(); |
There was a problem hiding this comment.
I think I'd use Point::new or Point::from here
There was a problem hiding this comment.
Ah, I see you're copying the extant style
In that case, I guess either is fine
There was a problem hiding this comment.
I also think it should be a common convention across the file. I'll try to make it consistent when I get to my next revision.
integrations/vello_svg/src/lib.rs
Outdated
| /// Append a [`usvg::Tree`] into a Vello [`SceneBuilder`], with default error handling | ||
| /// This will draw a red box over (some) unsupported elements | ||
| /// Append a [`usvg::Tree`] into a Vello [`SceneBuilder`], with default error | ||
| /// handling This will draw a red box over (some) unsupported elements |
There was a problem hiding this comment.
This is an odd change.
I must have failed to put two spaces after handling, previously. I'd propose at least putting a full stop between the two
There was a problem hiding this comment.
Will be fixed. It was a bad auto-format.
integrations/vello_svg/src/lib.rs
Outdated
| tx, | ||
| ty, | ||
| } = elt.abs_transform(); | ||
| let (a, b, c, d, e, f) = ( |
There was a problem hiding this comment.
I'd be tempted to make a single array, then map it through f64::from and pass that straight to the constructor
There was a problem hiding this comment.
Sorry, what? Like this?
let x: [f64; 6] = [sx, ky, ...].into();There was a problem hiding this comment.
Yeah, so:
let arr = [...].map(f64::from);
let transform = Affine::new(arr);(Where ... is the components in the right order)
integrations/vello_svg/src/lib.rs
Outdated
| render_tree_with(sb, svg, default_error_handler).unwrap_or_else(|e| match e {}); | ||
| /// See the [module level documentation](crate#unsupported-features) for a list | ||
| /// of some unsupported svg features | ||
| pub fn render_tree(sb: &mut SceneBuilder, svg: &usvg::Tree, transform: Option<Affine>) { |
There was a problem hiding this comment.
Why is this transform needed? You can append a fragment created using this to a scene with an applied transform
There was a problem hiding this comment.
Hm, I think once upon a time I needed this to pass an Affine, like this:
However, it seems like I don't need this anymore, you're right! I'll get this fixed.
| }; | ||
|
|
||
| /// Re-export vello. | ||
| pub use vello; |
There was a problem hiding this comment.
Hmm, I'm less sure if we should do this, although I can't put my finger on why
There was a problem hiding this comment.
I think this is reasonable - vello-svg is a separate crate the way it's currently laid out. It seems weird to expose usvg but not vello.
If your only intention is to render a list of SVG images and save them with vello-svg, you're out of luck, since there is no re-export of vello. You would need to ensure you also pull in vello, which means maintaining more dependency versions which can get mismatched.
|
@simbleau just want to check on the status of this - I think we're waiting on your changes |
Acknowledged. Will try to prioritize this today or tomorrow. |
|
@DJMcNab Ready to go. |
