remove overlaps failures

[kaydeearts]

Recording an older issue here that Zachary reported, medium priority:

Zachary: For some reason booleanOperations seems to fail on a few glyphs here and there. This happened when removing overlaps using makeinstancesufo and also just running removeOverlaps() with fontParts. I believe both call the same booleanOperations code. Running tx +V removes the overlaps correctly in this case.

Miguel: I've suggested leveraging tx's overlap removal functionality at adobe-type-tools/afdko: Issue #1054

Zachary: There were other glyphs, but I didn't spend time investigating. I saw how screwed up a few were and went the other route using tx +V. This test UFO has only this glyph in it. I ran checkoutlinesufo --all -e test-remove-overlaps.ufo

I used tx +V on Source Han Serif and ran visual diffs on the previous version to verify that nothing was off. That seemed fine when I tested months ago so used that for the actual release and then someone reported adobe-fonts/source-han-serif: Issue #117 which is happening in tx +V. Not sure why it only seems to be on this glyph, but is something to investigate.

Test file TEST-overlap-removal-tx.ufo.zip has a basic file where theta gets messed up when running tx -t1 +V TEST-overlap-removal-tx.ufo > test.pfa

test-remove-overlaps.ufo.zip TEST-overlap-removal-tx.ufo.zip

[skef]

The "just use tx's facility instead" approach only works if tx's code is better across the board, which I find doubtful.

If we were willing to pay the cost of detecting these problems by rasterizing the before and the after we could consider feeding problem glyphs into an alternate implementation. That will only detect change-related problems, of course, not instances were overlap wasn't removed. (Actually, if there is remaining overlap that doesn't affect the CFF1 winding rule then maybe we don't care anyway.)

[skef]

Recording this here prior to a PR.

This change to booleanOperatons fixes the problem and doesn't seem to cause other problems:

258,280d257
<     def tValueToPoint(self, t):
<         if self.segmentType == "curve":
<             on1 = self.previousOnCurve
<             off1 = self.points[0].coordinates
<             off2 = self.points[1].coordinates
<             on2 = self.points[2].coordinates
<             return _getCubicPoint(t, on1, off1, off2, on2)
<         elif segmentType == "line":
<             return _getLinePoint(t, self.previousOnCurve, self.points[0].coordinates)
<         elif segmentType == "qcurve":
<             raise NotImplementedError
<         else:
<             raise NotImplementedError
< 
<     def isOn(self, p):
<         if p is None:
<             return False
<         for t in self.tValueForPoint(p):
<             pp = self.tValueToPoint(t)
<             if _distance(p, pp) < _approximateSegmentLength/2.0:
<                 return True
<         return False
< 
747c724
<                             mergeFirstSegments = True
---
>                                 mergeFirstSegments = True
829,830d805
<                     if not inputSegment.isOn(previousIntersectionPoint):
<                         previousIntersectionPoint = None
832a808,809
>                         # if previousIntersectionPoint is None:
>                         #    previousIntersectionPoint = self._scalePoint(flatSegment[-1])
1150,1155d1126
< def _getLinePoint(t, pt0, pt1):
<     if t == 0:
<         return pt0
<     if t == 1:
<         return pt1
<     return pt0[0] + (pt1[0]-pt0[0]) * t, pt0[1] + (pt1[1]-pt0[1]) * t