Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expose and fix broken processing of bbox for components whose transform is more than scale and transform #1050

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Expose and fix broken processing of bbox for components whose transform is more than scale and transform #1050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31061da
Add simplified reproduction of ttx_diff failure
rsheeter Oct 17, 2024
7fbea81
Expose & fix issue with composite bboxes
rsheeter Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 96 additions & 89 deletions fontbe/src/glyphs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@
//! 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,
orchestration::{Access, AccessBuilder, Work},
types::GlyphName,
};
use fontir::{
ir,
ir::{self, GlyphOrder},
orchestration::{Flags, WorkId as FeWorkId},
variations::{VariationModel, VariationRegion},
};
Expand All @@ -30,6 +33,7 @@ use write_fonts::{
},
gvar::{iup::iup_delta_optimize, GlyphDelta},
},
types::GlyphId16,
OtRound,
};

Expand Down Expand Up @@ -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 {
Expand All @@ -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),
Expand All @@ -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,
Expand All @@ -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(),
Expand Down Expand Up @@ -731,22 +742,62 @@ 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 {}

pub fn create_glyf_loca_work() -> Box<BeWork> {
Box::new(GlyfLocaWork {})
}

/// See <https://github.com/fonttools/fonttools/blob/42c1a52c5facd0edbc9c685787b084af44f6f607/Lib/fontTools/ttLib/tables/_g_l_y_f.py#L1244>
fn bbox_of_composite(
glyph_order: &GlyphOrder,
glyphs: &HashMap<&GlyphName, Arc<Glyph>>,
composite: &CompositeGlyph,
affine: Affine,
) -> Result<Option<Rect>, 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait to see if that pops out in a profile

did you actually try to profile before/after this? I suggest filing an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, #1061

// Because transforms can skew/rotate the control box computed for the simple glyph isn't always reusable

let mut bbox: Option<Rect> = 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
Comment on lines +765 to +766
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 'real' transform from source or IR doesn't actually matter any more, it's the one that's stored in the glyf table that matters for bbox computation, that's what fonttools TTGlyph.recalcBounds() does (and ufo2ft uses the same method on the compiled TTGlyphs when building the glyf table).

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();

Expand All @@ -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<GlyphName, Rect> = HashMap::new();
let mut composites = glyphs
.values()
.filter(|glyph| glyph.is_composite())
.collect::<Vec<_>>();

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)?;
Copy link
Member

@anthrotype anthrotype Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you went for a recursive traversal instead of iterative one. I'm ok with it (fonttools does the same), but I was wondering, even though unlikely, should you prevent from a component cycle triggering an infinite recursion? Python has a max recursion depth after which it raises an exception. Does Rust protect you as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for glyphs sources at least we already check against component cycles when we propagate anchors, see glyphs-reader's propagate_anchors.rs (depth_sorted_composite_glyphs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me take confirmation of no cycles as a follow-on. I have the vague notion we might already do it but I'll double-check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1062

bbox_acquired.insert(glyph_name.clone(), bbox.unwrap_or_default());
}

// It'd be a shame to just throw away those nice boxes
Expand Down Expand Up @@ -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));
}
}
44 changes: 43 additions & 1 deletion fontc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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::<Vec<_>>();

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::<Vec<_>>();

assert_eq!(vec![expected_rot30_bbox, expected_rot60more_bbox], boxes);
}
}
68 changes: 68 additions & 0 deletions resources/testdata/glyphs2/MatrixComponent.glyphs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
{
.appVersion = "3219";
familyName = MatrixComponent;
fontMaster = (
{
id = m01;
}
);
glyphs = (
{
glyphname = rot60more;
layers = (
{
components = (
{
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;
}
);
unicode = 221E;
},
{
glyphname = triangle;
layers = (
{
layerId = "m01";
paths = (
{
closed = 1;
nodes = (
"100 100 LINE",
"400 150 LINE",
"100 200 LINE",
);
}
);
width = 474;
}
);
unicode = 0038;
}
);

unitsPerEm = 1000;
versionMajor = 42;
versionMinor = 42;
}
Loading