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

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

Open
simoncozens opened this issue Jul 31, 2022 · 26 comments
Open

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

simoncozens opened this issue Jul 31, 2022 · 26 comments

Comments

@simoncozens
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Collaborator Author

Welcome to OpenType.

@vv-monsalve
Copy link

vv-monsalve commented Jan 17, 2023

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
Copy link

HugoJourdan commented Feb 20, 2023

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
Copy link
Collaborator Author

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
Copy link

BaGsn commented Jul 12, 2023

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
Copy link

jpt commented Dec 10, 2023

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
Copy link

jpt commented Dec 11, 2023

@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
Copy link
Collaborator Author

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
Copy link

jpt commented Dec 11, 2023

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
Copy link
Collaborator Author

simoncozens commented Dec 11, 2023

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
Copy link

jpt commented Dec 11, 2023

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
Copy link
Collaborator

schriftgestalt commented Dec 11, 2023

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 (fonttools/fonttools@563730f (AFAIK)).

@anthrotype
Copy link
Member

@khaledhosny
Copy link
Collaborator

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
Copy link
Member

@khaledhosny ok so what is your point?

@simoncozens
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator

@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 fonttools/fonttools#3335.

@anthrotype
Copy link
Member

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
Copy link
Collaborator Author

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

@rsheeter
Copy link

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

@rsheeter
Copy link

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

@schriftgestalt
Copy link
Collaborator

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

@rsheeter
Copy link

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

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

9 participants