jkvargas / russimp

Assimp bindings for Rust
Other
84 stars 27 forks source link

Fixed Mesh::texture_coords only containing the first set of coords #15

Closed eliaperantoni closed 3 years ago

eliaperantoni commented 3 years ago

Hello!

I started using this crate but I noticed that Mesh::texture_coords contains all 8 possible UV channels but only the first set of coordinates for each.

But the Assimp docs say that, for each UV channel, there are mNumVertices sets of texture coordinates, not just one.

For example: I have a cube and this is what dbg!(&mesh.texture_coords) prints:

[src/model.rs:79] &mesh.texture_coords = [
    Some(
        Vector3D {
            x: 0.625,
            y: 0.5,
            z: 0.0,
        },
    ),
    None,
    None,
    None,
    None,
    None,
    None,
    None,
]

While the .obj contains 14 coordinates:

vt 0.625000 0.500000
vt 0.875000 0.500000
vt 0.875000 0.750000
vt 0.625000 0.750000
vt 0.375000 0.750000
vt 0.625000 1.000000
vt 0.375000 1.000000
vt 0.375000 0.000000
vt 0.625000 0.000000
vt 0.625000 0.250000
vt 0.375000 0.250000
vt 0.125000 0.500000
vt 0.375000 0.500000
vt 0.125000 0.750000

As you can see, only the first row is available from Russimp.

They are expanded to 24 coordinates once loaded by Assimp because I'm using triangulation (I think). Anyways the same thing happens in C++

This PR aims to fix this by making the texture_coordinates a Vec<Option<Vec<Vector3D>>>.

I did some testing with a .obj cube. On the left are the texture coordinates on UV channel 0 according to Russimp (with this PR applied) and on the right is the C++ version using pure Assimp.

image

Hope I did follow the style guide of the repo! I tried to do it as cleanly as possible but I'm still quite the noob at Rust.

Let me know what you think and thank you for providing this crate!

jkvargas commented 3 years ago

That is great mate. Thanks so much for contributing. Do you think you can add a test to covert this issue? Maybe you can add you obj to models folder :) Also, would you mind to raise the version in Cargo.toml as well? Thanks in advance.

eliaperantoni commented 3 years ago

Certainly! Here it is.

jkvargas commented 3 years ago

thanks a lot, man!