-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix rounding of VariationModel::deltas #1070
Conversation
where fontc' lack of rounding leads to deltas that are off-by-one compared with fontmake/fonttools in Gelasio's MVAR table #1043 (comment)
b46c005
to
b6eea92
Compare
3d93271
to
ecff0e7
Compare
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.
A bunch of Rust style nits inline. Thanks for taking this one on, it looks fiddly...
fontir/src/variations.rs
Outdated
/// | ||
/// This trait provides a generic way to apply banker's rounding to different types, | ||
/// such as `f64` and `kurbo::Vec2`. | ||
pub trait BankerRound<U, T = Self> { |
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.
Where does this trait have two type parameters? I would naively expect it not to need any, and just look like,
NameOfOurRoundTrait {
fn our_round_method(self) -> Self;
}
given that there are no arguments. The only time we would need an additional trait parameter would be if we might want to return some type different from the type of Self
(in which case we would want an associated type, like Mul::Output
) or else if there was another operand, as in Mul<Rhs>
.
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 dunno I copied this pattern from OtRound in write_fonts lol
I'll simplify, thanks :)
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.
OtRound is different because the output is not always the same as the implementing type. :)
the first commit is a repro for #1043 (comment)
the lack of rounding within the delta computation can lead to deltas that are off-by-one by the time they are eventually ot-rounded to i16, as it is shown in Gelasio's MVAR table (from which the reproducer is drawn).
The commits that follow will apply the fix for #1043 as well as for #235, and add more tests borrowed from fonttools models_test.py
Basically we need to apply the rounding within the very same loop in which deltas get subtracted away from the absolute master values. This is to make sure that the rounding errors are accounted for and don't accummulate beyond 0.5 when multiple regions overlap.
The use of the banker's "round half to even" method (
round_ties_even
in Rust) matches the fonttools varLib's choice of Python's built-inround()
function. The latter is preferred over OtRound for relative values such as deltas primarily because of its symmetry: e.g. round(1.5) == 2 and round(-1.5) == -2, not -1. OtRound ((v + 0.5).floor()
) is biased towards higher, positive values and makes more sense for absolute coordinates./cc @behdad