-
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
WIP Implement area functions on geometry traits #1021
Conversation
When I run into situations like this, I typically start incrementally loosening lifetime bounds that don't need to be explicitly specified. With these changes, your code compiles: https://gist.github.com/frewsxcv/75e719227b889ee2930ccb59930b26de |
x: linestring.coord(i).unwrap().x(), | ||
y: linestring.coord(i).unwrap().y(), | ||
}; | ||
c1.x = c1.x - shift.x(); |
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 switched these to not use -=
to avoid having to require the SubAssign
type on T
. Maybe requiring SubAssign
and similar is such a low bar that we should require it?
Thanks! That was helpful! I was able to make some progress. But it seems like the trait methods that really need So I can make progress by removing for polygon_idx in 0..geom.num_polygons() { Especially with the I tried to implement |
pub trait GeometryTrait<'a> { | ||
type T: CoordNum; | ||
type Point: 'a + PointTrait<T = Self::T>; | ||
type LineString: 'a + LineStringTrait<'a, T = Self::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.
This can now be:
type LineString<'a>: 'a + LineStringTrait<'a, T = Self::T>;
Or something like that
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.
What's the benefit to adding <'a>
onto the type name 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.
You can remove the lifetime from the trait itself, which means any references to the trait don't need to specify that lifetime. Similar to https://github.com/georust/geo/pull/908/files
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.
Oh wow that is super nice!
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.
It ended up being a lot more work than expected but I implemented that change in geoarrow/geoarrow-rs#253 (I'm incubating and testing out trait impls there); I'll make a PR to the traits branch with those updated changes
(I still had lifetime issues connecting to geos but that's life 🤷♂️ )
From what I understand, #1115 is a newer version of this so I'm going to close this one. Feel free to reopen if this is wrong |
Change list
Send + Sync
restriction onT
on the trait. I don't know whether this restriction makes sense here. I originally added it because rust-postgis included it. But it appeared thatCoordFloat
doesn't include those trait bounds so it was easier in this context to remove them.T: CoordNum
on each trait, and define other item types in terms ofT
. I needed to do this so that some algorithms could enforceCoordFloat
and not justCoordNum
.I'm not sure if
type ItemType = LineString<Self::T>;
on theimpl
is necessary instead oftype ItemType = LineString<T>;
.Questions:
temporary value dropped while borrowed
error? It's on this line. I'm not sure if this is an issue with the trait design or implementation or with my code