Robust Intersect and Contains #511

rmanoka · 2020-09-13T16:52:18Z

Use robust predicates in Intersects and Contains traits

use robust preds (doesn't need Float anymore)
define interior, boundary, and exterior of geometry types (similar to geos/jts)
define Intersect and Contains using DE-9IM model
fix contains logic in a few corner cases

Future work:

The polygon contains line / polygon logic is not yet fully correct. I've added a few TODO comments, and an ignored test for cases where our predicate doesn't match the DE-9IM definition. Unfortunately, it doesn't seem to be a simple change to fix these corner cases. For instance, geos seems to use a concept called GeometryGraph for these cases which I'm yet to understand.

+ restructure: `intersects/mod.rs` and `contains/mod.rs` + robust intersection + macro for symmetric intersection implementations + robust contains

Fix some tests

rmanoka · 2020-09-13T17:00:14Z

A couple of clarifications needed:

These two tests seem inconsistent with definition of Contains which says it does not include the boundary.

geo/geo/src/algorithm/contains.rs

Line 778 in a6a3218

fn triangle_contains_point_on_edge() {

geo/geo/src/algorithm/contains.rs

Line 785 in a6a3218

fn triangle_contains_point_on_vertex() {
Implementation of Rect: Contains<Point> also seems to allow the point to be on the edges. Again, not consistent with definition, and implementation on Polygon.

Also checked against shapely/geos which seems to concur with our definition (so points on edges / vertices of the polygon are not considered to be "contained"). Shall we go with the definition (so fix Rect and Triangle)?

@michaelkirk @urschrei

michaelkirk · 2020-09-15T22:48:59Z

Shall we go with the definition (so fix Rect and Triangle)?

Yes. Apparently I wrote that broken triangle implementation 😞: b3944ca

For my own benefit, I've just now referred to http://portal.opengeospatial.org/files/?artifact_id=25355 (thanks @urschrei 😉)

Contains
a.Contains(b) ⇔ b.Within(a)

So it's defined in terms of within, which itself is defined as:

Within
The Within relationship is defined as
a.Within(b) ⇔ (a∩b=a) ∧ (I(a)∩E(b)=∅)
Expressed in terms of the DE-9IM:
a.Within(b) ⇔ [ I(a)∩I(b)≠∅ ∧ I(a)∩E(b)=∅ ∧ B(a)∩E(b)=∅ ]
    ⇔ a.Relate(b, “T*F**F***”) 

And from the jts impl:

contains
...
[T*****FF*]

So for a.contains(b), boundary contact is not sufficient; The interior of b must reside (wholly) within the interior of a.

In the future, I'll be careful to be rigorous about the cases represented in DE-9IM.

rmanoka · 2020-09-16T05:33:10Z

Yeah, DE-9IM based rigorous definitions of our geometry types, and traits definitely seems useful. I believe jts, geos (hence postgis), shapely all use the same definition of Intersects, and Contains so makes sense for us to with the same for interop with the community. Will add some doc to the types trying to formalize the definitions.

Aside: why didn't they just name it denim? Possible trademark issues? I'm so tempted to use it as it is so much simpler to type (and fashionable)

+ define interior, boundary for geometry types + define `Intersects` and `Contains` as per DE-9IM

michaelkirk

I have yet to review the Contains portion, but I ran out of time for now.

I have reviewed the Intersecs and other changes, and they look great! Just some optional feedback for clarity.

I'll finish reviewing up hopefully yet tonight, or if not, tomorrow.

geo-types/src/line_string.rs

geo/src/algorithm/intersects/mod.rs

geo/src/utils.rs

geo/src/algorithm/intersects/polygon.rs

geo-types/src/line.rs

geo-types/src/line_string.rs

geo/src/algorithm/contains/mod.rs

Co-authored-by: Corey Farwell <[email protected]>

+ remove unnecessary PhantomData in Kernel defn. + cargo fmt

rmanoka · 2020-09-19T06:56:37Z

Thanks for the comments @frewsxcv @michaelkirk ; I have addressed them, also added a few lines in CHANGES.md to highlight the recent robust pred. changes. Will wait a couple more days to see there are any minor fixes we spot, and merge this PR.

closes #415

michaelkirk · 2020-09-20T03:27:38Z

geo-types/CHANGES.md

@@ -1,5 +1,10 @@
 # Changes

+## Master (not released)


Yes, thank you for doing this! Tracking as we go will make the eventual release easier.

michaelkirk · 2020-09-20T03:32:36Z

geo/src/algorithm/contains/line.rs

+{
+    fn contains(&self, coord: &Coordinate<T>) -> bool {
+        if self.start == self.end {
+            &self.start == coord


This is a surprising consequence.

Given a coordinate: (0,0)
And a line Line((0,0), (0,1))

The line does not contain the coordinate, since the coordinate is only on the boundary of the line, not in the line's interior.

This continues to be true as we shrink the line — the line Line((0,0), (0,0.5)) doesn't contain the coordinate.

...until the line is shortened all the way to degeneracy - Line((0,0), (0,0)) now contains the coordinate.

Is that right? 🤯

Yeah, it is confusing, but that is the precise expected behavior. I cross verified with shapely (geos), and it does the same.

michaelkirk · 2020-09-20T04:07:00Z

geo/src/algorithm/contains/polygon.rs

+}
+
+// TODO: ensure DE-9IM compliance: esp., when
+// line.start and line.end is on the boundaries


Right as your TODO alludes, it seems like the current logic would fail for a case like:

Where the line touches, but does not pass, the interior.
Or if the line touches, but does not pass, the exterior.

Yeah. Also see the test line_in_polygon_edgecases in contains/mod.rs. We might be able to use the winding order trait and a few additions to contains to solve these cases, but wanted to attempt this in a different PR.

michaelkirk · 2020-09-20T04:14:06Z

geo/src/algorithm/contains/polygon.rs

+    }
+}
+
+// TODO: also check interiors


Right, this one also looks wrong. I wouldn't want to include new broken implementations, but I believe you are just moving these, right?

Yeah, these implementations were just moved into this file. We should fix the two traits for de9im compliance in another pr

michaelkirk · 2020-09-20T04:24:35Z

geo/src/algorithm/contains/polygon.rs

+impl<G, T> Contains<G> for MultiPolygon<T>
+where
+    T: CoordinateType,
+    Polygon<T>: Contains<G>,


It looks like we're newly accepting any type which implements the Contains trait, whereas before MultiPolygon only supported Coordinate and Point, is that right?

Does this existing implementation work for types with more dimensionality than a point?

e.g. If the two squares are part of a single multipolygon, even though the black line is contained by neither square individually, is it contained by the multipolygon?

So, MultiPolygon should not contain overlapping constituent polys:

As per the OGC SFS specification, the Polygons in a MultiPolygon may not overlap, and may only touch at single points. This allows the topological point-set semantics to be well-defined.

With the above defn., I think we can lift any Polygon: Contains to work on MultiPolygon

michaelkirk · 2020-09-20T04:27:40Z

geo/src/algorithm/contains/rect.rs

+{
+    fn contains(&self, bounding_rect: &Rect<T>) -> bool {
+        // All points of LineString must be in the polygon ?
+        self.min().x <= bounding_rect.min().x


So a degenerate Rect which is either a Line or a Point will be contained if it's on the boundary - I don't think that's correct, is it?

Good catch! The previous implementation was the same with strict checking, which would also miss some edge cases. Want to address these in a different PR (needs a few extra impls, utils). Let me add a todo comment here so we get back to it.

michaelkirk · 2020-09-20T04:29:49Z

geo/src/algorithm/contains/triangle.rs

+    fn contains(&self, coord: &Coordinate<T>) -> bool {
+        let ls = LineString(vec![self.0, self.1, self.2, self.0]);
+        use utils::*;
+        coord_pos_relative_to_ring(*coord, &ls) == CoordPos::Inside


Thank you for fixing this!

michaelkirk · 2020-09-20T05:04:53Z

Ok, this LGTM. I agree it’s fine to address the pre-existing issues in a follow up PR. Thanks for making this better! I’d say merge this whenever you’re ready.

…

On Sep 19, 2020, at 21:57, rmanoka ***@***.***> wrote: @rmanoka commented on this pull request. In geo/src/algorithm/contains/rect.rs: > +impl<T> Contains<Point<T>> for Rect<T> +where + T: CoordinateType, +{ + fn contains(&self, p: &Point<T>) -> bool { + self.contains(&p.0) + } +} + +impl<T> Contains<Rect<T>> for Rect<T> +where + T: CoordinateType, +{ + fn contains(&self, bounding_rect: &Rect<T>) -> bool { + // All points of LineString must be in the polygon ? + self.min().x <= bounding_rect.min().x Good catch! The previous implementation was the same with strict checking, which would also miss some edge cases. Want to address these in a different PR (needs a few extra impls, utils). Let me add a todo comment here so we get back to it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

michaelkirk · 2020-09-20T05:05:48Z

Ah, I didn’t realize polygons in a multi-polygon couldn’t overlap. Thank you for explaining.

…

On Sep 19, 2020, at 21:53, rmanoka ***@***.***> wrote: @rmanoka commented on this pull request. In geo/src/algorithm/contains/polygon.rs: > + .interiors() + .iter() + .any(|ring| ring.intersects(linestring)) + } else { + false + } + } +} + +// ┌──────────────────────────────────┐ +// │ Implementations for MultiPolygon │ +// └──────────────────────────────────┘ +impl<G, T> Contains<G> for MultiPolygon<T> +where + T: CoordinateType, + Polygon<T>: Contains<G>, So, MultiPolygon should not contain overlapping constituent polys: As per the OGC SFS specification, the Polygons in a MultiPolygon may not overlap, and may only touch at single points. This allows the topological point-set semantics to be well-defined. With the above defn., I think we can lift any Polygon: Contains to work on MultiPolygon — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

rmanoka · 2020-09-20T05:33:19Z

Ah, I didn’t realize polygons in a multi-polygon couldn’t overlap. Thank you for explaining.

Actually, you're right. The polygons can touch at a single point which introduces some edge cases. For eg. two rectangles touching at a single point, and a line passing through that point. It won't be in any of the individual polygons but is contained in the multipolygon.

Will leave a comment, and we can get to this when we ensure correctness of these two traits.

+ update CHANGES.md + add TODO comments for DE-9IM compliance

rmanoka · 2020-09-20T08:01:04Z

bors r=michaelkirk

rmanoka · 2020-09-20T08:05:23Z

bors r=michaelkirk

bors · 2020-09-20T08:17:38Z

Build succeeded:

rmanoka added 5 commits September 13, 2020 01:15

Remove CoordinateType bound when not needed in impl

110db3e

Intersects and Contains using robust predicates

11a0833

+ restructure: `intersects/mod.rs` and `contains/mod.rs` + robust intersection + macro for symmetric intersection implementations + robust contains

Triangle contains

c0ea571

Fix some tests

Merge remote-tracking branch 'origin/master' into fix/robust-intersects

d02ee5b

Cargo fmt

22e54d0

rmanoka added 2 commits September 16, 2020 17:58

Define formal specs in docs

8169caa

+ define interior, boundary for geometry types + define `Intersects` and `Contains` as per DE-9IM

Fix edge cases for DE-9IM compliance

98b3058

rmanoka force-pushed the fix/robust-intersects branch from c84a5ab to 98b3058 Compare September 17, 2020 05:05

Cargo fmt

53cc717

rmanoka marked this pull request as ready for review September 17, 2020 05:12

Fix a few more edge cases in Line: Contain<T>

63558ee

michaelkirk approved these changes Sep 17, 2020

View reviewed changes

frewsxcv reviewed Sep 18, 2020

View reviewed changes

geo-types/src/line.rs Outdated Show resolved Hide resolved

geo-types/src/line_string.rs Show resolved Hide resolved

geo/src/algorithm/contains/mod.rs Show resolved Hide resolved

rmanoka and others added 4 commits September 18, 2020 10:52

Update geo-types/src/line.rs

8463b55

Co-authored-by: Corey Farwell <[email protected]>

Add review suggestions

f6c9bc8

Simplify Kernel definition

353f3ad

+ remove unnecessary PhantomData in Kernel defn. + cargo fmt

Triangle Intersects trait

04f25fe

rmanoka mentioned this pull request Sep 19, 2020

Use robust crate for orientation tests #415

Closed

6 tasks

michaelkirk reviewed Sep 20, 2020

View reviewed changes

Add recent PRs to changes.md

b93fd10

+ update CHANGES.md + add TODO comments for DE-9IM compliance

rmanoka force-pushed the fix/robust-intersects branch from 719131e to b93fd10 Compare September 20, 2020 08:00

bors bot merged commit 6fb4fb7 into georust:master Sep 20, 2020

michaelkirk mentioned this pull request Sep 21, 2020

Correctness issues with Contains #513

Closed

amatissart mentioned this pull request Nov 7, 2020

Fix ray-tracing algorithm when ray passes through a vertex #526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robust Intersect and Contains #511

Robust Intersect and Contains #511

rmanoka commented Sep 13, 2020 •

edited

Loading

rmanoka commented Sep 13, 2020

michaelkirk commented Sep 15, 2020 •

edited

Loading

rmanoka commented Sep 16, 2020 •

edited

Loading

michaelkirk left a comment

rmanoka commented Sep 19, 2020

michaelkirk Sep 20, 2020

michaelkirk Sep 20, 2020 •

edited

Loading

rmanoka Sep 20, 2020

michaelkirk Sep 20, 2020

rmanoka Sep 20, 2020

michaelkirk Sep 20, 2020

rmanoka Sep 20, 2020

michaelkirk Sep 20, 2020

rmanoka Sep 20, 2020

michaelkirk Sep 20, 2020

rmanoka Sep 20, 2020

michaelkirk Sep 20, 2020

michaelkirk commented Sep 20, 2020 via email

michaelkirk commented Sep 20, 2020 via email

rmanoka commented Sep 20, 2020

rmanoka commented Sep 20, 2020 •

edited

Loading

rmanoka commented Sep 20, 2020

bors bot commented Sep 20, 2020

Robust Intersect and Contains #511

Robust Intersect and Contains #511

Conversation

rmanoka commented Sep 13, 2020 • edited Loading

rmanoka commented Sep 13, 2020

michaelkirk commented Sep 15, 2020 • edited Loading

rmanoka commented Sep 16, 2020 • edited Loading

michaelkirk left a comment

Choose a reason for hiding this comment

rmanoka commented Sep 19, 2020

Choose a reason for hiding this comment

michaelkirk Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Sep 20, 2020 via email

michaelkirk commented Sep 20, 2020 via email

rmanoka commented Sep 20, 2020

rmanoka commented Sep 20, 2020 • edited Loading

rmanoka commented Sep 20, 2020

bors bot commented Sep 20, 2020

rmanoka commented Sep 13, 2020 •

edited

Loading

michaelkirk commented Sep 15, 2020 •

edited

Loading

rmanoka commented Sep 16, 2020 •

edited

Loading

michaelkirk Sep 20, 2020 •

edited

Loading

rmanoka commented Sep 20, 2020 •

edited

Loading