Implement a subset of FontTools Pens for write-fonts, leaning on kurbo for things like transform

[rsheeter]

Having this makes porting algorithms implemented against pens in FontTools and nanoemoji easier.

My immediate need is to match https://github.com/googlefonts/ufo2ft/blob/dd738cdcddf61cce2a744d1cafab5c9b33e92dd4/Lib/ufo2ft/util.py#L205 for googlefonts/fontc#123 so I need transform and reverse. I will presumably need a glyf pen too but I haven't done that yet.

[dfrg]

Looks good with some suggestions for more idiomatic code.

[anthrotype]

what you're doing here (replacing the last LineTo of the reversed path with a ClosePath when it overlaps the starting point) is problematic because you may inadvertently drop a duplicate lineTo point that immediately followed moveTo pre-reversal and which deliberately has the same (x,y) coordinates, thus potentially making VF sources incompatible after reversing.

In fontTools ReverseContourPen (as well as in the PointToSegmentPen that converts between points and segment-oriented pens) we have an option outputImpliedClosingLine that when True it always output the implied closing line regardless of whether the last point overlaps moveTo or not. Setting this option is preferable when working with VF masters.
eg. see this font where some contour points collaps onto one another but only in some masters googlefonts/fontmake#572

But even when outputImpliedClosingLine=False, fonttools ReverseContourPen will not omit the last lineTo (which was the first one, before reversing) if it overlaps with the starting point, otherwise the number of points and segments would change after reversing
See googlefonts/cu2qu#51 (fixed by fonttools/fonttools#1080)

see this test case:
https://github.com/fonttools/fonttools/blob/bf265ce49e0cae6f032420a4c80c31d8e16285b8/Tests/pens/reverseContourPen_test.py#L227-L250

and also this one: https://github.com/fonttools/fonttools/blob/bf265ce49e0cae6f032420a4c80c31d8e16285b8/Tests/pens/reverseContourPen_test.py#L38-L53

actually all the tests in reverseContourPen_test.py would be nice to port over...

[anthrotype]

what you're doing here (replacing the last LineTo of the reversed path with a ClosePath when it overlaps the starting point)

I read the code again and I think I got confused.. from my testing, it looks like you are already emitting reversed paths which look like the ones from fonttools ReverseContourPen with outputImpliedClosingLine=True
I'll take a break and come back to this

[rsheeter]

Good call re porting tests, will do.

[rsheeter]

I'm dubious about the FontTools practice of eliminating the line from a close during reversal. Imagine you have several versions of move/line/line/close and in some of them the last line ends at the start ... if you eliminate the "unnecessary" line from close as suggested by https://github.com/fonttools/fonttools/blob/bf265ce49e0cae6f032420a4c80c31d8e16285b8/Tests/pens/reverseContourPen_test.py#L23 - note that an input MLLLC turns into an output MLLC - then these shapes will no longer interpolate.

[rsheeter]

I suppose ^ is to say I like the outputImpliedClosingLine=True behavior and do not currently implement the FontTools default mode of dropping the closing lines.

[anthrotype]

I think I found an issue. All your closed test shapes end with a line_to so this does not show up in your tests.
Basically, whenever the last pen command before the close is not a line_to (e.g. a curve_to or quad_to or move_to even), then after reversing you end up with an additional line segment that overlaps the move.
E.g. given this path (notice it ends with a curve_to before the closing 'Z'):

"M100,100 L150,200 L50,200 C75,200 75,100 100,100 Z"

If I reverse it using your current code, I get this (I added commas for legibility):

"M100,100 L100,100 C75,100 75,200 50,200 L150,200 Z"

Note that first 'L' segment following the 'M' is a duplicate and was not in the original closed path.

Compare this with the output from fontTools ReverseContourPen:

from fontTools.pens.recordingPen import RecordingPen
from fontTools.pens.reverseContourPen import ReverseContourPen
rec = RecordingPen()
rev = ReverseContourPen(rec)
rev.moveTo((100.0, 100.0))
rev.lineTo((150.0, 200.0))
rev.lineTo((50.0, 200.0))
rev.curveTo((75.0, 200.0), (75.0, 100.0), (100.0, 100.0))
rev.closePath()
print(rec.value)
"""
[('moveTo', ((100.0, 100.0),)),
 ('curveTo', ((75.0, 100.0), (75.0, 200.0), (50.0, 200.0))),
 ('lineTo', ((150.0, 200.0),)),
 ('closePath', ())]
"""

[anthrotype]

All this talk about implied closing line lead me to discover what I believe is a bug in fonttools' implementation as well LOL

fonttools/fonttools#2995

The ReverseContourPen's outputImpliedClosingLine parameter is a very recent addition by behdad, we haven't released it yet (a parameter with that name previously existed only in the PointToSegmentPen). It turns out it has a very similar bug whereby a duplicate lineTo, overlapping moveTo, gets inserted after reversing a closed path.
The bug is not present in the default case, when outputImpliedClosingLine=False.

[rsheeter]

I added commas for legibility

@dfrg are we allowed to change the to_svg format for Kurbo? - it is quite illegible right now

[rsheeter]

Forgive my density but I don't quite follow.

E.g. given this path (notice it ends with a curve_to before the closing 'Z'):

"M100,100 L150,200 L50,200 C75,200 75,100 100,100 Z" => MLLCZ

If I reverse it using your current code, I get this (I added commas for legibility):

"M100,100 L100,100 C75,100 75,200 50,200 L150,200 Z" => MLCLZ

So before and after we have 1 M, 2 L's, 1 C, and a Z. In the original the Z is pointless - we already ended the C at 100,100 - so when reversed the first command is pointless. I feel like I'm missing something.

[dfrg]

I don’t think kurbo makes any particular guarantees about the svg output. I assume you’d prefer a format like the comment above?

[rsheeter]

I assume you’d prefer a format like the comment above?

Yes. The , helps spot which values are a point and the space before the letter makes it far easier to spot the commands.