Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate collecting kern classes from features.fea? #203

Closed
anthrotype opened this issue Jan 10, 2018 · 10 comments
Closed

deprecate collecting kern classes from features.fea? #203

anthrotype opened this issue Jan 10, 2018 · 10 comments

Comments

@anthrotype
Copy link
Member

anthrotype commented Jan 10, 2018

The KernFeatureWriter has a _collectFeaClasses that uses a regular expression to parse the features.fea text and collect all the @MMK_L_ and @MMK_R_ prefixed glyph classes. Apart from the problem that this doesn't take into account include statements.. why do that when UFO has groups.plist, where one can define public.kern1 and public.kern2 classes?

Do people actually use this feature? If so, why don't they use groups.plist to store their kerning classes and instead write them as FEA classes?

I can't remember the original purpose of this feature. Was it because some kerning tool (Metrics Machine?) used to do that?

I would like to simplify the API of the kern feature writer (before it actually gets more complex as we add more feature to it, e.g. dist, or refactoring RTL support), so I was thinking of dropping this.

@anthrotype
Copy link
Member Author

There's also a _collectFeaClassKerning method that spits up the UFO kerning.plist rules into three dicts (classPairKerning, leftClassKerning and rightClassKerning) based on the first or "key" glyph from these kerning class definitions in features.fea.

What's this about? cc @moyogo

@anthrotype
Copy link
Member Author

anthrotype commented Jan 10, 2018

I suspect this has to do with the peculiar way Roboto UFO sources stores their kerning data.

The kerning classes are defined in features.fea rather than groups.plist:
https://github.com/google/roboto/blob/master/src/v2/Roboto-Regular.ufo/features.fea

And the kerning.plist only has single glyph to single glyph kerning, no classes. The reference to the kerning class is implicit in the fact that a glyph is the first (i.e. the "key").

Perhaps Roboto was kerned in FontLab.

Anyway, this does not look to me like the way UFO (v3 at least) normally stores kerning data.
Ideally one has kerning.plist referencing either single glyph names or public.kern{1,2} classes, and groups.plist containing definition of such kerning classes.

If Roboto deviates from this, the sources could easily be normalised.

Am I correct?

@anthrotype
Copy link
Member Author

there's a method that returns all kern classes together (both those @... defined in the features.fea and those from groups.plist):

https://github.com/googlei18n/ufo2ft/blob/b3cdd768f7d67d5f8a7e74b57fd4e28c97b40042/Lib/ufo2ft/featureWriters/kernFeatureWriter.py#L393-L399

Since we use update method of dict, the ones from the groups.plist will override the ones from features.fea, if class with same name is defined in both.

if I'm not convinced about keeping support for kerning-classes-in-FEA, I'm even more reluctant about supporting the case when they are defined both in features.fea and groups.plist.

I think we should only support the latter, for simplicity's sake.

@anthrotype
Copy link
Member Author

I'm gonna remove this feature in the next release, as part of the kernFeatureWriter clean up, unless someone gives me a good reason not to (apart from Roboto which doesn't support latest ufo2ft anyway).

@madig
Copy link
Collaborator

madig commented Jan 10, 2018

Denis told me that at DM, fonts commonly have kerning information written to the feature file because e.g. kerning.plist can't support language-specific kerning.

@anthrotype
Copy link
Member Author

anthrotype commented Jan 10, 2018

"commonly"? I don't remember that... Perhaps you misunderstood, or I didn't get what you mean.

Here I'm not saying that we should drop support for compiling manually written kern features (which will have both their own classes and lookups explicitly defined in the features.fea); I'm saying the KernFeatureWriter should only be concerned about generating a kern feature by simply looking at the contents of groups.plist and kerning.plist, without also parsing any kerning classes that may be already defined in features.fea.

This auto-generated kern feature is by default only written if no kern feature is already defined in features.fea (the "skip" default mode); this block can optionally be either appended or pre-pended to features.fea, so that the FEA compiler will merge any existing kern lookups with the autogenerated ones in a sigle kern OTL feature, following the desired order.

@moyogo
Copy link
Collaborator

moyogo commented Jan 11, 2018

If the auto-generated kern feature is only written if no kern feature is already in the features file, then it make sense to not collect kerning classes from the features file.

@madig I didn’t mean that we used that commonly at DM. That was an example of why kern features would have to sit in the features file instead of the kerning.plist/groups.plist, along with more complex kerning like contextual kerning, etc.

@anthrotype
Copy link
Member Author

If the auto-generated kern feature is only written if no kern feature is already in the features file, then it make sense to not collect kerning classes from the features file.

I still don't understand. Can you make an example of when it does make sense to collect kerning classes from the feature file and merge them with the UFO groups.plist?

If the features.fea contains some snippet of, say, contextual kern, it will be self contained and will have its own class definitions (if any), with names that don't clash with the ones in groups.plist. The kerning lookups added by the feature writer will be completely independent from those already present. The only options is whether one wants them before or after (by setting the mode).
Am I missing somethings else?

@moyogo
Copy link
Collaborator

moyogo commented Jan 11, 2018

@anthrotype I think it’s fine to not collect even if there is a kern feature in the features file. UFOs should use the standard UFO kerning groups for simple kerning.

anthrotype added a commit that referenced this issue Mar 2, 2018
Only use the public.kern* prefixed classes in groups.plist.

Fixes #203
@anthrotype
Copy link
Member Author

only UFO3 style kerning (public.kern1.* and public.kern2.* prefixes in groups.plist and kerning.plist) is now supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants