booleanOperations icon indicating copy to clipboard operation
booleanOperations copied to clipboard

Improved mergeFirstSegments handling

Open skef opened this issue 2 years ago • 2 comments

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 avatar Oct 24 '22 08:10 skef

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

skef avatar Oct 30 '22 07:10 skef

Similarly, this commit – thank you @anthrotype for taking a look, time permitting.

frankrolf avatar Sep 30 '24 13:09 frankrolf

hey, sorry but I don't think I have enough bandwith to do a proper review, I'm afraid. If @typemytype is happy about the change, go on with it..

anthrotype avatar Oct 03 '24 07:10 anthrotype