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

issues with composites' anchor propagation #368

Closed
anthrotype opened this issue May 31, 2018 · 23 comments · Fixed by #1011
Closed

issues with composites' anchor propagation #368

anthrotype opened this issue May 31, 2018 · 23 comments · Fixed by #1011

Comments

@anthrotype
Copy link
Member

anthrotype commented May 31, 2018

while working on the ufo2ft MarkFeatureWriter (I'm adding support for abvm and blwm), I noticed a few issues with glyphsLib's current implementation of the propagate anchor feature in Glyphs.app, whereby composite glyphs inherit anchors from their components.

I started the discussion here https://gitter.im/fonttools-dev/glyphsLib?at=5b0eef9793dc78791c9a5a64

I'll try to summarize my comments again here.

The way glyphsLib's anchor propagation currently works is more or less the following: anchors are automatically propagated from the component's base glyphs to the composite glyphs that reference them; if the target composite glyph already has an anchor with the same name, the anchor from the component is not propagated. In addition, if the composite references more than one components (either the same component more than once, or different components having anchors with same name), then the anchors with clashing names are duplexed with a _1, _2, suffix.
The latter is a convention used in Glyphs.app to denote anchors on ligature components (the index is the ligature component number, starting from 1). They are translated in pos ligature ... ligComponent ...; or mark2liga (LookupType 5) lookups.

One problem I noticed is that, neither glyphsLib nor Glyphs.app (from what I can tell) handle the case when in a ligature glyph, one of the ligature components has no marks attached.
The Feature File spec defines a special <anchor NULL> for this purpose:

If there are no anchor points on a component, it must still specify at least one anchor, which should be the NULL anchor.

https://www.adobe.com/devnet/opentype/afdko/topic_feature_file_syntax.html#6.d

It's not clear what should happen when there is a gap in the sequence of _1, _2, etc. numbers, if one or more ligature components contains no anchors. How is one supposed to denote such a NULL anchor (i.e. absence of anchors in a ligature component) within this anchor naming scheme? FWIW, Glyphs.app completely ignores such anchors on ligatures unless they start from _1, and stops as soon as there is a gap. 🤔

I also noticed a crucial difference between our anchor propagation algorithm and Glyphs.app's. We do the propagation to any composite glyph, and do the numbered suffix trick whenever there is any name collision in the propagated anchors.
Glyphs.app, on the other hand, does not seem to do any automatic anchor propagation when the composite glyph is marked as subCategory "Ligature" (either in the built-in GlyphData.xml database, or in the CMD-ALT-I panel for overriding these properties on a glyph-by-glyph basis). Marking the glyph to something else like 'Letter" re-enables the anchor propagation.

Another important difference, which is related to this point, is that: we (i.e. ufo2ft MarkFeatureWriter) always write pos ligature rules every time we see such top_1, bottom_2, etc. anchors; Glyphs.app, on the other hand, only generates mark2liga lookups for glyphs that are marked explicitly as subCategory "Ligature" (and, as I noted above, contains some numbered anchors starting from 1 and with no gaps).

Finally, there's the thing that in our current implementation the numbered suffix only gets added if there is more than one anchor with the same name being propagated from components to composite glyph. Now say a ligature glyph is made of multiple components but only one of them has a given anchor; in this case, this propagated anchor will not get the _1, _2, etc. suffixes because it does not clash with other components' anchors since it is the only one. But then the ligature composite glyph may end up with a mixture of ligature-ish anchors (suffixed with _1, _2, etc.) and other normal base anchors without the suffix. How should the MarkFeatureWriter treat this hybrid glyphs? Are they "ligature" (because they contains _1, _2, etc.) and so we add them to the mark2liga lookup and the GDEF Ligature class? Or are they "base" glyphs and we add them to the mark2base lookup? For the current implementation in ufo2ft MarkFeatureWriter, they are a bit of both: they are included in both the mark2base and the mark2liga lookups, referencing different markClasses. This looks kind of weird to me, that for some classes of marks the same glyph can be treated as either a ligature in the mark2liga lookup, or a single base glyph in the mark2base lookup. What about the GDEF GlyphClassDefinitions for this? Is it 1 (Base) or 2 (Ligature)? It can't be both.

I'm still not sure how I'm gonna fix these problems in the MarkFeatureWriter. However, as far as glyphsLib anchor propagation is concerned, I'm thinking that maybe we should disable it for composite glyphs that are classified as subCategory "Ligature" to better match Glyphs.app output.

@moyogo
Copy link
Collaborator

moyogo commented May 31, 2018

A couple of thoughts:

  1. Anchors propagation shouldn’t be done in glyphsLib but in ufo2ft’s anchor propagation filter instead. The rationale being that UFOs need their anchors propagated as well.
  2. Glyph data should be exported to UFOs and used in ufo2ft filters and feature writers.

@belluzj
Copy link
Collaborator

belluzj commented May 31, 2018

Another thing might be that depending on whether the script is RTL or LTR, the numbering _1, _2... should change direction as well? I'm not sure about that one and I don't know what Glyphs.app does actually.

@anthrotype
Copy link
Member Author

Anchors propagation shouldn’t be done in glyphsLib but in ufo2ft’s anchor propagation filter

I know. I opened the issue here because that ufo2ft filter was copied from glyphsLib originally, and in glyphsLib that feature is enabled by default whereas in ufo2ft is opt-in (and hasn't been released either I think).

depending on whether the script is RTL or LTR, the numbering _1, _2... should change direction as well?

hm, this is getting too complicated and error prone.

@anthrotype
Copy link
Member Author

anthrotype commented Jun 1, 2018

I made another discovery in Glyphs.app, by the way.
Not only, as I already noted above, component anchors do not propagate from non-ligature base glyphs to composite glyphs classified as subCategory "Ligature" (e.g., if "f" has anchor top, "f_f" will not get "top_1" and "top_2" automatically).

But also, if one manually places top_1, bottom_1, etc. anchors in a ligature glyph, and the latter glyph is used as component in another composite glyph, which also is classified as "Ligature", then the anchors are propagated. So for example, in Noto Sans Arabic, the "lam_alef-ar" glyph (a Ligature) contains the following anchors: top_1, top_2, bottom_1 and bottom_2. The composite glyph "lam_alef-ar.fina" (and other similar composite glyphs that include lam_alef-ar as component) will have the propagated anchors and thus will be included in the mark2liga_arabic lookup with its own pos ligature ... rule.

So, it appears that at least for the ligature glyphs anchor propagation is limited to glyphs that belong to the same subCategory.

It would be nice if @schriftgestalt could confirm this or add his comments to this thread.

The problem with this approach is that it'd be trickier to extend it for the generic UFO workflow. In glyphsLib we can look up the GlyphData.xml for a glyph's subCategory. In the ufo2ft filter, how do we know whether a glyph is a "Ligature". Parsing the glyph name for underscores is not a good idea. We might check for the presence of the private, glyphsLib-specific subCategory key in the GLIF lib. But I don't like that very much either.. 🤔

@moyogo
Copy link
Collaborator

moyogo commented Jun 1, 2018

Like I said:

  1. Glyph data should be exported to UFOs and used in ufo2ft filters and feature writers.

@anthrotype
Copy link
Member Author

the most important finding so far is that I could not reproduce anywhere in Glyphs.app the behaviour that is implemented here:

https://github.com/googlei18n/glyphsLib/blob/efe1ab169c9f5b197a6e76b9b21952bc93c4b5ac/Lib/glyphsLib/builder/anchors.py#L84-L88

that code adds a numbered prefix _1, _2 etc. to the propagated anchors whenever the components have more than 1 anchor with the same name.

This never happens in Glyphs.app to my knowledge. The _1, _2, etc. anchors have to be placed manually by the font designer. If they are there, and the glyph is a Ligature, the mark2liga will be generated. If that glyph is in turn used as component in another composite glyph also marked as Ligature, the latter composite glyph will inherit the same _1, _2. etc. anchors of its component. And if there is a name clash in this situation, similarly to what happens in the case of non-ligature composite glyphs, the last component in the list wins, overwriting propagated anchors from earlier components. There is never a numbered suffix added.

We need to revise this algorithm accordingly if we care about matching Glyphs.app behaviour.

@anthrotype
Copy link
Member Author

Glyph data should be exported to UFOs and used in ufo2ft filters and feature writers.

Sorry I don't understand what you're talking about.

@moyogo
Copy link
Collaborator

moyogo commented Jun 1, 2018

We should store the glyph’s subCategory in the GLIF lib.

@schriftgestalt
Copy link
Collaborator

I’m working on a big chunk of code and will comment here when I’m done.

@anthrotype
Copy link
Member Author

It's not just that we don't match Glyphs.app's output -- which for me would already be a good enough argument for changing this. But our current anchor propagation algorithm has serious problems on its own.

The mere fact that two components have anchors with the same name does not make the composite glyph a ligature, for which a mark2liga lookup is needed. It could well be that the composite glyph happens to be using two components that happens to have anchor with same name, but itself it is not used as a ligature. It may have its own unicode codepoint, or it may not have one but it is in turn used as a component is some other compound glyph, but never accessed as such nor via liga features.

Moreover, the anchor's numbered suffix is not simply the number of repetitions of that mark class in the ligature. It defines the index of the ligature component which the anchor attaches to. So to do it right, one would need to know, not only that a glyph is a ligature to begin with, but also how many "components" (in the GSUB or GDEF sense) this ligature is made of: i.e. the base characters one needs to type in order to trigger a ligature substitution, or how many times the caret splits the ligature glyph (+1). And then, one would need to map the component outlines (in the glyf sense) to these ligature components to properly number the propagated anchors with repeated names. And one should also take into account those ligature component that may have no anchors at all.

A stupid example (I couldn't come up with a better one, sorry): make a ligature glyph called "f_a_t", composed of three components, "f", "a", and "t"; the "f" and "t" glyphs have a "top" anchor; "a" has none (for whatever reason). The propagated anchors I expect to get in the "f_a_t" glyph are: top_1 (from "f"), and "top_3" (from "t"); and a "NULL_2" (or maybe just "_2"?) placed anywhere because its position is not important, only the name is, which indicates that the second component of the ligature, "a", does not have any anchor. The mark feature will then have:

pos ligature f_a_t <anchor 100 500> @MC_top
    ligComponent <anchor NULL>
    ligComponent <anchor 600 500> @MC_top;

@schriftgestalt
Copy link
Collaborator

This never happens in Glyphs.app to my knowledge. The _1, _2, etc. anchors have to be placed manually by the font designer.

That is right.

A stupid example

what about acute, there the a and the acutecomb have a top anchor?

There is one tricky thing with the ligature anchors. If you have a lam_alef with some mark attached as a precomposed glyph. The ligature pos feature should use the top anchor from the mark and not from the base lam_alef. That can be automated when the mark is directly aligned to the right anchor. That can set from the info box with the anchor symbol.

lam_alefhamzaabove-ar

@anthrotype anthrotype changed the title composites' anchor propagation and ligature glyphs issues with composites' anchor propagation Jun 18, 2018
@anthrotype
Copy link
Member Author

ufo2ft (since v2.0) has the propagateAnchors filters (copied from glyphsLib's). I guess we should now remove this functionality in glyphsLib and fix the ufo2ft's implementation to match the output of Glyphs.app.

I'm thinking we could use the table GDEF { GlyphClassDef ... } statement in the features.fea to define what is a ligature glyph, and exclude from the anchor propagation glyphs that don't belong to the same Ligature sub-category.

@madig
Copy link
Collaborator

madig commented Feb 10, 2019

I ran into this (I assume) while trying to port Cantarell over to using UFOs and a Designspace: https://gitlab.gnome.org/GNOME/cantarell-fonts/merge_requests/14

Basically, by default, non-letters like numero have their anchors propagated, which I do not want, while e.g. ocircumflextilde only has the anchors from the base o, while the circumflexcomb_tildecomb diacritic ligature used in the glyph has no anchors.

Should glyphsLib just write the filter with explicit includes to the UFO's lib key?

@madig
Copy link
Collaborator

madig commented Feb 10, 2019

Made a test case: Cantarell-Regular-anchor-propagation-test.ufo.zip

ocircumflextilde does not have the correct top anchor because anchors aren't propagated to circumflexcomb_tildecomb. Is this a known problem?

@anthrotype
Copy link
Member Author

@schriftgestalt Georg, when you have some spare time, could you please describe in some details (i.e. in a way that we could reliably replicate it in our pipeline) how automatic anchor propagation for composite glyphs is supposed to work in Glyphs.app?
This could also be relevant for the discussion on formalizing anchors definitions in UFO spec: unified-font-object/ufo-spec#32

@schriftgestalt
Copy link
Collaborator

schriftgestalt commented May 13, 2019

I’ll try:

  1. If it is a Mark and there are anchors, it will not look into components.
  2. Anchors are stored as a dict with the names as keys
  3. Then it recursively goes over all components and adds the anchos to a dict.
  4. In the end, it extends the dict with the anchors from that layer. So local anchors take precedence.
  • There is special treatment for components that have the anchor set. That can be something like top or top_1. If if is top_1, it will ignore the top_1 anchor and will rename the top to top_1. That sounds a bit strange but it must have made some sense on some point ;)
  • If a component is mirrored horizontally, all its anchors are renamed left > right and right > left (meaning topleft becomes topright). If the component is mirrored vertically, top and bottom are switched. So rotating it 180° means both replacements are done.
  • if it finds a _bottom anchor, it will remove top and _top anchors and the other way around. That is to safeguard when a subcomponent was shifted (a dotaccentcomb becoming dotbelowcomb)

@anthrotype
Copy link
Member Author

looks like @schriftgestalt will or has already added a new attribute for GSComponent that allows to disable anchor propagation for that specific glyph, see https://github.com/schriftgestalt/GlyphsDev/issues/91#issuecomment-1256887150
I haven't tried and don't know how exactly it works, but we should consider adding support for this when someone takes up this issue again

@cmyr
Copy link
Member

cmyr commented May 8, 2024

ran into an issue related to this just now in comparing the output of fontc and fontmake: the current anchor propagation implementation here is generating ligature-like anchors for glyphs like 'DZ', which are not ligatures. Concretely this means that fontmake thinks these glyphs are ligatures for the purposes of mark-to-base (because it sees that there are numbered anchors) but thinks they are not ligatures for the purposes of mark2lig (since they aren't in the ligature subcategory.)

fontc has the advantage that the anchor propagation there is based directly on source provided by Georg, whereas iirc the implementation here was more guesswork. The solution in any case is to just do 'last writer wins' in the case of duplicate names unless the propagation target is an explicit ligature glyph.

@schriftgestalt
Copy link
Collaborator

For none ligature glyphs, you should get the anchors from the last component they show up. I just tried the DZ in Glyphs. The D and Z have a "top" anchor so you get the one from the Z. The D has a center anchor (for the bar to make a "Eth"). As the Z doesn’t have a center have it, you get the one from the D.

@moyogo
Copy link
Collaborator

moyogo commented May 9, 2024

@schriftgestalt This is inconsistent with your logic:

  • Glyphs > Set Anchors does not propagate the anchors from the component glyphs, whether they are defined in GlyphsData.xml or not.
  • Glyphs > Reset Anchors does not either, and removes any present anchor if not defined in GlyphsData.xml.

Why is the anchor propagation copying anchors that neither of these functions add or preserve?

For example, from Set/Reset Anchors:

  • d gets top, topright@ascender, bottom, ogonek
  • z gets top, bottom
  • dz gets top.

After anchor propagation:

  • dz already has top and gets d's topright@ascender, ogonek and z's bottom. If it didn’t have top from Set Anchors/Reset Anchors it would have got z's top.

Besides the inconsistencies, one can wonder why dz should have those additional anchors at all or why where they end up.

@schriftgestalt
Copy link
Collaborator

Set Anchors is not checking the components. It "just" locks at the glyphData. What I was speaking about is the function Layer.anchorsTraversingComponents(). It is used when writing the mark feature for glyphs that do not have anchors themselves.

@moyogo
Copy link
Collaborator

moyogo commented May 9, 2024

@schriftgestalt I am well aware.

@schriftgestalt
Copy link
Collaborator

I’m sorry. I didn't read you post.

For example, from Set/Reset Anchors:
d gets top, topright@ascender, bottom, ogonek

That the @ascender was not supposed to show up in the anchor name. And it is fixed already in the latest version.

Besides the inconsistencies, one can wonder why dz should have those additional anchors at all or why where they end up.

SetAnchor and traverseAnchors are meant for different circumstances and can’t be applied interchangeably. In the DZ case, I would expect that the user would add a "top" anchor if needed at all.

anthrotype added a commit that referenced this issue Jul 30, 2024
… 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.
anthrotype added a commit that referenced this issue Jul 30, 2024
…onent wins

#368 (comment)

test borrowed from #1007 (which this PR will supersede)
anthrotype added a commit that referenced this issue Jul 30, 2024
… 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.
anthrotype added a commit that referenced this issue Jul 30, 2024
…onent wins

#368 (comment)

test borrowed from #1007 (which this PR will supersede)
anthrotype added a commit that referenced this issue Aug 1, 2024
…onent wins

#368 (comment)

test borrowed from #1007 (which this PR will supersede)
schriftgestalt pushed a commit that referenced this issue Oct 23, 2024
… 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.
schriftgestalt pushed a commit that referenced this issue Oct 23, 2024
…onent wins

#368 (comment)

test borrowed from #1007 (which this PR will supersede)
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 a pull request may close this issue.

6 participants