Skip to content

Conversation

@AregevDev
Copy link
Contributor

Closes #6
Test are included.
LMK what you think.

@AregevDev
Copy link
Contributor Author

AregevDev commented Jun 27, 2025

  • Add docs
  • Change the Adjugate implementation to use the minor and cofactor matrix?

@polymonster
Copy link
Owner

great stuff thanks, this all looks good. I will take a proper look at it tomorrow just before approving, I'm just finishing up work for the day now.

Yes you can go ahead add docs and change the adj implementation as well.

Just a quick question at a glance, the change to from matrix for the quat from mat3, was that implemented incorrectly in the first place then? I'll have to check where I was using it, index of 10 in a 3x3 matrix doesn't make sense though, so it seems like a mistake copied from a 4x4 matrix or something. mustn't have been any tests for it?

@AregevDev
Copy link
Contributor Author

AregevDev commented Jun 27, 2025

I believe so, it was accessing an array out of bonds. The compiler yelled at me when running cargo test. I had to change it.

If this function is tested, the test passed.

@polymonster
Copy link
Owner

Ahh ok that makes sense, I copied some of this implementation from my c++ library so maybe I was using it there and hadn’t actually tested this one.

I’ll write some tests for it. Thanks for catching

@polymonster polymonster merged commit 3db191a into polymonster:master Jun 29, 2025
1 check passed
@polymonster
Copy link
Owner

thanks for this, I will tag an push a new build to crates.io.

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.

Matrix minor / cofactor?

2 participants