georust / geo

Rust geospatial primitives & algorithms
https://crates.io/crates/geo
Other
1.58k stars 199 forks source link

Iterator for borrowed Line geometries? #1266

Closed urschrei closed 2 weeks ago

urschrei commented 2 weeks ago

The current lines() iterator produces Line because it's a Copy type. However, in the case of large geometries, does this have a perf overhead compared to producing &Line? I'm asking because rstar now has an ObjectRef wrapper that allows tree construction with borrowed items. This is useful in situations where the tree is short-lived (e.g. lives in and is dropped in the same function as the borrowed geometries) such as here.

So: is there an obvious benefit to adding an iterator with Item=&'a Line<T>? We currently construct short-lived trees containing Line in a couple of places, and I'm wondering whether this would be more memory-efficient?

michaelkirk commented 2 weeks ago

&Line is a reference to an already existing Line, but our current geo-type geometries don't already have a Line based representation.

I don't think this is feasible without fundamentally rewriting the geo-types inner representation, which would likely have a lot of consequences.

urschrei commented 2 weeks ago

&Line is a reference to an already existing Line, but our current geo-type geometries don't already have a Line based representation.

Oh yeah. Damn.

I don't think this is feasible without fundamentally rewriting the geo-types inner representation, which would likely have a lot of consequences.

I absolutely do not want to think about that.

OK, looks like I need to think harder about how to do this. I know why rstar can't give us back a line segment from a stored Polygon (borrowed or otherwise), but it's still very annoying.

michaelkirk commented 2 weeks ago

Maybe something could be done with geo-traits (pseudo code follows):

impl geo_traits::Line for &[geo_types::Coord<T>; 2] {
   ...
}

And then:

impl geo_types::Polygon {
   fn line_refs(&self) -> impl Iterator<Item = impl LineTrait> {
       self.0.iter().window(2)
   }
}
michaelkirk commented 2 weeks ago

TBH, I'm not quite ready to support integrating geo-traits into geo just yet given the rate of churn, but we break geo at least a couple times a year, and finding cases where geo-traits is useful to geo will be helpful in making the case for integrating it.

michaelkirk commented 2 weeks ago

I guess since this example was an impl on geo_types, not geo, I'd expect quite a bit of stability in geo-traits before being comfortable with merging it:

impl geo_types::Polygon {
   fn line_refs(&self) -> impl Iterator<Item = impl LineTrait> {
       self.0.iter().window(2)
   }
}

But we could stick a similar method wherever we wanted, it could even start as private if its goal is to be used under the hood of specific algorithms.

fn line_refs(polygon: &geo_types::Polygon) -> impl Iterator<Item=geo_traits::Line>{
       self.0.iter().window(2)
}
urschrei commented 2 weeks ago

But we could stick a similar method wherever we wanted, it could even start as private if its goal is to be used under the hood of specific algorithms.

Do you mean make geo-traits available to internal APIs only?

michaelkirk commented 2 weeks ago

Yes, that'd be one way. Or an even more conservative version would be to put the method in the geo-traits crate somewhere.