-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add triangle_math tests and fix Triangle3d::bounding_sphere bug #13467
Conversation
Diff on the final commit looks good to me :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the computation for the degenerate triangle should be done in the same way as for the obtuse one, which will remove a division by zero bug as well as making the code more regular. (Also depending on what optimisations the compiler misses it might make this path faster by avoiding unnecessary math.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the computation for the degenerate triangle should be done in the same way as for the obtuse one, which will remove a division by zero bug as well as making the code more regular. (Also depending on what optimisations the compiler misses it might make this path faster by avoiding unnecessary math.)
I've simplified the code paths as mentioned, and also added I've just realised that |
Yes please: these are really nice little utility methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it's pretty good, only thing I'd change is optimising away the square roots in the actute and obtuse functions.
Thanks for the work on this PR! |
Objective
Adopted #12659.
Resolved the merge conflicts on #12659;
triangle_tests
added by this PR and by Math primitives cleanup #13020.#[should_panic]
.