-
Notifications
You must be signed in to change notification settings - Fork 17
Move anchor propagation from glyphs-reader to fontir #1832
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
base: main
Are you sure you want to change the base?
Conversation
b687d8a to
3b425b0
Compare
Compute GDEF categories in two phases to break the circular dependency between anchor propagation (needs mark/ligature info) and category computation (needs to see propagated anchors for Base detection). 1. PreliminaryGdefCategories - from source metadata only, no anchor inspection. Used during anchor propagation. 2. GdefCategories - final categories after propagation. For glyphsLib sources (infer_from_anchors=true), Base is inferred from anchors. For UFO sources (infer_from_anchors=false), categories used as-is. Source loaders now compute preliminary categories and store them in context.preliminary_gdef_categories (no longer in StaticMetadata). Final categories are computed later by GlyphOrderWork after anchor propagation completes.
Add a new tri-state compilation flag to control anchor propagation. Enabled by default for Glyphs sources (native behavior), or optionally enabled for UFO sources when propagateAnchors filter is present. Glyphs sources can opt-out via regular "Propagate Anchors" custom parameter, as well as via ufo2ft propagateAnchors filter lib key in userData (for .glyphs sources converted by ufo2glyhs). This follows the pattern of existing flags like FLATTEN_COMPONENTS and ERASE_OPEN_CORNERS.
This is the source-agnostic equivalent of what glyphs-reader previously did as preprocessing. One missing thing is support for Glyphs.app-specific component.anchor, which I'll tackle in a separate commit.
If PROPAGATE_ANCHORS flag is set, call propagate_all_anchors() to copy anchors from components to composites Always call recompute_gdef_categories() to compute final categories: - For glyphsLib (infer_from_anchors=true): infer Base from anchors, require anchors for Ligature - For ufo2ft (infer_from_anchors=false): copy preliminary as-is The recompute_gdef_categories() function: - Takes preliminary categories (Mark/Ligature/Base from source) - Optionally inspects anchors to infer Base and prune Ligatures (matching glyphsLib) - Writes final categories to context.gdef_categories Added test for recompute_gdef_categories to verify that glyphs with pre-existing anchors get classified as Base even when anchor propagation is disabled.
fontbe now reads gdef_categories from context.ir.gdef_categories instead of static_metadata.gdef_categories. Also update fontc job scheduling to include GdefCategories. Removed the old glyphs-reader implementation, as the migration to fontir should now be complete. Moved the `CompositeLike` impl for `glyphs_reader::font::Glyph` to glyphs-reader/font.rs since it's still needed by `align_bracket_layers()`. Also, moved some integration tests from the deleted propagate_anchors.rs module to fontc/src/lib.rs tests, adapted to work on IR.
Implement the same `maybe_rename_component_anchor` logic which was previously in glyphs-reader's propagate_anchors.rs. For that I needed to add a new `anchor` field to `ir::Component` to store the explicit component attachment anchors. When a base has multiple anchors with the same prefix (e.g., `top`, `top_1`, `top_alt`), this specifies which one a mark component attaches to (e.g. ligature numbered anchors `top_2` for second letter of `f_i`, or alternative anchors `top_alt` for Vietnamese diacritics). In addition to preserving `component.anchor` from .glyphs sources in glyphs2fontir, I also parse `com.schriftgestaltung.Glyphs.ComponentInfo` in ufo2fontir from a UFO glyph's lib, for sources that were exported to DS+UFO by glyphsLib.
3b425b0 to
c774e66
Compare
cmyr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no major notes on the impl, but this is causing a bunch of regressions for existing fonts.
Some examples of previously identical sources that are now producing diffs:
python3 -m ttx_diff 'https://github.com/mara-aa/elms-sans?bc5a52f349#sources/ElmsSans.glyphs'
python3 -m ttx_diff 'https://github.com/positype/Murecho-Project?0efba44c1c#sources/Murecho.glyphs'
python3 -m ttx_diff 'https://github.com/cyrealtype/Lora-Cyrillic?c44a1dde19#sources/Lora.glyphs'
python3 -m ttx_diff 'https://github.com/reddit/redditsans?60e19b50bd#sources/RedditMono.glyphs'
python3 -m ttx_diff 'https://github.com/solmatas/BitterPro?3238d7ae2c#sources/Bitter.glyphs'
|
Thanks for taking a look, I'll fix the regressions tomorrow! |
- [kern.rs] Move GdefCategories import in with other imports - Make Component::new() and with_anchor() accept impl Into<GlyphName> and simplify call sites
| flags.set(Flags::ERASE_OPEN_CORNERS, true); | ||
| } | ||
| // TODO: add PROPAGATE_ANCHORS flag when we have that implemented | ||
| if self.propagate_anchors == Some(true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actionable, just looking to learn.
In practice, how often do these flags change? Will user's want to experiment with propagate_anchors for the foreseeable future or is it a mechanism to roll out a feature that basically every user will leave alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's mostly to be able to test the effect of anchor propagation (or its absence) without needing to modify the source itself. I'd expect users to prefer enabling/disabling this in the source via appropriate custom parameter (or ufo lib key).
| // If set, open corners will be erased (Glyphs-native feature) | ||
| const ERASE_OPEN_CORNERS = 0b1000000000; | ||
| // If set, anchors will be propagated from components to composites | ||
| const PROPAGATE_ANCHORS = 0b10000000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused for a second and thought these were all the same! What do you think of separating bytes with underscore?
0b100_00000000
| Default::default(), // TODO: glyph locations we really do need | ||
| Default::default(), | ||
| Default::default(), | ||
| Default::default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used to build the StaticMetadata for fontra2fontir/src/source.rs. In that file, also_completes has not been updated. Does it get generated somewhere else for fontra or does fontra not need GDef categories?
StaticMetadataWork for fontir
impl Work<Context, WorkId, Error> for StaticMetadataWork {
fn id(&self) -> WorkId {
WorkId::StaticMetadata
}
fn also_completes(&self) -> Vec<WorkId> {
vec![WorkId::PreliminaryGlyphOrder] // No PreliminaryGdef categories
}
fn exec(&self, context: &Context) -> Result<(), Error> {
debug!("Static metadata for {:#?}", self.fontdata_file);
context
.preliminary_glyph_order
.set(self.glyph_info.keys().cloned().collect());
context
.static_metadata
.set(create_static_metadata(&self.fontdata_file)?);
Ok(())
}
}Two issues were preventing anchors from propagating correctly when composites reference non-exporting glyphs: 1. Anchors were only collected for exporting glyphs. Non-exporting glyphs with anchors (used as components) weren't having their anchors stored in the IR, so propagation couldn't find them. 2. flatten_all_non_export_components() ran BEFORE propagate_all_anchors(). This converts component references to paths, so by the time propagation ran, the composite no longer knew it referenced a non-exporting component. The fix involves collecting anchors from ALL glyphs (with graceful error handling for invalid anchors on non-exporting glyphs per issue #1397), and reordering the operations so propagation happens while component references still exist. Fixes regressions in ElmsSans and Bitter (now identical), and RedditMono's GDEF There are still a bunch of other diffs which I'm currently investigating.
Fixes #1704
This huge PR refactors anchor propagation to be source-agnostic by moving it from
glyphs-reader(Glyphs-specific preprocessing) tofontirso that it works with not only for Glyphs sources but also for DS + UFOs or any other frontends.GoogleSans.designspace markkern.txt goes from the current 82.760% to 97.262% (the remaining diffs are bugs on ufo2ft propagateAnchors filter's side which I'll fix separately).
The main problem when moving the implementation from glyphs-reader to fontir was that there was a circular dependency in the way GDEF categories are computed for Glyphs sources in particular, whereby on the one hand the anchor propagation algorithm needs mark/ligature information, but on the other hand some GDEF glyph categories (Base, Ligature) are inferred based on the presence of anchors and thus need anchors to be propagated first.
In the previous code on
main, this wasn't a problem because glyphs-reader would use the glyph category/subCategory fields to do its propagate_anchors.rs, which would be performed at the very beginning when the Font was first loaded, so that by the time the GDEF categories were computed all the anchors (including propagated) would already be there.To resolve the circularity, I resorted to a two-phase approach similar to the one we use for PreliminaryGlyphOrder vs GlyphOrder: i.e. first each frontends populate PreliminaryGdefCategories using only source metadata (such as Glyphs' category/subCategory or the UFO public.openTypeCategories) but without checking the anchors; then after anchor propagation is done inside fontir, the final GdefCategories are computed by also taking the anchors into account.
Note that for DS+UFOs workflow, the
public.openTypeCategoriesare presumed to already be "final" and are not extented/pruned based on anchors (at least that's how ufo2ft treats these), so in fontc I follow this (source frontends can set a bool field on the PreliminaryGdefCategory struct to control whether final GdefCategories shoud be inferred from anchors or not).I kept the current source-level defaults: i.e. for .glyphs source we run anchor propagation by default unless disabled via either "Propagate Anchors" (false) custom parameter (as well as a "propagateAnchors" ufo2ft filter hidden in the masters' userData which is a remnant of ufo2glyphs workflows); whereas for DS + UFO sources anchor propagation is disabled by default but it can be enabled via "propagateAnchors" filter in "com.github.googlei18n.ufo2ft.filters" lib key (like GoogleSans does). And in any case, a tri-state CLI flag --propagate-anchors={true,false} can be used to override these (omitted means follow source).
I ported all the tests from the previous implementation, while adding a few more, and deleted the glyphs-reader/src/propagate_anchors.rs.
I know it's a big change, and I tried to split into logical commits that make review easier as much as possible, but it's inevitable that a lot of modules are affected. I suggest to review single commits in the order the appear.
Last thing, #1661 is not implemented here yet but this should make it easier to implement since we are already interpolating glyph geometry in fontir (for decomposing and flattening) so adding anchor interpolation should be relatively simple.