From aa566e49b152c51ede7d79de2acf451a82586476 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Mon, 20 Nov 2023 18:24:52 +0000 Subject: [PATCH 1/2] [fontbe/avar] don't skip no-op segment maps GS contains a GRAD axis whose avar segment maps does not have any interesting mappings. It happens to be the last one in the axis order. We were filtering out axis segment maps that contain only identity maps, leading to an avar table with fewer segment maps than there are fvar axes, which is invalid as OTS correctly points out. So we now still emit these when bulding avar and there's at least one segment map that is not an identity map. Note that, despite the OT spec allows for empty segment maps for axis that don't distort the default normalization, we want to match fontmake/fonttools and emit the required {-1:1, 0:0, 1:1} mappings even when no other interesting mappings are present for an axis. Fixes #582 avar axis mismatch OTS error after this GS avar table is the same when built with fontc vs fontmake. --- fontbe/src/avar.rs | 51 +++++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/fontbe/src/avar.rs b/fontbe/src/avar.rs index a222af1b1..eccfd8aa0 100644 --- a/fontbe/src/avar.rs +++ b/fontbe/src/avar.rs @@ -1,5 +1,6 @@ //! Generates a [avar](https://learn.microsoft.com/en-us/typography/opentype/spec/avar) table. +use font_types::F2Dot14; use fontdrasil::{ coords::{CoordConverter, DesignCoord, NormalizedCoord}, orchestration::{Access, Work}, @@ -21,7 +22,30 @@ pub fn create_avar_work() -> Box { Box::new(AvarWork {}) } -fn to_segment_map(axis: &Axis) -> Option { +/// Return true if the SegmentMaps is not a boring identity map +fn is_interesting(segment_map: &SegmentMaps) -> bool { + segment_map + .axis_value_maps + .iter() + .any(|av| av.from_coordinate != av.to_coordinate) +} + +/// Return a default avar SegmentMaps containing the required {-1:-1, 0:0, 1:1} maps +fn default_segment_map() -> SegmentMaps { + // The OT avar spec would allow us to leave the axis value maps empty, however some + // implementations want the 3 required maps to always be present even when the default + // normalization for an axis was not modified. + // We are matching fontTools.varLib here: + // https://github.com/fonttools/fonttools/blob/51e70f9/Lib/fontTools/varLib/__init__.py#L151-L157 + // https://learn.microsoft.com/en-us/typography/opentype/spec/avar#table-formats + SegmentMaps::new(vec![ + AxisValueMap::new(F2Dot14::from_f32(-1.0), F2Dot14::from_f32(-1.0)), + AxisValueMap::new(F2Dot14::from_f32(0.0), F2Dot14::from_f32(0.0)), + AxisValueMap::new(F2Dot14::from_f32(1.0), F2Dot14::from_f32(1.0)), + ]) +} + +fn to_segment_map(axis: &Axis) -> SegmentMaps { // default normalization let default_converter = CoordConverter::new( vec![ @@ -57,9 +81,9 @@ fn to_segment_map(axis: &Axis) -> Option { // avar maps from the default normalization to the actual one, // using normalized values on both sides. - // All identity mappings are not interesting so we can skip them. + // All identity mappings are not interesting so we return the default mapping. if mappings.iter().all(|(k, v)| k == v) { - return None; + return default_segment_map(); } let mappings = mappings @@ -69,7 +93,7 @@ fn to_segment_map(axis: &Axis) -> Option { }) .collect(); - Some(SegmentMaps::new(mappings)) + SegmentMaps::new(mappings) } impl Work for AvarWork { @@ -91,13 +115,12 @@ impl Work for AvarWork { debug!("Skip avar; this is not a variable font"); return Ok(()); } - let axis_segment_maps: Vec<_> = static_metadata - .axes + let axis_segment_maps: Vec<_> = static_metadata.axes.iter().map(to_segment_map).collect(); + // only when all the segment maps are uninteresting, we can omit avar + let avar = axis_segment_maps .iter() - .filter_map(to_segment_map) - .filter(|sm| !sm.axis_value_maps.is_empty()) - .collect(); - let avar = (!axis_segment_maps.is_empty()).then_some(Avar::new(axis_segment_maps)); + .any(is_interesting) + .then_some(Avar::new(axis_segment_maps)); context.avar.set_unconditionally(avar.into()); Ok(()) } @@ -113,7 +136,7 @@ mod tests { use std::{cmp, str::FromStr}; use write_fonts::tables::avar::SegmentMaps; - use super::to_segment_map; + use super::{default_segment_map, to_segment_map}; fn axis(mappings: Vec<(UserCoord, DesignCoord)>, default_idx: usize) -> Axis { let default_idx = cmp::min(mappings.len() - 1, default_idx); @@ -150,7 +173,7 @@ mod tests { ]; for i in 1..mappings.len() { let mappings = mappings[0..i].to_vec(); - assert!(to_segment_map(&axis(mappings, 1)).is_none()); + assert_eq!(to_segment_map(&axis(mappings, 1)), default_segment_map()); } } @@ -164,7 +187,7 @@ mod tests { ]; assert_eq!( vec![(-1.0, -1.0), (0.0, 0.0), (0.75, 0.95), (1.0, 1.0),], - dump(to_segment_map(&axis(mappings, 1)).unwrap()) + dump(to_segment_map(&axis(mappings, 1))) ); } @@ -179,7 +202,7 @@ mod tests { ]; assert_eq!( vec![(-1.0, -1.0), (0.0, 0.0), (0.3333, 0.4943), (1.0, 1.0),], - dump(to_segment_map(&axis(mappings, 0)).unwrap()) + dump(to_segment_map(&axis(mappings, 0))) ); } } From 88459fa8be6c4d727b7fd57cdb18a5455c8221b4 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 21 Nov 2023 13:06:27 +0000 Subject: [PATCH 2/2] rename is_intersting => !is_identity as per review we shall use the SegmentMaps instance method once that is added to write-fonts also, use then() instead of then_some() so it's lazily eval --- fontbe/src/avar.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/fontbe/src/avar.rs b/fontbe/src/avar.rs index eccfd8aa0..8e85d3bf9 100644 --- a/fontbe/src/avar.rs +++ b/fontbe/src/avar.rs @@ -22,12 +22,13 @@ pub fn create_avar_work() -> Box { Box::new(AvarWork {}) } -/// Return true if the SegmentMaps is not a boring identity map -fn is_interesting(segment_map: &SegmentMaps) -> bool { +/// Return true if the SegmentMaps is an identity map +// TODO: make this a method of SegmentMaps in write-fonts +fn is_identity(segment_map: &SegmentMaps) -> bool { segment_map .axis_value_maps .iter() - .any(|av| av.from_coordinate != av.to_coordinate) + .all(|av| av.from_coordinate == av.to_coordinate) } /// Return a default avar SegmentMaps containing the required {-1:-1, 0:0, 1:1} maps @@ -119,8 +120,8 @@ impl Work for AvarWork { // only when all the segment maps are uninteresting, we can omit avar let avar = axis_segment_maps .iter() - .any(is_interesting) - .then_some(Avar::new(axis_segment_maps)); + .any(|sm| !is_identity(sm)) + .then(|| Avar::new(axis_segment_maps)); context.avar.set_unconditionally(avar.into()); Ok(()) }