-
Notifications
You must be signed in to change notification settings - Fork 205
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
Provide basic operations for geometry types #9
Comments
the postgis list is also a good base http://postgis.net/docs/manual-2.1/reference.html |
I used to divide these operations into a few categories:
|
I'd more differentiate about whether they create new geometry
|
Before doing a PR I would like to view with you how to organize the code For operation that define a properties, like area or centroid we want to define a trait.
I am a lot less inspired about relationship operation, the easy way would be a free function taking Geometry
It's the approach of postgis and boost geometry, but I would like an operation to fail at compilation time if it isn't possible for the current geometries. Of course we could define a function for every possible combination, but there is probably a better way. |
There is another way with generic trait, it will result in something like that: pub trait Contains<RHS = Self> {
fn within(&self, rhs: &RHS) -> bool;
}
impl Contains<Point> for LineString{
fn within(&self, other: &Point) -> bool{
true
}
}
impl Contains<LineString> for LineString{
fn within(&self, other: &LineString) -> bool{
false
}
}
#[test]
fn within_test(){
let c = Coordinate {
x: 40.02f64,
y: 116.34
};
let p = Point(c);
let mut vecp = Vec::new();
vecp.push(p);
let l = LineString(vecp);
assert!(l.within(&p));
assert!(!l.within(&l));
} |
This last one is the preferred way: same pattern that Add and friends to overload operators. One method traits should have the same name as the method. |
For centroid, the result should be an associated type. |
I agree with @TeXitoi; I also prefer the last way.
I agree, it should probably be I think for now, all these operations can go here in rust-geo. In the future, it might make sense to extract them out into a separate crate, but I don't think that's necessary right now. |
I put some time towards this today. I'm brand new to rust (😱 ) , and looking for feedback on the approach before I go down the road too far. https://github.com/michaelkirk/rust-geo/blob/intersection-operation/src/operation/intersection.rs In particular, the way I'm using the type system starts to get pretty heavy. On the one hand, it would be nice if we could leverage the compiler and assert that e.g. A Is there a way to be this precise with our type system? Or do we just call it a day and have all operations return a Also, I'm open to the possibility that I'm completely off base with the entire approach, but wanted to get the ball rolling on a discussion. |
Welcome @michaelkirk! 🎉
I like your approach so far. In opinion,
One way to do this would be an enum LineStringIntersection {
Point(Point),
LineString(Point),
}
pub trait Intersection {
type Other;
type Return;
fn intersection(&self, other: &Self::Other) -> Vec<Self::Return>;
}
impl Intersection for LineString {
type Other = LineString;
type Return = LineStringIntersection;
fn intersection(&self, other: &Self::Other) -> Vec<Self::Return> {
...
}
} Using associated types here; haven't thought too hard about this design, so take it with a grain of salt. I'm half-awake right now, but thought I'd give you some quick feedback before I go to sleep. I'll revisit this tomorrow. |
Sorry I have responded to your question @michaelkirk; I've been distracted by other things going on in my life so I haven't put much mental thought into this. I'm still very interested in the work you've done so far, but it might be some time before I can take a closer look. If anyone else in the community wants to give their thoughts, it'd be much appreciated. |
We have basic operations now, so I'm going to close this! |
Reduce repetitive 'comma' implementations
http://turfjs.org/static/docs/ has a nice list of operations. Might be good to have a discussion about whether to have these in a separate library is more appropriate
The text was updated successfully, but these errors were encountered: