-
Notifications
You must be signed in to change notification settings - Fork 14
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
make sure masters for global/glyph/layout metrics are rounded before computing deltas #1082
Conversation
to be able to use impl OtRound<f64> for <f64> googlefonts/fontations#1209
…computing deltas Fixes #1043
// a VF at the masters' location is expected to be equivalent to building | ||
// individual masters as static fonts. fontmake does the same, see | ||
// https://github.com/googlefonts/fontc/issues/1043 | ||
let value: f32 = value.into_inner().ot_round(); |
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.
should we ot_round to f64 and avoid f32 then f64? - I recall we need to add that to otround, maybe we should do that first?
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 already added OtRound<f64> for f64
and OtRound<f32> for f32
here:
googlefonts/fontations#1209 (unreleased yet, which is why this PR is fetching fontations from github)
Are you saying we should also add OtRound<f64> for f32
? Then why not any other combinations of primitive number types? It feels to me a bit out of scope for a rounding trait that we are casting f32 as f64 because of some other API which works best with those.
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 if tested. Seems like there should be inputs with observably different results?
yes, I mentioned Teachers-Italic.glyphs in #1043 (comment), which contains a composite with a scaled component that gets its anchors propagated as float coordinates and produces different deltas when these get rounded beforehand. I can add a subset of that one the fontc/lib.rs integration tests. |
the deltas for Teachers-Italic.glyphs' ordfeminine 'top' anchor are off-by-one because the anchor's master values are un-rounded floats. #1082 will fix this and similar others.
I made a test in a separate branch to show it failing on |
the deltas for Teachers-Italic.glyphs' ordfeminine 'top' anchor are off-by-one because the anchor's master values are un-rounded floats. #1082 will fix this and similar others.
Fixes #1043