Add GeomWithCachedEnvelope combinator to support memoizing envelope computations.

[adamreichold]

• [x] I agree to follow the project's code of conduct.
• [x] I added an entry to rstar/CHANGELOG.md if knowledge of this change could be valuable to users.

Closes #108

[adamreichold]

Personally, I am very much open to bike-shedding the name as I think GeomWithCachedEnvelope is a bit of a mouthful. Maybe just CachedEnvelope? The geom method could also be dropped entirely due to the Deref impl.

[rmanoka]

Nice, this is great. Reg. the name: we could also merge this func. into GeomWithData<T::Envelope, T> and add the PointDist impl. We can then type alias it, and / or add a constructor GeomWtihData::cached_envelope.

Should we also add the explanation on why this is necessary somewhere more visible to most users? Probably the RTreeObject trait or the main data-structure itself?

[urschrei]

Should we also add the explanation on why this is necessary somewhere more visible to most users? Probably the RTreeObject trait or the main data-structure itself?

Yes, definitely. From the perspective of someone looking for a spatial index, it's currently not even obvious what rstar is for; that is, you have to know (or find out) what an R*-tree is. A light edit of the docs to make it a little clearer what's going on, and when to choose RTreeObject, GeomWithData, GeomWithCachedEnvelope would go a long way I think. We can do that in a follow-up PR.

[adamreichold]

Nice, this is great. Reg. the name: we could also merge this func. into GeomWithData<T::Envelope, T> and add the PointDist impl. We can then type alias it, and / or add a constructor GeomWtihData::cached_envelope.

I admittedly do not follow. You mean using GeomWithData and store the envelope instead of the underlying geometry in geom and the geometry in data? How would the PointDistance impl decided whether to forward to geom or data in these circumstances? Wouldn't the impls overlap like

error[E0119]: conflicting implementations of trait `object::PointDistance` for type `primitives::geom_with_data::GeomWithData<_, _>`
  --> rstar/src/primitives/geom_with_data.rs:69:1
   |
48 | impl<R: PointDistance, T> PointDistance for GeomWithData<R, T> {
   | -------------------------------------------------------------- first implementation here
...
69 | impl<T: PointDistance> PointDistance for GeomWithData<T::Envelope, T> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `primitives::geom_with_data::GeomWithData<_, _>`

[rmanoka]

Yes, I didn't think about the point dist implementation properly. So let's go with what you have; may be a shorter name if you find one.

[adamreichold]

So let's go with what you have; may be a shorter name if you find one.

I'd propose just using CachedEnvelope<T>, i.e. dropping the GeomWith part and exclusively relying on Deref instead of a geom method. Agreed?

[adamreichold]

Merging since the shorter name seems uncontroversial and the code itself was previously approved.

[adamreichold]

Ouch, I totally forgot that we use bors here and merged manually for which I am sorry!

@urschrei I think it should be possible to prevent manual merges via the GitHub UI using the branch protection rules?

[urschrei]

Oh don't worry! I'll have a look at the settings.