PointToSegmentPen: preserve duplicate last point #1720
Conversation
The PointToSegmentPen translates between PointPen and (Segment)Pen protocol. In the SegmentPen protocol, closed contours always imply a final 'lineTo' segment from the last oncurve point to the starting point. So the PointToSegmentPen omits the final 'lineTo' segment for closed contours -- unless the option 'outputImpliedClosingLine' is True (it is False by default, and defcon.Glyph.draw method initializes the converter pen without this option). However, if the last oncurve point is on a "line" segment and has same coordinates as the starting point of a closed contour, the converter pen must always output the closing 'lineTo' explicitly (regardless of the value of the 'outputImpliedClosingLine' option) in order to disambiguate this case from the implied closing 'lineTo'. If it doesn't do that, a duplicate 'line' point at the end of a closed contour gets lost in the conversion. See googlefonts/fontmake#572.
|
ignore for now the unrelated appveyor failure (see #1718 (comment)) |
| i + 1 != nSegments | or outputImpliedClosingLine | |
| or not closed | ||
| or pt == lastPt |
There was a problem hiding this comment.
the counterpart of this is in SegmentToPointPen.closePath, where there is a similar comparison of the coordinates of the last and first points, which determines whether there's an implied line or not:
fonttools/Lib/fontTools/pens/pointPen.py
Lines 247 to 250 in afdb967
|
I am confident this is the correct solution. I am surprised the bug didn't surface earlier, given how widely used the ufoLib pointPens are. I guess the loss of a duplicate point was not big issue in normal circumstances, unless strict point-wise compatibility of contours is required as in variable font workflows. |
|
Yeah, the varfont use case exposes this as an issue indeed. Before, I think it was pretty much never desirable to have a duplicate point anywhere. |
|
thanks for having look on that.. testing it with fontmake (origin of this issue) gives now different error |
|
If |
When collecting a glyph's segments, we can't simply call the glyphs' draw method with the GetSegmentsPen, but we must initialize the PointToSegmentPen explicitly with outputImpliedClosingLine=True. By default PointToSegmentPen does not outputImpliedClosingLine -- unless last and first point on closed contour are duplicated. Because we are converting multiple glyphs at the same time, we want to make sure the _get_segments function returns the same number of segments, whether or not the last and first point overlap. Fixes googlefonts/fontmake#572 Also see: fonttools/fonttools#1720
The PointToSegmentPen translates between PointPen and (Segment)Pen protocol.
In the SegmentPen protocol, closed contours always imply a final 'lineTo' segment from the last oncurve point to the starting point.
So the PointToSegmentPen omits the final 'lineTo' segment for closed contours -- unless the option 'outputImpliedClosingLine' is True (it is False by default, and defcon.Glyph.draw method initializes the
converter pen without this option).
However, if the last oncurve point is on a "line" segment and has same coordinates as the starting point of a closed contour, the converter pen must always output the closing 'lineTo' explicitly (regardless of the value of the 'outputImpliedClosingLine' option) in order to disambiguate this case from the implied closing 'lineTo'.
If it doesn't do that, a duplicate 'line' point at the end of a closed contour gets lost in the conversion.
See googlefonts/fontmake#572.