Skip to content

Commit

Permalink
[font-types] float to fixed point rounding (#845)
Browse files Browse the repository at this point in the history
We had two implementations for float to fixed conversion based on whether std was available and the no_std implementation had a very subtle rounding bug (adding -0.5 for negative values but 0.0 for positive ones) causing some tests to fail.

This fixes the bug, restores the original tests and adds a new one to check rounding behavior in font-types.
  • Loading branch information
dfrg authored Mar 18, 2024
1 parent 1f5ad9b commit d158188
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 9 deletions.
25 changes: 21 additions & 4 deletions font-types/src/fixed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,10 +239,10 @@ macro_rules! float_conv {
/// representable value.
#[inline(always)]
pub fn $from(x: $ty) -> Self {
Self(
(x * Self::ONE.0 as $ty + (0.5 * (-1.0 * x.is_sign_negative() as u8 as $ty)))
as _,
)
// When x is positive: 1.0 - 0.5 = 0.5
// When x is negative: 0.0 - 0.5 = -0.5
let frac = (x.is_sign_positive() as u8 as $ty) - 0.5;
Self((x * Self::ONE.0 as $ty + frac) as _)
}

#[doc = concat!("Returns the value as an ", stringify!($ty), ".")]
Expand Down Expand Up @@ -420,6 +420,23 @@ mod tests {
assert_eq!(Fixed(0x7fff_ffff), Fixed::from_f64(32768.0));
}

// We lost the f64::round() intrinsic when dropping std and the
// alternative implementation was very slightly incorrect, throwing
// off some tests. This makes sure we match.
#[test]
fn fixed_floats_rounding() {
fn with_round_intrinsic(x: f64) -> Fixed {
Fixed((x * 65536.0).round() as i32)
}
// These particular values were tripping up tests
let inputs = [0.05, 0.6, 0.2, 0.4, 0.67755];
for input in inputs {
assert_eq!(Fixed::from_f64(input), with_round_intrinsic(input));
// Test negated values as well for good measure
assert_eq!(Fixed::from_f64(-input), with_round_intrinsic(-input));
}
}

#[test]
fn fixed_to_int() {
assert_eq!(Fixed::from_f64(1.0).to_i32(), 1);
Expand Down
6 changes: 3 additions & 3 deletions read-fonts/src/tables/avar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ mod tests {
value_map(-0.6667, -0.5),
value_map(-0.3333, -0.25),
value_map(0.0, 0.0),
value_map(0.2000122, 0.3674),
value_map(0.4000244, 0.52246094),
value_map(0.6, 0.67755127),
value_map(0.2, 0.3674),
value_map(0.4, 0.52246),
value_map(0.6, 0.67755),
value_map(0.8, 0.83875),
value_map(1.0, 1.0),
]];
Expand Down
2 changes: 1 addition & 1 deletion read-fonts/src/tables/postscript/dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ mod tests {
-20.0, 0.0, 473.0, 491.0, 525.0, 540.0, 644.0, 659.0, 669.0, 689.0, 729.0, 749.0,
])),
FamilyOtherBlues(make_blues(&[-249.0, -239.0])),
BlueScale(Fixed::from_f64(0.0374908447265625)),
BlueScale(Fixed::from_f64(0.037506103515625)),
BlueFuzz(Fixed::ZERO),
StdHw(Fixed::from_f64(55.0)),
StdVw(Fixed::from_f64(80.0)),
Expand Down
2 changes: 1 addition & 1 deletion skrifa/src/outline/cff/hint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1136,7 +1136,7 @@ mod tests {
let state = make_hint_state();
assert!(!state.do_em_box_hints);
assert_eq!(state.zone_count, 6);
assert_eq!(state.boost, Fixed::from_f64(0.412445068359375));
assert_eq!(state.boost, Fixed::from_bits(27035));
assert!(state.supress_overshoot);
// FreeType generates the following zones:
let expected_zones = &[
Expand Down

0 comments on commit d158188

Please sign in to comment.