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 more implementations for Contains algorithm. #451
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.
Your changes look good - but see my comment about an existing issue that I think we should address.
} | ||
|
||
/// Calculate the position of a `Coordinate` relative to a `LineString` | ||
pub fn coord_pos_relative_to_line_string<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.
Assuming this was a straight move and rename from algorithm::contains::get_position
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.
yep, just moved, changed the param from point to coord, and renamed
where | ||
T: Float, | ||
{ | ||
fn contains(&self, p: &Point<T>) -> bool { |
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 example I contrived fails:
use geo::algorithm::contains::Contains;
use geo::{LineString, Point};
let line_string = LineString::from(vec![(0., 0.), (3., 3.)]);
let point_on_line = Point::new(1., 1.);
assert!(line_string.contains(&point_on_line)); // <- 💥 currently fails
I don't think this was a change in behavior with this PR, but at least one point of comparison, postgis, agrees that we should change it so that my example passes:
psql> SELECT ST_Contains(
ST_MakeLine(ST_MakePoint(0,0), ST_MakePoint(3,3)),
ST_MakePoint(1,1)
);
st_contains
-------------
t
LMK what you think. If you agree that we should change it and want to punt on the implementation, I can take a look!
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.
ooo good find. i'm going to merge this as-is, and open a separate issue for that
impl Contains<Coordinate> for Point
impl Contains<Coordinate> for MultiPoint
impl Contains<Point> for MultiPoint
impl Contains<Coordinate> for Line
impl Contains<Coordinate> for LineString
impl Contains<Coordinate> for MultiLineString
impl Contains<Point> for MultiLineString
impl Contains<Coordinate> for Polygon
impl Contains<Coordinate> for MultiPolygon
impl Contains<Coordinate> for Rect
impl Contains<Coordinate> for Triangle
impl Contains<Coordinate> for Geometry
impl Contains<Point> for Geometry
impl Contains<Coordinate> for GeometryCollection
impl Contains<Point> for GeometryCollection