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

Compiling NotoSansArabicUI fails because of feaLib #447

Closed
punchcutter opened this issue Jun 15, 2018 · 11 comments · Fixed by notofonts/noto-source#121
Closed

Compiling NotoSansArabicUI fails because of feaLib #447

punchcutter opened this issue Jun 15, 2018 · 11 comments · Fixed by notofonts/noto-source#121

Comments

@punchcutter
Copy link

This is really a ufo2ft and/or fontTools.feaLib issue, but I'm not sure where this should be handled since it seems to rely on Glyphs-specific behavior.

What happens is that there are non-exporting glyphs in the font that have anchors on them. These are added to the UFO, but since they are not exported the anchors should be ignored when building the GPOS. Since they are read by feaLib there are conflicts like this:

fontTools.feaLib.error.FeatureLibError: <features>:1956:9: Glyph twodotsverticalabove-ar.small cannot be in both @MC_top and @MC_center

This glyph is only used as a component so isn't exported, but it has some anchors on it (here it has both a 'top' and a 'center' anchor). In this particular case those anchors are totally pointless, but in many cases non-exporting glyphs might have anchors that "shine through" in Glyphs app so might be needed. But these should never be put in the GPOS for glyphs that will not export.

@anthrotype
Copy link
Member

googlefonts/ufo2ft#259

@anthrotype
Copy link
Member

in many cases non-exporting glyphs might have anchors that "shine through" in Glyphs app so might be needed. But these should never be put in the GPOS for glyphs that will not export.

by "shine through" you mean the semitransparent "cloud of accents" that is displayed as you click on an anchor in Glyphs.app, right?

This issue is also somewhat related to this one
googlefonts/ufo2ft#261

because those glyphs you mentioned should not be considered mark glyphs by the mere fact that they contain some _ prefixed anchor.

@anthrotype
Copy link
Member

@punchcutter slightly unrelated question: what do you expect to happen for those glyphs only used as components in other composite glyphs and which are marked as non-export? That the composite glyphs that use them will be decomposed?
If so, that's not what's happening, because the way fontmake gets rid of the glyphs that are marked as Export: false is using the fonttools subsetter after the font has been compiled, and the subsetter does not decompose a composite glyph when any of its components are not included in the subset; instead it automatically adds them to the subset (when doing the closure over the glyf table). I think this is a good thing, but maybe it's not what the export flag in Glyphs.app is supposed to do...
Anyway, probably a separate issue.

@punchcutter
Copy link
Author

No, “shine through” is the Glyphs term for using anchors from a component, the anchor propagation thing. I don’t know how Glyphs determines what should be propagated and what shouldn’t which is why I mention that as something to consider in a situation like this where a non exporting glyph is used as a component.

Right, the way Glyphs handles non exporting components is to decompose them on export as far as I can tell. But the problem I see in this particular situation is that a glyph set to Export: false should never ever have its anchors written out to GPOS. It’s not being exported so the mark writer should ignore it. But, like you say, the way Export: false is handled in fontmake is not the same as Glyphs. That’s just another of the million Glyphs specific behaviors we can’t, or maybe don’t want to, mimic exactly.

@m4rc1e
Copy link
Contributor

m4rc1e commented Jun 19, 2018

Getting a similar issue here whilst trying to generate comfortaa using fontmake v1.5.1

fontmake 1.5.1
fontTools.feaLib.error.FeatureLibError: <features>:1455:9: Glyph tonos cannot be in both @MC_top and @MC_topleft

It genned fine using fontmake v1.3.0.

I'll keep digging :-). I'm guessing this has been caused by the recent efforts to support abvm, blwm feats in ufo2ft.

@anthrotype
Copy link
Member

no need to dig, the issue is the same I pointed to above:
googlefonts/ufo2ft#259

@m4rc1e
Copy link
Contributor

m4rc1e commented Jun 19, 2018

Thank you. I'll revert to using v.1.4.0 for the time being.

@anthrotype
Copy link
Member

as I said, this is also related to googlefonts/ufo2ft#261

because that "tonos" glyph in Comfortaa should not be included in the mark feature, as it's not a Nonspacing combining mark, but it's a spacing mark. The current method is to treat as "marks" anything that contains some _ prefixed anchor. But apparently this is too "greedy", and may lead to that error, now that we are no longer creating separate lookups for each mark class.

@m4rc1e
Copy link
Contributor

m4rc1e commented Jun 19, 2018

because that "tonos" glyph in Comfortaa should not be included in the mark feature

Eugh, Glyphsapp thinks it is one.

screen shot 2018-06-19 at 11 57 08

In GlyphData.xml

<glyph unicode="0384" name="tonos" sortName="gr049" decompose="acutecomb" category="Mark" subCategory="Spacing" script="greek" description="GREEK TONOS" anchors="_top" />

Whilst it's actually a Symbol modifier

I'll see what Georg says about this, https://forum.glyphsapp.com/t/glyphdata-xml-revise-mark-category-glyphs/9234

@anthrotype
Copy link
Member

the subCategory for tonos is Spacing.
for marks to be considered as combining the subCategory must be "Nonspacing", see
https://github.com/googlei18n/glyphsLib/blob/0393173dc594b15bb06318e8a7fb4ce28959b25d/Lib/glyphsLib/builder/features.py#L139

@anthrotype
Copy link
Member

GDEF GlyphClassDef 3 is for "combining marks" only (those with 0 advance width that are positioned via GPOS mark/mkmk), not any mark glyphs
https://docs.microsoft.com/en-us/typography/opentype/spec/gdef#glyph-class-definition-table-overview

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

Successfully merging a pull request may close this issue.

3 participants