From 31061da887d153fd4eed9b43b14c232865dd28ff Mon Sep 17 00:00:00 2001 From: rsheeter Date: Wed, 16 Oct 2024 20:18:12 -0700 Subject: [PATCH 1/2] Add simplified reproduction of ttx_diff failure --- .../testdata/glyphs2/MatrixComponent.glyphs | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 resources/testdata/glyphs2/MatrixComponent.glyphs diff --git a/resources/testdata/glyphs2/MatrixComponent.glyphs b/resources/testdata/glyphs2/MatrixComponent.glyphs new file mode 100644 index 00000000..0b2928a2 --- /dev/null +++ b/resources/testdata/glyphs2/MatrixComponent.glyphs @@ -0,0 +1,56 @@ +{ +.appVersion = "3219"; +familyName = MatrixComponent; +fontMaster = ( +{ +id = m01; +} +); +glyphs = ( +{ +glyphname = infinity; +layers = ( +{ +components = ( +{ +name = eight; +transform = "{0.9, -0.1, -0.1, 0.9, 500, 300}"; +} +); +layerId = "m01"; +width = 705; +} +); +note = infinity; +unicode = 221E; +}, +{ +glyphname = eight; +layers = ( +{ +layerId = "m01"; +paths = ( +{ +closed = 1; +nodes = ( +"250 -20 OFFCURVE", +"300 0 OFFCURVE", +"350 50 CURVE SMOOTH", +"400 50 OFFCURVE", +"421 127 OFFCURVE", +"421 192 CURVE SMOOTH", +); +} +); +width = 474; +} +); +note = eight; +unicode = 0038; +} +); + +unitsPerEm = 1000; +versionMajor = 42; +versionMinor = 42; +} From 7fbea81e590e59f3eb287503c985887497c90af3 Mon Sep 17 00:00:00 2001 From: rsheeter Date: Wed, 16 Oct 2024 22:51:28 -0700 Subject: [PATCH 2/2] Expose & fix issue with composite bboxes --- fontbe/src/glyphs.rs | 185 +++++++++--------- fontc/src/lib.rs | 44 ++++- .../testdata/glyphs2/MatrixComponent.glyphs | 36 ++-- 3 files changed, 163 insertions(+), 102 deletions(-) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index e9847235..51e71b0c 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -3,7 +3,10 @@ //! Each glyph is built in isolation and then the fragments are collected //! and glued together to form a final table. -use std::collections::{BTreeSet, HashMap, HashSet}; +use std::{ + collections::{BTreeSet, HashMap, HashSet}, + sync::Arc, +}; use fontdrasil::{ coords::NormalizedLocation, @@ -11,7 +14,7 @@ use fontdrasil::{ types::GlyphName, }; use fontir::{ - ir, + ir::{self, GlyphOrder}, orchestration::{Flags, WorkId as FeWorkId}, variations::{VariationModel, VariationRegion}, }; @@ -30,6 +33,7 @@ use write_fonts::{ }, gvar::{iup::iup_delta_optimize, GlyphDelta}, }, + types::GlyphId16, OtRound, }; @@ -77,19 +81,10 @@ fn can_reuse_metrics( coeffs == Affine::IDENTITY.as_coeffs() } -fn create_component( - context: &Context, - ref_glyph_name: &GlyphName, +fn create_component_ref_gid( + gid: GlyphId16, transform: &Affine, ) -> Result<(Component, Bbox), GlyphProblem> { - // Obtain glyph id from static metadata - let gid = context - .ir - .glyph_order - .get() - .glyph_id(ref_glyph_name) - .ok_or(GlyphProblem::NotInGlyphOrder)?; - // No known source does point anchoring so we just turn transform into a 2x2 + offset let [a, b, c, d, e, f] = transform.as_coeffs(); let flags = ComponentFlags { @@ -99,8 +94,8 @@ fn create_component( let component = Component::new( gid, Anchor::Offset { - x: e as i16, - y: f as i16, + x: e.ot_round(), + y: f.ot_round(), }, Transform { xx: F2Dot14::from_f32(a as f32), @@ -115,6 +110,22 @@ fn create_component( Ok((component, Bbox::default())) } +fn create_component_ref_name( + context: &Context, + ref_glyph_name: &GlyphName, + transform: &Affine, +) -> Result<(Component, Bbox), GlyphProblem> { + // Obtain glyph id from static metadata + let gid = context + .ir + .glyph_order + .get() + .glyph_id(ref_glyph_name) + .ok_or(GlyphProblem::NotInGlyphOrder)?; + + create_component_ref_gid(gid, transform) +} + fn create_composite( context: &Context, glyph: &ir::Glyph, @@ -139,7 +150,7 @@ fn create_composite( } }) .filter_map(|(ref_glyph_name, transform)| { - create_component(context, ref_glyph_name, transform) + create_component_ref_name(context, ref_glyph_name, transform) .map_err(|problem| { errors.push(Error::ComponentError { glyph: glyph.name.clone(), @@ -731,15 +742,6 @@ fn affine_for(component: &Component) -> Affine { ]) } -fn bbox2rect(bbox: Bbox) -> Rect { - Rect { - x0: bbox.x_min.into(), - y0: bbox.y_min.into(), - x1: bbox.x_max.into(), - y1: bbox.y_max.into(), - } -} - #[derive(Debug)] struct GlyfLocaWork {} @@ -747,6 +749,55 @@ pub fn create_glyf_loca_work() -> Box { Box::new(GlyfLocaWork {}) } +/// See +fn bbox_of_composite( + glyph_order: &GlyphOrder, + glyphs: &HashMap<&GlyphName, Arc>, + composite: &CompositeGlyph, + affine: Affine, +) -> Result, Error> { + // For simple scale+translate transforms, which seem to be common, we could just transform a bbox + // Let's wait to see if that pops out in a profile and do the simple solution for now + // Because transforms can skew/rotate the control box computed for the simple glyph isn't always reusable + + let mut bbox: Option = None; + for component in composite.components() { + // The transform we get here has changed because it got turned into F2Dot14 and i16 parts + // We could go get the "real" transform from IR but ... this seems to match fontmake so far + let affine = affine * affine_for(component); + + let ref_glyph_name = glyph_order + .glyph_name(component.glyph.to_u16() as usize) + .unwrap(); + let Some(ref_glyph) = glyphs.get(ref_glyph_name) else { + return Err(Error::MissingGlyphId(ref_glyph_name.clone())); + }; + match &ref_glyph.data { + RawGlyph::Empty => continue, // no impact on our bbox + RawGlyph::Simple(ref_simple) => { + // Update our bbox to include the transformed points + for pt in ref_simple.contours().iter().flat_map(|c| c.iter()) { + let pt = affine * Point::new(pt.x as f64, pt.y as f64); + bbox = Some(if let Some(current) = bbox { + current.union_pt(pt) + } else { + Rect::from_points(pt, pt) + }); + } + } + RawGlyph::Composite(ref_composite) => { + // Chase our components using an updated transform + if let Some(child_bbox) = + bbox_of_composite(glyph_order, glyphs, ref_composite, affine)? + { + bbox = bbox.map(|bbox| bbox.union(child_bbox)).or(Some(child_bbox)); + } + } + } + } + Ok(bbox) +} + fn compute_composite_bboxes(context: &Context) -> Result<(), Error> { let glyph_order = context.ir.glyph_order.get(); @@ -763,71 +814,13 @@ fn compute_composite_bboxes(context: &Context) -> Result<(), Error> { // Simple glyphs have bbox set. Composites don't. // Ultimately composites are made up of simple glyphs, lets figure out the boxes let mut bbox_acquired: HashMap = HashMap::new(); - let mut composites = glyphs - .values() - .filter(|glyph| glyph.is_composite()) - .collect::>(); - - trace!("Resolve bbox for {} composites", composites.len()); - while !composites.is_empty() { - let pending = composites.len(); - - // Hopefully we can figure out some of those bboxes! - for composite in composites.iter() { - let glyph_name = &composite.name; - let RawGlyph::Composite(composite) = &composite.data else { - unreachable!("we just checked that these are all composites"); - }; - - let mut missing_boxes = false; - let boxes: Vec<_> = composite - .components() - .iter() - .filter_map(|c| { - if missing_boxes { - return None; // can't succeed - } - let ref_glyph_name = glyph_order.glyph_name(c.glyph.to_u16() as usize).unwrap(); - let bbox = bbox_acquired.get(ref_glyph_name).copied().or_else(|| { - glyphs - .get(ref_glyph_name) - .map(|g| g.as_ref().clone()) - .and_then(|g| match &g.data { - RawGlyph::Composite(..) => None, - RawGlyph::Empty => None, - RawGlyph::Simple(simple_glyph) => Some(bbox2rect(simple_glyph.bbox)), - }) - }); - if bbox.is_none() { - trace!("Can't compute bbox for {glyph_name} because bbox for {ref_glyph_name} isn't ready yet"); - missing_boxes = true; - return None; // maybe next time? - }; - - // The transform we get here has changed because it got turned into F2Dot14 and i16 parts - // We could go get the "real" transform from IR...? - let affine = affine_for(c); - let transformed_box = affine.transform_rect_bbox(bbox.unwrap()); - Some(transformed_box) - }) - .collect(); - if missing_boxes { - trace!("bbox for {glyph_name} not yet resolveable"); - continue; - } - - let bbox = boxes.into_iter().reduce(|acc, e| acc.union(e)).unwrap(); - trace!("bbox for {glyph_name} {bbox:?}"); - bbox_acquired.insert(glyph_name.clone(), bbox); - } - // Kerplode if we didn't make any progress this spin - composites.retain(|composite| !bbox_acquired.contains_key(&composite.name)); - if pending == composites.len() { - return Err(Error::CompositesStalled( - composites.iter().map(|g| g.name.clone()).collect(), - )); - } + for (glyph_name, glyph) in glyphs.iter().filter_map(|(gn, g)| match &g.data { + RawGlyph::Composite(composite) => Some((*gn, composite)), + RawGlyph::Simple(..) | RawGlyph::Empty => None, + }) { + let bbox = bbox_of_composite(&glyph_order, &glyphs, glyph, Affine::IDENTITY)?; + bbox_acquired.insert(glyph_name.clone(), bbox.unwrap_or_default()); } // It'd be a shame to just throw away those nice boxes @@ -1027,4 +1020,18 @@ mod tests { expected_segments ); } + + // Contributor to https://github.com/googlefonts/fontc/pull/1050 + #[test] + fn component_translation_otrounds() { + let (c, _) = create_component_ref_gid( + GlyphId16::new(42), + &Affine::new([1.0, 0.0, 0.0, 1.0, 0.4, 0.9]), + ) + .unwrap(); + let Anchor::Offset { x, y } = c.anchor else { + panic!("Must be an offset"); + }; + assert_eq!((0, 1), (x, y)); + } } diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs index bb8d5786..533d0668 100644 --- a/fontc/src/lib.rs +++ b/fontc/src/lib.rs @@ -588,7 +588,7 @@ mod tests { types::F2Dot14, FontData, FontRead, FontReadWithArgs, FontRef, TableProvider, }, - GlyphId16, MetadataProvider, Tag, + GlyphId, GlyphId16, MetadataProvider, Tag, }; use tempfile::{tempdir, TempDir}; use write_fonts::{ @@ -3264,4 +3264,46 @@ mod tests { fn obeys_source_codepage_ranges_designspace() { assert_expected_codepage_ranges("designspace_from_glyphs/WghtVarOS2.designspace"); } + + #[test] + fn bbox_of_nested_components() { + let compile = TestCompile::compile_source("glyphs2/MatrixComponent.glyphs"); + let font = compile.font(); + let glyf = font.glyf().unwrap(); + let loca = font.loca(false).unwrap(); + + // original glyph and transformed derivatives sketched in https://codepen.io/rs42/pen/wvVqVPL?editors=1000 + // "correct" values taken from fontmake compilation + + let expected_rot30_bbox = Rect::new(75.0, 107.0, 359.0, 300.0); + let expected_rot60more_bbox = Rect::new(50.0, 149.0, 150.0, 449.0); + + let gids = ["rot30", "rot60more"] + .iter() + .map(|gn| { + compile + .fe_context + .glyph_order + .get() + .glyph_id(&GlyphName::new(gn)) + .map(|gid16| GlyphId::new(gid16.to_u32())) + .unwrap() + }) + .collect::>(); + + let boxes = gids + .iter() + .map(|gid| loca.get_glyf(*gid, &glyf).unwrap().unwrap()) + .map(|glyf| { + Rect::new( + glyf.x_min() as f64, + glyf.y_min() as f64, + glyf.x_max() as f64, + glyf.y_max() as f64, + ) + }) + .collect::>(); + + assert_eq!(vec![expected_rot30_bbox, expected_rot60more_bbox], boxes); + } } diff --git a/resources/testdata/glyphs2/MatrixComponent.glyphs b/resources/testdata/glyphs2/MatrixComponent.glyphs index 0b2928a2..d8d79828 100644 --- a/resources/testdata/glyphs2/MatrixComponent.glyphs +++ b/resources/testdata/glyphs2/MatrixComponent.glyphs @@ -8,24 +8,40 @@ id = m01; ); glyphs = ( { -glyphname = infinity; +glyphname = rot60more; layers = ( { components = ( { -name = eight; -transform = "{0.9, -0.1, -0.1, 0.9, 500, 300}"; +name = rot30; +transform = "{0.5, 0.866, -0.866, 0.5, 179.9, -11.6}"; +} +); +layerId = "m01"; +width = 705; +} +); +unicode = 221D; +}, +{ +glyphname = rot30; +layers = ( +{ +components = ( +{ +name = triangle; + +transform = "{0.866, 0.5, -0.5, 0.866, 88.4, -29.9}"; } ); layerId = "m01"; width = 705; } ); -note = infinity; unicode = 221E; }, { -glyphname = eight; +glyphname = triangle; layers = ( { layerId = "m01"; @@ -33,19 +49,15 @@ paths = ( { closed = 1; nodes = ( -"250 -20 OFFCURVE", -"300 0 OFFCURVE", -"350 50 CURVE SMOOTH", -"400 50 OFFCURVE", -"421 127 OFFCURVE", -"421 192 CURVE SMOOTH", +"100 100 LINE", +"400 150 LINE", +"100 200 LINE", ); } ); width = 474; } ); -note = eight; unicode = 0038; } );