Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfixes and minor API changes #39

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

sutajo
Copy link
Contributor

@sutajo sutajo commented Apr 4, 2023

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
Copy link
Owner

jkvargas commented Apr 5, 2023

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
Copy link
Contributor Author

sutajo commented Apr 5, 2023

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
Copy link
Owner

jkvargas commented Apr 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants