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

Implement to_kurbo on Contour #116

Merged
merged 10 commits into from
May 12, 2021
Merged

Implement to_kurbo on Contour #116

merged 10 commits into from
May 12, 2021

Conversation

simoncozens
Copy link
Contributor

This adds a to_kurbo method on a Contour, as discussed in #110.

Most of this has been tested in fonttools-rs, apart from the implied on-curve stuff because I can't find any UFOs that do that.

@simoncozens simoncozens marked this pull request as draft May 9, 2021 20:52
@simoncozens simoncozens marked this pull request as ready for review May 9, 2021 21:22
@madig
Copy link
Collaborator

madig commented May 10, 2021

I also wrote something: https://github.com/linebender/norad/blob/8c9038b/examples/letterspacer.rs#L523-L680. Assembles PathEls instead of calling drawing methods.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I think my coffee just kicked in, so there's way more feedback here than is realistically necessary. Feel free to ignore most of it at your discretion; I thought it might be helpful to see how I would think through some of these problems in a slightly more idiomatic way. :)

I'll give you a chance to respond, but I'm happy to merge this more-or-less as is.

src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved

/// Converts the contour to a kurbo::BezPath
#[cfg(feature = "kurbo")]
pub fn to_kurbo(&self) -> Result<kurbo::BezPath, ErrorKind> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be fixed here, but I want to discourage using ErrorKind as an actual error type; it's intended more to indicate the cause of some other error type, such as a parsing error or a validation error. I believe we do use it like this elsewhere in the code, but I'd like to rectify that at some point.

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'm going to leave this part for now, because I don't think there's a better error type right now - things which happen when using the curve are not exactly parsing, writing, or spec errors - and I think I'll leave you to design the error hierarchy you want to see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that's fine!

@simoncozens
Copy link
Contributor Author

This is fantastic feedback, and will really help me think about idiomatic Rust. :-) Thank you! I shall work on addressing the issues above.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for revisiting this @simoncozens. I'm going to make a couple of doc tweaks and then merge, and I'll look into the error types before releasing 0.4.0.

Another high-level idiomatic-rust thought, here: I think the ideal implementation of this method would return an impl Iterator<Item=kurbo::PathEl>, instead of a kurbo::BezPath. A BezPath is just a Vec<PathEl> under the hood, and by having this return an iterator, you could do fun things like take the elements and pass them directly to some drawing code, or accumulate multiple contours into a single BezPath, all without needing any intermediate allocation.

In order to fully avoid allocation, the other thing we would need to get rid of is the VecDeque. For this I would probably add some custom helper type, like:

enum OffCurves {
   None,
   One(Point),
   Two(Point, Point),
}

impl OffCurves {
    fn points(&self) -> &[Point];
    fn push(&mut self, point: Point) -> Result<(), ErrorKind>;
    fn clear(&mut self);
}

I may go ahead and play around with this after merging, because I think it would be fun 😬

src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
src/glyph/mod.rs Outdated Show resolved Hide resolved
@cmyr cmyr merged commit 1c14735 into linebender:master May 12, 2021
@cmyr cmyr mentioned this pull request May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants