googlefonts / fontmake

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

AssertionError with open contours when removing overlaps #354

Open kalapi opened 7 years ago

kalapi commented 7 years ago

Hello,

While trying to generate OTFs for Noto Serif Kannada from a Glyphs source (cubic outlines) I get the following error:

INFO:fontmake.font_project:Building master UFOs and designspace from Glyphs source
INFO:glyphsLib:Parsing .glyphs file
INFO:glyphsLib:Casting parsed values
INFO:glyphsLib:Loading to UFOs
INFO:glyphsLib.util:Writing master_ufo/NotoSerifKannada-Light.ufo
INFO:glyphsLib.util:Writing master_ufo/NotoSerifKannada-Regular.ufo
INFO:glyphsLib.util:Writing master_ufo/NotoSerifKannada-SemiBold.ufo
INFO:glyphsLib.util:Writing master_ufo/NotoSerifKannada-Bold.ufo
Took 10.869s to run 'build_master_ufos'
INFO:fontmake.font_project:Interpolating master UFOs from designspace
INFO:mutatorMath:Reading master_ufo/NotoSerifKannada.designspace
INFO:mutatorMath:   Generating instance NotoSerifKannada-Thin.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-ExtraLight.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-Light.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-Regular.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-Medium.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-SemiBold.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-Bold.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-ExtraBold.ufo
INFO:mutatorMath:   Generating instance NotoSerifKannada-Black.ufo
INFO:fontmake.font_project:Building OTFs
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-Thin
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-ExtraLight
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-Light
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-Regular
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-Medium
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-SemiBold
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-Bold
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-ExtraBold
INFO:fontmake.font_project:Decomposing glyphs for NotoSerifKannada-Black
Took 23.512s to run 'decompose_glyphs'
INFO:fontmake.font_project:Removing overlaps for NotoSerifKannada-Thin
Traceback (most recent call last):
  File "/Users/employee-1/Documents/_Code/fntmk/bin/fontmake", line 11, in <module>
    sys.exit(main())
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontmake/__main__.py", line 183, in main
    project.run_from_glyphs(glyphs_path, **args)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontmake/font_project.py", line 378, in run_from_glyphs
    designspace_path, instance_data=instance_data, **kwargs)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontmake/font_project.py", line 434, in run_from_designspace
    interpolate_layout_from=interpolate_layout_from, **kwargs)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontmake/font_project.py", line 473, in run_from_ufos
    ufos, remove_overlaps, **kwargs)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontmake/font_project.py", line 174, in build_otfs
    self.remove_overlaps(ufos)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontTools/misc/loggingTools.py", line 372, in wrapper
    return func(*args, **kwds)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/fontmake/font_project.py", line 115, in remove_overlaps
    union(contours, glyph.getPointPen())
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/booleanOperations/booleanOperationManager.py", line 104, in union
    return _performOperation("union", contours, [], outPen)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/booleanOperations/booleanOperationManager.py", line 64, in _performOperation
    subjectInputContours = [InputContour(contour) for contour in subjectContours if contour and len(contour) > 1]
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/booleanOperations/flatten.py", line 68, in __init__
    contour.drawPoints(pointPen)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/defcon/objects/contour.py", line 603, in drawPoints
    pointPen.addPoint((point.x, point.y), segmentType=point.segmentType, smooth=point.smooth, name=point.name, identifier=point.identifier)
  File "/Users/employee-1/Documents/_Code/fntmk/lib/python2.7/site-packages/booleanOperations/flatten.py", line 346, in addPoint
    assert segmentType != "move"
AssertionError

Any help is appreciated :)

kalapi commented 7 years ago

Overlapping points/nodes in some outlines was causing this issue. This has now been resolved.

anthrotype commented 7 years ago

Thanks Kalapi. Do you mind sending me the outline that triggered this? If this is some configuration of points that booleanOperations doesn't support, it would be best to raise a more meaningful error message. Or maybe it's something we can work around instead of raising an exception.

anthrotype commented 7 years ago

The AssertionError here means that there were some open contours (i.e. ones whose starting node has type "move"). BooleanOperations currently can only perform operations on closed contours. I believe the assumption is that clients should exclude open contours from the input passed to booleanOperations.

anthrotype commented 7 years ago

So, one way to prevent the error is that fontmake only runs booleanOperations on the contours that are actually closed, and leaves the open contours untouched. There may be good reasons to have open (i.e. unprintable) contours, e.g. a single point to anchor components in a TrueType composite glyph (/cc @moyogo). Or would you like fontmake to silently drop them instead? I prefer to not add another option.

anthrotype commented 7 years ago

It appears that when one uses the BooleanGlyph class to perform operations, the open paths are automatically closed: https://github.com/typemytype/booleanOperations/commit/b7a067f245024a2571bc2b871b9fa57efd376aae

fontmake uses the union function directly on a glyph's contours, without wrapping it into a BooleanGlyph so that code isn't used here.

So that leaves us with three options: (1) ignore open paths and leave them unotuched; (2) remove all open paths; (3) auto-close open paths before removing overlaps...

moyogo commented 7 years ago

For the attached GPOS anchors, I don’t think there’s a need to have single points in the UFOs since one can add those when generating the TTFont, but they may appear if a UFO was extracted from a TTFont. Either way, they should just be skipped.

anthrotype commented 7 years ago

by "skipped", do you mean option (1) "ignore open paths and leave them untouched", or (2) "remove all open paths"?

moyogo commented 7 years ago

I mean (1) in the case of point used in instructions.

kalapi commented 7 years ago

Hello @anthrotype and @moyogo,

Thanks Cosimo, for your insights regarding this. If the AssertionError was caused due to open counters, then it is possible that the culprits were '_part' style corner component glyphs which I used to control a terminal in the shapes. This was however, set to 'not export' in Glyphs. So maybe a possible approach would be to not export this glyphs to a UFO in the first place?

anthrotype commented 7 years ago

I mean (1) in the case of point used in instructions.

but we can't detect these, as there's nothing in a UFO GLIF that says that a point has a component anchored to it; GLIF components can only represent the XY type of components. I prefer to keep it simple, and simply check for open paths (i.e. starting with "move"), and either restore them after the union operation on the closed paths only, or drop them altogether.

anthrotype commented 7 years ago

Hey @kalapi

it is possible that the culprits were '_part' style corner component glyphs

smart components are not yet supported by glyphsLib and fontmake https://github.com/googlei18n/glyphsLib/issues/91

This was however, set to 'not export' in Glyphs. So maybe a possible approach would be to not export this glyphs to a UFO in the first place?

hm.. fontmake does read that flag but only after the remove overlap operation, in order to subset the generated OTF/TTF files. We could use the same data to filter the glyphs that are sent to booleanOperations. But I'd still prefer a more global treatment as regards all open paths. We all agree raising an AssertionError is not good. I think I'll go for appending them back to the merged glyph outline (their order should not matter, right?).

kalapi commented 7 years ago

smart components are not yet supported by glyphsLib and fontmake googlei18n/glyphsLib#91

I parse the Glyphs source through a 'preflight' process, so the smart components are flattened and aren't in the source when it goes through fontMake, so that's not an issue.

[...] We could use the same data to filter the glyphs [...]

Agree.

I think I'll go for appending them back [...]

This is ideal as it would be closest to source?

their order should not matter, right?

I don't suppose it does. They could be put in the end?

anthrotype commented 7 years ago

I was thinking that this notion of an "open" path only exists in the source (UFO or Glyphs) formats, not in their binary TTF or CFF counterparts, in which contours are always treated as implicitly closed by a lineto. This seems to support option (3) that we should auto-close open paths before applying booleanOperations (this is also the default for the BooleanGlyph class).

Just to confuse things a little more, I just tried to export a font containing open paths from Glyphs.app and RoboFont.

In Glyphs.app, when exporting a TTF, open paths are dropped if the "Remove Overlaps" option is checked at export time, otherwise they are kept as is; whereas when exporting a CFF font, open paths are always dropped whether I check the "Remove Overlaps" option or not.

With RoboFont.app, on the other hand, when I export a TTF, open paths are dropped if the "Remove Overlaps" option is not checked at export time; otherwise they are auto-closed and merged; whereas when exporting a CFF, open paths are kept when "Remove Overlaps" option is not checked, else they are auto-closed and merged.