georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
410 stars 68 forks source link

SelectionIterator is private #26

Closed jerry73204 closed 3 years ago

jerry73204 commented 4 years ago

The SelectionIterator type is not documented nor public. It restricts the usage to define a custom function to return the iterator. Suppose we could make it public as well as the algorithm mod?

OvermindDL1 commented 3 years ago

Seems to be the case with all iterator types. I'm trying to return an iterator that is returned from the locate_in_envelope_intersecting function but its a private type so I can't access it. Replication case is just:

pub fn iterate_all_in_bounds(&self, aabb: &AABB<CoordType>) -> rstar::algorithm::iterators::LocateInEnvelopeIntersecting<ThingType> {
    self.rtree.locate_in_envelope_intersecting(aabb)
}

Gives an error of:

   |
17 |     ) -> rstar::algorithm::iterators::LocateInEnvelopeIntersecting<Chunk> {
   |                 ^^^^^^^^^ private module
   |
note: the module `algorithm` is defined here
  --> /home/<user>/.cargo/registry/src/github.com-1ecc6299db9ec823/rstar-0.8.2/src/lib.rs:27:1
   |
27 | mod algorithm;
   | ^^^^^^^^^^^^^^

And it doesn't seem to be being hoisted anywhere else publically, so unsure how to use this iterator or the others defined in that namespace...

rmanoka commented 3 years ago

This seems like an oversight in not exposing the iterators. However, in the interest of not creating breaking changes if the underlying iterator types change, would using impl Iterator work for you? Something like:

fn iterate_in_tree<'a, T: RTreeObject>(rtree: &'a RTree<T>, e: &T::Envelope) -> impl Iterator<Item = &'a T> {
    rtree.locate_in_envelope_intersecting(e)
}
OvermindDL1 commented 3 years ago

Not in my case, I was needing to store it for use in something else so I need the actual type. Right now I'm boxing it but that has annoyingly measureable overhead in a very tight loop.

Semver is fine enough for breaking changes on such things I'd say, something like an rtree is used in high performance so needing to update and recompile for a type change seems quite reasonable.

EDIT: Plus, how often would those iterators API's change anyway, an Iterator type is pretty well set and moving an object is as well, not like the iterators have other public API's we call on their specific types.

rmanoka commented 3 years ago

Ya, sounds fair. Could you provide a PR?

OvermindDL1 commented 3 years ago

I can try to get some time later this week but please remind me? Would give a chance for some of the other pull requests to be completed first as well, there is one iterator looking related one there. Toddler has eaten all my time lately... ^.^;

If anyone else can get to it then that would be better of course.

adamreichold commented 3 years ago

At least the request for explicit public iterator types should resolve the stalled discussion in https://github.com/georust/rstar/pull/67#discussion_r632707044

rmanoka commented 3 years ago

Closed by #77