[reverseContourPen] don't imply closing lineTo if == moveTo #1080
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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
added a commit
that referenced
this pull request
Oct 25, 2017
This test is not normally run. It is skipped when ufoLib is not importable, as it's the case from the test runner's virtual environment. The only reason it exists is so that I could check that the ReverseContourPen I was writing behaves the same as using ufoLib's ReverseContourPointPen when the latter is run through the SegmentToPointPen and PointToSegmentPen converters. The two methods for reversing contours now diverge since #1080, but only nominally because both produce effectively the same results. The only difference is that, when using the point pens with outputImpliedClosingLine=True, the closing lineTo is always there even when it's redundant. In our implmentation, we only output the closing lineTo when it is the same as moveTo (this was necessary in order to fix googlefonts/cu2qu#51) Nevertheless, the total number of points is the same in both cases. Maybe this test should not be here, as it's testing functionalities from a different package. Closes #1081
6FC1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, for closed paths, we are always dropping a lineTo segment that follows moveTo, because after reversing the contour this lineTo becomes the last segment, and in the Pen protocol a closePath always implies a line to the fist point.
This is OK when in the original contour the move point and the following lineTo's oncurve point (which will become the last point after reversal) do not overlap.
However, if they do overlap, we end up dropping the duplicate point.
This cu2qu issue exemplifies the problem: googlefonts/cu2qu#51
(Note that cu2qu actually uses the
ReverseContourPointPenwrapped by ufoLib's converter pens, butfontTools' ReverseContourPen works the same)
Now, with this patch, the ReverseContourPen will emit the last lineTo when it is the same as moveTo, thus keeping the duplicate points in this case.