-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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 tests file for Quaternion unit tests, with initial UTs #50907
Conversation
codepatzer
commented
Jul 26, 2021
•
edited by akien-mga
Loading
edited by akien-mga
- Test constructors and quaternion product.
- Add test case for Axis-Angle construction about Y-axis.
- Add test case for xform of i-, j-, & k-unit vectors.
- Add test case for construction from Basis.
- Add test case for xform of arbitrary vector.
- Add stress test case: many Quaternions xform many vectors.
- Make comments consistent with style guide.
Thanks for kicking off the checks! And thanks very much for the links. I see that I have not been following the process quite as the project lays it out. I will mend my ways in future, or at least move on to a new set of mistakes. |
1c22b5a
to
b793e4f
Compare
Rebased to have a single commit after checks passed, following a half dozen separate commits. |
101a1cf
to
b666657
Compare
Rebased onto current upstream master branch. |
b666657
to
bdd5bfe
Compare
Sorry for the late review. @codepatzer Are you available to rebase this PR and squash the commits? The test should also be moved to |
- Test constructors and quaternion product. - Add test case for Axis-Angle construction about Y-axis. - Add test case for xform of i-, j-, & k-unit vectors. - Add test case for construction from Basis. - Add test case for xform of arbitrary vector. - Add stress test case: many Quaternions xform many vectors. - Make comments consistent with style guide.
bdd5bfe
to
d0cb0ff
Compare
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.
Apologies for the delay reviewing this.
The tests look great! I force pushed an update to rebase and squash the commits, and did a few minor changes:
- Removed usage of
M_PI
and redefinition ofdeg2rad
: we have all this already incore/math/math_defs.h
andcore/math/math_funcs.h
so you just need to use those. - Fix build issue for "Construct Basis Axes" since
get_axis
was renamed toget_column
. - The comment there about not being able to construct Basis from vectors may no longer be true, as there is a
Basis(const Vector3 &row0, const Vector3 &row1, const Vector3 &row2)
constructor. It's rows and not columns so might not be directly useful here though. I left it as is for now, could be revisited in a follow up PR.
Thanks! And congrats for your first merged Godot contribution 🎉 |