-
-
Notifications
You must be signed in to change notification settings - Fork 21k
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
Proposal-1778: Updating Transform.rotated() docs and test cases. #43694
Changes from 1 commit
f289b13
6def32d
7ce02b6
e8efe62
cc23d6d
10801b9
4e0552a
d267777
fb4fadf
252ae43
b8a6eb6
5c1195e
32ab77e
5e4a712
11e7963
53306cb
f7e152b
e70a0d9
edf3d22
7bf5fc7
b9e6cc8
5f61b2c
7c81ce4
2522632
89c718c
6cb8154
ead6d10
2d810e8
01d505e
ccf05ae
8b27954
46cc0af
ed5f6cc
d395dba
1a3d53c
442c65f
0abe464
994c120
8734437
de91700
67db227
9c13e91
cdd912c
9293c76
7a62bd1
3143e7d
97c6851
a5332ca
9013771
1b55d72
ff2f1f5
e84861d
14dcb97
044daf9
1858c92
c7f67da
dd70daa
1731030
eae7f1c
1363fe3
79e33f8
84faf39
53efc55
41e43e0
082f924
9359bee
a402bf0
73668c5
3f0fe0b
3221fb4
8138280
f3e76a5
112985c
3540e71
911c276
4f4f73b
2d08457
e96e268
a0ca7a6
b024d16
140b54f
d090925
4633f8c
1ae922a
ee238b2
a984536
8d2dedd
ab280de
4122834
281d0bd
9e18106
27417c0
62f8b0e
0fa574c
321419d
30cf49e
e2b58ef
11a49c2
dbb37e5
5503059
541701f
8abd460
bf523a2
3bcf03c
7d53755
1da948a
0623d36
4735a03
8879efc
a56e8f8
c89c287
ed5267f
e4cfcfc
cbdde9d
60988a0
cd57053
46607ec
0320532
97f0500
d18cbdf
b197fc2
5aa099a
60fee25
7fabbe2
32bf7c4
4296539
3e18cc2
483b8a5
9676651
2cc2ade
06b9ea1
0a3aa85
9556c3a
27b9b3e
c38ef94
dd5f01e
c86ab40
f72419d
6a9f374
341b532
fc9767a
50c0cee
1bf0e87
ad63556
23b51a1
4fc246e
f410852
2cd052f
b1237b5
1979266
6c0f44c
475facb
82f7f2c
e2e1a50
9302b65
ea0a9e6
4c1d2e9
3db672c
7468dd6
9dd0d3f
c1f5913
991f4d8
5e609d0
0276c2e
72c0770
bd573f8
27d4e2f
2fc31fd
f2caab4
3c00594
7926d75
ea54b61
1e0fe9f
5fe1857
d3c6395
485eac3
5ffda27
4028583
7db3dba
0013d6f
1ab8f3f
e1f7e4e
86a2e10
92d88fd
f91afeb
ba65730
3f6ed10
6e6f292
4a9a231
71f8b80
b55fd93
b8c9282
41c1cfe
f23b917
ac7505e
70871c6
bc0f5d3
e0f8410
6596c7b
998974f
3a19400
a662ca6
5d6a98f
213612b
50bb089
eec3f3e
bd411ae
570cdc1
5b7ec95
f4bebc2
a201f30
7f8e508
43c7448
767bde8
d2b8560
d0ec46b
c370b4c
7e8385f
94b27eb
770bd61
810d8f0
1c0ae31
3e1b630
e4dfa69
c63b185
77721b3
3fa76df
663e480
214bbfb
8dd8630
d46ac42
9543289
d1c2013
a28f44f
b32f84d
df57aa6
2e03527
8be97e3
4baddc1
ad30b0a
e519ef3
6ed3e9c
769691a
a7011fa
06c1b40
0c0b5c8
928c002
93cb71c
2a920a2
f34d67e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -31,11 +31,55 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
#ifndef TEST_MATH_H | ||||||||||||||||||||||||||||||||||||||||||||||||||
#define TEST_MATH_H | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
#include "core/os/main_loop.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||
#include "core/math/math_defs.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||
#include "core/math/math_funcs.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||
#include "core/math/transform.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||
#include "core/os/os.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
#include <math.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||
#include <stdio.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
#include "tests/test_macros.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
namespace TestMath { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
MainLoop *test(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
TEST_CASE("[Transform] Rotate around global origin") { | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Start with the default orientation, but not centered on the origin. | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Rotating should rotate both our basis and the origin. | ||||||||||||||||||||||||||||||||||||||||||||||||||
Transform transform = Transform(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
transform.origin = Vector3(0, 0, 1); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Transform expected = Transform(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
expected.origin = Vector3(0, 0, -1); | ||||||||||||||||||||||||||||||||||||||||||||||||||
expected.basis.set_axis(0, Vector3(-1, 0, 0)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
expected.basis.set_axis(2, Vector3(0, 0, -1)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI); | ||||||||||||||||||||||||||||||||||||||||||||||||||
REQUIRE(rotated_transform.is_equal_approx(expected)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// Make sure that the rotated transform isn't sharing references with the original transform. | ||||||||||||||||||||||||||||||||||||||||||||||||||
REQUIRE(&rotated_transform.basis != &transform.basis); | ||||||||||||||||||||||||||||||||||||||||||||||||||
REQUIRE(&rotated_transform.origin != &transform.origin); | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the first
Suggested change
This way, both "basis" and "origin" will be tested (without immediately exiting the test case if the first one fails). Same for the test case below. Also, not sure about referencing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems reasonable, I'll change the latter REQUIREs to CHECKs. As far as referencing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking more closely now, in this case So this should rather be:
Suggested change
Yes, this is done for a lot of existing core types in Godot, so usually you don't have to worry about this and just do equality checks. If those fail due to floating point accumulation, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the pointer memory addresses are what I wanted to check; I thought I could make sure that
But that's maybe not necessary, I think I'll just remove those assertions. Though in general, as a new Godot user, I'm not always clear on whether a function will modify the existing object or return a new object. |
||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
TEST_CASE("[Transform] Rotate in-place") { | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Start with the default orientation, centered on the origin. | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Rotating should rotate us around the origin, so the origin shouldn't change. | ||||||||||||||||||||||||||||||||||||||||||||||||||
Transform transform = Transform(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Transform expected = Transform(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
expected.basis.set_axis(0, Vector3(-1, 0, 0)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
expected.basis.set_axis(2, Vector3(0, 0, -1)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI); | ||||||||||||||||||||||||||||||||||||||||||||||||||
REQUIRE(rotated_transform.is_equal_approx(expected)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the code in the test cases is very nice. But two things:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll move the code over into a new file and make sure it compiles after the renamings, and change |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// Make sure that the rotated transform isn't sharing references with the original transform. | ||||||||||||||||||||||||||||||||||||||||||||||||||
REQUIRE(&rotated_transform.basis != &transform.basis); | ||||||||||||||||||||||||||||||||||||||||||||||||||
REQUIRE(&rotated_transform.origin != &transform.origin); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
#endif |
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.
While technically this should be implied from the current text, for clarity's sake we should mention "relative to the parent" somewhere here.