Skip to content

Commit

Permalink
need to follow fvar axes order when building gvar GlyphDeltas
Browse files Browse the repository at this point in the history
TupleBuilder was iterating over VariationRegion's axis_tents which is a BTreeMap, but it should instead iterate over the region's tents following the fvar's axis order, that's what the gvar Tuples must follow. Otherwise we store deltas for the wrong axes!
  • Loading branch information
anthrotype committed Aug 9, 2023
1 parent 287e6bc commit aafc68c
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 8 deletions.
12 changes: 10 additions & 2 deletions fontbe/src/gvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,29 @@ impl Work<Context, AnyWorkId, Error> for GvarWork {
Access::Custom(Arc::new(|id| {
matches!(
id,
AnyWorkId::Fe(FeWorkId::GlyphOrder) | AnyWorkId::Be(WorkId::GvarFragment(..))
AnyWorkId::Fe(FeWorkId::StaticMetadata)
| AnyWorkId::Fe(FeWorkId::GlyphOrder)
| AnyWorkId::Be(WorkId::GvarFragment(..))
)
}))
}

/// Generate [gvar](https://learn.microsoft.com/en-us/typography/opentype/spec/gvar)
fn exec(&self, context: &Context) -> Result<(), Error> {
// We built the gvar fragments alongside glyphs, now we need to glue them together into a gvar table
let static_metadata = context.ir.static_metadata.get();
let axis_order: Vec<_> = static_metadata
.variable_axes
.iter()
.map(|a| a.tag)
.collect();
let glyph_order = context.ir.glyph_order.get();

let variations: Vec<_> = make_variations(&glyph_order, |glyph_name| {
context
.gvar_fragments
.get(&WorkId::GvarFragment(glyph_name.clone()).into())
.to_deltas()
.to_deltas(&axis_order)
});
let gvar = Gvar::new(variations).map_err(Error::GvarError)?;

Expand Down
13 changes: 7 additions & 6 deletions fontbe/src/orchestration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ pub struct GvarFragment {
}

impl GvarFragment {
pub fn to_deltas(&self) -> Vec<GlyphDeltas> {
pub fn to_deltas(&self, axis_order: &[Tag]) -> Vec<GlyphDeltas> {
self.deltas
.iter()
.filter_map(|(region, deltas)| {
Expand All @@ -266,7 +266,7 @@ impl GvarFragment {
})
.collect();

let tuple_builder = TupleBuilder::new(region);
let tuple_builder = TupleBuilder::new(region, axis_order);
let (min, peak, max) = tuple_builder.build();
Some(GlyphDeltas::new(peak, deltas, Some((min, max))))
})
Expand Down Expand Up @@ -300,9 +300,10 @@ struct TupleBuilder {
}

impl TupleBuilder {
fn new(region: &VariationRegion) -> Self {
fn new(region: &VariationRegion, axis_order: &[Tag]) -> Self {
let mut builder = TupleBuilder::default();
for (tag, tent) in region.iter() {
for tag in axis_order {
let tent = region.get(tag).unwrap();
builder.axes.push(*tag);
builder.min.push(F2Dot14::from_f32(tent.min.to_f32()));
builder.peak.push(F2Dot14::from_f32(tent.peak.to_f32()));
Expand Down Expand Up @@ -642,7 +643,7 @@ mod tests {
vec![None, Some((1.0, 0.0).into()), None],
)],
}
.to_deltas();
.to_deltas(&[Tag::new(b"wght")]);
assert!(!deltas.is_empty(), "{deltas:?}");
}

Expand All @@ -652,7 +653,7 @@ mod tests {
glyph_name: "blah".into(),
deltas: vec![(non_default_region(), vec![None, None, None])],
}
.to_deltas();
.to_deltas(&[Tag::new(b"wght")]);
assert!(deltas.is_empty(), "{deltas:?}");
}
}
4 changes: 4 additions & 0 deletions fontir/src/variations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,10 @@ impl VariationRegion {
pub fn is_default(&self) -> bool {
self.active_axes.is_empty()
}

pub fn get(&self, tag: &Tag) -> Option<&Tent> {
self.axis_tents.get(tag)
}
}

/// The min/peak/max of a masters influence.
Expand Down

0 comments on commit aafc68c

Please sign in to comment.