georust / rstar

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

add mint feature for Point trait impl for mint::Point2 and mint::Point3 #134

Closed ripytide closed 10 months ago

ripytide commented 10 months ago

Helps with #28

adamreichold commented 10 months ago

I think if we do this, we should aim for broader coverage, e.g. include at least also mint::Point3.

ripytide commented 10 months ago

I've added an impl for mint::Point3

adamreichold commented 10 months ago

I am sorry, for not asking for this earlier, but I do think we need at least some smoke test. Less for the actual code which is simple enough but for the integrate to actually work.

Could you add one of the crates supporting mint as a dev-dependency and write simple smoke tests for the two integrations, e.g. adding points using the foreign types to a tree and running some query on it? Thanks!

ripytide commented 10 months ago

I'm not sure I understand what the purpose of such a smoke test would be? These impls only allow rstar::Point to work on mint::Points as far as I am aware, so you couldn't put something like a nalgebra::Point2 in an rtree::RTree since rstar::Point is not implemented for nalgebra::Point2.

ripytide commented 10 months ago

Unless the rstar library decided to use impl Into<T> where T: Point for accepting points in every interface in the lib which doesn't seem to be the case.

adamreichold commented 10 months ago

Indeed mint::Point2/3 are concrete types, but then that observation is basically the point: How would usage of this integration look in practice? What do I have to do to convert between this and the actual target types?

Actually, we might want to make these doctests to show case to readers of the documentation and ensure to the enable the mint dependency on docs.rs.

ripytide commented 10 months ago

That's a good point, I should have probably given my use-case up front to help illustrate why impl Point for mint::PointN is useful.

In my use-case I am using a foreign point type, let's say nalgebra::Point2 for example, I then want to store these in an rtree:RTree which gives me a few options since nalgebra::Point2 rightfully does not implement rtree::Point:

  1. Make a local newtype wrapper for nalgebra::Point2 and manually implement rtree::Point on that newtype and then convert nalgebra::Point2s into my newtype before storing them in the rtree::RTree.
  2. Convert my nalgebra::Point2s into mint::Point2 (already implemented in the nalgebra crate with the "mint" feature) and then store those in the rtree::RTree (assuming this PR was merged so mint::Point2: rtree::Point)
  3. Overhaul the API trait bounds here to be able to store nalgebra::Point2s directly

Option 1 does not require changing the rstar library but is not very ergonomic for users as they have to make their own newtype everytime they want to store a foreign type. Option 2 is more ergonomic as users can just store mint::Point2s and use the ready-to-go conversions with this PR Option 3 would require an overhaul of the libraries trait bounds on most things which is a lot of library work but would allow users to store raw foreign types such as nalgebra::Point2 (given both libs had mint features enabled). These impls in this PR would also be required for this method.

ripytide commented 10 months ago

I am happy to add doc-tests for a use-case of Option 2 if you are happy to go that route

adamreichold commented 10 months ago

Yes, option 2 was my understanding of how this is supposed to be used and I would be glad if we could add doctests with working examples e.g. using nalgebra as the integration target.

ripytide commented 10 months ago

I've added documentation to the mint module with a doctest example of an integration with nalgebra. I also fixed a non-deterministic error with the nearest_neighbors doctest that was failing for me.