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

Fix rounding of VariationModel::deltas #1070

Merged
merged 6 commits into from
Oct 29, 2024
Merged

Conversation

anthrotype
Copy link
Member

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-in round() 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

where fontc' lack of rounding leads to deltas that are off-by-one compared with fontmake/fonttools in Gelasio's MVAR table

#1043 (comment)
Fixes #1043 and #235

This commit also adds a test from fonttools Tests/varLib/models_test.py that demostrates how our previous approach of late-rounding of deltas can actually lead to an excessive accummulation of rounding errors when the deltas are actually used for interpolation.
fontir/src/variations.rs Outdated Show resolved Hide resolved
fontir/src/variations.rs Outdated Show resolved Hide resolved
Copy link
Member

@cmyr cmyr left a 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...

///
/// 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> {
Copy link
Member

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>.

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 dunno I copied this pattern from OtRound in write_fonts lol

I'll simplify, thanks :)

Copy link
Member

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. :)

fontir/src/variations.rs Outdated Show resolved Hide resolved
fontir/src/variations.rs Outdated Show resolved Hide resolved
fontir/src/variations.rs Outdated Show resolved Hide resolved
fontir/src/variations.rs Outdated Show resolved Hide resolved
fontir/src/variations.rs Outdated Show resolved Hide resolved
@anthrotype anthrotype added this pull request to the merge queue Oct 29, 2024
Merged via the queue into main with commit 723e233 Oct 29, 2024
10 checks passed
@anthrotype anthrotype deleted the varmodel-banker-round branch October 29, 2024 09:34
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