googlefonts / glyphsLib

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

Support (or don't) Glyphs-specific variable feature syntax #800

Open simoncozens opened 2 years ago

simoncozens commented 2 years ago

Glyphs has a different variable feature syntax to Adobe/fonttools/FontCreator. Instead of:

conditionset bold {
    wght 140 180;
} bold;

variation rlig bold {
   sub dollar by dollar.bold;
   sub cent by cent.bold;
} rlig;

Glyphs produces

feature rlig {
#ifdef VARIABLE

condition 140 < wght < 180;
sub dollar by dollar.bold;
sub cent by cent.bold;

#endif
} rlig;

Do we want to support this? I think probably not. Glyphs should do the right thing here.

anthrotype commented 2 years ago

would be nice to be able to automatically translate from one to the other form when converting glyphs=>UFOs, but if it's too much trouble we shouldn't bother. But I also think Glyphs.app should be updated to support the official form (if it doesn't work already, does it?)

schriftgestalt commented 2 years ago

It would be nice if there where a spec that all parties had discussed and had agreed to. Now we have a suggestion that some parties just found good enough and went ahead and implemented it.

simoncozens commented 2 years ago

Welcome to OpenType.

vv-monsalve commented 1 year ago

would be nice to be able to automatically translate from one to the other form when converting glyphs=>UFOs, but if it's too much trouble we shouldn't bother.

This would be nice, indeed. Would it be too much trouble?

Glyphs is suggesting since 2021, so I can see more cases coming (I have on at the moment).

HugoJourdan commented 1 year ago

would be nice to be able to automatically translate from one to the other form when converting glyphs=>UFOs, but if it's too much trouble we shouldn't bother.

Same problem here.

I also tried to use syntax from Adobe/fonttools/FontCreator in Glyphs and export variable with fontmake, but I got the same problem.

simoncozens commented 1 year ago

The Adobe/fontTools variable fea syntax should work already; the Glyphs 3 variable fea syntax should not. What's happening in your case, Hugo?

BaGsn commented 1 year ago

I tried Adobe syntax and got the same error as with Glyphs one, but with conditionset instead of condition: Expected glyph class definition or statement: got NAME condition Expected glyph class definition or statement: got NAME conditionset

jpt commented 11 months ago

Do we want to support this? I think probably not. Glyphs should do the right thing here.

I for one think glyphsLib should support this. This issue has been open long enough that there are probably a bunch of Glyphs files in the wild that already do it the Glyphs way

jpt commented 11 months ago

@anthrotype

would be nice to be able to automatically translate from one to the other form when converting glyphs=>UFOs, but if it's too much trouble we shouldn't bother. But I also think Glyphs.app should be updated to support the official form (if it doesn't work already, does it?)

I'm not currently familiar with glyphsLib's codebase, so I'm not really sure how much trouble it would be, but for what it's worth, to do this using the actual Glyphs API it would be something like:

from fontTools.designspaceLib import DesignSpaceDocument, RuleDescriptor
import re

def getAxisNameByTag(font, tag: str) -> str:
    return next((axis.name for axis in font.axes if axis.axisTag == tag), None)

def getBoundsByTag(font, tag: str) -> list:
    index = next((i for i, axis in enumerate(font.axes) if axis.axisTag == tag), None)
    if index is not None:
        coords = [master.axes[index] for master in font.masters]
        return [min(coords), max(coords)]
    return [None, None]

def parseCondition(condition, font):
    tag = re.search("< (\w{4})", condition).group(1)
    axis_name = getAxisNameByTag(font, tag)
    bounds = re.findall("\d+(?:\.|)\d*", condition)
    bounds = [float(b) for b in bounds]
    if len(bounds) < 2:
        _, max_bound = getBoundsByTag(font, tag)
        bounds.append(max_bound)
    return {'name': axis_name, 'minimum': bounds[0], 'maximum': bounds[1]}

def getConditionsFromOT(font):        
    feature_code = next((feature_itr.code for feature_itr in font.features if "condition" in feature_itr.code), "")

    condition_list, replacement_list, condition_index = [], [[]], 0
    for line in feature_code.splitlines():
        if line.startswith("condition"):
            conditions = [parseCondition(cond, font) for cond in line.split(",")]
            condition_list.append(conditions)
            condition_index += 1
        elif line.startswith("sub"):
            replace = tuple(re.findall("sub (.*) by (.*);", line)[0])
            if len(replacement_list) < condition_index:
                replacement_list.append([])
            replacement_list[condition_index-1].append(replace)

    return [condition_list, replacement_list]

def applyConditionsToRules(doc: DesignSpaceDocument, condition_list: list, replacement_list: list):
    rules = [RuleDescriptor(name=f"Rule {i+1}", conditionSets=[cond], subs=repl) for i, (cond, repl) in enumerate(zip(condition_list, replacement_list))]
    doc.rules = rules
    return doc

Font = Glyphs.font
doc = DesignSpaceDocument()

condition_list, replacement_list = getConditionsFromOT(Font)
doc = applyConditionsToRules(doc, condition_list, replacement_list)

print(doc.rules)

...and after adding the rules you'd want to remove the #ifdef VARIABLE stuff from rlig or whatever feature is being used.

But it seems like this is still an open question as to whether it would even be reviewed/approved if someone opened a PR?

@schriftgestalt Any plans or new ideas around supporting the Adobe/fontTools feature syntax?

simoncozens commented 11 months ago

I think there are multiple problems with the approach you suggest. It looks like an alternative way of getting part of Glyphs' variable rules (specifically the conditionsets) directly into the designspace document (and so it would need to be plugged deep into glyphsLib.builder.features, and would work only for the purposes of simple substitution rules. But there's a much easier way to express simple variable conditional substitution rules in a Glyphs file: use alternate layers. The only reason someone would be writing variable feature code by hand is when you're trying to do something more complex than alternate layers provide!

Even then, you'd still need to rewrite the feature code to remove all the #ifdef VARIABLE stuff, so that it can be parsed and built by fontTools.feaLib. At which point, it may be simpler just to transform the Glyphs syntax into the Adobe syntax.

(Sidenote: Most of the complexities of glyphsLib would be eliminated if we moved to a system of progressively transforming Glyphs source files into something directly compilable, instead of trying to resolve all the smart stuff while building a designspace file.)

And of course, simpler still if Glyphs just went with what everyone else is using instead of inventing its own syntax...

jpt commented 11 months ago

The only reason someone would be writing variable feature code by hand is when you're trying to do something more complex than alternate layers provide!

I disagree. I don't think this decision is necessarily dependent on complexity - ultimately it's just a workflow preference. It can be easier to work with alternate glyphs, visible in the Font view, rather than alternate layers. Especially so if you have a lot of masters, or if you have any brace layers. There's a page on the Glyphs website that goes into some detail about why you might choose one approach over the over. In it, Rainer expresses his own preferences for alternate glyphs (https://glyphsapp.com/learn/switching-shapes section: What to choose when: alternate glyphs or layers?)

At which point, it may be simpler just to transform the Glyphs syntax into the Adobe syntax.

Good point (I'm assuming glyphsLib already supports the Adobe syntax?)

And of course, simpler still if Glyphs just went with what everyone else is using instead of inventing its own syntax...

I agree, though I assume there are enough Glyphs files already doing it this way that glyphsLib would benefit more users by supporting both the Adobe and Glyphs-specific syntax. Currently I don't believe the Adobe syntax is supported

simoncozens commented 11 months ago

I disagree. I don't think this decision is necessarily dependent on complexity

Fine. But any solution will need to also support more complex use of variable feature programming, such as multiple substitutions, contextual substitutions and so on; not all of which can be represented by designspace rules.

Currently I don't believe the Adobe syntax is supported

It certainly is in fontTools, and therefore in glyphsLib. Getting it to work in fontmake is... what I spent most of the last two weeks working on. :-)

jpt commented 11 months ago

not all of which can be represented by designspace rules

True...

It certainly is in fontTools, and therefore in glyphsLib. Getting it to work in fontmake is... what I spent most of the last two weeks working on. :-)

Oh, I meant it is not supported in Glyphs itself. But that is great news; broader ecosystem support makes more of a case that Glyphs should support it, which I guess brings us back to square one with this issue

schriftgestalt commented 11 months ago

Can you point me to the official fea spec where that "Adobe syntax" is defined.

And of course, simpler still if Glyphs just went with what everyone else is using instead of inventing its own syntax...

For that to have been happening, I would need to be a clairvoyant. Glyphs 3 shipped and published "our" syntax in November 2020. FontTools/feaLib supports variable feature syntax since October 2021 (https://github.com/fonttools/fonttools/commit/563730f8cee0ebc039b4eb4078eb5b977df64886 (AFAIK)).

anthrotype commented 11 months ago

https://github.com/adobe-type-tools/afdko/pull/1350

khaledhosny commented 11 months ago

adobe-type-tools/afdko#1350

Not to nitpick, but this is a proposal that have not yet been accepted into the spec and Adobe might change the final version, if it gets accepted at all.

anthrotype commented 11 months ago

@khaledhosny ok so what is your point?

simoncozens commented 11 months ago

Not to nitpick, but this is a proposal that have not yet been accepted into the spec and Adobe might change the final version, if it gets accepted at all.

True, and it's worth mentioning that Adobe have sat on this proposal for over two years now. (And the discussion started seven years ago!)

anthrotype commented 11 months ago

yeah but that is stil the only "spec" that we can point Georg to, unless your suggesting he should just look at the code..

khaledhosny commented 11 months ago

@khaledhosny ok so what is your point?

feaLib should support both syntaxes, since the stewards of FEA spec has failed the community and caused this fragmentation in the first place. Trying to make glyphsLib “translate” from one syntax to another is too much work (you basically need a full fledged FEA parser to begin with) for little gain. At the end, I want to build fonts, not debate who should do what. I can work on this, but I don’t want it to end up being rejected like https://github.com/fonttools/fonttools/pull/3335.

anthrotype commented 11 months ago

Trying to make glyphsLib “translate” from one syntax to another is too much work... for little gain

by making a tool that translates between the two syntaxes, we gain that font files that contain the Glyphs.app's syntax can be updated so they can be built with existing open-source compilers without needing to extend all the FEA implementations to support both of them. Only one of them is likely to ever be added to the public FEA spec and thus expected to be implemented more widely, we do not need two ways to do the same thing, FEA is already complicated as is. Yes, I agree it's a pity that we ended up in this situation, but we can still manage to improve this.

simoncozens commented 11 months ago

glyphsLib does seem the more sensible place for Glyphs-specific extensions.

rsheeter commented 11 months ago

fea-rs would need an update as well. Do we have evidence of the Glyphs variable syntax being used? At what scale?

rsheeter commented 11 months ago

@schriftgestalt is it plausible for the Glyphs compiler to support both it's own variable feature syntax and the proposed one?

schriftgestalt commented 11 months ago

Probably not soon. We’ll have another look when the final spec is out.

rsheeter commented 11 months ago

No rush, it would just be nice to have a path to eventually settle on a syntax that is in the fea spec.

arrowtype commented 5 months ago

Running into this problem while trying to make a fontmake build of a Glyphs source made by another designer. This would be great to have a solution to, at some point! Thanks so much to everyone for hashing out some of the details, here.

simoncozens commented 5 months ago

I've just added Glyphs3 variable feature support to babelfont, so you could try that, either as a font compiler or as a filter to produce a "clean" Glyphs file.