dimforge / nalgebra

Linear algebra library for Rust.
https://nalgebra.org
Apache License 2.0
3.95k stars 471 forks source link

2D point converts into 3D vector #995

Open hannobraun opened 3 years ago

hannobraun commented 3 years ago

I've come across a weird behavior that really baffled me initially.

This compiles with nalgebra 0.29:

let _: nalgebra::SVector<f32, 3> = nalgebra::point![1.0, 2.0].into();

It converts an OPoint<f32, 2> into an SVector<f32, 3>, using Into.

Here's the same, with debug output:

let v: nalgebra::SVector<f32, 3> = dbg!(nalgebra::point![1.0, 2.0]).into();
dbg!(v);

Output of first dbg!:

nalgebra::point![1.0, 2.0] = OPoint {
    coords: Matrix {
        data: [
            [
                1.0,
                2.0,
            ],
        ],
    },
}

Second dbg!:

v = Matrix {
    data: [
        [
            1.0,
            2.0,
            1.0,
        ],
    ],
}

I have some code that is generic over dimensions and uses an Into<SVector<f32, D>> ([1]), and this was really confusing. I was expecting that Into to get me conversions from arrays, maybe points of the same dimension, but not this.

This seems to be the From implementation that's responsible: https://github.com/dimforge/nalgebra/blob/1bc919e0dbf2f7e83fab8aed37c1a06fe1b4fc93/src/geometry/point_conversion.rs#L76-L85

It calls to_homogeneous. I'm sure this is useful in some cases, but it seems way too magical to me. Personally, I would remove this conversion as too confusing, but maybe someone with a more extensive math background than me would disagree. In any case, I wanted to leave my feedback.

sebcrozet commented 2 years ago

Hi! I agree this is a bit too magical and should be removed.

JosARodriguez commented 2 years ago

I think you may be getting this behavior in cases where the offset and the vector to which you apply it are not of the same dimension in which case to_homogeneous performs a conversion so that the dimensions match and it makes sense to work with the two vectors. The function converts a vector to its coordinates in projective space, which in short, takes a vector (x,y) and returns a vector (xz,yz,z) where z is non-zero. For simplicity to_homogeneous chooses z to be equal to 1, and thats why in your example you get (1,2) -> (1,2,1). Perhaps to avoid this it would be better to add a validation that verifies that the objects are of the same dimension before the call to into(), since the current implementation comes in very handy for people working with projective geometry.

hannobraun commented 2 years ago

Perhaps to avoid this it would be better to add a validation that verifies that the objects are of the same dimension before the call to into(), since the current implementation comes in very handy for people working with projective geometry.

Having to make a check before the call to into() would take away much of the convenience of using Into in the first place. It would also be one more thing I'd have to remember everywhere I'm doing such a conversion. In that case, it would be better not to use Into at all, in my opinion.

I realize that this conversion is useful in specific use cases, but as I said above, it is unexpected and baffling in others. And, in my opinion, it's too magical. I'd argue that code that wants to convert a point to homogeneous coordinates would be better off calling to_homogeneous directly instead of relying on a general (and possibly implicit) conversion. I think that would be much more clear and readable.