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

defcon.errors.DefconError: The kerning data is not valid while processing NotoSansEthiopic-MM.glyphs #13

Closed
marekjez86 opened this issue Feb 12, 2016 · 14 comments

Comments

@marekjez86
Copy link
Contributor

The pipeline built on both 10/Feb and 12/Feb fails to build NotoSansEthiopic fonts.
I have attached the input file used for the pipeline.
NotoSansEthiopic-MM.glyphs.zip

==== building src/NotoSansEthiopic-MM.glyphs ====
WARNING: Illegal glyph name "geminationandvowellengthcomb_ethiopic". If this is used in the font's feature syntax, it could cause errors.
WARNING: Non-existent glyph class @MMK_R_ddhe_ethiopic found in kerning rules.
WARNING: Non-existent glyph class @MMK_R_ddho_ethiopic found in kerning rules.
WARNING: Non-existent glyph class @MMK_R_ddo_ethiopic found in kerning rules.
[... deleted hundreds of similar WARNINGs ...]
WARNING: Non-existent glyph class @MMK_R_ddo_ethiopic found in kerning rules.
WARNING: Non-existent glyph class @MMK_R_de_ethiopic found in kerning rules.
WARNING: Non-existent glyph class @MMK_L_qyu_ethiopic found in kerning rules.
WARNING: Non-existent glyph class @MMK_L_lo_ethiopic found in kerning rules.
WARNING: Non-existent glyph class @MMK_L_qho_ethiopic found in kerning rules.
>> Checking Glyphs source for illegal glyph names
Found 555 glyph names containing hyphens: aGlottal-ethiopic, aPharyngeal-ethiopic, aaGlottal-ethiopic, aaPharyngeal-ethiopic, ba-ethiopic, ...
Replacing all hyphens with underscores.
>> Interpolating master UFOs from Glyphs source
>>> Parsing .glyphs file
>>> Casting parsed values
>>> Loading to RFonts
>>> Writing masters
>>> Building instances
Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/usr/local/lib/python2.7/dist-packages/fontmake/__main__.py", line 34, in <module>
    main()
  File "/usr/local/lib/python2.7/dist-packages/fontmake/__main__.py", line 30, in main
    project.run_all(glyphs_path, **args)
  File "/usr/local/lib/python2.7/dist-packages/fontmake/font_project.py", line 139, in run_all
    ufos = self.build_instances(glyphs_path, is_italic)
  File "/usr/local/lib/python2.7/dist-packages/fontmake/font_project.py", line 64, in build_instances
    return build_instances(glyphs_path, master_dir, instance_dir, is_italic)
  File "/usr/local/lib/python2.7/dist-packages/glyphs2ufo/glyphslib.py", line 102, in build_instances
    master_ufos, master_dir, instance_dir, designspace_path, instance_data)
  File "/usr/local/lib/python2.7/dist-packages/glyphs2ufo/interpolation.py", line 49, in interpolate
    build(designspace_path)
  File "/usr/local/lib/python2.7/dist-packages/mutatorMath/ufo/__init__.py", line 56, in build
    reader.process()
  File "/usr/local/lib/python2.7/dist-packages/mutatorMath/ufo/document.py", line 369, in process
    self.readSources()
  File "/usr/local/lib/python2.7/dist-packages/mutatorMath/ufo/document.py", line 403, in readSources
    sourceObject = self._fontClass(sourcePath)
  File "/usr/local/lib/python2.7/dist-packages/defcon/objects/font.py", line 161, in __init__
    k = self.kerning
  File "/usr/local/lib/python2.7/dist-packages/defcon/objects/font.py", line 430, in _get_kerning
    self._loadKerningAndGroups()
  File "/usr/local/lib/python2.7/dist-packages/defcon/objects/font.py", line 403, in _loadKerningAndGroups
    raise error
defcon.errors.DefconError: The kerning data is not valid.
@marekjez86
Copy link
Contributor Author

Same error happens when:
==== building src/NotoSerif-ItalicMM.glyphs ====
==== building src/NotoSerif-RomanMM.glyphs ====

Inputs also for these two additional builds are attached.
NotoSerif.zip

@jamesgk
Copy link
Contributor

jamesgk commented Feb 17, 2016

I think the problem here is that the Glyphs source contains conflicting kerning rules, which are rejected on UFO load by defcon. We didn't have this problem with RoboFab (the conflicting kerning rules are removed when the feature is written), but interpolation via MutatorMath uses defcon so we have to deal with this earlier than on feature write.

@adrientetar do you know of some tool that can remove conflicting kerning in defcon or elsewhere?

@adrientetar
Copy link
Contributor

I don't.

Btw kerning is normally loaded lazily but needs to be processed on open when converting from a legacy ufo version.

@jamesgk
Copy link
Contributor

jamesgk commented Feb 17, 2016

Hm. This is still a bit of a pain to deal with because robofab doesn't support ufo3.

FYI @behdad it's starting to look like we need to scrap use of robofab universally in favor of defcon, if only for compatibility with MutatorMath (this would also apply to Roboto, if we want to use it there).

@marekjez86
Copy link
Contributor Author

Sounds like more work than we previously planned. However, I'm all for
doing "the right thing".

On Wed, Feb 17, 2016 at 3:06 PM, James Godfrey-Kittle <
notifications@github.com> wrote:

Hm. This is still a bit of a pain to deal with because robofab doesn't
support ufo3.

FYI @behdad https://github.com/behdad it's starting to look like we
need to scrap use of robofab universally in favor of defcon, if only for
compatibility with MutatorMath (this would also apply to Roboto, if we want
to use it there).


Reply to this email directly or view it on GitHub
#13 (comment).

Marek Z Jeziorek [ 老马 ] | marekj@google.com | 312 725-6958

@adrientetar
Copy link
Contributor

Great idea (cc @graphicore), ufo3 has things like layers standardized so you don't need to use robofont extensions to the spec anywhere. defcon itself has many refinements in the ufo3 branch and, as you found out, validates data.
If you're doing this I'll work on getting our patches upstreamed.

@anthrotype
Copy link
Member

👍 for dropping robofab in favour of defcon+ufo3!

@behdad
Copy link
Contributor

behdad commented Feb 18, 2016

FYI @behdad it's starting to look like we need to scrap use of robofab universally in favor of defcon, if only for compatibility with MutatorMath (this would also apply to Roboto, if we want to use it there).

Correct.

I'll take a look into this issue. I'm new to defcon, but see what I can figure out.

@jamesgk
Copy link
Contributor

jamesgk commented Feb 23, 2016

But, kerning is still validated by defcon on UFO save so using defcon doesn't really help us here. It's looking to me like we have no choice but to remove the duplicate kerning earlier if we use MutatorMath. Of course we can still switch to defcon since it seems like the right thing to do.

@behdad
Copy link
Contributor

behdad commented Feb 29, 2016

So, is the kerning data in one of the masters invalid? Then that's a source bug. @jamesgk can you make defcon report the invalid pairs? validateKerning definitely generates those, but they are lost in the exception.

It also occurs to me that:

  1. defcon's validateKerning can be improved a bit. I'll send PR for that separately,
  2. mathFont.mathKerning._processMathOne() needs to be more sophisticated to handle adding two kernsets with differing groups, half-class exceptions, etc. I'll send a PR for that as well as I work it out this week.

@jamesgk
Copy link
Contributor

jamesgk commented Feb 29, 2016

So, is the kerning data in one of the masters invalid? Then that's a source bug.

So, is it OK if fontmake chokes on invalid kerning? My concern is that Glyphs does not.

@jamesgk can you make defcon report the invalid pairs? validateKerning definitely generates those, but they are lost in the exception.

They are passed on by defcon via an attribute of the exception object; we can print that out in glyphs2ufo where mutatorMath is called.

@jamesgk
Copy link
Contributor

jamesgk commented Feb 29, 2016

BTW, there are quite a lot (948) of these conflicting rules in Noto Sans Ethiopic. The first 10:

public.kern1.@MMK_L_li_ethiopic, qa_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qu_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qi_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qee_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qe_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qo_ethiopic (-60) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qwa_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qwi_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qwe_ethiopic (-70) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qho_ethiopic (-73) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
...

@behdad
Copy link
Contributor

behdad commented Mar 1, 2016

Thanks. I'll take a look.

On Tue, Mar 1, 2016 at 8:27 AM, James Godfrey-Kittle <
notifications@github.com> wrote:

BTW, there are quite a lot (948) of these conflicting rules in Noto Sans
Ethiopic. The first 10:

public.kern1.@MMK_L_li_ethiopic, qa_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qu_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qi_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qee_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qe_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qo_ethiopic (-60) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qwa_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qwi_ethiopic (-140) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qwe_ethiopic (-70) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
public.kern1.@MMK_L_li_ethiopic, qho_ethiopic (-73) conflicts with hhi_ethiopic, public.kern2.@MMK_R_qa_ethiopic (-62)
...


Reply to this email directly or view it on GitHub
#13 (comment).

behdad
http://behdad.org/

@jamesgk
Copy link
Contributor

jamesgk commented Mar 8, 2016

The original issue here should be fixed. New issues can be spun off e.g. for migration to defcon (one already exists in ufoLib for updating validateKerning).

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

No branches or pull requests

5 participants