Fix issue #78 #81
Fix issue #78 #81
Conversation
Issue diagrams#78 "union does not produce expected result when used with difference" occurs because difference is returning loops that wind the opposite direction from what is expected. The solution is to enforce that the outermost loops wind counterclockwise to be consistent with the direction that the shape functions provided by diagrams-lib wind. A more precise solution would be to match the winding of the outermost loops to that of the input loops they are constructed from, but the intersection functions in diagrams-lib do not currently support finding the intersections between segments that overlap, which prevents us from determining which input loops correspond to the outermost output loops.
|
|
||
| -- Test if a loop winds clockwise. | ||
| isClockwise :: Located (Trail' Loop V2 Double) -> Bool | ||
| isClockwise l = sample l (atStart l) < 0 |
There was a problem hiding this comment.
I don't follow how this test is working.
There was a problem hiding this comment.
Points that are precisely on a loop as well as those inside the loop will have non-zero winding number. The winding number of points in a loop will be positive if the loop is counterclockwise and negative if the loop is clockwise.
| go ((Node n ns):ts) (l:ls) | l `contains` n = go [Node l [Node n ns]] ls | ||
| | n `contains` l = go (Node n (go ns [l]) : ts) ls | ||
| | otherwise = go (Node n ns : go ts [l]) ls | ||
| contains = containsBy fill |
There was a problem hiding this comment.
I don't understand how this could be right with Winding. If the loop contains another loop, then it must already be CC. If it isn't CC, it will "contain" loops that are outside it.
There was a problem hiding this comment.
clock vs anticlockwise doesn't change what is contained by a loop with Winding, since it just changes the sign of the winding number and the winding rule is /= 0
There was a problem hiding this comment.
Ah, ok. I was thinking > 0. What is a case where you would want a different FillRule for this test, or in other words, why not contains = containsBy Winding?
There was a problem hiding this comment.
If there are self intersections in l or n it may be possible that containsBy Winding will be true, but containsBy EvenOdd will be false.
| -- because the binary operations guarantee that the loops do not intersect. | ||
| containsBy :: FillRule -> Located (Trail' Loop V2 Double) -> Located (Trail' Loop V2 Double) -> Bool | ||
| containsBy Winding s t = isInsideWinding s (atStart t) | ||
| containsBy EvenOdd s t = isInsideEvenOdd s (atStart t) |
There was a problem hiding this comment.
This seems destined to corner cases where the start is right on the edge. This seems like a rather common case if these trails came from intersecting. I think we need some comments laying out the context of what loops we could be getting at this point.
There was a problem hiding this comment.
Not entirely sure what you mean by "right on the edge". Assuming you mean that (atStart t) is a point on s, in that case the binary operation will have produced a single loop, not two separate loops.
There was a problem hiding this comment.
Again, I'm not entirely clear on the context, but I'm thinking of something like the exclusion example in the manual. The corners of the interior loop are on the edge of the outer loop.
There was a problem hiding this comment.
You're right. It's definitely possible to detect this case, but resolving it does not seem straight forwards. I'l have to think about it.
There was a problem hiding this comment.
I know how to fix this. Now the fun part is that it relies on an unmerged changeset I've been working on for IntersectionExtras and it creates a circular module dependency with that changeset. Time to go finish that and break it out into two modules.
| containsBy Winding s t = isInsideWinding s (atStart t) | ||
| containsBy EvenOdd s t = isInsideEvenOdd s (atStart t) | ||
|
|
||
| -- Test if a loop winds clockwise. |
There was a problem hiding this comment.
Thanks for the clarification! Lets add this to the comment and somewhere we should add a test:
prop_sample_start :: Located (Trail' Loop V2 Double) -> Bool
prop_sample_start l = sample l (atStart l) /= 0| -- Test if a loop winds clockwise. | |
| -- Test if a loop winds clockwise. | |
| -- Points that are precisely on a loop as well as those inside the loop will | |
| -- have non-zero winding number. The winding number of points in a loop | |
| -- will be positive if the loop is counterclockwise and negative if the loop | |
| -- is clockwise. |
There was a problem hiding this comment.
I'll add the comment, but the prop isn't true because in the case that l contains zero area sample l (atStart l) == 0.
There was a problem hiding this comment.
Thinking about it more, I'm not really sure how reliably sample l (atStart l) /= 0. I don't entirely trust the code that finds winding numbers (see diagrams-lib #337). I can add a redundant check in the case that sample l (atStart l) == 0.
Issue #78 "union does not produce expected result when used with
difference" occurs because difference is returning loops that wind the
opposite direction from what is expected. The solution is to enforce
that the outermost loops wind counterclockwise to be consistent with the
direction that the shape functions provided by diagrams-lib wind. A more
precise solution would be to match the winding of the outermost loops to
that of the input loops they are constructed from, but the intersection
functions in diagrams-lib do not currently support finding the
intersections between segments that overlap, which prevents us from
determining which input loops correspond to the outermost output loops.