googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

markFeatureWriter mark class grouping is problematic #762

Closed khaledhosny closed 1 day ago

khaledhosny commented 1 year ago

I was debugging a rather odd InDesign bug, where all my mark positioning gets lost, and I tracked it down to: https://github.com/googlefonts/ufo2ft/blob/7807ef54076238fbe916905bca64a5830566c865/Lib/ufo2ft/featureWriters/markFeatureWriter.py#L456-L501

Usually I’d brush this as an InDesign bug and move on, but I noticed something with HarfBuzz. Try with this version of the font with mark class grouping:

$ hb-view Amiri-Regular.ttf "لَكم محَبر"

before

and now with this version without grouping: after

The later output is the correct/expected one.

The file without grouping is smaller as well despite having more number of lookups (I noticed this with other fonts too), so I’m not sure what is the benefit of this mark class grouping over doing the simple thing?

belluzj commented 1 year ago

From what I remember, the point of grouping was to have fewer lookups because each lookup = one more loop over the input string to apply the lookup, if I understand correctly (so it was more a performance consideration than file size). That being said, I don't know how much the performance penalty would be, and whether some people would be ready to trade it for smaller file size, especially if the gains are not negligible.

I remember from discussing this issue some time ago that you used to use "ambiguous" anchor attachments, as per your comment here: https://github.com/googlefonts/ufo2ft/pull/416#issuecomment-728257591 ; could this be such a case? Do you get the info message about ambiguous stuff?

khaledhosny commented 1 year ago

Yes, I get messages about ambiguous anchors, but I get that with pretty much all of my fonts that I build with fontmake.

khaledhosny commented 1 year ago

I’m using ufo2ft directly here, though, and I have some hacky code to force a particular lookup order. This probably gets broken when some anchors are merged together in the same lookup (This font has a long history, and for most of its life it was developed using FontForge where I had complete control on how the lookups are built and ordered, which I switched to UFO/ufo2ft I had to hack my way to get a matching output).

belluzj commented 1 year ago

Maybe you could use your own copy of the mark feature writer in which you disable grouping?

anthrotype commented 1 year ago

maybe grouping should be smarter and avoid grouping ambiguous attachements

khaledhosny commented 1 year ago

I’m essentially using my own mark feature writer, it is just this is second time I get issues with this grouping logic (the other one was in a certain secret project, and I ended up forking the mark feature writer there as well, but I don’t remember the exact details of the issue).

I’m also not sure the number of lookups can be that a performance issue, it is not like you are going to end up with thousands of them. This font has too many anchor classes than most typical projects, and I got only 8 more lookups (that is out of 62 GPOS lookups).

khaledhosny commented 1 day ago

This should be fixed now.