Expose and fix broken processing of bbox for components whose transform is more than scale and transform #1050
Conversation
rsheeter
added this to the 100 families pass ttx_diff in default and gftools mode milestone
rsheeter
changed the title
rsheeter
changed the title
rsheeter
force-pushed
the
matrix
branch
2 times, most recently
from
cce9792
to
0c25b6d
Compare
rsheeter
changed the title
| // 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 |
There was a problem hiding this comment.
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).
| RawGlyph::Composite(composite) => Some((*gn, composite)), | ||
| RawGlyph::Simple(..) | RawGlyph::Empty => None, | ||
| }) { | ||
| let bbox = bbox_of_composite(&glyph_order, &glyphs, glyph, Affine::IDENTITY)?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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
Simplified reproduction of problem impacting Luxurious Roman (
python resources/scripts/ttx_diff.py https://github.com/googlefonts/luxurious-roman#sources/Luxurious-Roman.glyphs)This appears to be due to fontc assuming it can simply apply the component transform to the control box when in fact it needs to transform the points and compute the control box of the new points. https://codepen.io/rs42/pen/wvVqVPL?editors=1000 sketches the impact of transformation for the new test font.
Reproduced in simplified test that now passes. Luxurious Roman is now identical when ttx_diff'd locally.