Bugfixes and minor API changes

jkvargas / russimp

Assimp bindings for Rust

Other

84 stars 27 forks source link

Bugfixes and minor API changes #39

Open sutajo opened 1 year ago

sutajo commented 1 year ago

Hi!

I fixed some issues:

Material properties were not parsed correctly, because their type was treated as a bitflag.
Scene metadata was not parsed correctly because of the same reason.
String metadata was not cast correctly from raw data.
The Scene struct caused a memory leak because it contained Rc cycles. The correct way to implement a tree with pointer to parents is using weak pointers to the parents.

Minor API changes:

Changed the PostProcess flags to bitflags so that the flags can be computed at compile time.
Added additional orthographic_width property cameras to support ortographic cameras
Added helper functions to query material properties

Let me know what you think :) I would be happy to contribute!

jkvargas commented 1 year ago

Hey brother, thanks so much for the contribution. I would like to ask you some things.

Can you, please, split the improvements and the bug fixes into 2 different pull requests? The bug fixes specially if they don't break the user space then you can bump the minor version of the package.

For the bugs, can you please add tests to cover the issues? If I may give more info regarding that, can you please write a tests that you know it will fail, make it fail and then proceed to fix the code to make those tests pass?

Lets try this and I will review your PR again?

Quick one about the bitflags, adding that fixes any issues you mentioned?

sutajo commented 1 year ago

Sure, I will split up the PR.

The metadata parsing errors were not detected because the tests had errors too, for example a string property was expected to be a float array. I fixed the tests so that they expect the correct values. For the memory leak issue I'm not sure if I can write a test.

Changing PostProcess to bitflags did not fix any issues, but I think it is more convenient because:

there is no need to allocate a temporary vector, it is more efficient
const variables can be initialized with bitflags, but not with vectors

jkvargas commented 1 year ago

Thanks for the comprehension. For the memory leak it is ok to not have a test if you don't find a way to cover it =) But other issues would be nice to be covered with new tests, or get existing tests fixed in case those are supposed to cover the issue. A version with bitflags is welcome but that has to be on a second PR bumping the major version.