Allow multiple anchor classes per mark glyph in the mark feature #416

belluzj · 2020-10-30T17:19:19Z

Hello, this PR aims to fix #303

While the tests should be green, I'm not sure that I'm not introducing regressions for people who rely on very precise implementation of the Glyphs.app "tricks" with ordering _bottom and _top before others. With the new version of the code, all mark anchors are output, and if there are conflicts the last one will win, not necessarily the one Glyphs.app was picking. For example, if /a and /acutecomb can be attached through both a top/_top and an accent/_accent mark pairs, before it would only output top/_top, now it will output both in different lookups, and probably the last lookup will win (or who knows what will happen actually).

Also, @anthrotype you said you had a patch at some point using the classifier, but I haven't been able to see how to apply the classifier to the problem. I'm using a graph coloring instead. Does that sound OK? How would that be solved instead using the classifier?

Thanks in advance for some reviews!

madig · 2020-11-02T09:24:59Z

(Side-note: if this can potentially break files made with Glyphs' expectations, maybe we need an "anchor strategy" lib key?)

google-cla · 2020-11-02T10:45:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

belluzj · 2020-11-02T10:47:40Z

@googlebot I fixed it.

anthrotype

Thanks Jany, I like how you framed this as a graph colouring problem.
I left some comments.

Lib/ufo2ft/featureWriters/markFeatureWriter.py

moyogo · 2020-11-03T21:10:34Z

By the way, technically at the OpenType level, these could be in a single lookup, they just need to be in different subtables. Having to have them in different lookups is an AFDKO restriction: adobe-type-tools/afdko#106 (comment)

belluzj · 2020-11-04T12:05:17Z

Here is my investigation to confirm @khaledhosny's explanation of how the two conflicting lookups would work. I added the test UFO in commit 03feba7, compiled it with fontmake using this work-in-progress version of ufo2ft to get the two conflicting lookups, took a few screenshots, then swapped the two lookups in TTX, and tested again. See results below:

$ hb-shape --shapers=ot master_ttf/MultipleAnchorClassesConflict.ttf -u 00E6,0301
[ae=0+973|acutecomb=0@-141,-10+0]

$ hb-shape --shapers=ot master_ttf/MultipleAnchorClassesConflict#1.ttf -u 00E6,0301
[ae=0+973|acutecomb=0@-530,-11+0]

tests/featureWriters/markFeatureWriter_test.py

anthrotype · 2020-11-04T17:55:59Z

technically at the OpenType level, these could be in a single lookup, they just need to be in different subtables

That's a good point. Can we see if we can instead use the subtable statement to force a break-up instead of having to define separate lookups? I think if we did that, then I believe the first subtable that matches would win.

moyogo · 2020-11-04T19:53:36Z

technically at the OpenType level, these could be in a single lookup, they just need to be in different subtables

That's a good point. Can we see if we can instead use the subtable statement to force a break-up instead of having to define separate lookups? I think if we did that, then I believe the first subtable that matches would win.

We would need to update the OpenType Feature File Specification. I think feaLib will raise an error at this point.

khaledhosny · 2020-11-11T22:38:06Z

Just confirming that I built the font from #303 with this PR and my original issue is now fixed, thanks @belluzj!

belluzj · 2020-11-13T15:57:25Z

I think I'm done now. If that's OK I'd rather keep generating several lookups for the time being rather than trying to implement subtable breaks in feaLib etc.

khaledhosny · 2020-11-13T16:27:09Z

I think I'm done now. If that's OK I'd rather keep generating several lookups for the time being rather than trying to implement subtable breaks in feaLib etc.

feaLib supports subtable breaks almost everywhere.

belluzj · 2020-11-13T16:32:04Z

Ah ok, I didn't try anything because Denis was suggesting otherwise in his comment here: #416 (comment)

What do you think? Would this be better as subtables instead of lookups? (provided the subtables work out of the box)

belluzj · 2020-11-13T17:32:13Z

I just pushed a version that uses subtable breaks instead of multiple lookups. @khaledhosny could you please check whether it still solves your issue on your font from #303?

khaledhosny · 2020-11-14T20:53:03Z

I just pushed a version that uses subtable breaks instead of multiple lookups. @khaledhosny could you please check whether it still solves your issue on your font from #303?

I get actually an error, apparently this is one of the few remaining places where feaLib does not support explicit subtable break!

I get also the new warning for too many glyphs. It seems to be heavy-handed. It might look ambiguous but it is intentional. May be an INFO would be better?

belluzj · 2020-11-16T11:17:43Z

I reverted the subtable commit, so that it's still in the history in case someone wants to look into supporting that.

I get also the new warning for too many glyphs. It seems to be heavy-handed. It might look ambiguous but it is intentional. May be an INFO would be better?

I looked into your warnings, they're mostly due to the madda-ar being able to attach to both hamzaAbove and markAbove. It seems to me that the warning is warranted, unless there's something I don't understand. For example, in the situation below, where do you expect the madda to go? Is the "correct"/intended madda position made obvious by another rule that I'm not aware of?

This is what I get from compiling the font and typing ىٓ:

But it could also have been this:

khaledhosny · 2020-11-16T12:43:15Z

I can work around it, but I also don’t see why it warrants a warning. ufo2ft is generating perfectly valid feature code that builds perfectly valid GPOS lookups. There may or may not be legitimate reasons for having such duplication and a warning is too much IMO (it comes mostly from the very limited way both UFO and Glyphs handle anchors, giving the designer limited control).

belluzj · 2020-11-16T13:08:31Z

My point of view here is that ufo2ft doesn't know what you mean (maybe because the UFO format doesn't have enough information, but really whatever the reason is), so it will do something random; and even if the result is valid fea/GPOS, it's still "undefined behaviour" in a way, and I think that's not a good situation in general and is noteworthy enough for a warning.

I also think that the ambiguous situation can happen for two reasons:

as in your case, you know what you're doing and you want to keep the situation as is even though ufo2ft can't promise to put the mark in what you consider the "right" place;
in other potential situations, the designers would introduce this kind of ambiguity by mistake and would like to be told if they're feeding ufo2ft with unclear information, and then would want to fix the situation to make sure their marks go in the same place reliably.

I imagine that situation number 2. would be the most common one and I'd like to take care of it in priority. In that situation the ambiguity is a mistake and ufo2ft is expected to warn (loudly enough) that it's going to have to take random decisions.

Does that make sense? @anthrotype @moyogo what do you think?

I'm happy to put the warning as info if my understanding of the situation is incorrect

khaledhosny · 2020-11-16T18:55:00Z

That is why I’m in favor in alphabetic sort of the lookups, this way if the order is not what I want I can carefully rename the anchors. Little control, but better than no control at all.

I’d keep warnings when something is wrong but ufo2ft still can produce some output (otherwise it would be an error), which is not the case here.

anthrotype · 2020-11-16T19:04:19Z

I get actually an error, apparently this is one of the few remaining places where feaLib does not support explicit subtable break!

can you please open an issue in fonttools upstream about this so we don't forget? thanks

anthrotype · 2020-11-16T19:05:38Z

Lib/ufo2ft/featureWriters/markFeatureWriter.py

@@ -239,8 +339,7 @@ class MarkFeatureWriter(BaseFeatureWriter):

    # Glyphs moves "_bottom" and "_top" (if present) to the top of
    # the list and then picks the first to use in the mark feature.
-    # https://github.com/googlei18n/noto-source/issues/122
-    # #issuecomment-403952188
+    # https://github.com/googlei18n/noto-source/issues/122#issuecomment-403952188
    anchorSortKey = {"_bottom": -2, "_top": -1}


this anchorSortKey doesn't seem to be used any more.

what's the order now? You said that the last lookup wins. Should we not place _bottom and _top last so that they win and we don't change existing fonts that may rely on this "feature"? Maybe we can sort alphabetically (like khaled is suggesting) but do an exception for those two.

I implemented what you suggested above.

anthrotype · 2020-11-16T19:14:43Z

I'm ok to demote the logging message to an INFO level and simply inform about the potential ambiguity. We can document that the multiple markPos lookups will follow a predefined order (alphabetic + special case for legacy bottom/top anchors).

belluzj · 2020-11-18T09:22:12Z

I demoted the warning to INFO.

Lib/ufo2ft/featureWriters/markFeatureWriter.py

anthrotype · 2020-11-18T15:06:45Z

Lib/ufo2ft/featureWriters/markFeatureWriter.py

@@ -59,6 +78,26 @@ class MarkToLigaPos(AbstractMarkPos):

    Statement = ast.MarkLigPosStatement

+    def warnIfAmbiguous(self, log):


the two warnIfAmbiguous methods look almost identical, they could be probably factored out and reused

ae can receive the acute either on top of the a or of the e

…ill generated

This reverts commit 4f83b71.

anthrotype

Thank you Jany for working on this! LGTM

belluzj force-pushed the fix-multiple-anchor-classes branch from ff99274 to afd5c16 Compare October 30, 2020 18:08

belluzj requested review from anthrotype and madig October 30, 2020 18:09

belluzj force-pushed the fix-multiple-anchor-classes branch from f7fba58 to afd5c16 Compare November 2, 2020 10:47

anthrotype reviewed Nov 2, 2020

View reviewed changes

Lib/ufo2ft/featureWriters/markFeatureWriter.py Outdated Show resolved Hide resolved

Lib/ufo2ft/featureWriters/markFeatureWriter.py Outdated Show resolved Hide resolved

Lib/ufo2ft/featureWriters/markFeatureWriter.py Outdated Show resolved Hide resolved

belluzj force-pushed the fix-multiple-anchor-classes branch from f03d7c0 to a87a71a Compare November 3, 2020 17:29

belluzj commented Nov 4, 2020

View reviewed changes

tests/featureWriters/markFeatureWriter_test.py Show resolved Hide resolved

khaledhosny added a commit to aliftype/reem-kufi that referenced this pull request Nov 11, 2020

Use googlefonts/ufo2ft#416

4a47ac8

belluzj force-pushed the fix-multiple-anchor-classes branch from 1296be4 to 905e6e3 Compare November 13, 2020 15:55

anthrotype reviewed Nov 16, 2020

View reviewed changes

belluzj force-pushed the fix-multiple-anchor-classes branch from 0f3f463 to 462c3bc Compare November 18, 2020 09:25

anthrotype reviewed Nov 18, 2020

View reviewed changes

Lib/ufo2ft/featureWriters/markFeatureWriter.py Outdated Show resolved Hide resolved

anthrotype reviewed Nov 18, 2020

View reviewed changes

belluzj added 11 commits November 20, 2020 11:08

Allow multiple anchor classes per mark glyph in the mark feature

59bcabf

Remove Python 3.8+ idiom

a2c3dc2

Use itertools.combinations

f7f9aea

Add test for mark2liga, for review

b8427b0

Add test UFO with conflict in the marks

26476e4

ae can receive the acute either on top of the a or of the e

Also group mark2liga lookups

1beeefa

Add a warning for ambiguous base/mark combinations; ensure code is st…

aa1e040

…ill generated

Use subtable statements instead of different lookups

a093f62

Revert "Use subtable statements instead of different lookups"

dc16f20

This reverts commit 4f83b71.

Fix ordering of mark classes in case of conflict

134eacf

Demote the "ambiguous base-mark pair" warning to info

cb93d6c

belluzj force-pushed the fix-multiple-anchor-classes branch from 1da46ed to ac65ac5 Compare November 20, 2020 11:09

belluzj removed the request for review from madig November 20, 2020 11:10

belluzj force-pushed the fix-multiple-anchor-classes branch from ac65ac5 to ba54daa Compare November 20, 2020 14:28

Improve log message, refactor code

c0681c0

belluzj force-pushed the fix-multiple-anchor-classes branch from ba54daa to c0681c0 Compare November 20, 2020 14:31

anthrotype approved these changes Nov 20, 2020

View reviewed changes

belluzj merged commit 17b346b into googlefonts:master Nov 20, 2020

khaledhosny mentioned this pull request Jan 25, 2021

Diacritics overriding unicode harfbuzz/harfbuzz#2832

Closed

belluzj mentioned this pull request Dec 6, 2021

Make ambiguous mark attachment a WARNING instead of INFO #563

Open

belluzj mentioned this pull request Jul 4, 2023

markFeatureWriter mark class grouping is problematic #762

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple anchor classes per mark glyph in the mark feature #416

Allow multiple anchor classes per mark glyph in the mark feature #416

belluzj commented Oct 30, 2020

madig commented Nov 2, 2020

google-cla bot commented Nov 2, 2020

belluzj commented Nov 2, 2020

anthrotype left a comment

moyogo commented Nov 3, 2020

belluzj commented Nov 4, 2020

anthrotype commented Nov 4, 2020

moyogo commented Nov 4, 2020

khaledhosny commented Nov 11, 2020

belluzj commented Nov 13, 2020

khaledhosny commented Nov 13, 2020

belluzj commented Nov 13, 2020

belluzj commented Nov 13, 2020

khaledhosny commented Nov 14, 2020

belluzj commented Nov 16, 2020

khaledhosny commented Nov 16, 2020

belluzj commented Nov 16, 2020

khaledhosny commented Nov 16, 2020

anthrotype commented Nov 16, 2020

anthrotype Nov 16, 2020

anthrotype Nov 16, 2020 •

edited

belluzj Nov 18, 2020

anthrotype commented Nov 16, 2020

belluzj commented Nov 18, 2020

anthrotype Nov 18, 2020

belluzj Nov 19, 2020

anthrotype left a comment

		@@ -59,6 +78,26 @@ class MarkToLigaPos(AbstractMarkPos):

		Statement = ast.MarkLigPosStatement

		def warnIfAmbiguous(self, log):

Allow multiple anchor classes per mark glyph in the mark feature #416

Allow multiple anchor classes per mark glyph in the mark feature #416

Conversation

belluzj commented Oct 30, 2020

madig commented Nov 2, 2020

google-cla bot commented Nov 2, 2020

belluzj commented Nov 2, 2020

anthrotype left a comment

Choose a reason for hiding this comment

moyogo commented Nov 3, 2020

belluzj commented Nov 4, 2020

anthrotype commented Nov 4, 2020

moyogo commented Nov 4, 2020

khaledhosny commented Nov 11, 2020

belluzj commented Nov 13, 2020

khaledhosny commented Nov 13, 2020

belluzj commented Nov 13, 2020

belluzj commented Nov 13, 2020

khaledhosny commented Nov 14, 2020

belluzj commented Nov 16, 2020

khaledhosny commented Nov 16, 2020

belluzj commented Nov 16, 2020

khaledhosny commented Nov 16, 2020

anthrotype commented Nov 16, 2020

anthrotype Nov 16, 2020

Choose a reason for hiding this comment

anthrotype Nov 16, 2020 • edited

Choose a reason for hiding this comment

belluzj Nov 18, 2020

Choose a reason for hiding this comment

anthrotype commented Nov 16, 2020

belluzj commented Nov 18, 2020

anthrotype Nov 18, 2020

Choose a reason for hiding this comment

belluzj Nov 19, 2020

Choose a reason for hiding this comment

anthrotype left a comment

Choose a reason for hiding this comment

anthrotype Nov 16, 2020 •

edited