load into Vec3r struct from ply

[rezural]

This adds loading into structs directly from ply-rs.

as per your suggestions from #1.

Couple of things:
You may want me to pull the ply stuff out into a submodule, which I can do.
I may not have been using the repr(transparent) properly, I still had to access the inner struct through .0, so I have left that commented out for now.

This has to map from Vec<Vec3r> to Vec<Vector3<R>> manually, so I'm not sure if I'm doing this right.

Any advice welcome...

Thanks!

[w1th0utnam3]

Well, the trick is to use some unsafe code that we know is actually safe. When you annotate a type with #[repr(transparent)] this means that the new type is represented in memory exactly the same as the inner type. For details, see e.g. https://doc.rust-lang.org/1.26.2/unstable-book/language-features/repr-transparent.html. This allows us on one hand to implement traits and methods on the new type (like you did), but it also allows us to just view instances of the new type as instances of the inner type. This is done by transmuting (like casting without conversion in other languages). In our case, this would be

// This is safe, because we defined Vec3r<R> as transparent over Vector3<R>
let particles = unsafe { std::mem::transmute::<Vec<Vec3r<R>>, Vec<Vector3<R>>>(particles_ply) };

(we could drop the explicit type arguments on transmute and let type-inference figure it out, but the explicit arguments ensure that we don't accidentally transmute to a wrong type when refactoring)
In the end, the transmute is a no-op and saves us copying and manually converting each individual element.

However, for this to work, your Vec3r definition is missing R: Real as generic argument, which should also be the type used by the Vector3 (because this is the type returned by the particles_from_ply function). So the type definition would be

#[repr(transparent)]
struct Vec3r<R: Real>(Vector3<R>);

and you would have to slightly adapt your trait implementation.

Do you want to make these changes?

[w1th0utnam3]

Ah, and yes, it would probably make sense to modularize the IO stuff more (maybe move it to the lib) and add some unit tests with sample files, but I don't want to get into this right now.

[w1th0utnam3]

Whoops, made a mistake. The unsafe code actually has to be

    // Convert the particle vector from `Vec<Vec3r<R>>` to `Vec<Vector3<R>>`
    let particles = unsafe {
        // Ensure the original vector is not dropped
        let mut particles = std::mem::ManuallyDrop::new(particles_ply);
        // Construct the new vector by converting the data pointer
        Vec::from_raw_parts(
            // This is safe, because we defined Vec3r<R> transparent over Vector3<R>
            particles.as_mut_ptr() as *mut Vector3<R>,
            particles.len(),
            particles.capacity(),
        )
    };

because you are only allowed to transmute the actual data of the Rust Vec, not the Vec itself. See the Vec example here and the comment here.

[rezural]

Hrm,

How about we leave it with the (non-idiomatic) approach for now?

I am thinking of solving this on the Ply-rs side, i.e. adding a data marshalling interface for when your types arent local (honestly most people will be using 3rd party structs for their 3d data?).

Cheers

[w1th0utnam3]

Right, support for this in ply-rs itself would be much cleaner.

I'll add a few comments before merging.

[w1th0utnam3]

I think we should rename this to Vec3f32 for now, because currently we only support reading PLY files with floats.

[w1th0utnam3]

• you can remove the repr attribtute

[w1th0utnam3]

Maybe a better default is to use NAN here? So that if properties are missing below, it is not as silent as it would be with zeros? Btw. you can write

Self(Vector3::repeat(std::f32::NAN))

[w1th0utnam3]

Instead of unwrap

.context("Failed to read PLY header")?

[w1th0utnam3]

Ok, I moved the file format stuff into submodules. You would have to move your changes into the ply_format module.

[rezural]

Hey,

I'll have a look at this soon, been moving around the last week, so it has been hard to get time at the computer..

Cheers!

[w1th0utnam3]

No rush

[rezural]

So we should probably close this.

Apologies, it has been quite a long time ago.

IIRC I had a look into making it more idiomatic, but it was a PITA, and didnt make the code any clearer, or simpler.

Cheers