-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conversation
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.
assert!(glyph.has_overlapping_contours()) | ||
} else { | ||
assert!(!glyph.has_overlapping_contours()) | ||
} |
There was a problem hiding this comment.
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())
?
There was a problem hiding this comment.
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())
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); | ||
} |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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" :) |
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.