Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't use PointExt methods in custom implementation #46

Open
berkut0 opened this issue Aug 2, 2020 · 13 comments
Open

Can't use PointExt methods in custom implementation #46

berkut0 opened this issue Aug 2, 2020 · 13 comments

Comments

@berkut0
Copy link

berkut0 commented Aug 2, 2020

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
Copy link
Member

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

@berkut0
Copy link
Author

berkut0 commented Aug 2, 2020

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
Copy link
Author

berkut0 commented Aug 2, 2020

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

@adamreichold
Copy link
Member

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
Copy link
Member

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
Copy link
Author

berkut0 commented Aug 2, 2020

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
Copy link
Author

berkut0 commented Aug 2, 2020

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
Copy link
Collaborator

Stoeoef commented Aug 2, 2020

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:

  • Implement add and sub locally.
  • Use another linear algebra crate and convert their point type into rstar::Point before inserting them into rstar

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
Copy link
Author

berkut0 commented Aug 2, 2020

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

@adamreichold
Copy link
Member

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
Copy link
Collaborator

Stoeoef commented Aug 2, 2020

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
Copy link
Author

berkut0 commented Aug 3, 2020

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
Copy link
Author

berkut0 commented Aug 3, 2020

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants