Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jun 22, 2022

Conversation

anthrotype
Copy link
Member

Fixes #621

@anthrotype
Copy link
Member Author

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
Copy link
Member Author

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...

_propagate_glyph_anchors modifies not only the current composite glyph but all the nested composite glyphs used in between, however it was only reporting to be modifying the root glyph
@anthrotype anthrotype force-pushed the fix-filters-component-order branch from 3fcba73 to 44e5046 Compare June 22, 2022 16:38
@anthrotype
Copy link
Member Author

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).

@anthrotype anthrotype merged commit 72b2459 into main Jun 22, 2022
@anthrotype anthrotype deleted the fix-filters-component-order branch June 22, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DecomposeTransformedComponentsFilter: MM compatibility issues
2 participants