Conversation
Source/CoreTests/Vector3Tests.cpp
Outdated
| float arr[3] = { 7.0f, 8.0f, 9.0f }; | ||
| Vector3 v3(arr); | ||
| EXPECT_FLOAT_EQ(v3.x, 7.0f); | ||
| EXPECT_FLOAT_EQ(v3.y, 8.0f); | ||
| EXPECT_FLOAT_EQ(v3.z, 9.0f); |
There was a problem hiding this comment.
What's the difference between v1 and v3 in this case? Array construction 👍
A good habit to have in these cases is to re-use the values from the array as a expected result, especially when dealing with strings.
EXPECT_FLOAT_EQ(v3.x, arr[0]);
There was a problem hiding this comment.
Also, add the other constructors to this test
Source/CoreTests/Vector2Tests.cpp
Outdated
| TEST(Vector2Tests, DotProduct) { | ||
| Vector2 a(1.0f, 2.0f); | ||
| Vector2 b(3.0f, 4.0f); | ||
| EXPECT_FLOAT_EQ(a.Dot(b), 11.0f); // 1*3 + 2*4 |
There was a problem hiding this comment.
Nice that's you're explaining the math in the expected result 👍
Source/CoreTests/Vector2Tests.cpp
Outdated
| Vector2 n = v.Normalized(); | ||
| EXPECT_NEAR(n.Length(), 1.0f, 1e-6f); | ||
| EXPECT_NEAR(n.x, 0.6f, 1e-6f); | ||
| EXPECT_NEAR(n.y, 0.8f, 1e-6f); |
There was a problem hiding this comment.
1e-6f can be a global constexpr to be shared in the relevant places
Source/CoreTests/Vector2Tests.cpp
Outdated
| TEST(Vector2Tests, OperatorsArithmetic) { | ||
| Vector2 a(2, 3); | ||
| Vector2 b(1, -1); | ||
|
|
||
| EXPECT_EQ(a + b, Vector2(3, 2)); | ||
| EXPECT_EQ(a - b, Vector2(1, 4)); | ||
|
|
||
| EXPECT_EQ(a * 2.0f, Vector2(4, 6)); | ||
| EXPECT_EQ(2.0f * a, Vector2(4, 6)); | ||
| EXPECT_EQ(a / 2.0f, Vector2(1, 1.5f)); | ||
| } |
There was a problem hiding this comment.
Nice but is missing the assignment operators too?
+=
-=
/=
*=
Source/CoreTests/Vector2Tests.cpp
Outdated
| TEST(Vector2Tests, ConstructorFromArray) { | ||
| float arr[2] = { 3.5f, -2.0f }; | ||
| Vector2 v(arr); | ||
| EXPECT_FLOAT_EQ(v.x, 3.5f); | ||
| EXPECT_FLOAT_EQ(v.y, -2.0f); | ||
| } |
There was a problem hiding this comment.
Nice but you should also test all the other constructors too
Source/CoreTests/Vector2Tests.cpp
Outdated
| EXPECT_EQ(a + b, Vector2i(3, 2)); | ||
| EXPECT_EQ(a - b, Vector2i(1, 4)); | ||
|
|
||
| EXPECT_EQ(a * 2, Vector2i(4, 6)); | ||
| EXPECT_EQ(2 * a, Vector2i(4, 6)); | ||
| EXPECT_EQ(a / 2, Vector2i(1, 1)); |
There was a problem hiding this comment.
Is also missing assignment operators
Source/CoreTests/Vector3Tests.cpp
Outdated
| TEST(Vector3Tests, ScalarMultiplicationDivision) { | ||
| Vector3 v(2, -4, 6); | ||
| Vector3 scaled = v * 2.0f; | ||
| EXPECT_FLOAT_EQ(scaled.x, 4); | ||
| EXPECT_FLOAT_EQ(scaled.y, -8); | ||
| EXPECT_FLOAT_EQ(scaled.z, 12); | ||
|
|
||
| Vector3 divided = scaled / 2.0f; | ||
| EXPECT_FLOAT_EQ(divided.x, 2); | ||
| EXPECT_FLOAT_EQ(divided.y, -4); | ||
| EXPECT_FLOAT_EQ(divided.z, 6); | ||
| } |
There was a problem hiding this comment.
You should also make tests to make sure that we catch division by zero. You can check how I implemented "death-tests" in matrix3x3
Source/CoreTests/Vector3Tests.cpp
Outdated
| TEST(Vector3iTests, ConstructorAndEquality) { | ||
| Vector3i v1(1, 2, 3); | ||
| EXPECT_EQ(v1.x, 1); | ||
| EXPECT_EQ(v1.y, 2); | ||
| EXPECT_EQ(v1.z, 3); | ||
|
|
||
| Vector3i v2 = Vector3i::Zero; | ||
| EXPECT_EQ(v2.x, 0); | ||
| EXPECT_EQ(v2.y, 0); | ||
| EXPECT_EQ(v2.z, 0); | ||
|
|
||
| int arr[3] = { 7, 8, 9 }; | ||
| Vector3i v(arr); | ||
| EXPECT_EQ(v.x, 7); | ||
| EXPECT_EQ(v.y, 8); | ||
| EXPECT_EQ(v.z, 9); | ||
| } |
There was a problem hiding this comment.
Is missing some constructors
| friend Vector2 operator+(Vector2 a, const Vector2 &b) noexcept { | ||
| a += b; | ||
| return a; | ||
| } | ||
| friend Vector2 operator-(Vector2 a, const Vector2 &b) noexcept { | ||
| a -= b; | ||
| return a; | ||
| } | ||
| friend Vector2 operator/(Vector2 a, float s) noexcept { | ||
| assert(s != 0.0f && "Division by zero in Vector2::operator/"); | ||
| a /= s; | ||
| return a; | ||
| } | ||
| friend Vector2 operator*(Vector2 a, float s) noexcept { | ||
| a *= s; | ||
| return a; | ||
| } | ||
| friend Vector2 operator*(float s, Vector2 a) noexcept { | ||
| a *= s; | ||
| return a; | ||
| } |
There was a problem hiding this comment.
Is there a reason for these being friend declared? 🤔
There was a problem hiding this comment.
friend is to allow them to be defined within the class, to keep them along the other operators, it has no other purpose here
| return (*this) * (1.0f / sqrtf(lsq)); | ||
| } | ||
| [[nodiscard]] Vector2 Reflect(const Vector2 &normal) const noexcept { | ||
| assert(fabs(normal.LengthSquared() - 1.0f) < 1e-3f && "Vector3::Reflect received non-normalized normal"); |
There was a problem hiding this comment.
Nice! Don't forget to create a test for this too
| return *this; | ||
| } | ||
| Vector2i &operator/=(int s) noexcept { | ||
| assert(s != 0 && "Division by zero in Vector2i::operator/="); |
There was a problem hiding this comment.
Nice! Don't forget to test this
OlleKReutercrona
left a comment
OlleKReutercrona
left a comment
There was a problem hiding this comment.
Really nice, glad to see you start writing test! Added some minor comments here and there but I'd like you to implement the missing tests I suggested before you merge 👍
Good job!
|
|
|
||
| disablewarnings { | ||
| "4723" -- division by zero | ||
| } |
2 participants
Cleaned up implementation of Vector3 and Vector3 float/int versions.
Also added some tests.