-
Notifications
You must be signed in to change notification settings - Fork 12
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 polygon geometry #57
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR, looks very useful!
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Show resolved
Hide resolved
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Show resolved
Hide resolved
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Show resolved
Hide resolved
src/main/java/com/github/davidmoten/rtree2/geometry/internal/PolygonDouble.java
Show resolved
Hide resolved
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #57 +/- ##
============================================
+ Coverage 85.77% 86.76% +0.98%
- Complexity 520 554 +34
============================================
Files 43 44 +1
Lines 1322 1390 +68
Branches 199 217 +18
============================================
+ Hits 1134 1206 +72
+ Misses 145 143 -2
+ Partials 43 41 -2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
} | ||
|
||
/** | ||
* Constructor for a convex polygon shell. |
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.
Is it a requirement that the polygon be convex? That's a significant extra requirement. We can't really call this a PolygonDouble geometry if it only supports convex polygons. ConvexPolygonDouble? I don't like that either because we would want to deprecate it once we'd figured out how to handle concave polygons. Do you know what's involved in supporting concave polygons as well?
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.
To check any polygon, we would extend the point to the edge of the polygon bounding box. Whether the number of polygon edges this intersects with is odd or even determines whether it is inside. The issue is that there are a bunch of corner cases as described in the link below. If it ended up being relatively slower, it may be good to have a convex version of the class. Another option could be to check the orientation of the consecutive points similar to the point in polygon check and throw an unimplemented related exception if the direction changes (not convex).
https://www.geeksforgeeks.org/how-to-check-if-a-given-point-lies-inside-a-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.
another solution strikes me that reuses your current logic is to travel along the vertices and draw straight lines from a convex vertex to the next convex vertex (skip concave vertices). When you skip one or more concave vertices you have another convex polygon that you will check for NON-intersection. So overall intersection is true if and only if the convex polygon with skipped vertices intersects object AND NONE of the convex polygons using the skipped vertices as described intersects the object. Might also have to be careful with intersections with the boundary of the original polygon that we want to be accepted. In the simple case where the input is a convex polygon you would make a check that all vertices are convex (we should do that regardless) and the logic is the same as you have already (there are no non-containment polygons to check).
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 don't think I follow. A diagram / drawing might help.
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.
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.
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.
In this vein (breaking a concave polygon into convex ones): https://stackoverflow.com/questions/2457840/breaking-a-concave-polygon-into-convex-ones. Looks complicated.
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.
Another option could be to check the orientation of the consecutive points similar to the point in polygon check and throw an unimplemented related exception if the direction changes (not convex).
https://www.geeksforgeeks.org/how-to-check-if-a-given-point-lies-inside-a-polygon/
I'm open to this too. We can state in the javadoc for PolygonDouble that it only supports convex polygons for the moment (and suggest that if the user has a non-convex polygon that they split it into convex polygons themselves and make multiple calls).
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.
Throws UnsupportedOperationException if it is not a convex polygon, this is also documented in the javadoc.
|
||
@Override | ||
public boolean test(Geometry geometry, Polygon polygon) { | ||
if (geometry instanceof Line) |
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.
missing check on geometry being instanceof Polygon. Also required on other geometryIntersects* methods
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.
Is this what you mean (latest commit)?
Add support for a polygon geometry.
Can create an outer shell and check whether lines or points intersect it.
This is the basic implementation that we require. Open to any suggestions.