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

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

Open
skef opened this issue Oct 24, 2022 · 0 comments
Open

Comments

@skef
Copy link

skef commented Oct 24, 2022

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.

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

No branches or pull requests

1 participant