googlefonts / fontmake

Compile fonts from sources (UFO, Glyphs) to binary (OpenType, TrueType).
Apache License 2.0
776 stars 93 forks source link

Unexpected kerning introduced by fontmake to variable font export (but not to static ttf) #470

Closed thundernixon closed 5 years ago

thundernixon commented 6 years ago

I've exported a static TTF and a variable TTF via FontMake, and I'm finding that the Variable TTF gets unexpected kerning.

Amongst other differences, an obvious thing is that the i seems to get extra kerning to the left side, against even straight letters where there shouldn't need to be kerning. This kern doesn't exist in the Glyphs sources (unless I'm somehow missing it). Why might this being added in the variable font?

Here's a screencap showing a comparison between static and VF at the Regular weight/width, then a comparison with kerning taken away from both:

encode-var_vs_static-kerning_diff 2018-10-09 at 16 51 57

It also gets different kerning than a previously-generated static version. Here's an overlay to compare the two (variable font in green, just a bit wider from the added positive kerns): image

anthrotype commented 6 years ago

Which one of the two is the correct? In the title you say the unexpected kerning is in the variable font, whereas in the first sentence you say that the Static TTF gets different kerning, so I’m confused. Do you have sources available to reproduce this? What’s the fontmake command you used to build? Thanks

thundernixon commented 6 years ago

Sorry about that confusing phrasing, @anthrotype. The Variable font gets different kerning than the existing, status-quo static font file, which I am trying to match. The variable font kerning is incorrect.

I have edited the designspace slightly to make the variable font work, so I tested whether a newly-generated static font would have correct or incorrect kerning. When I use FontMake to export a new static TTF from the same edited designspace, it has no visible difference with the status-quo font file. The new Static TTF is correct (in terms of lettershapes and kerning, at least).

I am working in this repo: https://github.com/thundernixon/Encode-Sans/tree/make-variable.

The designspace file used for both the new variable TTF and the new static TTF is: https://github.com/thundernixon/Encode-Sans/blob/40a6ca8df6b54ca5e6e96657c0f3cbe08ba83526/master_ufo/EncodeSans-mathfix-ds_v3.designspace

I built the variable font with this script: https://github.com/thundernixon/Encode-Sans/blob/288a278e5dd8ce31b3c3db627eaf805f5a7280bd/scripts/makeVar-simplified.py

I built the static font with this command:

fontmake --mm-designspace /Users/stephennixon/type-repos/google-font-repos/Encode-Sans/master_ufo/EncodeSans-mathfix-normal_wdth.designspace -i --output-dir /Users/stephennixon/type-repos/google-font-repos/Encode-Sans/fonts

Please let me know if I can give any further information. Thanks for taking a look!

anthrotype commented 6 years ago

I’ll have a look, but just some quick comments:

thundernixon commented 6 years ago

Thanks for the info on command line vs scripting use, and thanks for the suggested command.

The repository contains a Glyphs source, why don’t you just compile directly from there

thundernixon commented 6 years ago

I've just learned from @mjlagattuta that there is a known bug in macOS CoreText that doesn't correctly represent the metrics in glyphs with components: https://forum.glyphsapp.com/t/component-sidebarings-in-variable-fonts-change/8620

That seems to point to this being an issue in CoreText, not in FontMake.

I'll dig further into it by exported a font without components, and report back if metrics are still wrong in the VF. Thanks for taking a look!

thundernixon commented 6 years ago

Reopening this because the testing I did seemed to point to the problem being produced in the FontMake stage. It certainly might be something I'm doing wrong, but I don't see errors in a font bakery QA that would seem to cause this issue, so it seems possible that it's coming up in the FontMake export.

Here's how I tested this.

Test 1: Checking if a decomposed VF would work as expected, generated via FontMake

Process:

  1. To test whether this was a CoreText issue caused by components in the VF, I made a copy of the Glyphs source and decomposed all glyphs in it. I also morphed the designspace values within the Glyphs file to make a rectangular designspace, so that the VF axes would work as expected.
  2. I used that decomposed source to generate a variable font and instances via FontMake commands. a. fontmake -g sources/Encode-Sans-decomposed.glyphs -o variable b. fontmake -g sources/Encode-Sans-decomposed.glyphs -i --output-dir fonts
  3. I made several overlay tests in Drawbot to compare metrics: a. The new, decomposed VF onto the new, decomposed static TTF b. The new, decomposed VF onto the static TTF currently in distribution via Google Fonts. c. The new, decomposed static TTF onto the static TTF currently in distribution via Google Fonts

Results:

Process

  1. To see whether a VF exported from Glyphs would have the same issue, I morphed the designspace values within the Glyphs file to make a rectangular designspace so that the VF axes would work as expected, then exported a VF font from the Glyphs file decomposed, and also from the morphed Glyphs file with components
  2. I made two overlay tests in Drawbot to compare metrics: a. The VF from the decomposed Glyphs file onto onto the static TTF currently in distribution via Google Fonts b. The VF from the Glyphs file with components onto onto the static TTF currently in distribution via Google Fonts

Results: In both cases, the Glyphs-exported VF aligned (nearly) perfectly onto the static TTF currently in distribution via Google Fonts encode-diff_test-glyphs_vf_vs_decomp_static-2018_10_10 encode-diff_test-glyphs_vf_vs_gf_static-2018_10_10

Files tested: my latest Drawbot test folder

What letters are impacted?

I haven't been able to find a perfect way to find all impacted letters yet. If this seems useful any ideas for testing this are welcome!

It is clear that it's most disruptive from the i and the l, and that these are affected when following a straight side image

I don't see these kerns when I open the VF TTF in In Illustrator (here's a positive 59 units):

image

However, I don't see the kern values if I open the VF TTF in Glyphs:

image


Have I somehow mistested this, or could these unexpected kerns be coming from some part of the FontMake export process?

mjlagattuta commented 6 years ago

I was able to replicate this error when building Encode Sans as well. Running varLib.mutator on the VF also generates a static ttf with the broken kerning.

It is worth noting that there is no difference between the fontmake instance export and vf export when exporting the default set of outlines (i.e. the variation origin), but the problem got worse as weight and width was increased. For reference as to the severity of the added kerning, Black Condensed (wght=900 wdth=75) had kerning between the \l and the \i of +240

mjlagattuta commented 6 years ago

Upon further examination I discovered that the kern value of +240 is coming from the kern between \lcaron and \l or \dcaron and \l

It appears that the way fontmake parses the kerning table led to the \l class taking on the same value as \lcaron. Once \lcaron and \dcaron were deleted from the source file, the VF had no weird kerning being applied to the \l class.

Any ideas on what may be causing this behavior?

thundernixon commented 6 years ago

Just to add a bit more detail to what Mike discovered with kerning groups...

It seems that the values from the /dcaron kern1 kerning group are being moved to the normal /d kern1 side. Things with straight kern1 sides should be in the /l kern1 group, but are moved into the /d kern1 group, gaining the kerning values that should only be in the /dcaron.

Meanwhile, things in the /l kern2 group are changed to a /b group. I can't find any cases where this causes problems, but it's unexpected.

I don't know if there are any smaller/less-noticeable inconsistencies or swapped kerning classes. However, these swaps start to explain the most noticeable kerning issues.

Here's a visual showing the swapped kerning classes for /i, which should have a class of /l on both sides: image

Here's a visual of several prominent places that the positive kerning is inserted due to the swaps: image

anthrotype commented 6 years ago

thanks @thundernixon and @mjlagattuta for the detailed report. Just so you know I'm not ignoring this, I had been busy with other things last week. I will try to take a look at this in the next few days.

thundernixon commented 6 years ago

Here's the TTF showing that shows kerning issues when opened in Glyphs

Here's the Glyphs Source

Here's the VF, also showing the issue

thundernixon commented 6 years ago

Thanks for letting us know that, @anthrotype. I know you have a lot on your plate!

I'll keep poking around to see if I can add more detail that might help diagnose this issue and update here as I can.

thundernixon commented 6 years ago

Possibly also useful to add: the kerning doesn't seem to get confused until after the UFO generation. That is, the proper kerning seems to be making it into the UFOs as expected, but the issues come up between that UFO and the final VF.

Here's the static TTF exported from the VF versus the ExtendedBold UFO generated by FontMake. image

anthrotype commented 5 years ago

there is actually too much details and info in this issue report.. I need a simple way to reproduce this. What is the current/latest glyphs or designspace file for EncodeSans that I can use to repro?

anthrotype commented 5 years ago

btw, I've noticed something that doesn't look right in the EncodeSans.glyphs file from thundernixon/EncodeSans master branch (as of commit https://github.com/thundernixon/Encode-Sans/commit/7dc54a0cd02b684eeac7b65ea57599c2b44cd89a), though I'm not sure yet if it's related.

The Width axis varies between 0 and 1000, whereas it is normally specified as a percentage, with 100 being the Normal width, 50 % being Ultra-Condensed, 200 % Ultra-expanded, etc.

https://docs.microsoft.com/en-us/typography/opentype/spec/os2#uswidthclass

There may be an issue in glyphsLib here, because it seems it is assuming that all the .glyphs source files follow this rule when it's generating the designspace axes map. In fact, it only does it for weight, whereas for width axis it simply assumes that user-space (what you see/control from the UI) and design-space (internal, for interpolation purpose) locations are the same.

https://github.com/googlei18n/glyphsLib/blob/cfef8fb7d189c3e52ba89b6c6ddaa99a1e85ff8d/Lib/glyphsLib/builder/axes.py#L310-L329

In the case of EncodeSans, the width axis' user locations respectively of the most condensed and extended masters are begin set to 0 and 1000 (their design locations), whereas they should probably be something below and above 100.

glyphsLib also supports the "Axis Location" master custom parameter, which specifies the user location for a master along a certain axis (the list of axes can be defined in a "Axes" font custom parameter). Maybe you could try to use that (for both weight and width) and see if it changes/improves things. In absence of a "Axis Location" parameters, glyphsLib will use the instances user+design locations to construct the designspace axes map elements. The latter are used to build an avar table, ie. they control the way user-space coordinates (used to define axes in the fvar) are mapped to a normalized scale (-1.0, 0.0, +1.0).

anthrotype commented 5 years ago

cc @belluzj (author of the above mentioned code in glyphsLib)

anthrotype commented 5 years ago

ok I can reproduce the varfont kerning issue. Here's what I did:

$ git clone https://github.com/thundernixon/Encode-Sans
$ fontmake -g sources/Encode-Sans.glyphs -o variable

then set the string diblil toggling kern feature on and off using harfbuzz's hb-shape to get the advances, or hb-view to get a visual (by default it prints ansi to console, you can optionally pass --output-file=something.png)

This is how it looks with kern feature disabled:

$ hb-shape variable_ttf/EncodeSans-VF.ttf --variations=wght=900,wdth=750 --features=-kern diblil
[d=0+1394|i=1+658|b=2+1394|l=3+658|i=4+658|l=5+658]
$ hb-view variable_ttf/EncodeSans-VF.ttf --variations=wght=900,wdth=750 --features=-kern diblil

encodesans-vf_900_750_no_kern

This is the same font, same axis coordinates, but with kern feature enabled (it definitely looks wrong):

$ hb-shape variable_ttf/EncodeSans-VF.ttf --variations=wght=900,wdth=750 --features=+kern diblil
[d=0+1715|i=1+979|b=2+1386|l=3+979|i=4+979|l=5+658]
$ hb-view variable_ttf/EncodeSans-VF.ttf --variations=wght=900,wdth=750 --features=+kern diblil

encodesans-vf_900_750_with_kern

I also tried to build a static TTF version from the same EncodeSans.glyphs file, with this command:

$ fontmake -g sources/Encode-Sans.glyphs -i "Encode Sans SemiExpanded Black" -o ttf

Note: "SemiExpanded Black" instance corresponds to wght=900 and wdth=750 user locations.

Comparing without and with the kern feature, in the case of the static TTF, does not show the huge difference seen above for the VF:

$ hb-shape instance_ttf/EncodeSansSemiExpanded-Black.ttf --features=-kern diblil
[d=0+1394|i=1+658|b=2+1394|l=3+658|i=4+658|l=5+658]
hb-view instance_ttf/EncodeSansSemiExpanded-Black.ttf --features=-kern diblil

encodesans-semiexpandedblack_no_kern

$ hb-shape instance_ttf/EncodeSansSemiExpanded-Black.ttf --features=+kern diblil
[d=0+1394|i=1+658|b=2+1386|l=3+658|i=4+658|l=5+658]
hb-view instance_ttf/EncodeSansSemiExpanded-Black.ttf --features=+kern diblil

encodesans-semiexpandedblack_with_kern

Something is happing when fonttools.varLib is generating the VF GPOS table... Need more time to investigate.

thundernixon commented 5 years ago

Thanks so much for taking a look! Thanks also for providing a good example of the right amount of information to show the issue. Part of the reason for all the information from me is that it took some time to narrow down the best way to show the problem.

Thanks for pointing out the width axis values. I'll find a way to correct that, and add it to my build script. Interesting point about the missing <map> for widths ... that's the first thing I'll try to fix.

For what it's worth, to reproduce my exact conditions, you can set up a Python 3 environment, then run the scripts/build.sh script. This applies non-destructive fixes to the Glyphs source, as I wanted to leave the original source in-tact as much as possible.

anthrotype commented 5 years ago

@thundernixon so, the sources/Encode-Sans.glyphs file is not the one already "fixed", but one has to run that scripts/fix-designspace.py script first?

I actually tried to run that script (I had to comment out import objc, which is not even used), but then when I build a variable font from the modified Encode-Sans.glyphs file with fontmake, fonttools fails while building the avar table with an AssertionError. This is triggered because the generated designspace has the axis locations not sorted in ascending order. In fact, the Extra Bold (OS/2 weight class 800) has design location 234, whereas the Black (which is supposedly heavier, OS/2 weight class 900) has 232.

$ fontmake -g sources/Encode-Sans.glyphs -o variable
[...]
INFO:fontTools.varLib:Generating avar
Traceback (most recent call last):
  File "/home/clupo/.pyenv/versions/fontmake-py27/bin/fontmake", line 11, in <module>
    load_entry_point('fontmake', 'console_scripts', 'fontmake')()
  File "/home/clupo/Github/fontmake/Lib/fontmake/__main__.py", line 258, in main
    project.run_from_glyphs(glyphs_path, **args)
  File "/home/clupo/Github/fontmake/Lib/fontmake/font_project.py", line 561, in run_from_glyphs
    self.run_from_designspace(designspace_path, **kwargs)
  File "/home/clupo/Github/fontmake/Lib/fontmake/font_project.py", line 646, in run_from_designspace
    **kwargs)
  File "/home/clupo/Github/fontmake/Lib/fontmake/font_project.py", line 715, in run_from_ufos
    master_bin_dir=master_bin_dir)
  File "/home/clupo/Github/fontmake/Lib/fontmake/font_project.py", line 268, in build_variable_font
    font, _, _ = varLib.build(designspace_path, finder)
  File "/home/clupo/Github/fonttools/Lib/fontTools/varLib/__init__.py", line 731, in build
    _add_avar(vf, ds.axes)
  File "/home/clupo/Github/fonttools/Lib/fontTools/varLib/__init__.py", line 146, in _add_avar
    assert sorted(vals) == vals
AssertionError
thundernixon commented 5 years ago

Correct, the source file is fixed by running the build script, and the VF is built from the temporary, fixed file.

I need to dig into the code more to understand why, but the values are applied correctly when I'm in my Python 3 environment, but incorrectly in my Python 2 environment. However, this script should also be applying proper width values, so I have some troubleshooting to do on my end. Thanks for flagging this for me.

UPDATE: I'm realizing that the fix-designspace.py was relying on variables to be set with correct wght values, and also that I must have run it directly on the source which I (incorrectly) thought was untouched. So, this was morphing wght values up inaccurately in condensed instances. Working to fix these mixups now.

anthrotype commented 5 years ago

hm.. ok. I was forced to use python2 to run the fix-designspace.py script because it contains print statements (also didn't want to fix it myself..). Also the build.sh script calls it as python2 scripts/fix-designspace.py.

Are you suggesting that building the same designspace yields different output when running from either python2 or 3?

mjlagattuta commented 5 years ago

One possibility for the difference between running fix-designspace.py in Python2 versus Python3 is the rounding function. Python2 rounds based on being above or below x.5 meanwhile Python3 rounds to the nearest even number.

anthrotype commented 5 years ago

fontTools.misc.py23 module has both round2 and round3 that works on either python. And it also has an otRound function in fontTools.misc.fixedTools which we use for the rounding that have to do with more visual values where a more statistical "banker's" rounding doesn't make sense.

your script throws SyntaxError on python3.

anthrotype commented 5 years ago

even after turning the print statements into calls to print() function (after __future__ import print_function), I still get the issue that the values in axes.map elements are not in ascending order (800 is mapped to 234, whereas 900 is mapped to 232), which trips AssertionError in varLib._add_avar

thundernixon commented 5 years ago

I've updated the glyphs source back to being the original, and the build script is now working better and less-manually. The width values are properly getting exported, based on tests at axis-praxis.

Here's the latest font build: Encode Sans VF, Oct 26, 2018

It still does have some fontbakery issues, but none that seem to explain the kern regrouping. Not sure if you really needed the update, but I appreciate your critique and wanted to fix the issues pointed out.

anthrotype commented 5 years ago

Thanks, I’ll have a look again next week.

anthrotype commented 5 years ago

I pulled the latest master of EncodeSans repo (I see you fixed the width axis values -- good), re-run the fix-designspace.py script, re-built the varfont with fontmake from the fixed Encode-Sans.glyphs, and I can confirm the kerning issue is still there.

I tried to subset the interpolatable master TTFs so that they only contain glyphs "b" "d" "i" "l", "idotaccent", "idotless", and "dotaccentcomb" (using the the command fonttools subset EncodeSans*.ttf --text="dibl" --glyph-names).

I then run fontTools varLib EncodeSans.designspace with the subsetted master TTFs, and the issue seems to disappear (i.e. toggling kern feature on and off doesn't produce that extra tracking effect shown in the pictures above).

I'm uploading below the set of interpolatable master TTFs (both the full fonts, and the subsetted ones) with the accompanying designspace file and the generated variable fonts:

EncodeSans-VF-kern-issue.zip

Here are the commands to build the two variable fonts, one that uses the full masters, the other using the subsetted ones:

$ fonttools varLib --master-finder "master_ttf_interpolatable_full/{stem}.ttf" -o EncodeSans-full-VF.ttf EncodeSans.designspace
$ fonttools varLib --master-finder "master_ttf_interpolatable_subset/{stem}.ttf" -o EncodeSans-subset-VF.ttf EncodeSans.designspace

Running hb-view/hb-shape with --variations=wght=900,wdth=130 instance, and toggling on/off kern feature, reproduces the kerning issue described above but only with EncodeSans-full-VF.ttf, the subsetted one looks ok.

@behdad do you mind taking a look?

thundernixon commented 5 years ago

Thanks for bearing with my learning curve here, for running the build again, and for experimenting some more! As @mjlagattuta said above, subsetting the font even helps when it's as simple as removing a couple of problematic letters:

Once \lcaron and \dcaron were deleted from the source file, the VF had no weird kerning being applied to the \l class.

If static TTFs are exported, the kerning (appears to be) correct. I wanted to try to diff the PairPos tables from a static Extended Bold instance generated from the Glyphs source and an Extended Bold instance generated from the VF. I used xmlStarlet to copy just the PairSet tables into their own files, then VSCode's "Compare Selected" to diff these files. There are definitely quite a few differences, but lines referring to /dcaron, /d, /b, /l, and /i seem to be similar aside from their index (even though these files come from TTFs with clearly different kerning in those glyphs). So, I'm not sure if I'm comparing the right or wrong table, but if someone knows of a better ttx table to compare, please let me know. Here are those files, if it's useful:

behdad commented 5 years ago

At least I remembered there's something smelly in that part of code so that was the big indeed. Unlike what the comment suggests, I remember the full issue now. Fix is correct for now. @anthrotype, is possible please add a test case for this so I don't break or again. Subsetting for dcaron etc may contain the bug without the commit, for test generation purpose?

anthrotype commented 5 years ago

thanks @behdad for taking care of this! I will add the test case soon. Let's keep this issue open until a new fonttools with that fix is released and fontmake is updated to use it. In the meantime, @thundernixon you should be able to clone the fonttools/fonttools repo, pip install -e . from it and run fonttools varLib EncodeSans.designspace with the master_ttf_interpolatable that you created with fontmake ... -o ttf-interpolatable

thundernixon commented 5 years ago

Thanks so much for taking a look and suggesting a current workaround, @behdad and @anthrotype!

thundernixon commented 5 years ago

After installing the latest fonttools from a git clone, things seem to be working well!

I just ran it with my typical build command:

fontmake -o variable -g sources/Encode-Sans.glyphs

This shows the old version (back) and the new version (front): image

(I suppose I'll still tweak the caron kerning slightly, seeing this).

Thanks again for the help!

VivaRado commented 5 years ago

I have been testing the same issue, trying to understand why it happens. I checked the XML via TTX and I see that after ...

</ClassDef2>
<!-- Class1Count=3 -->
<!-- Class2Count=64 -->
<Class1Record index="0">

I get significantly less records. It generates inconsistently between two states. Either it produces all the records or it produces less, thus losing interpolation.

When interpolation happens it also overflows to other pairs.

I am trying to find where the count is reduced and why. If you have any pointers it would help. Thank you!

VivaRado commented 5 years ago

So as to not be quick to react, but you know more about the issue. But here is what I found out: In the varLib.merger, function : _Lookup_PairPosFormat2_subtables_flatten When it fails for me the first 5 positions in cols, are None.

...
for cols in zip(*list(r.Class2Record for r in rows)):
    # here the cols are None on the first 5 positions
    # so I did a reverse when I see the first value is None
    if cols[0] == None:
        cols = cols[::-1]
...

And now it seems to always compile interpolated kerning. But I still experience overflow in other pairs. That might be something else - maybe in my class definitions. I will check.

anthrotype commented 5 years ago

I thought the issue had been fixed, as @thundernixon and you @VivaRado also confirmed. Can you explain in more details what the issue is? If you're seeing a new or different issue than the one originally reported please file a separate issue report. Note that the fact that you get offset overflows while compiling the GPOS (or GSUB) table doesn't mean there is anything faulty, it's just the way it is, that amount of data and the way fonttools packs it into binary may produce offsets that exceed unsigned short, thus requiring an extension lookup. The message you get on the console has an "INFO" level of severity, not WARNING or ERROR. (I'm thinking that we should just demote those logging messages to DEBUG, as several users seem confused by those /cc @behdad)

VivaRado commented 5 years ago

I'm not getting an error, it just compiles the same source inconsistently. I did a XML check and when I compared two generated instances there where different contents, but different consistently. Either there was interpolation or wasn't.

I raised a new issue for varLib merger. As I progress in compiling my font property I will submit whatever I find helps me do that.

Thank you

On 17 Dec 2018 16:31, "Cosimo Lupo" notifications@github.com wrote:

I thought the issue had been fixed, as @thundernixon https://github.com/thundernixon and you @VivaRado https://github.com/VivaRado also confirmed. Can you explain in more details what the issue is? If you're seeing a new or different issue than the one originally reported please file a separate issue report. Note that the fact that you get offset overflows while compiling the GPOS (or GSUB) table doesn't mean there is anything faulty, it's just the way it is, that amount of data and the way fonttools packs it into binary may produce offsets that exceed unsigned short, thus requiring an extension lookup. The message you get on the console has an "INFO" level of severity, not WARNING or ERROR. (I'm thinking that we should just demote those logging messages to DEBUG, as several users seem confused by those /cc @behdad https://github.com/behdad)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/googlei18n/fontmake/issues/470#issuecomment-447796940, or mute the thread https://github.com/notifications/unsubscribe-auth/AoFvb1gzCd9msuGPAsAN2D1I9Rn53zChks5u53JxgaJpZM4XUJ4q .