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

More extensive to_kurbo #132

Draft
wants to merge 10 commits into
base: fix-eq-lints
Choose a base branch
from
Draft

More extensive to_kurbo #132

wants to merge 10 commits into from

Conversation

madig
Copy link
Collaborator

@madig madig commented May 12, 2021

Based on https://github.com/linebender/norad/blob/8c9038b/examples/letterspacer.rs#L523-L680, which is based on the fontTools' point pen to pen adapter pen.

This handles some more corner cases like e.g. converting successive curves without off-curves to lines, and all-off-curve paths.

I'll hold off of implementing this on Component and Glyph until I have a use-case.

@madig madig marked this pull request as draft May 12, 2021 22:03
@madig madig changed the title More extensive Contour::to_kurbo More extensive to_kurbo May 12, 2021
@madig
Copy link
Collaborator Author

madig commented May 12, 2021

The ErrorKinds used here are really UFO specification violations. Maybe they can get their own enum later.

@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing 477c428 against 3d502fe

target old size new size difference
target/release/examples/open_ufo 1.71 MB 1.71 MB ---
target/debug/examples/open_ufo 7.14 MB 7.14 MB ---

@linebender linebender deleted a comment from github-actions bot May 12, 2021
src/glyph/mod.rs Outdated
_ => return Err(Error::ConvertContour(ErrorKind::TooManyOffCurves)),
};
offs.clear();
pub fn to_kurbo(&self) -> Result<Vec<kurbo::PathEl>, Error> {
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 have many of the same comments on this patch as I had on #116, it might be worth reading through those?

@madig madig force-pushed the more-extensive-to_kurbo branch 4 times, most recently from 6ae4065 to e86a178 Compare September 22, 2022 08:18
@madig
Copy link
Collaborator Author

madig commented Sep 22, 2022

@cmyr i got the following, converting a QCurve with some off-curves:

let c1 = Contour::new(
    vec![
        ContourPoint::new(0.0, 0.0, PointType::OffCurve, false, None, None, None),
        ContourPoint::new(2.0, 2.0, PointType::OffCurve, false, None, None, None),
        ContourPoint::new(4.0, 4.0, PointType::OffCurve, false, None, None, None),
        ContourPoint::new(100.0, 100.0, PointType::QCurve, false, None, None, None),
    ],
    None,
    None,
);

assert_eq!(
    c1.to_kurbo().unwrap(),
    vec![
        kurbo::PathEl::MoveTo((100.0, 100.0).into()),
        kurbo::PathEl::QuadTo((0.0, 0.0).into(), (1.0, 1.0).into(),),
        kurbo::PathEl::QuadTo((2.0, 2.0).into(), (3.0, 3.0).into(),),
        kurbo::PathEl::QuadTo((4.0, 4.0).into(), (100.0, 100.0).into(),),
        kurbo::PathEl::ClosePath,
    ]
);

Does the PathEl::MoveTo((100.0, 100.0)) as the first element strike you as right or should it be (0.0, 0.0)? I copied this behavior from fontTools.

@madig madig changed the base branch from master to fix-eq-lints September 22, 2022 10:49
@anthrotype
Copy link
Collaborator

what's the status of this PR? Are you planning to finish this at some point?

@madig
Copy link
Collaborator Author

madig commented Dec 8, 2022

One day. I think I needed to validate that the decomposer actually does what kurbo expects, i.e. is the code right to put the MoveTo(100.0, 100.0) before the QuadTo((0.0, 0.0), (1.0, 1.0)) in https://github.com/linebender/norad/pull/132/files#diff-19795d9bd46090f88223ed97fc7eecb8758e48ce418c4b2dbed4f57bf7814c15R919-R923 or is that just how fontTools stuff does it?

@anthrotype
Copy link
Collaborator

I think that's correct, c1 is a closed path, so the last on-curve point (100,100) becomes the first MoveTo

@anthrotype
Copy link
Collaborator

(also whatever fonttools does is as normative (if not more) in UFO world as the ufo spec itself)

@madig
Copy link
Collaborator Author

madig commented Dec 8, 2022

Yes, but I don't know if kurbo wants something different. @cmyr?

@anthrotype
Copy link
Collaborator

why would kurbo ever want anything different? those conventions for converting between GLIF points to segment-oriented APIs like fonttools pens are well in place since ages and work, I don't see why diverging from them now

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