Fix some issues with Sketch.hull() #1344
Conversation
| @@ -187,6 +189,9 @@ def _pt_arc(p: Point, a: Arc) -> Tuple[float, float, float, float]: | |||
| xc, yc = a.c.x, a.c.y | |||
| dx, dy = x - xc, y - yc | |||
| l = sqrt(dx ** 2 + dy ** 2) | |||
| ld = l ** 2 - r ** 2 | |||
| if (ld < 0.001): | |||
| return x, y, x, y | |||
There was a problem hiding this comment.
This is when the point is on the arc's circle, so I return a no-length segment (which is later ignored). The point might not necessarily be on the arc though, so this should return a segment to the start or end of the arc.
This is taken care of elsewhere because I add the start and end points of the arc to the list of points in convert_and_validate, so if the next segment is between the point and a start or end point of an arc, it's handled elsewhere as a pt_pt comparison.
| angles = atan2p(x - x1, y - y1), atan2p(x - x2, y - y2) | ||
| points = Point(x1, y1), Point(x2, y2) | ||
|
|
||
| ix = int(argmax(angles)) | ||
| ix = int(argmax([a + 2*pi if a < angle else a for a in angles])) |
There was a problem hiding this comment.
When the angle loops around back past 0 radians, this caused the argmin/argmax of pt_arc and arc_pt to choose the wrong side of the arc to continue from.
| return angles[ix], segments[ix] | ||
| segment_angle = atan2p(segments[ix].b.x - xc2, segments[ix].b.y - yc2) | ||
| if (segment_angle < a2.a1 or segment_angle > a2.a2): | ||
| return inf, Segment(Point(inf, inf), Point(inf, inf)) |
There was a problem hiding this comment.
I don't understand this function that well, so this might have been from something else wrong and is now redundant, but somehow it was selecting a point not actually on the arc, so this if statement throws that out if that happens.
I'd bet this isn't the best way to handle it and there's more edge cases that would fail here.
| if (current.s == e.e and not e.s == current.e): | ||
| return pt_pt(current.e, e.s) | ||
| if (e.s == current.e and not current.s == e.e): | ||
| return (current.a2 + pi) % (2 * pi), Segment(Point(inf, inf), Point(inf, inf)) |
There was a problem hiding this comment.
These changes allow a circle that's broken into multiple arcs continue around the circle
| # march around | ||
| while not finished: | ||
| while not finished and iterations < len(entities): | ||
| iterations += 1 |
There was a problem hiding this comment.
While working on this it was very easy for this to loop infinitely. Setting a sensible maximum number of iterations (if we've iterated more than the number of entities, something has gone wrong) seems better than having the system hang in an infinite loop.
| @@ -18,7 +18,7 @@ def test_hull(): | |||
|
|
|||
| h = hull.find_hull(edges) | |||
|
|
|||
| assert len(h.Vertices()) == 11 | |||
| assert len(h.Vertices()) == 10 | |||
There was a problem hiding this comment.
Somehow my changes made this hull have one fewer vertices. Based on how the points and arcs are laid out, I'm guessing there were multiple possible next entities with the same angle and my hull changes made it skip one.
There should probably be a 'tie-breaker' for choosing the smallest angle so that the system is more deterministic. (either always choosing the closer to ensure the loop always ends up closed, or choosing the further, but we'd have to be more careful about choosing the starting entity, lest we start on an entity we later skip and fail to close the loop)
I wanted to use hull for something I'm building, and immediately ran into this issue: #1224, so I gave it a shot at fixing it.
I wouldn't put money on this being perfect / not having an issue I missed, but this worked for the few examples I tried, and for the project I'm working on.