ReverseContourPen: duplicate LineTo after MoveTo when contour ends w/ CurveTo

[anthrotype]

Take this simple closed contour that starts and ends with a CurveTo:

let contour = BezPath::from_vec(vec![
    PathEl::MoveTo((0.0, 0.0).into()),
    PathEl::CurveTo(
        (1.0, 1.0).into(),
        (2.0, 2.0).into(),
        (3.0, 3.0).into(),
    ),
    PathEl::CurveTo(
        (4.0, 4.0).into(),
        (5.0, 5.0).into(),
        (0.0, 0.0).into(),
    ),
    PathEl::ClosePath,
]);
let mut bez_pen = BezPathPen::new();
let mut rev_pen = ReverseContourPen::new(&mut bez_pen);
write_to_pen(&contour, &mut rev_pen);
rev_pen
    .flush()
    .unwrap();
let reversed = bez_pen.into_inner();
println!("{:#?}", reversed);

After reversing with ReverseContourPen, a duplicate LineTo segment appears after the MoveTo which was not there to begin with.

BezPath(
    [
        MoveTo(
            (0.0, 0.0),
        ),
-        LineTo(
-           (0.0, 0.0),
-       ),
        CurveTo(
            (5.0, 5.0),
            (4.0, 4.0),
            (3.0, 3.0),
        ),
        CurveTo(
            (2.0, 2.0),
            (1.0, 1.0),
            (0.0, 0.0),
        ),
    ],
)

This will produce an extra point if we compile that BezPath to a glyf SimpleGlyph with fontc. We need to find where that exactly gets added and get rid of it.

[rsheeter]

IIRC this was meant to help retain interpolation compatibility when reversing.

[anthrotype]

no no, b/c fontmake does not add them and contours are still interpolation compatible, so we do have some mismatch somewhere

[anthrotype]

we don't want the duplicate points to be added, they only appear when ReverseContourPen gets used, so we should fix ReverseContourPen to match the python equivalent (which has no qualms about interpolation compatibility)

[anthrotype]

If you'd like to reproduce this duplicate points issue, you can use write-fonts from my filter-implied-oncurves PR #368 and then patch fontmake-rs to use the new simple_glyphs_from_kurbo method introduced there (fontmake-rs patch is in #368 (comment)). Then try to build Oswald.glyphs with fontc and inspect the differences between the font built with fontmake-py and the one built with the patched fontc, especially the glyph called "hryvnia" which contains the flipped component. You'll notice the duplicate oncurve (lineto) points being added (the + in the diff below):

@@ -10534,6 +10325,7 @@
       </contour>
       <contour>
         <pt x="219" y="-9" on="1"/>
+        <pt x="219" y="-9" on="1"/>
         <pt x="290" y="-9" on="0"/>
         <pt x="382" y="51" on="0"/>
         <pt x="430" y="157" on="0"/>
@@ -10579,6 +10371,7 @@
         <pt x="25" y="124" on="0"/>
         <pt x="75" y="37" on="0"/>
         <pt x="163" y="-9" on="0"/>
+        <pt x="219" y="-9" on="1"/>
       </contour>

[rsheeter]

Apparently when writing the pen I thought I did need the implicit closing line, per

fontations/write-fonts/src/pens.rs

Lines 156 to 163 in b1b7f7e

	/// Buffers commands until a close is seen, then plays in reverse on inner pen.
	///
	/// Reverses the winding direction of the contour. Keeps the first point unchanged. In FontTools terms
	/// we implement only reversedContour(..., outputImpliedClosingLine=True) as this appears to be necessary
	/// to ensure retention of interpolation.
	///
	/// <https://github.com/fonttools/fonttools/blob/78e10d8b42095b709cd4125e592d914d3ed1558e/Lib/fontTools/pens/reverseContourPen.py#L8>
	pub struct ReverseContourPen<'a, T: Pen> {

Either I botched my python test or the FontTools reverse doesn't give me compatible outlines when I reverse your example curve and then your example curve with the last curveTo ending at 6,6 instead of 0,0:

from fontTools.pens.basePen import BasePen
from fontTools.pens.reverseContourPen import ReverseContourPen

class CommandPen(BasePen):
    def __init__(self):
        self.commands = []

    def _moveTo(self, pt):
        self.commands.append("M")
    def _lineTo(self, pt):
        self.commands.append("L")
    def _curveToOne(self, pt1, pt2, pt3):
        self.commands.append("C")
    def _closePath(self):
        self.commands.append("Z")

def draw(pen, endp):
    pen.moveTo((0, 0))
    pen.curveTo((1,1), (2,2), (3,3))
    pen.curveTo((4,4), (5,5), endp)
    pen.closePath()

for implicit_close in (False, True):
    print(f"outputImpliedClosingLine={implicit_close}")
    pen = CommandPen()
    draw(ReverseContourPen(pen, implicit_close), (0, 0))
    print("  ", "".join(pen.commands))

    pen = CommandPen()
    draw(ReverseContourPen(pen, implicit_close), (6, 6))
    print("  ", "".join(pen.commands))

# if we get the same sequence of commands we're interpolation compatible ... but we never do?!
#outputImpliedClosingLine=False
#   MCCZ
#   MLCCZ
#outputImpliedClosingLine=True
#   MCCZ
#   MLCCZ

If I try the same in Rust the command sequences match:

    fn reverse(endp: Point) -> BezPath {
        let contour = BezPath::from_vec(vec![
            PathEl::MoveTo((0.0, 0.0).into()),
            PathEl::CurveTo(
                (1.0, 1.0).into(),
                (2.0, 2.0).into(),
                (3.0, 3.0).into(),
            ),
            PathEl::CurveTo(
                (4.0, 4.0).into(),
                (5.0, 5.0).into(),
                endp,
            ),
            PathEl::ClosePath,
        ]);
        let mut bez_pen = BezPathPen::new();
        let mut rev_pen = ReverseContourPen::new(&mut bez_pen);
        write_to_pen(&contour, &mut rev_pen);
        rev_pen
            .flush()
            .unwrap();
        bez_pen.into_inner()
    }

    fn cmd_seq(path: &BezPath) -> String {
        path.elements().iter()
            .map(|el| match el {
                PathEl::MoveTo(..) => "M",
                PathEl::LineTo(..) => "L",
                PathEl::CurveTo(..) => "C",
                PathEl::QuadTo(..) => "Q",
                PathEl::ClosePath => "Z",
            })
            .collect()
    }

    #[test]
    fn test_reverse_pen() {
        let rev1 = reverse((0.0, 0.0).into());
        let rev2 = reverse((6.0, 6.0).into());
        assert_eq!(cmd_seq(&rev1), cmd_seq(&rev2));
    }

[rsheeter]

My naive thought is for dropping that line_to to be safe you'd want to walk all the outlines in parallel and only drop it if it's unnecessary in all of them.

[anthrotype]

I'm not convinced. Your two example contours are indeed not interpolation compatible to begin with, one has an implicit closing line (because the last curveTo point != moveTo) whereas the other does not!
What we want is to start from list of points (as in the source), then convert to BezPath (i.e. segments) and then back to points (glyf) and make sure we do not add any extra points and that we stay interp-compatible.
We shall port all the tests from fontTools/Tests/pens/reverseContourPen_test.py and verify that fontations ReverseContourPen does exactly the same as the python one when outputImpliedClosingLine=True.

[anthrotype]

I ported all parametrized tests from reverseContourPen_test.py in 3806c4d

a lot of them fail, we want to make them all pass

[anthrotype]

Right now we only use ReverseContourPen for reversing contours of decomposed components when their transform has negative determinant, so this issue has less impact.
However when we will implement fontmake-py's default behavior of reversing contours' winding direction when building TrueType glyf outlines (unless --keep-direction option is used), all simple glyphs risk to be affected
Cf. googlefonts/fontc#174

[anthrotype]

the two contours from your example are not compatible because one (curveTo ending on top of move) has effectively 6 points (or 2 cubic segments), the other one has 7 points (2 cubics and one line) because the last oncurve point does not overlap the starting point and the closePath adds an implicit closing line:

from fontTools.pens.pointPen import SegmentToPointPen
from fontTools.pens.recordingPen import RecordingPointPen


def draw(pen, endp):
    pen.moveTo((0, 0))
    pen.curveTo((1, 1), (2, 2), (3, 3))
    pen.curveTo((4, 4), (5, 5), endp)
    pen.closePath()


# both the following contours are closed, but the second one is implicitly closed
# by a lineTo the starting point, whereas the first one ends with a curveTo
# the starting point.

rec = RecordingPointPen()
pen = SegmentToPointPen(rec)
draw(pen, (0, 0))
print(rec.value)
# this contour only contains 6 points in total, or two cubic curve segments
assert rec.value == [
    ("beginPath", (), {}),
    ("addPoint", ((0, 0), "curve", False, None), {}),
    ("addPoint", ((1, 1), None, False, None), {}),
    ("addPoint", ((2, 2), None, False, None), {}),
    ("addPoint", ((3, 3), "curve", True, None), {}),
    ("addPoint", ((4, 4), None, False, None), {}),
    ("addPoint", ((5, 5), None, False, None), {}),
    ("endPath", (), {}),
]


rec = RecordingPointPen()
pen = SegmentToPointPen(rec)
draw(pen, (6, 6))
print(rec.value)
# this contour contains 7 points in total, i.e. two cubic curves and one line
assert rec.value == [
    ("beginPath", (), {}),
    ("addPoint", ((0, 0), "line", False, None), {}),
    ("addPoint", ((1, 1), None, False, None), {}),
    ("addPoint", ((2, 2), None, False, None), {}),
    ("addPoint", ((3, 3), "curve", True, None), {}),
    ("addPoint", ((4, 4), None, False, None), {}),
    ("addPoint", ((5, 5), None, False, None), {}),
    ("addPoint", ((6, 6), "curve", False, None), {}),
    ("endPath", (), {}),
]

# the two contours are in fact incompatible!

[anthrotype]

(sorry I just edited the above code, in the haste I didn't even bother running it 🤣 )

[anthrotype]

basically a closePath command only adds a closing line if the last oncurve point of a closed contour is the same as the move point. To show that I am not making this up, e.g. take a look at the way kurbo segment iterator omits the last line segment unless last != start:
https://github.com/linebender/kurbo/blob/01b52cd85c3a9b1be2c6e39ab97fcf401a8911c4/src/bezpath.rs#L689-L695

[anthrotype]

conversely when going segments => points, we pop the last point of a closed path when it has the same coordinates as the move point, cf. #341

[anthrotype]

no matter where we start, either from points (as the sources) or from BezPath (segments), we should get back what we input if we go through the other representation. The fontTools.pens.pointPen.{SegmentToPointPen,PointToSegmentPen} (the latter with outputImpliedClosingLine=True to simplify ourselves) are the ones that we should take as canonical reference

[rsheeter]

After discussion with Behdad I have filed fonttools/fonttools#3093.