Add GDEF feature writer #480

moyogo · 2021-03-05T11:19:45Z

Fixes #456.
This adds a GDEF feature writer to the default feature writers (kern feature writer, mark feature writer).

The GdefFeatureWriter first compiles the GDEF from the feature code (feaLib uses the GDEF table definition or builds it from lookups) and uses it to get Glyph Class Definitions (bases, ligatures, marks, components) and Ligature Carets.
It then updates the Glyph Class Definitions with the UFO lib "public.openTypeCategories" data, except for glyphs already defined, and updates the Ligature Carets with the UFO anchors with names starting with "caret_" and "vcaret_", except for glyphs with ligature carets already defined.

khaledhosny · 2021-03-05T15:37:50Z

Lib/ufo2ft/featureWriters/gdefFeatureWriter.py

+    definition if present or from the lookups.
+    Then it updates the GDEF with the public.openTypeCategories and the
+    ligature anchors for glyphs that are not already in GlyphClassDefs or
+    do not have ligature carets.


Is there a way to say this glyph has no GDEF class but please don’t auto-generate one, would setting public.openTypeCategories to an empty string signal that?

No, public.openTypeCategories values can only be base, ligature, mark, component.

If no GDEF is defined in the feature code and the glyph is not in public.openTypeCategories, it will only be in a GDEF Glyph Class if a lookup type makes it so.

https://unifiedfontobject.org/versions/ufo3/lib.plist/#publicopentypecategories

Values must be one of base, mark, ligature or component.

So you can only leave it out, but then the writer would assume auto :o

but then the writer would assume auto :o

Which is exactly what I’m trying to avoid. I’m converting fonts from SFD where you can leave out glyph class while opting out from automatic behavior. I currently also generate a GDEF table, but I was checking if I can skip that bit now.

Could you describe how glyphs end up in GDEF Glyphs Class when you don't want to? What's the actual use case?

I have no specific use case, if the SFD says this glyph has no GDEF class and don’t guess one, I want to maintain that. Say I have a place holder glyph that gets decomposed in ccmp feature but uses components, it might get anchors propagated to it and classified as base glyph, but GDEF class does not matter for it and it would save a few bytes to not et a GDEF glyph class to it (and if you have a few hundreds of these glyphs it might add up).

moyogo · 2021-03-06T14:29:11Z

I have no specific use case, if the SFD says this glyph has no GDEF class and don’t guess one, I want to maintain that. Say I have a place holder glyph that gets decomposed in ccmp feature but uses components, it might get anchors propagated to it and classified as base glyph, but GDEF class does not matter for it and it would save a few bytes to not et a GDEF glyph class to it (and if you have a few hundreds of these glyphs it might add up).

OK, I see. It would be nice to be able to indicate a glyph should be assigned the GDEF Glyph Class 0. That could also be used by the mark feature writer to avoid using the anchors of such a glyph.
Values could then be unassigned for class 0, or base, ligature, mark, component for class 1, 2, 3, 4.

When a glyph does not have a value assigned, its default would be class 0 in the GDEF or any of the other values depending on anchor propagation and lookups it ends up in.

Let's open a PR for the UFO spec.

moyogo · 2021-03-06T14:47:23Z

@khaledhosny Actually, would assigning the component glyph class not have the same effect?

moyogo · 2021-03-06T14:58:38Z

@khaledhosny Actually, would assigning the component glyph class not have the same effect?

Nevermind, of course it doesn't.

anthrotype · 2021-03-09T11:36:01Z

first compiles the GDEF from the feature code ... then updates the Glyph Class Definitions with the UFO lib "public.openTypeCategories", except for glyphs already defined

I'm not 100% convinced this is the best approach. I think if one opts in to using this new openTypeCategories, those should be used as is (whether they are correct or not), instead of only when a class definition can't be inferred automatically by feaLib. Garbage in, garbage out.
Having to compile the whole of GPOS/GSUB when no explicit GDEF table definition is present in features.fea (which is all the time basically) is too costly in my opinion, just for the sake of obtaining the feaLib-inferred GDEF glyph class definitions -- and then throwing the resulting tables away (just to be recompiled later on in the build process).

anthrotype · 2021-03-09T11:40:06Z

when a user provides an explicit GDEF table definition in features.fea containing GlyphClassDefs statements, both ufo2ft and feaLib currently take that at face value, they don't question whether those are correct or complete. Similarly I would expect from this new public.openTypeCategories.
Either the compiler guesses when no explicit data from the user is present, or it's the responsibility of the font developer to make sure the classifications provided are correct and complete. A middle ground only makes things more confusing.

moyogo · 2021-03-09T12:07:13Z

The difference between GDEF in feature code and public.openTypeCategories is that GDEF in feature code is final, when it is defined in the feature code all the glyphs have a GDEF glyph class assigned, whereas public.openTypeCategories may be incomplete and only some glyphs have a glyph class defined while others don't.

If no GDEF is defined in the feature code, public.openTypeCategories should be used by the feature writers to exclude the glyphs that have public.openTypeCategories values that would create a conflict (for example don't include bases as marks in the mark feature).

If we don't want to spend cycles building a GDEF to get the glyph classes and then build it again (which we do for the GSUB, but that's fine for some reason), then the GDEF feature writer should be called after the features have been compiled and should be a GDEF table post processor.

moyogo · 2021-03-09T12:11:50Z

We can take the public.openTypeCategories that are defined instead of the glyph class inferred by the features, but that glyph class of the glyphs that don't have any public.openTypeCategories defined still need to come from somewhere.

anthrotype · 2021-03-09T12:21:06Z

public.openTypeCategories may be incomplete

that's what I am not conviced about. Why?

moyogo · 2021-03-09T12:22:47Z

that's what I am not conviced about. Why?

https://unifiedfontobject.org/versions/ufo3/lib.plist/#publicopentypecategories

The dictionary may contain glyph names that are not in the font. The dictionary may not contain a key, value pair for all glyphs in the font.

anthrotype · 2021-03-09T12:23:46Z

glyph names that are not in the font

we'd just skip those

dictionary may not contain a key, value pair for all glyphs in the font.

so what? if we don't have the data for specific glyphs, we do nothing for those

moyogo · 2021-03-09T12:24:57Z

we do nothing for those

What does that mean?

anthrotype · 2021-03-09T12:25:38Z

I mean, if a glyph has no public.openTypeCategory we don't add it to the GDEF

moyogo · 2021-03-09T12:26:27Z

I mean, if a glyph has no public.openTypeCategory we don't add it to the GDEF

Then we do something, we add to the GDEF glyph class 0. That's what that means.

anthrotype · 2021-03-09T12:26:59Z

I guess so, yeah.

moyogo · 2021-03-09T12:34:49Z

There are two issues I think:

What to do when the GDEF is in the feature code and there are public.openTypeCategories values.
We either take the GDEF only, take the GDEF and augment or update it with public.openTypeCategories values, or only take the public.openTypeCategories values to write the GDEF. Feature writers should follow the same logic.
What to do when a glyph is used as a base, ligature or mark in a feature when it doesn't have any public.openTypeCategories value defined, or when the public.openTypeCategories value contradicts how the glyph is used (which shouldn't happen in theory with feature writers).

anthrotype · 2021-03-09T12:39:46Z

when the GDEF is in the feature code and there are public.openTypeCategories

the default behavior would be "skip" like the other writers. If a GDEF GlyphClassDefs are already present they would take priority

when a glyph is used as ... when it doesn't have any public.openTypeCategories, or when the value contradicts how the glyph is used

then the compiler would not be at fault, but source data that's incorrect.

moyogo · 2021-03-09T13:02:02Z

#456 (comment)

another approach could be that you teach the mark feature writer about these new public.openTypeCategory key, so that they have the same effect of an explicit GDEF table override in the features.fea.

Then you only add a LigatureCaretsFeatureWriter or something to handle the carets only

Maybe this approach would work better then. At least for me and your past self.

If a GDEF is not defined in the feature code, only use the public.openTypeCategories values in feature writers and let feaLib build the GDEF.
Then update the ligature carets based on anchors with a post processor.

anthrotype · 2021-03-09T13:10:47Z

that could work. But I feel a bit uneasy that we'd ignore a user's explicit settings (in the form of public.openTypeCategories) when building the GDEF table itself.
Can we think of a legitimate case when one would want to override the feaLib-generated GDEF? I.e. omit some classifications or add some that won't be automatically inferred?

Also this may be relevant: fonttools/fonttools#2210

moyogo · 2021-03-09T13:23:28Z

Can we think of a legitimate case when one would want to override the feaLib-generated GDEF? I.e. omit some classifications or add some that won't be automatically inferred?

@khaledhosny assigning /space to the mark glyph class so it can be ignored in lookups with the ignore marks flag, for a style of Arabic script with contextual features happening between words separated by zero-width spaces.
But more generally some mark glyphs may be positioned correctly without needing anchors and mark feature lookups, while still needing to be ignored by lookups with ignore marks flag.

khaledhosny · 2021-03-09T13:30:44Z

Here is what I think would cause the least surprises and would give users the control they need:

If the font has no public.openTypeCategory, do what ufo2ft is currently doing (i.e. before this PR):
If the font has public.openTypeCategory, use these categories in feature writers and write a GDEF table based on them, no guessing at all.

This has the only downside of not allowing the user to explicitly define some categories and let the compiler guess the rest. Although I recognize this as legitimate use case (a one that I used in some fonts), in reality public.openTypeCategory is likely to come from glyphsLib and similar that know how to fill the data for most glyphs anyway and their guess is as good as feaLib’s.

anthrotype · 2021-03-09T13:33:42Z

I agree with least surprises and no guessing.

If we cared to support what Denis proposes, we could add an additional mode whereby the GdefFeatureWriter would infer GDEF classes (that were not explicitly defined via public.openTypeCategories) from the current GPOS/GSUB. But I would make that not the default.

moyogo · 2021-03-09T13:53:37Z

@khaledhosny See unified-font-object/ufo-spec#171

If we cared to support what Denis proposes, we could add an additional mode whereby the GdefFeatureWriter would infer GDEF classes (that were not explicitly defined via public.openTypeCategories) from the current GPOS/GSUB. But I would make that not the default.

That's an easy fix, and is almost the same, at least with feature writers that use public.openTypeCategories.
The only difference would be for lookups already in the feature code using glyphs in a way that conflicts with the public.openTypeCategories values.
I don't have a preference for glyph classes inferred from lookups or openTypeCategories.
The current PR does, to follow the "skip" logic (even if not explicit).

I can change the PR that way if that's fine.

anthrotype · 2021-03-09T15:40:29Z

explicit openTypeCategories should always take precedence over (implicit) inferred categories.

Is the intention of this proposed "unassigned" public.openTypeCategory to have the compiler automatically infer the GDEF classes for the glyphs that were not explicitly listed in openTypeCategories (neither assigned, nor explicitly "unassigned")?

I still don't like that we have to build the temporary GPOS and GSUB and the throw them away. For the kern feature writer, we have no way to classify pairs as LTR or RTL so we need to do the GSUB closure, but for this GDEF classes we do have explicit user settings (public.openTypeCategories).

moyogo · 2021-03-09T16:03:09Z

explicit openTypeCategories should always take precedence over (implicit) inferred categories.
OK.

Is the intention of this proposed "unassigned" public.openTypeCategory to have the compiler automatically infer the GDEF classes for the glyphs that were not explicitly listed in openTypeCategories (neither assigned, nor explicitly "unassigned")?

No. It's up to authoring tools (feature writers, compilers) to decide what to do with glyphs that have no openTypeCategories value, whether they follow the AFDKO spec and infer GDEF glyph class from lookup types or do something else it up to them.
The point of the "unassigned" openTypeCategories value is to tell the authoring tools, that those should be in the the GDEF glyph class 0 and no other glyph class.

The point is to make it easier for user to define what GDEF glyph class certain glyphs should be in, which authoring tools may put in another GDEF glyph class for othre reason, without having to do the same as writing a complete GDEF in the feature code.

I still don't like that we have to build the temporary GPOS and GSUB and the throw them away. For the kern feature writer, we have no way to classify pairs as LTR or RTL so we need to do the GSUB closure, but for this GDEF classes we do have explicit user settings (public.openTypeCategories).

We can have a GDEF post processor instead of a feature writer. Or we can store the GPOS and GSUB in the compiler object, like the kern and mark feature writer do for the GSUB, and use them.

anthrotype · 2021-03-09T16:13:28Z

Again, I'd like to keep things simple, basically what Khaled suggested above. If one wants to take advantage of the automatic GDEF class inference in feaLib, one does not use any public.openTypeCategories; or one can write a simple script that compiles the features and extract the inferred GDEF class info and store them as explicit public.openTypeCategories (and possibly amend them manually as they wish).

madig · 2021-03-09T17:13:06Z

I vote in favor of doing the simplest thing first (Khaled's suggestion) and going from there.

khaledhosny · 2021-03-09T17:23:49Z

An alternative, more complex approach:

Have a pre-processor that checks for glyphs without public.openTypeCategory:
- If the feature code has GDEF table, fill missing public.openTypeCategory from it, no guessing.
- If not, do the guessing done in this PR and fill public.openTypeCategory.
Feature writers as well as GDEF writer work exclusively with public.openTypeCategory.

But I vote for doing the simple thing first until someone has a compelling use case for the complex one.

moyogo · 2021-03-09T19:43:34Z

With a little change in the current PR, we should avoid building the GSUB and GPOS for inferred glyph classes when all glyphs have openTypeCategories values defined.

That said, I'm warming up to the idea of expecting the user or application generating the sources to define all glyphs' openTypeCategories values.
Inferring glyph classes is isn't that clearly defined and is rather messy if lookups use glyphs in multiple ways (like having two marks substituted for a single mark with a ligature substitution lookup inferring they are two components and a ligature).

Lib/ufo2ft/featureWriters/gdefFeatureWriter.py

anthrotype

LGTM

…s None

anthrotype · 2021-03-19T09:41:06Z

Thanks Denis!

moyogo · 2021-03-19T10:07:45Z

Thanks for the reviews @anthrotype, @madig, @khaledhosny.

moyogo mentioned this pull request Mar 5, 2021

Write GDEF feature writer #456

Closed

khaledhosny reviewed Mar 5, 2021

View reviewed changes

moyogo force-pushed the gdefFeatureWriter branch from 9635c94 to 90e0727 Compare March 17, 2021 17:50

anthrotype reviewed Mar 18, 2021

View reviewed changes

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

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

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

anthrotype approved these changes Mar 18, 2021

View reviewed changes

moyogo added 21 commits March 18, 2021 20:30

Add gdefFeatureWriter

ba6e9bf

Add BaseFeatureWriter.compileGDEF and util.compileGDEF

677191e

Add GdefFeatureWriter to default feature writers

259363a

Add GdefFeatureWriter tests

2d90d0b

getGDEFGlyphClasses() for BaseFeatureWriter and GdefFeatureWriter

136553c

markFeatureWriter test for openTypeCategories

761d1b8

gdefFeatureWriter: no update if GDEF present

40b9915

Update gdefFeatureWriter tests

d0a7970

featureWriters: use defined GDEF or openTypeCategories

0c24b44

baseFeatureWriter: getOpenTypeCategories, getGDEFGlyphClasses

d765c16

Update markFeatureWriter test

2f268b2

baseFeatureWriter: update after review

4de373b

gdefFeatureWriter: skip early

98be8ba

baseFeatureWriter: skip collectInsertMarkers when insertFeatureMark i…

8947177

…s None

getGDEFGlyphClasses docstring

650446a

gdefFeatureWriter: skip GlyphsClassDefs or LigatureCarets separately

f04e3d3

Update gdefFeatureWriter tests

37efa1f

baseFeatureWriter: warn on invalid openTypeCategories

303619a

test invalid openTypeCategories

c959af9

gdefFeatureWriter: merge _makeGDEF and _write, remove shouldContinue

c2351a9

getOpenTypeCategories returns unassigned as well

02cbdf1

moyogo force-pushed the gdefFeatureWriter branch from cdcdf67 to 02cbdf1 Compare March 18, 2021 21:18

moyogo merged commit dd88011 into googlefonts:main Mar 19, 2021

moyogo deleted the gdefFeatureWriter branch March 20, 2021 21:49

moyogo mentioned this pull request Apr 16, 2021

Add support for building GDEF LigatureCarets from "caret_X" anchors #329

Closed

Add GDEF feature writer #480

Add GDEF feature writer #480

Conversation

moyogo commented Mar 5, 2021

khaledhosny Mar 5, 2021

Choose a reason for hiding this comment

moyogo Mar 5, 2021

Choose a reason for hiding this comment

khaledhosny Mar 5, 2021

Choose a reason for hiding this comment

madig Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

khaledhosny Mar 5, 2021

Choose a reason for hiding this comment

moyogo Mar 6, 2021

Choose a reason for hiding this comment

khaledhosny Mar 6, 2021

Choose a reason for hiding this comment

moyogo commented Mar 6, 2021

moyogo commented Mar 6, 2021

moyogo commented Mar 6, 2021

anthrotype commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021 • edited Loading

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

khaledhosny commented Mar 9, 2021

anthrotype commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021 • edited Loading

moyogo commented Mar 9, 2021

anthrotype commented Mar 9, 2021

madig commented Mar 9, 2021

khaledhosny commented Mar 9, 2021

moyogo commented Mar 9, 2021

anthrotype left a comment

Choose a reason for hiding this comment

anthrotype commented Mar 19, 2021

moyogo commented Mar 19, 2021

madig Mar 5, 2021 •

edited

Loading

moyogo commented Mar 9, 2021 •

edited

Loading

anthrotype commented Mar 9, 2021 •

edited

Loading