Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved mergeFirstSegments handling #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skef
Copy link

@skef skef commented Oct 24, 2022

This is a set of changes I feel more confident about, in that:

  1. They fix some already filed issues (see mergefirst.zip for three glyphs discussed in the past)
  2. They also fix issues in my synthetic tests. Some examples: iso_input.zip
  3. They don't seem to cause rendering problems in other glyphs.

An earlier version of this PR was filed as a draft because it produced extra zero length lines. After looking at that in more depth I realized I was over-complicating the fix. This version is simpler and should be in better shape.

Some notes about what's going on here.

All this code at the start is worried about whether the first and last parts of segmentedFlatPoints has been split at a bad location. If it has been you'll get extra line segments corresponding to the last bit a curve, which ideally you'd like to avoid. (There may be other problems -- I haven't gone too far down that path of exploration.)

If you join the segments under the wrong circumstances, though, you'll wind up with missing points on the contour. When the missing point is part of a curve that's because of this sort of code:

elif flatSegment[0] != inputSegment.flat[0] and flatSegment[-1] != inputSegment.flat[-1]:
    # needed the a middle part of the segment
    if previousIntersectionPoint is None:
        previousIntersectionPoint = self._scalePoint(flatSegment[0])
    tValues = inputSegment.tValueForPoint(previousIntersectionPoint)
    searchPoint = self._scalePoint(flatSegment[-1])
    tValues.extend(inputSegment.tValueForPoint(searchPoint))
    curveNeeded = 1
    replacePointOnNewCurve = [(0, previousIntersectionPoint), (3, searchPoint)]
    previousIntersectionPoint = searchPoint

if flatSegment[-1] is some point actually on another segment, we both use a weird t value and, more importantly, don't end the segment at its actual end, resulting in a missing point.

(You get different problems when merging line segments in certain cases.)

The problem with the earlier code is that it is sometimes merging segments that are "turned around". If fp == lastInputSegment.scaledPreviousOnCurve and lp is in flatInputPoints and lastInputSegment, lp can be at lastInputSegment.flat[0] or lastInputSegment.flat[-1]. It's only correct to merge in the former case.

@skef skef marked this pull request as ready for review October 30, 2022 07:45
@skef
Copy link
Author

skef commented Oct 30, 2022

I pushed another commit to this PR and updated the explantion. I also upgraded it from a draft to ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant