Add kurbo conversions to scene API#171
Conversation
Adds kurbo as an optional dependency and implements conversions to/from the common types. This also removes the direct pinot dependency and changes moscato (temporarily) to a git based dep to allow iteration on the underlying glyph loading code without PR churn here.
Reassigns value 13 to PlusLighter to match the upcoming blend fixes
* Add impl Into<Affine> for pushing transforms. * Small QOL API changes to Scene and Fragment. * Add some missing docs.
This matches the changes in the fix_blends PR.
raphlinus
left a comment
raphlinus
left a comment
There was a problem hiding this comment.
I have these comments in my queue, I think it makes sense for you to see them even though I'm not fully settled on some of the decisions.
piet-scene/src/lib.rs
Outdated
| use super::geometry::{Affine, Point, Rect}; | ||
| use super::path::Element; | ||
|
|
||
| impl From<kurbo::Point> for Point { |
There was a problem hiding this comment.
This is ergonomic but doesn't exactly follow Rust conventions - usually From is for lossless conversions, so there's f32->f64 but not the other way around. There was a proposal for a standard lossy conversion trait but I'm not sure it went anywhere. I think this is good enough for now but want to check with a Rust expert to see if there is a better pattern. In any case, we can change it later.
There was a problem hiding this comment.
Good point. If you'd feel better with explicit to_kurbo/from_kurbo methods, I actually don't mind going that direction. I found that impl Into in the builder interface isn't all that useful anyway due to these types being mostly buried in options and iterators.
There was a problem hiding this comment.
The ->kurbo direction is fine. If "into" is not actually a big ergonomic win in practice then I think having an explicit method is a better choice.
There was a problem hiding this comment.
Removed the From impls and replaced them with from_kurbo methods for the lossy conversions.
piet-scene/src/scene/mod.rs
Outdated
| impl Scene { | ||
| /// Creates a new builder for filling the scene. Any current content in | ||
| /// the scene is cleared. | ||
| pub fn build<'a>(&'a mut self, rcx: &'a mut ResourceContext) -> Builder<'a> { |
There was a problem hiding this comment.
I think this name is confusing, usually "build" is at the end of a fluent-style chain, converting a builder type into a (usually non-mutable) instance of what's being build. Perhaps "start_build"? It's also possible I don't really understand the logic here.
There was a problem hiding this comment.
Yes, this name is poorly chosen. start_build could work, or maybe new_builder. I think I'll just remove these and keep the original free functions for now.
There was a problem hiding this comment.
These were redundant anyway so they've been removed. Easiest way to solve a naming issue :)
Moves the more descriptive comments to the free functions.
raphlinus
left a comment
raphlinus
left a comment
There was a problem hiding this comment.
Ok this looks good now, thanks!
2 participants
This is a small follow up PR to #168 that adds an optional dependency on kurbo to the scene crate along with the associated two-way conversions between common types when the feature is enabled.
There is a question of which version of kurbo we want to depend on. This currently uses 0.8.3 which is the latest available on crates.io while piet-gpu inherits version 0.7.0 from its older piet dependency.