PointToSegmentPen: preserve duplicate last point

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 https://github.com/googlefonts/fontmake/issues/572.

Lib/fontTools/pens/pointPen.py

@@ -180,18 +180,36 @@ class PointToSegmentPen(BasePointToSegmentPen):
 			pen.moveTo(movePt)
 		outputImpliedClosingLine = self.outputImpliedClosingLine
 		nSegments = len(segments)
+		lastPt = movePt
 		for i in range(nSegments):
 			segmentType, points = segments[i]
 			points = [pt for pt, smooth, name, kwargs in points]
 			if segmentType == "line":
 				assert len(points) == 1, "illegal line segment point count: %d" % len(points)
 				pt = points[0]
-				if i + 1 != nSegments or outputImpliedClosingLine or not closed:
+				# For closed contours, a 'lineTo' is always implied from the last oncurve
+				# point to the starting point, thus we can omit it when the last and
+				# starting point don't overlap.
+				# However, when the last oncurve point is a "line" segment and has same
+				# coordinates as the starting point of a closed contour, we need to output
+				# the closing 'lineTo' explicitly (regardless of the value of the
+				# 'outputImpliedClosingLine' option) in order to disambiguate this case from
+				# the implied closing 'lineTo', otherwise the duplicate point would be lost.
+				# See https://github.com/googlefonts/fontmake/issues/572.
+				if (
+					i + 1 != nSegments
+					or outputImpliedClosingLine
+					or not closed
+					or pt == lastPt
+				):
 					pen.lineTo(pt)
+					lastPt = pt
 			elif segmentType == "curve":
 				pen.curveTo(*points)
+				lastPt = points[-1]
 			elif segmentType == "qcurve":
 				pen.qCurveTo(*points)
+				lastPt = points[-1]
 			else:
 				assert 0, "illegal segmentType: %s" % segmentType
 		if closed:

Tests/pens/pointPen_test.py

@@ -156,6 +156,67 @@ class PointToSegmentPenTest(unittest.TestCase):
                          "addPoint((20, 20)) addPoint((20, 40), segmentType='curve') endPath()",
                          repr(tpen))
 
+    def test_closed_outputImpliedClosingLine(self):
+        tpen = _TestSegmentPen()
+        ppen = PointToSegmentPen(tpen, outputImpliedClosingLine=True)
+        ppen.beginPath()
+        ppen.addPoint((10, 10), "line")
+        ppen.addPoint((10, 20), "line")
+        ppen.addPoint((20, 20), "line")
+        ppen.endPath()
+        self.assertEqual(
+            "10 10 moveto "
+            "10 20 lineto "
+            "20 20 lineto "
+            "10 10 lineto "  # explicit closing line
+            "closepath",
+            repr(tpen)
+        )
+
+    def test_closed_line_overlapping_start_end_points(self):
+        # Test case from https://github.com/googlefonts/fontmake/issues/572.
+        tpen = _TestSegmentPen()
+        ppen = PointToSegmentPen(tpen, outputImpliedClosingLine=False)
+        # The last oncurve point on this closed contour is a "line" segment and has
+        # same coordinates as the starting point.
+        ppen.beginPath()
+        ppen.addPoint((0, 651), segmentType="line")
+        ppen.addPoint((0, 101), segmentType="line")
+        ppen.addPoint((0, 101), segmentType="line")
+        ppen.addPoint((0, 651), segmentType="line")
+        ppen.endPath()
+        # Check that we always output an explicit 'lineTo' segment at the end,
+        # regardless of the value of 'outputImpliedClosingLine', to disambiguate
+        # the duplicate point from the implied closing line.
+        self.assertEqual(
+            "0 651 moveto "
+            "0 101 lineto "
+            "0 101 lineto "
+            "0 651 lineto "
+            "0 651 lineto "
+            "closepath",
+            repr(tpen)
+        )
+
+    def test_roundTrip2(self):
+        tpen = _TestPointPen()
+        ppen = PointToSegmentPen(SegmentToPointPen(tpen))
+        ppen.beginPath()
+        ppen.addPoint((0, 651), segmentType="line")
+        ppen.addPoint((0, 101), segmentType="line")
+        ppen.addPoint((0, 101), segmentType="line")
+        ppen.addPoint((0, 651), segmentType="line")
+        ppen.endPath()
+        self.assertEqual(
+            "beginPath() "
+            "addPoint((0, 651), segmentType='line') "
+            "addPoint((0, 101), segmentType='line') "
+            "addPoint((0, 101), segmentType='line') "
+            "addPoint((0, 651), segmentType='line') "
+            "endPath()",
+            repr(tpen)
+        )
+
 
 class TestSegmentToPointPen(unittest.TestCase):
 
@@ -409,3 +470,23 @@ class TestReverseContourPointPen(unittest.TestCase):
                          "endPath() "
                          "addComponent('base', [1, 0, 0, 1, 0, 0], identifier='foo')",
                          repr(tpen))
+
+    def test_closed_line_overlapping_start_end_points(self):
+        # Test case from https://github.com/googlefonts/fontmake/issues/572
+        tpen = _TestPointPen()
+        pen = ReverseContourPointPen(tpen)
+        pen.beginPath()
+        pen.addPoint((0, 651), segmentType="line")
+        pen.addPoint((0, 101), segmentType="line")
+        pen.addPoint((0, 101), segmentType="line")
+        pen.addPoint((0, 651), segmentType="line")
+        pen.endPath()
+        self.assertEqual(
+            "beginPath() "
+            "addPoint((0, 651), segmentType='line') "
+            "addPoint((0, 651), segmentType='line') "
+            "addPoint((0, 101), segmentType='line') "
+            "addPoint((0, 101), segmentType='line') "
+            "endPath()",
+            repr(tpen)
+        )

Tests/pens/reverseContourPen_test.py

@@ -278,6 +278,26 @@ TEST_DATA = [
             ('lineTo', ((848, 348),)),  # the duplicate point is kept
             ('closePath', ())
         ]
+    ),
+    # Test case from https://github.com/googlefonts/fontmake/issues/572
+    # An additional closing lineTo is required to disambiguate a duplicate
+    # point at the end of a contour from the implied closing line.
+    (
+        [
+            ('moveTo', ((0, 651),)),
+            ('lineTo', ((0, 101),)),
+            ('lineTo', ((0, 101),)),
+            ('lineTo', ((0, 651),)),
+            ('lineTo', ((0, 651),)),
+            ('closePath', ())
+        ],
+        [
+            ('moveTo', ((0, 651),)),
+            ('lineTo', ((0, 651),)),
+            ('lineTo', ((0, 101),)),
+            ('lineTo', ((0, 101),)),
+            ('closePath', ())
+        ]
     )
 ]