typemytype / booleanOperations

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

Questionable code in `elif not hasOncurvePoints and _distance(fp, lp):` clause #66

Open skef opened 1 year ago

skef commented 1 year ago

After staring at this a long time I'm still not sure what do about it.

We ran into this geometry in a production font:

hole.zip

When run through booleanOperations "union" the no-on-curve-points hole winds up missing a point. I've repeatedly tried to synthesize other similar cases but I haven't had any luck so far.

The problem has to do with this code:

                    elif not hasOncurvePoints and _distance(fp, lp):
                        # Merge last segment with first segment if the distance between the last point and the first
                        # point is less than the step distance between the last two points. _approximateSegmentLength
                        # can be significantly smaller than this step size.
                        if len(segmentedFlatPoints[-1]) > 1:
                            f1 = segmentedFlatPoints[-1][-2]
                            f2 = segmentedFlatPoints[-1][-1]
                            stepLen = _distance(f1, f2)
                        else:
                            stepLen = _approximateSegmentLength*clipperScale

                        if _distance(fp, lp) <= stepLen:
                                mergeFirstSegments = True

and in particular the if len(segmentedFlatPoints[-1]) > 1 part, which was added to track a change made by Reed Roberts in Adobe's internal code. If you set this code back just the _approximateSegmentLength*clipperScale check the problem goes away.

"By inspection" I find this code very suspicious. We know almost nothing about the situation here (given the lack of any on-curve points), and merging the first and last segments at an inappropriate time is known to result in lost points. However, the enclosed glyph is the only identified case where it seems to fail. (I wrote a script to make tens of thousands of somewhat analogous cases but they all went through overlap removal fine.)

I played with this (which uses hasPoint() from the other PR):

                        if len(segmentedFlatPoints[-1]) > 1:
                            fpSet = set()
                            lpSet = set()
                            fps = self._scalePoint(fp)
                            lps = self._scalePoint(lp)
                            plps = self._scalePoint(segmentedFlatPoints[-1][-2])
                            for inputSegment in allInputSegments:
                                if (inputSegment.hasPoint(lps) and
                                        inputSegment.hasPoint(plps)):
                                    lpSet.add(inputSegment)
                                if inputSegment.hasPoint(fps):
                                    fpSet.add(inputSegment)
                            if lpSet and fpSet == lpSet:
                                mergeFirstSegments = True
                        elif _distance(fp, lp) < _approximateSegmentLength*clipperScale:
                            mergeFirstSegments = True

and while it does prevent the issue it also results in extra unneeded line segments in other cases. (That makes sense because this is being very conservative -- fp is often at an intersection between two splines.)

I'm happy to do more research on the problem but maybe some more eyes will help.