Added a test suite for the Plane object. #44492
Conversation
|
Please remove the empty lines shown here: https://github.com/godotengine/godot/pull/44492/checks?check_run_id=1576158367 When you're done, squash your commits following these instructions: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase And thank you for your first contribution! 🎉 |
6fa003a to
a5e507c
Compare
|
Please use tabs instead of spaces for indentation: https://docs.godotengine.org/en/stable/community/contributing/code_style_guidelines.html#c-and-objective-c |
03858ff to
7350c25
Compare
tests/test_plane.h
Outdated
| // Intersection functions need a pointer of a vector to modify | ||
| Vector3 *vec_out = new Vector3(); |
There was a problem hiding this comment.
Can be rewritten to:
| // Intersection functions need a pointer of a vector to modify | |
| Vector3 *vec_out = new Vector3(); | |
| Vector3 vec_out; |
And then use the variable by passing an address (&vec_out):
plane.intersect_3(y_facing_plane, z_facing_plane, &vec_out),
This way, there's no need to worry about delete afterward.
Also, if you do need to use new, Godot uses memnew macro instead (along with memdelete), used to better track memory.
tests/test_plane.h
Outdated
|
|
||
| CHECK_MESSAGE( | ||
| plane.get_normal().is_equal_approx(Vector3(32, 22, 16)), | ||
| "get_normal() should return the expected value."); |
There was a problem hiding this comment.
Unless the assertion is not obvious or requires some preparation steps (complex), I don't find it necessary to repeat what it does in words, so I'd just use plain CHECK macro in those cases, the testing framework is smart enough to showcase what are the actual and expected values.
Otherwise this adds too much verbosity, in my opinion.
But I realize this is already the case as seen in some other test suites. It's fine to have those messages. Perhaps documentation should also provide an objective criteria to elaborate what kind of assertions require to use either CHECK or CHECK_MESSAGE.
Added tests for intersection and plane-point methods.
|
Sorry for the delay further reviewing this. This seems like a good base for a Plane test suite, so let's get it merged. I rebased and amended the commit to address past review comments, and improve language/punctuation in some messages. |
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
Some parts are not implemented as I don't know how to test for the Variant bindings. This is also my first go at contributing as well, and i am not quite sure if the values i chose as test inputs and outputs are useful for the context of the class. It definitely needs tweaking and finishing up by someone with knowledge in vector math to give more helpful values.