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

Into/From traits to transform Point's into array's #286

Open
icefoxen opened this issue Sep 14, 2017 · 2 comments
Open

Into/From traits to transform Point's into array's #286

icefoxen opened this issue Sep 14, 2017 · 2 comments

Comments

@icefoxen
Copy link
Contributor

Similar to how Vector's have conversions to/from [N;2], [N;3] etc, it would be nice if Point's could have similar transformations.

I tried making a PR but I got tangled up in generics and traits. :-/

@jswrenn
Copy link
Contributor

jswrenn commented Jan 22, 2018

I gave this a shot; here's what happened. TL;DR: I probably should have heeded @icefoxen's warning:

I tried making a PR but I got tangled up in generics and traits.

Anyways, first, to expand @icefoxen's explanation, the state of nalgebra is that matrix/array conversions are smooth in both directions:

let matrix : VectorN<f32, U3> = [1., 2., 3.,].into();
let array: [f32; 3] = matrix.into();

...but this does not yet work for Point:

let point : PointN<f32, U3> = [1., 2., 3.,].into();
let array: [f32; 3] = point.into();

The matrix/array conversions are possible due to lots of codegen.

Approach 1: More Codegen

One solution is to perform similar codegen for Point. However, this would add further bloat to nalgebra and probably slow compile times.

Approach 2: Impl Madness

Given that Point just wraps a VectorN, can we just take advantage of all this existing codegen? Sort of...

From [N; T] to Point<N, T>

In the absence of const generics, we can't actually write down an impl of From that's generic over the length of the array. However, we can write an impl in the form impl Into<VectorN<N, D>> → Point<N, D> like so:

impl<N, D, T> From<T> for Point<N, D>
  where N: Scalar,
        D: DimName,
        T: Into<VectorN<N, D>>,
        DefaultAllocator: Allocator<N, D> {
    #[inline]
    fn from(arr: T) -> Self {
        Point::from_coordinates(arr.into())
    }
} 

Introducing this impl would allow users to write code like this:

let point : PointN<f32, U3> = [1., 2., 3.,].into();

From Point<N, D> to [N; D]

A similar approach for this direction does not seem possible.

Using Into

impl<N, D, T> Into<T> for Point<N, D>
  where N: Scalar,
        D: DimName,
        T: From<VectorN<N, D>>,
        DefaultAllocator: Allocator<N, D> {
    #[inline]
    fn into(self) -> T {
        self.coords.into()
    }
}

This conflicts with an implementation in Rust's core:

impl<T, U> std::convert::Into<U> for T
    where U: std::convert::From<T>;

Using From

impl<N, D, T> From<Point<N, D>> for T
  where N: Scalar,
        D: DimName,
        T: From<VectorN<N, D>>,
        DefaultAllocator: Allocator<N, D> {
    #[inline]
    fn from(point: Point<N, D>) -> T {
        point.coords.into()
    }
}

This is a blanket impl, and blanket impls may only be defined in the same module as the implemented trait. Clearly, nalgebra should be integrated into the Rust standard library. ;-)

Consequences

We could add only the conversion implementation for [N; T] to Point<N, T> conversions and leave it to users to type .coords.into() for conversions in the other direction. However, adding the conversion implementation for [N; T] to Point<N, T> precludes adding an impl for conversions from Point to Vector, like this:

impl<N, D> From<Point<N, D>> for VectorN<N, D>
  where N: Scalar,
        D: DimName,
        DefaultAllocator: Allocator<N, D> {
    #[inline]
    fn from(point: Point<N, D>) -> VectorN<N, D> {
        point.coords
    }
}

...because the standard library provides the blanket impl of impl std::convert::From<T> for T.

Approach 3: Wait

Given that const-generics will resolve all of these issues and that this is, really, a paper-cut, it might be sensible to just wait.

@jswrenn
Copy link
Contributor

jswrenn commented Jun 29, 2018

This issue was recently solved to a limited extent (for U1 to U6) by #328.

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

2 participants