googlefonts / ufo2ft

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

Cannot set post table format 3 in TTF variable font using UFO key #462

Open punchcutter opened 3 years ago

punchcutter commented 3 years ago

Because of this line forcing False instead of passing None it doesn't seem to be possible to set post table format 3.0 with only the UFO key <key>com.github.googlei18n.ufo2ft.keepGlyphNames</key> <false/>

https://github.com/googlefonts/ufo2ft/blob/648e7a19dd02215a6f720f2ec7f3980f16d05ca5/Lib/ufo2ft/__init__.py#L604

For most fonts it will probably work fine as is, but when the font contains 65535 glyphs it can fail while running through the names (that seems like a whole different issue in fontTools, but can be ignored for this case). The fontmake error is a bit useless: fontmake: Error: In 'Test.designspace': Generating fonts from Designspace failed: unsigned short is greater than maximum

That's only useful if you know that it means the post table (2.0) and that it means you are trying to add indices > 65535 to the array at https://github.com/fonttools/fonttools/blob/7878c32a85b93296141b35d15b909eff220e46a9/Lib/fontTools/ttLib/tables/_p_o_s_t.py#L172

If I change False to useProductionNames then it will make post format 3.0 and compile fine. The same line is in compileVariableCFF2, but that will fail for different reasons so my question here is what's the reason to set this to False instead of passing the value along?

punchcutter commented 3 years ago

It could be that we only need to fix the fontTools issue. I fixed it locally and then it all seems fine without touching ufo2ft, but it goes through the whole process of building a post 2.0 table and then throws that out in the end and makes it 3.0. Not sure how much time that wastes, but with 65535 glyphs it probably wastes a bit.

anthrotype commented 3 years ago

the lib key is called com.github.googlei18n.ufo2ft.useProductionNames, not keepGlyphNames. It has to be set (at least) on the default master UFO, which is where the PostProcessor will be looking at. Are you sure you're setting the correct lib key? Can you provide a test font that reproduces the issue?

anthrotype commented 3 years ago

sorry my fault (why is this so complicated). yes, there is indeed a keepGlyphNames lib key, you're right. I still don't completely understand what the issue is. You provide the fix before explaing what you're attempting to do and how/what you did exactly to build.

anthrotype commented 3 years ago

It could be that we only need to fix the fontTools issue.

what issue? And how come you have more than 0xFFFF glyphs in your font, such that compiling post 2.0 fails?

anthrotype commented 3 years ago

maybe what's exceeding 0xFFFF is not the number of glyphs, but the lenght of the extraNames array? (ie. the names that are outside the standard macintosh glyph name list which are encoded at the end of post 2.0)

https://github.com/fonttools/fonttools/blob/7878c32a85b93296141b35d15b909eff220e46a9/Lib/fontTools/ttLib/tables/_p_o_s_t.py#L169

punchcutter commented 3 years ago

Forget everything I wrote (don't worry I'll get back to that) and my question is why is this forcing False instead of passing the provided value? By forcing False it forces post format 2.0 even when we know from the start that we don't want it.

anthrotype commented 3 years ago

why is this forcing False instead of passing the provided value?

because: compileVariableTTF delegates to compileInterpolatableTTFsFromDS (or OTFs) to build the masters, but we don't want to process the production names for each individual master, we only want to do the renaming (if any) one time at the end, on the varfont itself. We don't want any glyph renaming to happen before we build the VF, because designspace can contain rules that reference glyphs using the human-readable names (not the uniXXXX production names).

anthrotype commented 3 years ago

hm maybe I know what's going on. the extra glyph names must be stored in the post table after index 258.

https://docs.microsoft.com/en-us/typography/opentype/spec/post#version-20.

If the extraNames are > 65535 - 258 then we're out of luck, they won't fit. We have to name as many glyphs as we can using the standard macintosh name set.

Zachary, can you try naming the first 257 glyphs using this list and see if it works

https://github.com/fonttools/fonttools/blob/master/Lib/fontTools/ttLib/standardGlyphOrder.py

That makes me wonder, maybe it's impossible to have a post table format 2.0 font containing 65535 glyphs where the first 257 do not follow the standard glyph order..

punchcutter commented 3 years ago

Yes, that's exactly the issue, but I don't think fontTools is handling that list of glyphs properly. The OT spec does not say these glyphs are required and only that if they are in this list they can be used. In my case all of the glyphs are named cidXXXXX because it's a 65535 glyph CJK font. So nothing at all is found in standardGlyphOrder.

punchcutter commented 3 years ago

Sure it would work to rename 257 of the glyphs, but that's also completely wrong. If I change the code in fontTools then I can build the font with no problems. I don't think it should be adding 258 blindly.

punchcutter commented 3 years ago

"A given font may map some of its glyphs to the standard Macintosh glyph names" (emphasis mine). Like a lot of the OT spec it's not 100% clear, but may sounds to me like not required to map to these names. Of course then it describes in detail how to handle these names, but at least in this particular case it's all meaningless. We will throw out everything in the next step.

anthrotype commented 3 years ago

If I change the code in fontTools

do you have a patch you can share?

We will throw out everything in the next step.

yeah, that's true as well. Let me think if we can find another workaround

punchcutter commented 3 years ago

The patch I did to work for my case doesn't work for everything and tons of tests fail.

anthrotype commented 3 years ago

I think there's no workaround. We need to build post 2.0 table (even if only temporarily) in order to keep the glyph names in the master TTFs, if we drop it too earlier the VF will not contain the human-readable glyph names any more and the expectation that DS rules work on human-readable names is broken.

punchcutter commented 3 years ago

My simple way was to just replace the hard-coded 258 with standardCount where

standardCount = len([g for g in glyphOrder if g in standardGlyphOrder])

It would also be possible to check which glyphs are part of standardGlyphOrder and let those use the expected indices and fill in the others with the remaining names. Seems kind of ugly, though.

anthrotype commented 3 years ago

but the value 258 is hard-coded in the very definition of the post 2.0 table

anthrotype commented 3 years ago

maybe we should just not attempt to build post 2.0 for the masters when keepGlyphNames is set to False; in that case, the user will have to use production names in their designspace rules

punchcutter commented 3 years ago

Sure, it says 0-257 are special, but if nothing in the font uses those actual names and the font contains 65535 glyphs then what else can you do except use those indices for glyphs that aren't in the "set of Macintosh glyph names"?

anthrotype commented 3 years ago

well, if they (ab)use < 258 indices it'd be the same as assigning them the wrong names

anthrotype commented 3 years ago

because glyph name indices between 0 and 257 are reserved for standard mac names

anthrotype commented 3 years ago

and the problem is the glyph name indices must be unsigned 16-bit integers so they can't exceed 0xFFFF (65535)

anthrotype commented 3 years ago

in practice this means, one can't have more than (65535 - 258) == 65,277 non-standard extra glyph names in post 2.0

punchcutter commented 3 years ago

Oh well. I'll just keep my local fontTools hacked so this all works for me.

khaledhosny commented 3 years ago

Oh well. I'll just keep my local fontTools hacked so this all works for me.

This means any tool reading the glyph names, will use the standard names for the first 258 glyphs and will ignore your names, so you may as well just rename the glyphs and not patch fontTools at all, the end result is effectively the same.

khaledhosny commented 3 years ago

But I think we are approaching this issue the wrong way. Do you have steps to reproduce this issue? Is it a CFF or TTF font? Why the code tries to compile post table if it is going to through it away?

anthrotype commented 3 years ago

it's a TTF, otherwise ufo2ft would use post 3.0. it's a VF with the max 65k glyphs with non-standard glyph names (cidXXXX). As I explained above, the compileVariable methods try to delay the renaming of glyph names until after the VF is built (so that DS rules can work on human-readable names, not production names); they delegate to the other compileInterpolatable methods overriding useProductionNames=False to skip post-processing and keep the original glyph names for the masters, then call postprocessor on the VF applying whatever user settings. The problem in his case is that the master TTFs will have (only temporarily) a post 2.0 table (to retain the original names) but the latter fails to compile because extra names can only be 65k - 258... It's a mess :(

khaledhosny commented 3 years ago

I understand that, but why the post table is compiled, do we save the files to desk or compile it for some other reason, and why is this happening?

khaledhosny commented 3 years ago

I mean why the intermediate files AKA the masters are being compiled at all?

anthrotype commented 3 years ago

good question. it could be that TTFont.save() call to a temporary buffer in the PostProcessor constructor maybe (which is always looked suspicious). A reproducer would help with debugging

punchcutter commented 3 years ago

With that PR I still have problems, one being ufo2ft.outlineCompiler.OutlineTTFCompiler.makeMissingRequiredGlyphs which adds a .notdef glyph which I don't need since cid00000 is already that glyph. I stopped trying to use the defaults and wrote my own setup that overrides makeMissingRequiredGlyphs and setupTable_post to do what I need. So far that works fine.

anthrotype commented 2 years ago

makeMissingRequiredGlyphs which adds a .notdef glyph which I don't need since cid00000 is already that glyph

Then we should probably check that a glyph named "cid00000" exists as well and is the first glyph, in which case assume .notdef is already present and don't make one up.