search

googlefonts / glyphsLib

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

Clarification of logic in `_build_public_opentype_categories` #1024

Open cmyr opened 2 months ago

cmyr commented 2 months ago

Investigating another minor difference between fontc and fontmake. When assigning the 'ligature' class, the stated logic involves two checks:

However the logic for deciding if something is an attaching anchor is very simple and considers any anchor that does not start with an underscore to be an "attaching anchor": https://github.com/googlefonts/glyphsLib/blob/e2ebf5b517d59bec0c9437da3a748c58f2999911/Lib/glyphsLib/builder/features.py#L241-L245

This has the concrete consequence that caret anchors ('caret_1', etc) are considered 'attaching anchors'. As these anchors are common in ligature glyphs, this happens to mean that various ligature glyphs will end up in the ligature category, even if they do not have any attaching marks.

Is this behaviour desired? If it is, then I think this can be addressed with a comment, or by renaming the has_attaching_anchors variable. If it is not desired, then the logic should be reworked somewhat.

anthrotype commented 2 months ago

caret anchors ('caret_1', etc) are considered 'attaching anchors'.

yeah I noticed that too, the logic is arguably too liberal. On the other hand, the fact that glyphs which contain caret_ sort of anchors (and which have subcategory 'Ligature') are classified in GDEF as ligatures is not that bad.. It certainly would not be un-desirable.

khaledhosny commented 2 months ago

It should be enough to consider a glyph a ligature when it has a Ligature subcategory, why do we check for anchors at all?

anthrotype commented 2 months ago

we could even go further and ignore the attaching-anchor check, and just take any glyph whose subCateogory is 'Ligature' to be in a ligature class for GDEF.

honestly I'm not entirely sure why the code currently restricts it to only ligatures with (non-underscore-prefixed) anchors; the GDEF ligature class can potentially be of use e.g. setting lookupflag IgnoreLigatures independently of them having anchors or not

anthrotype commented 2 months ago

ofc khaled beat me to it

khaledhosny commented 2 months ago

https://github.com/googlefonts/glyphsLib/blob/e2ebf5b517d59bec0c9437da3a748c58f2999911/Lib/glyphsLib/builder/features.py#L206-L228

Someone needs to re-read the linked old discussions, though I feel the check for the anchors is superfluous, or at least should be a fallback when no explicit subcategory is set.

cmyr commented 2 months ago

okay so for the time being I am going to match the implementation here, and treat any anchor that doesn't begin with '_' as 'an attaching anchor'.