-
Notifications
You must be signed in to change notification settings - Fork 200
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
Introduce the geomgraph module for DE-9IM Relate trait #639
Conversation
Since ec69add, `Rect::new` already sorts it's input by min/max, and so there's no longer a need to manually sort min/max points.
6f55e90
to
25232cf
Compare
a85fc29
to
11bd41e
Compare
636: Add `line_intersection` to compute point or segment intersection of two Lines. r=frewsxcv a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Fixes #288 I pulled this out of my [Geometry Graph](https://github.com/georust/geo/tree/mkirk/geomgraph) work (which is getting close, I swear!). Co-authored-by: Michael Kirk <[email protected]>
8874edc
to
bb911f0
Compare
@@ -19,6 +19,7 @@ num-traits = "0.2" | |||
serde = { version = "1.0", optional = true, features = ["derive"] } | |||
rstar = { version = "0.8" } | |||
geographiclib-rs = { version = "0.2" } | |||
log = "0.4.11" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been waffling on doing this for a while, but finally bit the bullet.
FIXES #596 if you're on board.
geo/Cargo.toml
Outdated
approx = "0.4.0" | ||
criterion = { version = "0.3" } | ||
# jts-test-runner is an internal crate which exists only to be part of the geo test suite. | ||
# As such it's kept unpublished. It's in a separate repo primarily because it's kind of large. | ||
jts-test-runner = { git = "https://github.com/georust/jts-test-runner", rev = "3294b9af1d3e64fcc9caf9646a93d0116f7d8321" } | ||
jts-test-runner = { git = "https://github.com/georust/jts-test-runner", branch = "mkirk/relate" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -7,19 +7,19 @@ use crate::*; | |||
|
|||
impl<T> Contains<Coordinate<T>> for Geometry<T> | |||
where | |||
T: GeoNum, | |||
T: GeoFloat, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is a breaking change - I'm dropping support for Contains
for integer Polygons, and thus integer Geometries.
If this is controversial, I could revert this part, but IMO it's more important to fix #513 than to support Contains
for integer geometries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of this move; support for multiple scalar types was a key distinguishing feature of this crate. I suppose we don't have a choice if the previous implementations had bugs for this specific case (Coordinate
in Geometry
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmanoka - You've previously convinced me that integer geometries are neat, so I'd like to support them, but it seems unreasonable to leave things broken for the mainstream use case in order to hold on to a possibility of something more esoteric.
Assuming specialization won't be finalized any time soon, do you have any thoughts on how we could support both?
I'd be curious to see how well something like this geometry graph approach lends itself to integer geometries. I took a brief spelunking just to see what would break from a code POV: cf56814
The large commented out portions are the things that don't compile as GeoNum.
I don't have any experience working with integer geometries - some of it seems readily addressable, but the biggest immediate hangup for me is that the approach in this PR relies heavily on knowing where lines intersect - is there a corollary in integer geometries? I'm assuming you can't simply round
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should say, it is my preference to get this mega PR merged without adding even more changes to support Relate for integers. I think the Relate functionality is useful even if it's just for Floats (which I expect is almost all users).
As I said, if it's controversial to drop support for Contains, I could revert this portion. I'd still add the new relate trait, just for Floats, but revert the fix for the Contains edge cases. I'd be OK with that approach if that's your preference @rmanoka.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelkirk Relate is a brilliant trait and I think we should definitely add it even if can only supports GeoFloat
. What I was wondering is whether we could leave the previous implementations of Contains
as long as they pass all test cases as GeoNum
subsumes GeoFloat
.
For the above case, is it possible to use our hand-rolled Contains<Coordinate>
and Contains<Point>
implementations for the geometries? From what I recall, we did handle the edge cases, and hopefully the JTS test suite will point out any edge cases we missed out for these. I do agree Contains<LineString>
, and Contains<Polygon>
should use the geomgraph techniques you've implemented as there were corner cases we couldn't handle easily earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I understand what you're requesting. I've pushed up a new commit, which restores:
impl Contains<Coordinate<GeoNum>> for Polygon<GeoNum>
impl Contains<Point<GeoNum>> for Polygon<GeoNum>
impl Contains<Coordinate<GeoNum>> for Geometry<GeoNum>
impl Contains<Point<GeoNum>> for Geometry<GeoNum>
I think that's what you were requesting, and it makes sense - no reason to lose that functionality since it was working fine. Thanks for pointing it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just to be clear, I'm still removing the API for other GeoNum backed variants, like LineString<GeoNum>
, which now only support LineString<GeoFloat>
}; | ||
|
||
/// The position of a `Coordinate` relative to a `Geometry` | ||
#[derive(PartialEq, Clone, Debug)] | ||
#[derive(PartialEq, Clone, Copy, Debug)] | ||
pub enum CoordPos { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a point of confusion if you are comparing to the reference JTS code...
There are two enums JTS uses ubiquitously in the geomgraph module:
JTS::Location: { Interior, Boundary, Exterior }
and JTS::Position: { On, Left, Right }
Mapping to geo is thus:
JTS::Location
-> geo::CoordPos
JTS::Position
-> geo::Direction
Somewhat confusingly JTS::Location
maps to our existing geo::CoordPos
.
We didn't yet have a concept of JTS::Position
, and I wanted to call it something other than Position
to differentiate it from our CoordPos
, so I've introduced pub(crate) enum geomgraph::Direction { On, Left, Right }
.
Unfortunately, this somewhat collides with pub enum orient::Direction { Default, Reversed }
, but I don't have a better solution at this point. FWIW geomgraph::Direction
is not pub
, so we can always revisit later without breaking public APIs.
} | ||
impl<'a, T: GeoNum> CoordinatePosition for GeometryCow<'a, T> { | ||
type Scalar = T; | ||
crate::geometry_cow_delegate_impl! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the new enum GeometryCow
for details...
Some relevant discussion #421 (comment)
1c63c83
to
4bce3bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this is ready for review!
/// | ||
/// As an example, see the [`Relate`] trait which uses `GeometryCow`. | ||
#[derive(PartialEq, Debug, Hash)] | ||
pub(crate) enum GeometryCow<'a, T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this is pub(crate)
. I'm not yet sure if it's a good abstraction so wanted to shield the innocent public.
for edge0 in edges.iter() { | ||
for edge1 in edges.iter() { | ||
if check_for_self_intersecting_edges || edge0.as_ptr() != edge1.as_ptr() { | ||
self.compute_intersects(edge0, edge1, segment_intersector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there were two reasons for the unfortunate use of Rc<RefCell<>>
.
The lesser one is that RelateOperation grabs a duplicated reference to Edge
at some point. I think we could get rid of that usage by storing an arena-style index-of-edge instead of a pointer-to-edge.
The bigger reason is here: compute_intersection
calls segment_intersector.add_intersection
which, as well as computing the intersections, also mutates the edge to record those intersections and it might be intersecting an edge with itself.
This all makes for some pretty unremarkable Java code, but rust makes it painfully clear that this is less-than-nice.
I think it would be possible to remove the Rc<RefCell<>>
stuff with more work, but I decided to leave it as is for now.
FWIW I did some profiling and the RefCell/Rc stuff does not significantly contribute to runtime.
638: Don't redundantly sort Rect inputs (cleanup - no change in behavior) r=frewsxcv a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [ ] ~~I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.~~ --- Since ec69add, `Rect::new` already sorts it's input by min/max, and so there's no longer a need to manually sort min/max points. Co-authored-by: Michael Kirk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not done reviewing, but here's some early comments
geo/src/types.rs
Outdated
/// fn foo_1(&self, coord: Coordinate<T>) -> bool { false } | ||
/// fn foo_2(&self) -> i32 { 1 } | ||
/// } | ||
/// ... and so on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this was changed to /// // ... and so on
could we remove the ignore
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comments about these macros! They were kind of a mess. I've done some juggling which I think addresses all your feedback:
- I wanted an example of how to use it in the docs, so I fleshed out the entire example, and removed the
ignore
. - This revealed a bug... I need to prefix the inner helper macro with
$crate::
notcrate::
. - I also moved the doc comment from this helper method to the public
geometry_delegate_impl!
macro. - I prefixed the helper method with
__
to indicate that it's not intended to be called directly. - I marked
geometry_cow_delegate_impl
asdoc(hidden)
. SinceGeometryCow
ispub(crate)
I wantgeometry_cow_delegate_impl
to have similarly restricted visibility, but I don't know enough about macros to know if there's a better way than just hiding it from the docs. Is there a better way?
geo/src/types.rs
Outdated
} | ||
|
||
#[macro_export] | ||
macro_rules! geometry_delegate_impl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big deal, but prior to this pull request, we had documentation for geometry_delegate_impl
. maybe we can copy/paste from geometry_delegate_impl_helper
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded in #639 (comment)
geo/src/types.rs
Outdated
} | ||
} | ||
)+ | ||
}; | ||
} | ||
|
||
#[macro_export] | ||
macro_rules! geometry_cow_delegate_impl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as written, i think these are publicly available to anyone who uses geo
. do we want to expose these publicly if GeometryCow
itself isn't public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
responded in #639 (comment)
$( | ||
impl<F: GeoFloat> Relate<F, $t> for $k { | ||
fn relate(&self, other: &$t) -> IntersectionMatrix { | ||
GeometryCow::from(self).relate(&GeometryCow::from(other)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: this line is why I introduced GeometryCow
.
Unlike much of our other code, the GeometryGraph was implemented to deal only with Geometry
, rather than have a bunch of different implementations for each Geometry
variant.
That means if we wanted to be able to call something like:
multi_line_string.relate(polygon)
We'd actually have to do something like:
Geometry::from(multi_line_string.clone()).relate(Geometry::from(polygon.clone()))
Those clones are really unfortunate - hence GeometryCow
, which is an enum like Geometry
but can be made from a variant by borrowing - without cloning.
I am available over the weekend if anyone would like a review docent. |
geo/benches/relate.rs
Outdated
}; | ||
|
||
bencher.iter(|| { | ||
criterion::black_box(criterion::black_box(&polygon).relate(&sub_polygon)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall in the depths of my brain that every parameter of the function your testing should be wrapped in a black_box
, but I can't find a source for that now 🤷🏻
Related to rust-lang/rust#80480
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added another block_box around the argument. So now it's:
criterion::black_box(criterion::black_box(&polygon).relate(criterion::black_box(&sub_polygon)));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finally found some time to look through all this and wow is this over my head lol. i did try to follow the best i could given the capacity i have right now, and i think i have a rough understanding of all the pieces here! i wish i had more time to learn more about the nitty gritty details, but no reason that need for that to wait on me. this is so exciting @michaelkirk, thanks for adding all this!!
I think it's good to go, especially given all the JTS tests, and our edge case tests (prev. ignored) all pass! Thanks again for the wonderful contrib. |
2db9af9
to
8bf3e03
Compare
bor r+ (rebased and squashed) |
bors r+ |
639: Introduce the geomgraph module for DE-9IM Relate trait r=michaelkirk a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Fixes #513, #515 (I'm sorry it's so large) ~~I'm going to leave it as a draft (edit: 🤦 I failed to actually open the PR as a draft) while I wait to merge #636 and #638 and then do some rebasing, but I don't anticipate doing other large changes before review.~~ *update: ready for review!* Here's some of the earlier work in pursuit of this: #514 #516 #523 #524 #538 #552 #561 #611 #628 #629 #636 Primarily, this introduces the geomgraph module for a DE-9IM `Relate` trait. geomgraph implements a topology graph largely inspired by JTS's module of the same name: https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph You can see some of the reference code if you omit the "REMOVE JTS COMMENTS" commit. In some places the implementation is quite close to the JTS source. The overall "flow" is pretty similar to that of JTS, but in the small, there were some divergences. It's not easy (or desirable) to literally translate a Java codebase making heavy use of inheritance and pointers to rust. Additionally, I chose to take advantage of `Option` and rust's enums with associated data to make some case analysis more explicit. There is a corresponding PR in our [jts-test-runner](georust/jts-test-runner#6) crate which includes the bulk of the tests for the new Relate trait. ## Algorithm Overview This functionality is accessed on geometries, via the `Relate` trait, e.g. `line.relate(point)` which returns a DE-9IM [`IntersectionMatrix`](https://en.wikipedia.org/wiki/DE-9IM#Matrix_model). The `Relate` trait is driven by the `RelateOperation`. The `RelateOperation` builds a `GeometryGraph` for each of the two geometries being related. A `GeometryGraph` is a systematic way to organize the "interesting" parts of a geometry's structure - e.g. where its vertices, lines, and areas lie relative to one another. Once the `RelateOperation` has built the two `GeometryGraph`s, it uses them to efficiently compare the two Geometries's structures, outputting the `IntesectionMatrix`. Co-authored-by: Michael Kirk <[email protected]> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
bors r- (whoops - I want to merge the jts-tests first) |
Canceled. |
8bf3e03
to
6f7942a
Compare
geomgraph implements a topology graph largely inspired by JTS's module of the same name: https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph
6f7942a
to
7c6f32d
Compare
bors r=frewsxcv,rmanoka |
Build succeeded: |
642: Introduce the geomgraph module for DE-9IM Relate trait r=frewsxcv,rmanoka a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- This is just a dupe of #639, but targeting master. I'd forgotten #639 had targeted #636 (at the time, unmerged) for the purposes of minimizing the diff. ...I suppose an epically long PR wouldn't be complete without an epically long merge process. 😓 Co-authored-by: Michael Kirk <[email protected]>
CHANGES.md
if knowledge of this change could be valuable to users.Fixes #513, #515
(I'm sorry it's so large)
I'm going to leave it as a draft (edit: 🤦 I failed to actually open the PR as a draft) while I wait to merge #636 and #638 and then do some rebasing, but I don't anticipate doing other large changes before review.update: ready for review!Here's some of the earlier work in pursuit of this:
#514
#516
#523
#524
#538
#552
#561
#611
#628
#629
#636
Primarily, this introduces the geomgraph module for a DE-9IM
Relate
trait.geomgraph implements a topology graph largely inspired by JTS's module of the same name:
https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph
You can see some of the reference code if you omit the "REMOVE JTS COMMENTS" commit. In some places the implementation is quite close to the JTS source.
The overall "flow" is pretty similar to that of JTS, but in the small, there were some divergences. It's not easy (or desirable) to literally translate a Java codebase making heavy use of inheritance and pointers to rust. Additionally, I chose to take advantage of
Option
and rust's enums with associated data to make some case analysis more explicit.There is a corresponding PR in our jts-test-runner crate which includes the bulk of the tests for the new Relate trait.
Algorithm Overview
This functionality is accessed on geometries, via the
Relate
trait, e.g.line.relate(point)
which returns a DE-9IMIntersectionMatrix
.The
Relate
trait is driven by theRelateOperation
. TheRelateOperation
builds aGeometryGraph
for each of the two geometries being related.A
GeometryGraph
is a systematic way to organize the "interesting" parts of a geometry's structure - e.g. where its vertices, lines, and areas lie relative to one another.Once the
RelateOperation
has built the twoGeometryGraph
s, it uses them to efficiently compare the two Geometries's structures, outputting theIntesectionMatrix
.