adobe-type-tools / python-modules

Python modules
MIT License
30 stars 17 forks source link

[WriteFeaturesKernFDK] Warnings in FontLab, errors in FontValidator #4

Closed jenskutilek closed 8 years ago

jenskutilek commented 8 years ago

I’m adding a kern feature in FontLab with WriteFeaturesKernFDK.py. The resulting kern feature code generates warnings when compiling the font (as well as in FL or with makeotf). This is the generated kern feature:

feature kern {
@LAT_i_LEFT = [i igrave iacute icircumflex idieresis itilde imacron ibreve iogonek];
@LAT_i_RIGHT = [i igrave iacute icircumflex idieresis itilde imacron ibreve iogonek];

# glyph, glyph:
pos iacute icircumflex 29;
pos iacute igrave 38;
pos ibreve icircumflex 38;
pos icircumflex ibreve 38;
pos icircumflex icircumflex 38;
pos icircumflex idieresis 38;
pos icircumflex imacron 38;
pos icircumflex itilde 55;
pos idieresis icircumflex 38;
pos idieresis itilde 55;
pos imacron icircumflex 38;
pos itilde icircumflex 48;

# glyph, group exceptions:
enum pos icircumflex @LAT_i_RIGHT 14;
enum pos idieresis @LAT_i_RIGHT 19;
enum pos itilde @LAT_i_RIGHT 17;

# group, glyph exceptions:
enum pos @LAT_i_LEFT icircumflex 14;
enum pos @LAT_i_LEFT idieresis 14;
enum pos @LAT_i_LEFT imacron 14;
enum pos @LAT_i_LEFT itilde 14;

# glyph, group:

# group, glyph and group, group:
} kern;

And the warning is:

[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: iacute icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: icircumflex icircumflex
[NOTE] <MarsTestSerif-BoldItalic> Removing duplicate pair positioning in 'kern' feature: icircumflex icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: icircumflex idieresis
[NOTE] <MarsTestSerif-BoldItalic> Removing duplicate pair positioning in 'kern' feature: icircumflex idieresis
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: icircumflex itilde
[NOTE] <MarsTestSerif-BoldItalic> Removing duplicate pair positioning in 'kern' feature: icircumflex itilde
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: icircumflex imacron
[NOTE] <MarsTestSerif-BoldItalic> Removing duplicate pair positioning in 'kern' feature: icircumflex imacron
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: icircumflex ibreve
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: idieresis icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: idieresis icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: idieresis idieresis
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: idieresis itilde
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: idieresis itilde
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: idieresis imacron
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: itilde icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: itilde icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: itilde idieresis
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: itilde itilde
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: itilde imacron
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: imacron icircumflex
[WARNING] <MarsTestSerif-BoldItalic> Pair positioning has two different values in 'kern' feature; choosing the smaller absolute value: ibreve icircumflex

The FL kerning seems to be valid ...

In addition, there is a problem that makeotf doesn’t actually seem to ignore or choose one of the duplicated positioning entries, so both are written into the GPOS table. This results in a FontValidator error E4101 The array order is incorrect (glyph ids are repeated, thus they are not sorted ascending as required by the OT spec).

I’m not sure if what’s at fault here ... FL, WriteFeaturesKernFDK, or makeotf?

A minimal example VFB is attached: MarsTestSerif-BoldItalic.zip

frankrolf commented 8 years ago

Jens, thank you very much for this example.

I am sure you are aware of the kernDump repository, which is helpful for analyzing kerning in all kinds of formats (actually your example helped me fix a bug in getKerningPairsFromFEA.py)

Here is my analysis:

According to my dump scripts, the (flattened) kerning expected from your VFB and the kern feature you reference above is absolutely the same.

– an OTF generated from FontLab changes a couple of values in comparison to the VFB:

19 → 14 idieresis idieresis
19 → 14 idieresis macron
17 → 14 itilde idieresis
17 → 14 itilde itilde
17 → 14 itilde imacron

– an OTF generated via makeotf v2.0.90 Nov 19 2015 changes a couple more, and other values:

38 → 14 icircumflex icircumflex
38 → 14 icircumflex idieresis
38 → 14 icircumflex imacron
55 → 14 icircumflex itilde
38 → 14 idieresis icircumflex
55 → 14 idieresis itilde
48 → 14 itilde icircumflex

This is slightly unsettling, and points at makeotf as a part of the problem. I would like to invite @readroberts to comment here.


Just to make sure I get this right, an example: I assume that writing this …

@i_group = [ i iacute ];

pos icaron i 20;
enum pos icaron @i_group 30;

… is equivalent to writing this …

pos icaron i 20;
pos icaron i 30;
pos icaron iacute 30;

… and would result in one warning, and two kerning pairs:

icaron i 20;
# [WARNING] icaron i already exists
icaron acute 30;

Is that correct, @readroberts?

frankrolf commented 8 years ago

The FontValidator problem is another one for @readroberts to look into.

readroberts commented 8 years ago

Hi Frank; You are correct about the enum pos statement is expanded. However, the feature file spec does not specify what happens when conflicting non-class kern paris are specified. WHat makeotf does is to keep the last one. This means that the example above would be written in the font as: icaron i 30; icaron acute 30;

This explains the observation that "makeotf changes a couple more, and other values:". It is not in fact changing any values - it is simply using the last one specified. I also notice that the message "Pair positioning has two different values in 'kern' feature" is not in the makeotf code, so FontLab must be doing some preprocessing of its own to remove conflicting pairs.

I am surprised by the FontValidator report. makeotf does not write conflicting non-class kern entries in the GPOS table, and in both a test case I built and in the otf built by FontLab from the example font file supplied above, the glyph IDs are correctly sorted. Can you provide me with source data from which makeotf builds an offending font?

frankrolf commented 8 years ago

@readroberts We need to have a discussion about this, probably makeotf should change. I am currently preparing some test cases for next week.

I can reproduce Jens’s observations, in FontValidator and TTX. This is from TTX:

<PairSet index="2">
  <!-- PairValueCount=13 -->
  <PairValueRecord index="0">
    <SecondGlyph value="i"/>
    <Value1 XAdvance="14"/>
  </PairValueRecord>
  <PairValueRecord index="1">
    <SecondGlyph value="iacute"/>
    <Value1 XAdvance="14"/>
  </PairValueRecord>
  <PairValueRecord index="2">
    <SecondGlyph value="icircumflex"/>
    <Value1 XAdvance="38"/>
  </PairValueRecord>
  <PairValueRecord index="3">
    <SecondGlyph value="icircumflex"/>
    <Value1 XAdvance="14"/>
  </PairValueRecord>
  <PairValueRecord index="4">
    <SecondGlyph value="idieresis"/>
    <Value1 XAdvance="38"/>
  </PairValueRecord>

  ...

Source files attached. array order.zip

readroberts commented 8 years ago

Hi Frank;

When I build the font data in “array order.zip” with the command “makeotf –o test.otf”, I get the attached ttx dump, which looks correct to me. What is incorrect here?

frankrolf commented 8 years ago

Read, no TTX dump is attached here. In my dump above you will see icircumflex occur twice.

readroberts commented 8 years ago

I see the duplicate entry in PairSet[2]. Clearly a bug. Will fix/

readroberts commented 8 years ago

More specifically, this is a bug in makeotf, not WriteFeaturesKern.py. I will close the issue here, and will open issue for this in the afdko repository. The bug will be fixed in makeotfexe.

frankrolf commented 8 years ago

Thanks Read!

jenskutilek commented 8 years ago

@frankrolf and @readroberts, thank you very much for looking into this!