googlefonts / glyphsLib

A bridge from Glyphs source files (.glyphs) to UFOs
Apache License 2.0
178 stars 51 forks source link

Anchors propagation for Arabic mark ligatures #931

Open yanone opened 11 months ago

yanone commented 11 months ago

I'm not sure whether this is a bug, a regression, or was never implemented. But I could swear this used to work (I've tried as far back as release 6.2.1):

Imagine a mark ligature fathatan-ar that's composed of two stacking fatha-ar components. Glyphs correctly propagates their anchors when decomposing, _top being the lower one, and top being on top of the upper one.

But glyphsLib doesn't propagate and save any anchors for them in the UFO. FWIW, all marks are manually defined as non-spacing marks just to make sure.

I thought this used to work but I could be wrong. I could decompose the ligatures in the sources, but I'd rather not and keep them as composites for future authoring.

Is this something that can or should be addressed?

Bildschirmfoto 2023-07-17 um 21 53 14 Bildschirmfoto 2023-07-17 um 21 53 20

anthrotype commented 5 months ago

@yanone What do you mean by a "ligature mark"? I think that "fathatan-ar" glyph is not a "ligature" but a non-spacing combining mark. Not sure a glyph can be both. I presume you simply mean a composite glyph containing a bunch of mark components.

We have some code to handle composite glyphs that only comprise other mark components (i.e. whose base glyph is in turn a non-spacing mark because it contains some "_" prefixed mark anchor). But right now we only process these so-called "ligature marks" if their gyph name contains some "_" underscore (is this where the "ligature"-ness comes from?) e.g. "shadda_fatha-ar" in the tests/data/AnchorPropagation.glyphs file that I believe Yanone added.

I think if we just remove that _is_ligature_mark altogether, you get what you are after and your "fathatan-ar" glyph will inherit the anchors from its components (despite not having underscores in its name).

diff --git a/Lib/glyphsLib/builder/anchor_propagation.py b/Lib/glyphsLib/builder/anchor_propagation.py
index e3bf62a6..54b0f1da 100644
--- a/Lib/glyphsLib/builder/anchor_propagation.py
+++ b/Lib/glyphsLib/builder/anchor_propagation.py
@@ -40,7 +40,7 @@ def _propagate_glyph_anchors(self, ufo, parent, processed):
                 base_components.append(component)
                 anchor_names |= {a.name for a in glyph.anchors}

-    if mark_components and not base_components and _is_ligature_mark(parent):
+    if mark_components and not base_components:
         # The composite is a mark that is composed of other marks (E.g.
         # "circumflexcomb_tildecomb"). Promote the mark that is positioned closest
         # to the origin to a base.
@@ -131,10 +131,6 @@ def _adjust_anchors(anchor_data, ufo, parent, component):
             anchor_data[anchor.name] = t.transformPoint((anchor.x, anchor.y))

-def _is_ligature_mark(glyph):
-    return not glyph.name.startswith("_") and "_" in glyph.name
-
-
 def _component_closest_to_origin(components, glyph_set):
     """Return the component whose (xmin, ymin) bounds are closest to origin.
yanone commented 5 months ago

I presume you simply mean a composite glyph containing a bunch of mark components.

Yes. At the top of this issue, I described: "Imagine a mark ligature fathatan-ar that's composed of two stacking fatha-ar components."

I think if we just remove that _is_ligature_mark altogether, you get what you are after and your "fathatan-ar" glyph will inherit the anchors from its components (despite not having underscores in its name).

Since I already created a test for the anchor positions, you're free to change the code as you please.

anthrotype commented 5 months ago

free to change the code as you please

yeah, I was just asking if the proposed change looks correct to you. Right now we consider "ligature marks" (we should really find a better term) only those glyphs that are named like an actual ligature ("f_f_i" is a ligature in the strict sense, as substituted via GSUB, "fathatan-ar" is not quite a ligature in the same sense, it actually has its own codepoint). In the patch above, I simply got rid of the check _is_ligature_mark and simply consider as such any composite glyph that is only made of other mark components.

anthrotype commented 5 months ago

I simply got rid of the check _is_ligature_mark and simply consider as such any composite glyph that is only made of other mark components

but the composite glyph could potentially also be 'mixed' and contain both components and contours... This situation can actually occur with the current code as well, where we consider "ligature mark" any glyph that has a glyph name containing underscores (and consquently treat the mark component closest to the origin as a "base" component thus propagating all its anchors including any _-prefixed mark anchors that we normally do not propagate). A mixed composite glyph could have only mark components plus other decomposed outlines and we might inadvertently treat it as a "ligature mark" whereas in fact is not even a mark.. Maybe I also need to check if the composite glyph is pure and only has (mark) components and no contours, in which case it may be safe to consider it a "ligature mark".

madig commented 4 months ago

I don't quite understand, are you asking how to make anchors propagate during font compilation?

I used a lib key in Cantarell, with an explicit list of glyphs to handle, because otherwise you get anchors propagated in symbols that use letters as components (e.g. dollar): https://gitlab.gnome.org/GNOME/cantarell-fonts/-/blob/61aed64a426c4db861b21e4af0705d896d359d0d/src/Cantarell-Regular.ufo/lib.plist#L5-835