From 350b5a30fdb2917bdff7fc6d6e9a6e0787cd4676 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 27 Sep 2023 16:21:06 -0400 Subject: [PATCH 1/3] Omit all-zero deltas for composite glyphs Call it 'IUP lite'. --- fontbe/src/glyphs.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index 0b0b12e20..b784a1a09 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -277,7 +277,12 @@ fn compute_deltas( } else { let deltas = deltas .into_iter() - .map(|delta| GlyphDelta::required(delta.x.ot_round(), delta.y.ot_round())) + .map(|delta| match delta.to_point().ot_round() { + // IUP only applies to simple glyphs; for composites we + // just mark the zero deltas as being interpolatable. + (0, 0) => GlyphDelta::optional(0, 0), + (x, y) => GlyphDelta::required(x, y), + }) .collect(); Ok((region, deltas)) From e1062954fd1e4b3bfaafa10f5c23c9b649fb6cdb Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 28 Sep 2023 12:26:39 -0400 Subject: [PATCH 2/3] Include single delta for composite glyphs This matches the behaviour in fontmake, which is a workaround for a bug in macOS. --- fontbe/src/glyphs.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index b784a1a09..55713aa41 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -275,7 +275,7 @@ fn compute_deltas( iup_delta_optimize(deltas, coords.clone(), tolerance, contour_ends) .map(|iup_deltas| (region.clone(), iup_deltas)) } else { - let deltas = deltas + let mut deltas = deltas .into_iter() .map(|delta| match delta.to_point().ot_round() { // IUP only applies to simple glyphs; for composites we @@ -283,7 +283,18 @@ fn compute_deltas( (0, 0) => GlyphDelta::optional(0, 0), (x, y) => GlyphDelta::required(x, y), }) - .collect(); + .collect::>(); + + // match fonttools behaviour, ensuring there is at least one + // delta required, to ensure we write an entry for the glyf. + // + // + + if deltas.iter().all(|delta| !delta.required) { + if let Some(first) = deltas.first_mut() { + first.required = true; + } + } Ok((region, deltas)) } From 558e5c5fe8f4992d41845c3a4fde8ca67bf192b5 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Thu, 28 Sep 2023 15:35:07 -0400 Subject: [PATCH 3/3] Factor out composite glyph delta processing, add tests --- fontbe/src/glyphs.rs | 87 +++++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index 55713aa41..dfe30eda5 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -18,7 +18,7 @@ use fontir::{ orchestration::WorkId as FeWorkId, variations::{VariationModel, VariationRegion}, }; -use kurbo::{cubics_to_quadratic_splines, Affine, BezPath, CubicBez, PathEl, Point, Rect}; +use kurbo::{cubics_to_quadratic_splines, Affine, BezPath, CubicBez, PathEl, Point, Rect, Vec2}; use log::{log_enabled, trace, warn}; use read_fonts::{ @@ -264,9 +264,11 @@ fn compute_deltas( .map_err(|e| Error::GlyphDeltaError(glyph_name.clone(), e))? .into_iter() .map(|(region, deltas)| { - // Spec: inferring of deltas for un-referenced points applies only to simple glyphs, not to composite glyphs. + // Spec: inferring of deltas for un-referenced points applies only + // to simple glyphs, not to composite glyphs. if should_iup { - // Doing IUP optimization here conveniently means it threads per-glyph + // Doing IUP optimization here conveniently means it threads + // per-glyph if log_enabled!(log::Level::Trace) { // I like the point string better than the vec2 let deltas = deltas.iter().map(|d| d.to_point()).collect::>(); @@ -275,27 +277,7 @@ fn compute_deltas( iup_delta_optimize(deltas, coords.clone(), tolerance, contour_ends) .map(|iup_deltas| (region.clone(), iup_deltas)) } else { - let mut deltas = deltas - .into_iter() - .map(|delta| match delta.to_point().ot_round() { - // IUP only applies to simple glyphs; for composites we - // just mark the zero deltas as being interpolatable. - (0, 0) => GlyphDelta::optional(0, 0), - (x, y) => GlyphDelta::required(x, y), - }) - .collect::>(); - - // match fonttools behaviour, ensuring there is at least one - // delta required, to ensure we write an entry for the glyf. - // - // - - if deltas.iter().all(|delta| !delta.required) { - if let Some(first) = deltas.first_mut() { - first.required = true; - } - } - + let deltas = process_composite_deltas(deltas); Ok((region, deltas)) } }) @@ -303,6 +285,30 @@ fn compute_deltas( .map_err(|e| Error::IupError(glyph_name.clone(), e)) } +/// convert raw deltas to the write-fonts representation (composite glyphs only) +fn process_composite_deltas(deltas: Vec) -> Vec { + let mut deltas = deltas + .into_iter() + .map(|delta| match delta.to_point().ot_round() { + // IUP only applies to simple glyphs; for composites we + // just mark the zero deltas as being interpolatable. + (0, 0) => GlyphDelta::optional(0, 0), + (x, y) => GlyphDelta::required(x, y), + }) + .collect::>(); + + // match fonttools behaviour, ensuring there is at least one + // delta required, to ensure we write an entry for the glyf. + // + // + if deltas.iter().all(|delta| !delta.required) { + if let Some(first) = deltas.first_mut() { + first.required = true; + } + } + deltas +} + impl Work for GlyphWork { fn id(&self) -> AnyWorkId { WorkId::GlyfFragment(self.glyph_name.clone()).into() @@ -880,11 +886,11 @@ impl Work for GlyfLocaWork { #[cfg(test)] mod tests { + use super::*; + use fontir::ir; use kurbo::Affine; - use crate::glyphs::can_reuse_metrics; - /// Returns a glyph instance and another one that can be its component fn create_reusable_component() -> (ir::GlyphInstance, ir::GlyphInstance) { let parent = ir::GlyphInstance { @@ -928,4 +934,33 @@ mod tests { assert!(!can_reuse_metrics(&glyph, &component, &Affine::new(coeffs))); } } + + #[test] + fn all_zero_composite_deltas() { + let zeros = vec![Vec2::ZERO; 6]; + // if a composite glyph has all zero deltas we want to finish with a single + // required zero delta, and skip the rest: + let deltas = process_composite_deltas(zeros); + assert!(deltas[0].required); + assert!(deltas.iter().skip(1).all(|d| !d.required)); + } + + #[test] + fn elide_composite_zero_deltas() { + // if a composite glyph has some non-zero deltas than all zero deltas + // become optional + let deltas = vec![ + Vec2::ZERO, + Vec2::new(1., 0.), + Vec2::ZERO, + Vec2::ZERO, + Vec2::new(0., 5.), + ]; + + let processed = process_composite_deltas(deltas.clone()); + for (pre, post) in deltas.iter().zip(processed.iter()) { + let should_be_required = *pre != Vec2::ZERO; + assert_eq!(should_be_required, post.required) + } + } }