Skip to content
This repository has been archived by the owner on Aug 9, 2023. It is now read-only.

ReverseContourPen drops points #51

Closed
jamesgk opened this issue Sep 27, 2016 · 4 comments
Closed

ReverseContourPen drops points #51

jamesgk opened this issue Sep 27, 2016 · 4 comments

Comments

@jamesgk
Copy link
Contributor

jamesgk commented Sep 27, 2016

The following test will fail:

class TestCu2QuReverseContourPen(unittest.TestCase):
    def _draw_test_contour(self, pen):
        pen.moveTo((848, 348))
        pen.lineTo((848, 348))
        pen.qCurveTo((848, 526), (649, 704), (449, 704))
        pen.qCurveTo((449, 704), (248, 704), (50, 526), (50, 348))
        pen.lineTo((50, 348))
        pen.qCurveTo((50, 348), (50, 171), (248, -3), (449, -3))
        pen.qCurveTo((449, -3), (649, -3), (848, 171), (848, 348))
        pen.closePath()

    def test_duplicate_points_remain(self):
        pen1 = DummyPen()
        pen2 = DummyPen()
        self._draw_test_contour(pen1)
        self._draw_test_contour(ReverseContourPen(pen2))
        self.assertEqual(
            len(pen1.commands), len(pen2.commands),
            '\n%s\nreversed to:\n%s;\nsome points dropped.' % (pen1, pen2))
@anthrotype
Copy link
Member

Ouch. Does it happen with the ReverseContourPointPen as well?
If not, then it might be one of the Point->Segment->Point converter pens...

@anthrotype
Copy link
Member

This one passes, so it must not be the original ReverseContourPointPen that's dropping the point, but something else in the converter pens...

class TestCu2QuReverseContourPointPen(unittest.TestCase):
    def _draw_test_contour(self, pen):
        pen.beginPath()
        pen.addPoint((848, 348), segmentType='line')
        pen.addPoint((848, 348), segmentType='line')
        pen.addPoint((848, 526))
        pen.addPoint((649, 704))
        pen.addPoint((449, 704), segmentType='qcurve')
        pen.addPoint((449, 704))
        pen.addPoint((248, 704))
        pen.addPoint((50, 526))
        pen.addPoint((50, 348), segmentType='qcurve')
        pen.addPoint((50, 348), segmentType='line')
        pen.addPoint((50, 348))
        pen.addPoint((50, 171))
        pen.addPoint((248, -3))
        pen.addPoint((449, -3), segmentType='qcurve')
        pen.addPoint((449, -3))
        pen.addPoint((649, -3))
        pen.addPoint((848, 171))
        pen.addPoint((848, 348), segmentType='qcurve')
        pen.endPath()

    def test_duplicate_points_remain(self):
        pen1 = DummyPointPen()
        pen2 = DummyPointPen()
        self._draw_test_contour(pen1)
        self._draw_test_contour(ReverseContourPointPen(pen2))
        self.assertEqual(
            len(pen1.commands), len(pen2.commands),
            '\n%s\nreversed to:\n%s;\nsome points dropped.' % (pen1, pen2))

anthrotype added a commit to anthrotype/fonttools that referenced this issue Oct 24, 2017
Previously, for closed paths, we were always dropping a lineTo segment
that followed moveTo, because after reversing the contour this lineTo
would become the last segment, and in the Pen protocol a closePath
always implies a line to the fist point.

This is OK when the move point and the following lineTo oncurve point
(which becomes last after reversal) don't overlap.

However, if they do, we ended up dropping the duplicate point.

This cu2qu issue exemplify the problem (cu2qu actually uses the
ReverseContourPointPen wrapped by ufoLib's converter pens, but
fontTools' ReverseContourPen does exactly the same):

googlefonts/cu2qu#51

With this patch, the ReverseContourPen now emits the last lineTo
when it is the same as moveTo.
anthrotype added a commit to anthrotype/fonttools that referenced this issue Oct 24, 2017
One way to work around googlefonts/cu2qu#51
when using the ufoLib's ReverseConturPointPen via the converter pens
would be to pass the outputImpliedClosingLine=True argument
(False by default) to the PointToSegmentPen.

This way, all the final lineTos are explicitly outputted, even when
they are redundant and could be implied (i.e. when they are not
duplicate points).

Note that this test is skipped by the CI, because ufoLib is not a
dependency of fonttools; you can run locally if you wish.
@anthrotype
Copy link
Member

It turns out it was the PointToSegmentPen which defaults to outputImpliedClosingLine=False. Making that True would fix this.
Even better, cu2qu could now use fontTools' reverseContourPen directly, without wrapping ufoLib's point pen with the convert pens.
I fixed this issue in fonttools/fonttools#1080, I will update cu2qu soon after releasing the next fonttools.

@anthrotype
Copy link
Member

not so fast. let's keep this open until we update cu2qu's fonttools requirement.

@anthrotype anthrotype reopened this Oct 25, 2017
anthrotype added a commit to anthrotype/cu2qu that referenced this issue Oct 30, 2017
we need that for the bugfix in ReverseContourPen
googlefonts#51

also bump ufoLib to >= 2.1.1, just because.
anthrotype added a commit to anthrotype/cu2qu that referenced this issue Oct 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants