jkvargas / russimp

Assimp bindings for Rust
Other
84 stars 27 forks source link

Change Argument Type from u32 to aiTextureType #35

Closed AregevDev closed 1 year ago

AregevDev commented 1 year ago

This addresses #34.

Essentially the Rust TextureType has a #[repr(u32)] which is fine. However the original C++ aiTextureType is not typed, the compiler decides it's underlying type.

This causes an issue when building on Windows MSVC. The compiler seems to decide that aiTextureType's underlying type is signed int.

On line 118 in material.rs, you cast your u32 to an i32, and later, on line 123 you call get_texture_filename which expects a u32 argument, with an i32 value. This causes the compilation error.

Simple solution: change the texture_type argument type in get_texture_filename to aiTextureType instead of u32. By doing this, you let the implementation decide whether your type is signed or not, fixing the error.

I hope I explained it right
Peace 😄

jkvargas commented 1 year ago

Awesome dude, thanks so much for your contribution. Can you, please, raise the Cargo.toml to 2.0.2? :)

AregevDev commented 1 year ago

Sure, I'll do it when I wake up

AregevDev commented 1 year ago

Would you like me to run rustfmt and clippy as well before submitting? The codebase could use a little clean-up... I see a lot of outdated code from previous Rust editions as well.

jkvargas commented 1 year ago

of course dude! =) thanks

AregevDev commented 1 year ago

I have no idea why it fails to build, locally everything works fine...

P.S do you have Discord? It will be much easier to discuss there

AregevDev commented 1 year ago

I notice you use Unix-style newlines inside rustfmt.toml. Consider removing that line. git handles the conversion between LF and CRLF automatically. When I clone your repo on my Windows machine it clones with CRLF. However when running rustfmt, it changes back to LF and marks all of my files as changed when committing.

AregevDev commented 1 year ago

I would like to clean up the codebase once you merge the PR. I don't want to bloat this branch so I'll open a new one.