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.

Sign up for GitHub

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

Fix issue #78 #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issue #78 #81

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions src/Diagrams/TwoD/Path/Boolean.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ module Diagrams.TwoD.Path.Boolean
loopUnion, loopDifference,
loopIntersection, loopExclusion,)
where
import Control.Lens hiding (at)
import Control.Lens hiding (at, contains)
import Data.Maybe
import Data.Tree
import Diagrams.Located
import Diagrams.Parametric
import Diagrams.Path
import Diagrams.Points
import Diagrams.Query
import Diagrams.Segment
import Diagrams.Trail
import Diagrams.TrailLike
Expand Down Expand Up @@ -215,15 +218,15 @@ exclusion' tol fill path1 path2 =
loopUnion :: Double -> FillRule
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
loopUnion tol fill p =
loopUnion tol fill p = normalizeWindings fill $
map path2loop $ C.union (map loop2path p) (fillrule fill) tol

-- | Difference between loops. The loops in both lists are first merged using `union`.
loopDifference :: Double -> FillRule
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
loopDifference tol fill path1 path2 =
loopDifference tol fill path1 path2 = normalizeWindings fill $
map path2loop $ C.difference (map loop2path path1)
(map loop2path path2) (fillrule fill) tol

Expand All @@ -232,7 +235,7 @@ loopIntersection :: Double -> FillRule
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
loopIntersection tol fill path1 path2 =
loopIntersection tol fill path1 path2 = normalizeWindings fill $
map path2loop $ C.intersection (map loop2path path1)
(map loop2path path2) (fillrule fill) tol

Expand All @@ -241,6 +244,32 @@ loopExclusion :: Double -> FillRule
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
-> [Located (Trail' Loop V2 Double)]
loopExclusion tol fill path1 path2 =
loopExclusion tol fill path1 path2 = normalizeWindings fill $
map path2loop $ C.exclusion (map loop2path path1)
(map loop2path path2) (fillrule fill) tol

-- Force all top level loops to wind counterclockwise and revese inner loops as needed.
normalizeWindings :: FillRule -> [Located (Trail' Loop V2 Double)] -> [Located (Trail' Loop V2 Double)]
normalizeWindings fill = concat . map forceCC . nestedGroups fill where
forceCC ls | (l:_) <- ls, isClockwise l = map reverseLocLoop ls
| otherwise = ls

-- Group a list of loops such that the first element of each group contains all the others.
nestedGroups :: FillRule -> [Located (Trail' Loop V2 Double)] -> [[Located (Trail' Loop V2 Double)]]
nestedGroups fill = map flatten . go [] where
go ts [] = ts
go [] (l:ls) = go [Node l []] ls
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


-- To test if s contains t, we can merely test if any point of t is within s
-- 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good luck!


-- Test if a loop winds clockwise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

isClockwise :: Located (Trail' Loop V2 Double) -> Bool
isClockwise l = sample l (atStart l) < 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow how this test is working.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.