googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
152 stars 43 forks source link

Unexported glyphs being added to mark filtering set #721

Open simoncozens opened 1 year ago

simoncozens commented 1 year ago

Sorry, this is my fault. In #720, we grovel the UFO for spacing glyphs and put them into a mark filtering set. Unfortunately, we don't check if they're exporting or not. We should use the glyphset:

            # We only want to filter the spacing marks
            marks = set(self.context.gdefClasses.mark) & set(self.context.glyphSet)
            spacing = []

But I just tried that, and now I have a merge failure in the GDEF table, which I don't really understand, caused by different glyph class definitions in each master; I don't know if this is related or not. (Mark class definitions would make sense, but I'm seeing some glyphs in GDEF in some masters but not in others.)

anthrotype commented 1 year ago

it might be that different masters have different glyph export status. I think ufo2ft checks for skipExportGlyphs at the designspace.lib level, for that info should be considered global to the VF, not master-specific

https://github.com/googlefonts/ufo2ft/blob/87c688180f983dd2dd9a77f2d3d6b1784340ab8c/Lib/ufo2ft/__init__.py#L379

anthrotype commented 1 year ago

in designspace v5 where there can be more than one VF per .designspace file, ideally we'd source that list from each specific <variable-font>'s lib element (but I guess that's a different issue).

Perhaps glyphsLib doesn't even set that lib key at the designspace.lib level? /cc @madig who added that piece of logic in ufo2ft

simoncozens commented 1 year ago

Nah, it's completely my fault. I just did a bit of bisecting and the merge failure only happens after cc02f399, so it's also being caused by #720.

simoncozens commented 1 year ago

Oh, urgh. It's because some masters have width==0 for a spacing combining glyph and some masters don't. I knew it was quite a brittle test (but the best we could do), but I didn't expect that failure mode.

Chalk another one up to "treating each master as an independent font".

anthrotype commented 1 year ago

ouch. how do we fix this one now..

khaledhosny commented 1 year ago

That was fast. My suggestion, if these are Glyphs sources, is to explicitly check for category and subCategory (which I believe we keep in lib).

anthrotype commented 1 year ago

My suggestion, if these are Glyphs sources, is to explicitly check for category and subCategory (which I believe we keep in lib).

hm yeah, we could do that but it'd be weird for ufo2ft to explicitly check for the presence of a lib key that's Glyphs' specific.. We could also do the GSUB closure (similarly to how we group glyphs in RTL scripts etc.) to get all glyphs that are mapped (via cmap or GSUB substituions) to Unicode codepoints whose category is "Mc" aka "Spacing Combining Marks".

anthrotype commented 1 year ago

while we discuss this, maybe we should revert https://github.com/googlefonts/ufo2ft/pull/720 ?

simoncozens commented 1 year ago

I'm uneasy about doing a GSUB closure. You may wish to make a mark into a spacing mark for design reasons. Real example: Noto Serif Telugu (until recently) didn't support the stacked -jnya conjunct: shape Instead it anchors the nya alongside the first conjunct: shape but of course it needs to be spacing so that it doesn't end up underneath the క.

There's nothing in Unicode which says the the -nya conjunct should be spacing or non-spacing; doing a GSUB closure won't help you.