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

Capture overlap bits from TrueType glyphs #582

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Capture overlap bits from TrueType glyphs #582

merged 2 commits into from
Aug 24, 2023

Conversation

dfrg
Copy link
Member

@dfrg dfrg commented Aug 23, 2023

First step in addressing #581. Captures simple and composite overlap bits and plumbs them through internally.

Still needs to be exposed in skrifa public API. This will be done in a follow up.

First step in addressing #581. Captures simple and composite overlap bits and plumbs them through internally.

Still needs to be exposed in skrifa public API. This will be done in a follow up.
dfrg added a commit that referenced this pull request Aug 23, 2023
Changes `Scaler::outline` to return a new `ScalerMetrics` struct that contains the overlap flag. This also has optional fields to represent metrics that may be adjusted by hinting.

Based on #582 which must be merged first.
Fixes #581
assert!(glyph.has_overlapping_contours())
} else {
assert!(!glyph.has_overlapping_contours())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like the verbose version of assert_eq!(gid == 3, glyph.has_overlapping_contours()) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. Actually even then it's a bit more vulnerable to broken test data than I like; I propose we directly assert that only gid 3 has the bit:

assert_eq!(vec![3], (0..glyph_count).filter(...retain only if has overlaps).collect())

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this style as well. Fixed!

}

#[test]
fn composite_overlapping_contour_flag() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The loading of test data, num glyphs, etc is repetitive noise; perhaps a test helper that lets you easily iterate over (gid, &Glyph) would help tidy?

Copy link
Member Author

Choose a reason for hiding this comment

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

The refactoring part of my brain seems to turn off when writing tests. Good call, ty!

Ok(Some(Glyph::Composite(glyph))) => glyph,
_ => continue,
};
// Only GID 2, component 1 has the overlap bit set
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above I might prefer it as something like assert_eq!(vec![(2, 1)], ...collect (gid, component index) of things with overlap compound)

// GID 2 is a composite glyph with the overlap bit on a component
// GID 3 is a simple glyph with the overlap bit on the first flag
assert_eq!(glyph.has_overlaps, gid == 2 || gid == 3);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, I'd prefer assert_eq!(vec[2, 3], ...collect gids with overlap...)

Copy link
Collaborator

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

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

LGTM

@dfrg
Copy link
Member Author

dfrg commented Aug 24, 2023

Oops, I pushed all the fixes requested here to the branch for #583. Since both PRs are approved and related, I'll consider these "fixed in a followon" :)

@dfrg dfrg merged commit 590b3fc into main Aug 24, 2023
9 checks passed
@dfrg dfrg deleted the simple-overlap branch August 24, 2023 06:11
dfrg added a commit that referenced this pull request Aug 24, 2023
Changes `Scaler::outline` to return a new `ScalerMetrics` struct that contains the overlap flag. This also has optional fields to represent metrics that may be adjusted by hinting.

Based on #582 which must be merged first.
Fixes #581
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.

2 participants