search

googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
152 stars 43 forks source link

filters: sort glyphs by decreasing component depth to avoid order-dependent issues #625

Closed anthrotype closed 2 years ago

anthrotype commented 2 years ago

Fixes https://github.com/googlefonts/ufo2ft/issues/621

anthrotype commented 2 years ago

Ugh.. looks like the PropagateAnchorsFilter also have some order-dependent problems.

the propagateAnchors_test.py expect "amacron" to be in the modified set but after my change it no longer is...

https://github.com/googlefonts/ufo2ft/runs/7007492994?check_suite_focus=true#step:5:138

anthrotype commented 2 years ago

it turns out it's the reporting of modified glyphs that is wrong in the PropagateAnchorsFilters, not the actual effect. Basically 'amacron' glyph is used as component from 'amacrondieresis' and as such it is modified as well (even when only the latter is included and not the former!), however if 'amacrondieresis' gets filtered before 'amacron' (like after this patch which sorts deeper composites first), only the 'amacrondieresis' is reported to be modified by the filter, the 'amacron' is skipped because already processed and modified as part of the 'amacrondieresis' propagation...

anthrotype commented 2 years ago

The test that was failing on main branch #626 now passes here, so I think I'm happy with this. Basically we just changed the order in which filters process the glyphs by making sure the composite glyphs with more deeply nested components get processed before those with shallower trees (or no components at all), to prevent the modifications in glyphs that are used as components to affect the filtering of the composite glyphs that reference them. The assumption being that the per-glyph filtering operation only modifies in-place the current given glyph it is run on (but we just saw PropagateAnchorsFilter happily modifying "amacron" even if only "amacrondieresis" was included... though it's kind of ok in the latter case because it has a guard against processing the same glyph twice.. and propagate anchors is weird by definition).

I also added tests to confirm that nothing changed in propagateAnchors_test.py (besides the hidden bug that not all the "modified" glyphs were actually reported as such, but that's minor).