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

Add test for multipoly #56

Closed
wants to merge 5 commits into from
Closed

Add test for multipoly #56

wants to merge 5 commits into from

Conversation

rowanwins
Copy link
Collaborator

Adding a test fixture for multipolygon features. Lots of weirdness going on with all the operations. Currently the results of operations include points that it shouldn't. Have done much digging yet.

One thing that comes to mind is that the existing multipoly test fixtures don't overlap eachother, eg they are disjoint from one another.

PS Not helped that leaflet is having some troubles working out how to render it properly :(

@rowanwins
Copy link
Collaborator Author

So just did some quick playing and think Im progressing towards the solution, basically whereever there was a se1.isSubject === se2.isSubject test I also added a se1.contourId === se2.contourId test

So for example

  if (nintersections === 2 && se1.isSubject === se2.isSubject) {
  \\ becomes
  if (nintersections === 2 && (se1.isSubject === se2.isSubject || se1.contourId === se2.contourId)) {

The general approach is looking promising, I could use your help though in working out what the best way to make the adjustments (eg sometimes it should be an && and other times perhaps an || etc.

@rowanwins
Copy link
Collaborator Author

rowanwins commented Feb 2, 2018

So taking into account the changes in #58 & #60 Im still getting the following error when dealing with mutlipolygons.

See the screen grab from below from http://localhost:5000/demo/#multi. That one segment is consistently picked up incorrectly irrespective of the operation chosen (eg it occurs with union, intersect, diff)
error

I've been trying to look into why that might be the case but I'm not to sure yet. I've been trying to work out if it's related to the divideSegment routine given that the initial part of the line is in the result.

Any ideas @mfogel or @w8r ?

@w8r
Copy link
Owner

w8r commented Feb 2, 2018

I can take a look at it bit by bit, but can you please reduce the amount of points? I usually do that by removing/simplifying the points on the outside of the problematic area.

@mfogel
Copy link
Contributor

mfogel commented Feb 2, 2018

TLDR: My take is supporting overlapping or side-sharing multipolygons while maintaining O(n*log(n)) performance is more than a bug fix. I don't see this as supported by the algorithm as described in the 2009 paper. However, I think the algorithm can be expanded to cover this case...

The SweepEvent object needs to be expanded and more information needs to be tracked as the sweep line moves across the plane. Right now a SweepEvent only know if it is inside or outside the 'other' polygon (ie. if this line segment is part of the clipping, is it inside the subject?). This is tracked in the boolean event.otherInOut. This is sufficient information for non-overlapping polygons because any given edge can be within, at most, only one other polygon. This is not true for overlapping multipolygons - an edge can be within multiple other polygons.

In order to support overlapping multipolygons (while maintaing O(n*log(n)) performance - ie one pass of the sweep line) to determine if an edge should be in the result set you need to know if a line segment is within any of the polygons of the other type (ie clipping vs subject), and also if it is within any other polygons of the same type. One way to do that would be to expand event.otherInOut to an array of booleans of length n - 1, where n is the total number of polygons in the clipping and subject. The logic on how this information is passed down the sweep line would need to be expanded to handle this situation.

I've been going off a 2009 version of the algorithm, I haven't read the 2013 version but from the abstract it doesn't note anything about supporting overlapping multi polygons.

Now if you don't care about maintaining O(nlog(n)) - a relative easy change to support overlapping multipolygons would be to first perform a cascading union on the subject (the result would be gauranteed to be non-overlapping), then do a cascading union on the clipping, then do the originally requested operation on the result of those two cascaded unions.

@rowanwins
Copy link
Collaborator Author

Thanks for the detailed write up @mfogel . I'm not 100% convinced that your correct, purely based on the above image where most segments (even where subject multipoly overlaps) appear to be correctly marked... Although you appear to be much smarter than me at the geometry algorithms so perhaps your correct!

I'll try and create some very basic test cases so we can investigate more simply.

@rowanwins
Copy link
Collaborator Author

So been doing a bit of thinking and checking on this

  • Currently the implementation works fine so long as the multipolys aren't overlapping (eg if the polys within the subject poly don't overlap eachother)

  • If multipolys are overlapping then martinez encounters issues

    • QGIS also doesn't like this, it throws an error saying the multipoly is an invalid geometry
      • If you convert the multipoly in two single polys in a featurecollection then it doesn't have an issue. This makes me think perhaps they are doing some sort of iterative loop

So I wonder if can we simply state we don't support overlapping multipolys via the regular methods (eg union, intersect). Any treatment of overlapping multipolys requires the user to iteratively call martinez. I think this seems fairly reasonable...

@rowanwins
Copy link
Collaborator Author

Further to this the OGC simple feature specification states that

  • The interiors of 2 Polygons that are elements of a MultiPolygon may not intersect
  • The Boundaries of any 2 Polygons that are elements of a MultiPolygon may not ‘cross’ and may touch
    at only a finite number of points. (Note that crossing is prevented by assertion 1 above)

I think this first point is inline with what I'm proposing. And we think we just need a use case or two to check the output against point 2...

@rowanwins
Copy link
Collaborator Author

Superseded

@rowanwins rowanwins closed this Feb 18, 2020
@rowanwins rowanwins deleted the multipoly-union branch February 18, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants