typemytype / booleanOperations

Boolean operations on paths
MIT License
39 stars 18 forks source link

Improved mergeFirstSegments handling #67

Open skef opened 2 years ago

skef commented 2 years ago

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 commented 2 years ago

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

frankrolf commented 2 months ago

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

anthrotype commented 2 months ago

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..