georust / rstar

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

Can't use PointExt methods in custom implementation #46

Open berkut0 opened 4 years ago

berkut0 commented 4 years ago

I'm trying something like:

impl<P> CustomPoint<P> where P: Point {
    fn new(cenet: P, size: f64) {
        let p = center.add( value );
        ...
    }
}

and it drops the error: items from traits can only be used if the trait is in scope

In 'rstar-demo' there is an example:

pub fn create_random_points<P: Point<Scalar = f32>>(num_points: usize) -> Vec<P>

and it's works, but the same doesn't work for me.

adamreichold commented 4 years ago

Could you produce a minimal but complete example so we can build and hopefully fix this locally?

berkut0 commented 4 years ago
use rstar::{Point, AABB};
// use rstar::point::PointExt; //module 'rstar::point' is private

fn main() {
    let p = Sample::new([0., 0.], 1.);
}

struct Sample<P>
    where
        P: Point
{
    aabb: AABB<P>,
}

impl<P> Sample<P>
    where P: Point
{
    fn new(center: P, size: f64) -> Self <> {
        let p1 = center.sub(P::from_value(size / 2.));
        let p2 = center.add(P::from_value(size / 2.));
        Sample {
            aabb: AABB::from_corners(p1, p2)
        }
    }
}
error[E0599]: no method named `sub` found for type parameter `P` in the current scope
  --> src\main.rs:19:25
   |
19 |         let p1 = center.sub(P::from_value(size / 2.));
   |                         ^^^ method not found in `P`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
1  | use rstar::point::PointExt;
   |
berkut0 commented 4 years ago

And what do you think about implementing AABB::from_center() by default?

adamreichold commented 4 years ago

Thank you for the worked example. So I think the request here is to make the PointExt trait part of the public API of rstar which it currently is not? I guess @Stoeoef will have to decide whether to commit to keeping these internals stable.

adamreichold commented 4 years ago

And what do you think about implementing AABB::from_center() by default?

If that would suffice to fulfill your use case, then IMHO it would be preferable to exposing all of PointExt.

berkut0 commented 4 years ago

So I think the request here is to make the PointExt trait part of the public API of rstar which it currently is not?

In 'rsrar-demo' it works somehow and I decided that it should work for me too.

If that would suffice to fulfill your use case, then IMHO it would be preferable to exposing all of PointExt.

Not sure what does it exactly mean, but most likely you are right. I understand that as if I need this functionality, then it's better to implement it by myself?

berkut0 commented 4 years ago

In 'rstar-demo' it works somehow and I decided that it should work for me too.

let new_point = P::generate(|i| points[i]);

изображение

Stoeoef commented 4 years ago

The demo also only uses the public API (https://docs.rs/rstar/0.8.2/rstar/trait.Point.html). It's just sub and add that are private.

PointExt is private by design. Making it public would mean to commit to an API for Points, which I don't want to. Doing so would shift rstar from being purely about RTrees to a more generic computational geometry library. There are other ones that will always be better for that purpose like nalgebra or cgmath.

The possible alternatives at the moment are scarce:

I know that both options add quite a bit of boilerplate. I've opened #28 a while ago to investigate how to better support other libraries but have not had a stroke of genius so far.

Please let me know if you have any remaining questions, otherwise I'll go ahead and close this issue in favour of #28.

berkut0 commented 4 years ago

@Stoeoef Thanks for the explanation! No more questions, it's pretty clear.

adamreichold commented 4 years ago

I understand that as if I need this functionality, then it's better to implement it by myself?

I was actually suggesting that we add a AABB::from_centre method to rstar itself assuming that is the only reason that you need access to PointExt for.

Stoeoef commented 4 years ago

Adding AABB::from_centre(center: Point, dimensions: Point) would be a mostly okay addition IMO. The only thing grudging me is that the second parameter should be rather a Vector (which doesn't exist in rstar) than a Point. I don't see a better solution at the moment though. If there is high enough demand, I would be fine with having this method added to AABB.

berkut0 commented 4 years ago

For experimentation, I cloned the repository and tried to create my own primitive. And that works (don't know is it interesting or not):

impl<P> AABB<P>
    where
        P: Point<Scalar=f64>
{
    pub fn from_center(center: P, size: f64) -> Self {
        let size: P = P::from_value(size / 2.);
        let p1: P = center.add(&size);
        let p2: P = center.sub(&size);
        AABB::from_corners(p1, p2)
    }
}

Actually, AABB implementation has something similar (but more clever):

fn center(&self) -> Self::Point {
    let one = <Self::Point as Point>::Scalar::one();
    let two = one + one;
    self.lower.component_wise(&self.upper, |x, y| (x + y) / two)
}

The only thing grudging me is that the second parameter should be rather a Vector (which doesn't exist in rstar) than a Point

This is a dilemma) And I don't have any wise solution to this problem, I'm too nooby for this type of situations. I don't think that it's necessary to add AABB::from_centre(center: Point, dimensions: Point) with a point rather than a vector, but it was the only reason why I asked about sub and add

berkut0 commented 4 years ago

And, if to be clear, I'm trying to implement something like Circle on the basis of rstar point or rectangle. Maybe it's a good idea to add this in primitives at least experimentally)