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

Port fontc' propagate_anchors.rs to .py; apply it as preflight transformation #1011

Merged
merged 5 commits into from
Aug 8, 2024

Conversation

anthrotype
Copy link
Member

Fixes #368

This PR translates the fontc' propagate_anchors.rs from Rust to Python and applies the transformation to the GSFont in the 'preflight' step before it gets passed on to the UFOBuilder.
The Rust code was based off the original Glyphs.app's Objective-C code (Georg shared a snippet with us privately) and is as such more 'correct' then the old, reverse-engineered implementation.
The previous glyphsLib.builder.anchor_propagation module is now deprecated but is kept around in case users may instantiate the UFOBuilder class directly with propagate_anchors=True.
Also, the existing propagate_anchors parameters of to_designspace, to_ufos and other high-level methods continues to have an equivalent effect but now controls the new propagate anchors logic. Clients such as fontmake do not need to do anything besides upgrading glyphsLib to use this.

@anthrotype anthrotype force-pushed the new-propagate-anchors branch from 8ee8e45 to 4220706 Compare July 30, 2024 16:43
… on GSFont

Fixes #368

Translate fontc' propagate_anchors.rs from Rust to Python and apply to the GSFont in the 'preflight' step before it's passed on to UFOBuilder
The Rust code was based off the original Glyphs.app's Objective-C code (Georg shared a snippet with us privately) and is as such more 'correct' then the old, reverse-engineered implementation.
The old glyphsLib.builder.anchor_propagation module is deprecated but kept around in case users may instantiate the UFOBuilder class directly with propagate_anchors=True.
Also, the existing `propagate_anchors` parameters of `to_designspace`, `to_ufos` and other high-level methods continues to have an equivalent effect but now controls the **new** propagate anchors logic. Clients such as fontmake do not need to do anything besides upgrading glyphsLib to use this.
…onent wins

#368 (comment)

test borrowed from #1007 (which this PR will supersede)
@anthrotype anthrotype force-pushed the new-propagate-anchors branch from 6da5db9 to d6ae2d3 Compare August 1, 2024 11:20
@anthrotype
Copy link
Member Author

I skimmed the regression tests failures (too many to actually vet each single one), and they all seem as expected and for the good.
The only remaining known difference between this Python (and Rust) implementation, on the one hand, and the built-in Glyphs.app's implementation of anchor propagation, on the other hand, is that the former only handles anchor propagation for "master" layers (i.e. those present by definition which have the same layerId == associatedMasterId in all glyphs), whereas the latter also handles it for non-master (intermediate, alternate, color, etc.) layers.
This limitation is due to the fact that glyphsLib does not currently implement the componentLayer property, which would allow to dereference components even when the base glyph does not contain a layer with corresponding layerId (existing non-master layers may get matched by their attributes or new layers get interpolated on the fly): #853 (comment)

However, the same limitation already exists for the current (soon-to-be legacy) anchor_propagation.py, as this is similarly performed for each glyph in each master UFO (for glyph in ufo: etc.), that is only on the default layer of such ufo, and not considering additional ufo layers where Glyphs intermediate (brace) layers are stored by glyphsLib.

So I don't consider this limitation blocking. Implementing componentLayer (with all the required on-the-fly interpolation of missing intermediate layers) is complicated and would prefer not to block on that.

Copy link
Contributor

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

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

I won't claim deep understanding of anchor prop but I don't see any obvious problems and the tests seem to agree, lgtm

The current propagate_anchors implementation only finds a matching component layer that has the same layerId so only works for 'master' layers; to reduce the verbosity of the log for fonts that contain lots of special non-master (brace, bracket, etc) layers, for which we currently can't retrieve the corresponding component layer's anchors, for now we demote this from a warning to a debug message.
Note that the old anchor_propagation.py was only processing 'master' layers as well, so ignoring the special layers during anchor propagation is not a regression.
@anthrotype anthrotype merged commit 8698f2c into main Aug 8, 2024
9 of 10 checks passed
@anthrotype anthrotype deleted the new-propagate-anchors branch August 8, 2024 13:38
khaledhosny added a commit that referenced this pull request Oct 5, 2024
- Don’t propagate contextual anchors, GlyphsApp does not propagate them.
- When copying anchors, copy also userData, otherwise the context of
  contextual anchors of any processed glyph will be dropped.

This is a regression from #1011
khaledhosny added a commit that referenced this pull request Oct 5, 2024
- Don’t propagate contextual anchors, GlyphsApp does not propagate them.
- When copying anchors, copy also userData, otherwise the context of
  contextual anchors of any processed glyph will be dropped. This is a
  regression from #1011
khaledhosny added a commit that referenced this pull request Oct 7, 2024
- Don’t propagate contextual anchors, GlyphsApp does not propagate them.
- When copying anchors, copy also userData, otherwise the context of
  contextual anchors of any processed glyph will be dropped. This is a
  regression from #1011
khaledhosny added a commit that referenced this pull request Oct 7, 2024
- Don’t propagate contextual anchors, GlyphsApp does not propagate them.
- When copying anchors, copy also userData, otherwise the context of
  contextual anchors of any processed glyph will be dropped. This is a
  regression from #1011
schriftgestalt pushed a commit that referenced this pull request Oct 23, 2024
- Don’t propagate contextual anchors, GlyphsApp does not propagate them.
- When copying anchors, copy also userData, otherwise the context of
  contextual anchors of any processed glyph will be dropped. This is a
  regression from #1011
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.

issues with composites' anchor propagation
2 participants