Add Matrix4 and Quaternion to Core; rebuild Transform on native math#54
Add Matrix4 and Quaternion to Core; rebuild Transform on native math#54jayrulez wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis change adds Matrix4 and Quaternion math modules, refactors Transform to typed position/rotation/scale fields, replaces vector ChangesMath and Runtime Transformation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RenderSample
participant Transform
participant Quaternion
participant Matrix4
RenderSample->>Transform: assign position and quaternion rotation
Transform->>Quaternion: convert rotation to Matrix4
Quaternion-->>Transform: return rotation matrix
Transform->>Matrix4: compose scale, rotation, and translation
Matrix4-->>Transform: return transform matrix
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Engine/cpp/Runtime/Core/Math/Vector4.cppm (1)
236-261: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the intrinsics include for
Vector4::dot
Vector4.cppmstill uses__m128/_mm_*onARCH_X64, but neither this module nor its imported:common/core.defschain brings in the intrinsics declarations. Re-addRuntime/Platform/intrinsics.hor include<immintrin.h>here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/Vector4.cppm` around lines 236 - 261, Restore the required SIMD intrinsics include for the ARCH_X64 branch of Vector4::dot, adding Runtime/Platform/intrinsics.h or the appropriate immintrin header at the module scope. Ensure the existing __m128 and _mm_* usage in dot compiles with its declarations available.
🧹 Nitpick comments (5)
Engine/cpp/Runtime/Core/Math/Math.test.cpp (3)
1501-1514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the antipodal slerp path.
The current inputs only exercise a positive quaternion dot product. Add interpolation between
qand-q, asserting that the midpoint remains finite and represents the same rotation; this covers shortest-path sign correction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/Math.test.cpp` around lines 1501 - 1514, Extend the “slerp endpoints and midpoint” test with an antipodal pair using a quaternion and its negation. Assert the midpoint is finite and, when applied to a vector, represents the same rotation as the original quaternion, covering shortest-path sign correction.
1429-1434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the near/far depth mapping.
These assertions never validate the coefficients derived from
nearandfar, so a broken depth projection would still pass. Project points on both clipping planes and assert their intended NDC depths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/Math.test.cpp` around lines 1429 - 1434, Extend the “perspective has the expected projective structure” test for Matrix4::perspectiveFovRH to project points at the near and far clipping planes and validate their resulting NDC depths. Retain the existing projective-structure and scale assertions, and ensure both near/far-derived depth coefficients are exercised.
1489-1491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse non-commuting rotations to verify composition order.
half * halfonly tests angle accumulation; reversing multiplication order produces the same result. Compose rotations around distinct axes and compare against sequential vector rotation or matrix multiplication to lock down the row-vector convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/Math.test.cpp` around lines 1489 - 1491, Update the quaternion composition test around half to use rotations about distinct axes whose multiplication order produces different results. Compare the composed quaternion’s rotateVector result with the expected sequential rotation or equivalent matrix multiplication, preserving the row-vector convention and ensuring reversed multiplication would fail.Engine/cpp/Runtime/Core/Math/TypesCommon.cppm (1)
66-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
inline constexprfor constants.Declaring these as
static constinstead ofinline constexprmeans they cannot be evaluated at compile time, which prevents them from being used inconstexprcontexts (such as default member initializers for otherconstexprtypes).If the
Vector3constructors are fully defined in this module or a directly imported one, consider changing these toinline constexprand initializing them inline here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/TypesCommon.cppm` around lines 66 - 69, Update the Vector3 constants zero and one in TypesCommon.cppm from static const declarations to inline constexpr definitions, initializing them inline when the available Vector3 constructors support constant evaluation; otherwise ensure the required constructor definition is imported or defined before these declarations.Engine/cpp/Runtime/Core/Math/Matrix4.cppm (1)
201-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark
nearlyEqualasconstexpr.To maintain consistency with the
Vectortypes and enable compile-time equality checks, consider marking this function asconstexpr.♻️ Proposed refactor
- [[nodiscard]] inline bool nearlyEqual(const Matrix4& a, const Matrix4& b, f32 epsilon = CMP_EPSILON) noexcept + [[nodiscard]] constexpr bool nearlyEqual(const Matrix4& a, const Matrix4& b, f32 epsilon = CMP_EPSILON) noexcept🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Engine/cpp/Runtime/Core/Math/Matrix4.cppm` around lines 201 - 212, Mark the Matrix4 overload of nearlyEqual as constexpr while preserving its existing noexcept signature, default epsilon, element-wise comparison, and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Engine/cpp/Runtime/Core/Math/Transform.cppm`:
- Around line 15-17: Replace the Transform default initializers for position and
scale with direct constexpr Vector3 construction, rather than referencing
Vector3::zero or Vector3::one; keep Quaternion::identity unchanged unless it has
the same constexpr issue, and ensure identityTransform and other constexpr
Transform usages compile.
---
Outside diff comments:
In `@Engine/cpp/Runtime/Core/Math/Vector4.cppm`:
- Around line 236-261: Restore the required SIMD intrinsics include for the
ARCH_X64 branch of Vector4::dot, adding Runtime/Platform/intrinsics.h or the
appropriate immintrin header at the module scope. Ensure the existing __m128 and
_mm_* usage in dot compiles with its declarations available.
---
Nitpick comments:
In `@Engine/cpp/Runtime/Core/Math/Math.test.cpp`:
- Around line 1501-1514: Extend the “slerp endpoints and midpoint” test with an
antipodal pair using a quaternion and its negation. Assert the midpoint is
finite and, when applied to a vector, represents the same rotation as the
original quaternion, covering shortest-path sign correction.
- Around line 1429-1434: Extend the “perspective has the expected projective
structure” test for Matrix4::perspectiveFovRH to project points at the near and
far clipping planes and validate their resulting NDC depths. Retain the existing
projective-structure and scale assertions, and ensure both near/far-derived
depth coefficients are exercised.
- Around line 1489-1491: Update the quaternion composition test around half to
use rotations about distinct axes whose multiplication order produces different
results. Compare the composed quaternion’s rotateVector result with the expected
sequential rotation or equivalent matrix multiplication, preserving the
row-vector convention and ensuring reversed multiplication would fail.
In `@Engine/cpp/Runtime/Core/Math/Matrix4.cppm`:
- Around line 201-212: Mark the Matrix4 overload of nearlyEqual as constexpr
while preserving its existing noexcept signature, default epsilon, element-wise
comparison, and return behavior.
In `@Engine/cpp/Runtime/Core/Math/TypesCommon.cppm`:
- Around line 66-69: Update the Vector3 constants zero and one in
TypesCommon.cppm from static const declarations to inline constexpr definitions,
initializing them inline when the available Vector3 constructors support
constant evaluation; otherwise ensure the required constructor definition is
imported or defined before these declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 809f98a5-3faa-4156-b6a3-48fc851a9c2d
📒 Files selected for processing (15)
Engine/cpp/Runtime/Core/CMakeLists.txtEngine/cpp/Runtime/Core/Math/Functions.cppmEngine/cpp/Runtime/Core/Math/Math.cppmEngine/cpp/Runtime/Core/Math/Math.test.cppEngine/cpp/Runtime/Core/Math/Matrix4.cppEngine/cpp/Runtime/Core/Math/Matrix4.cppmEngine/cpp/Runtime/Core/Math/Quaternion.cppmEngine/cpp/Runtime/Core/Math/Transform.cppEngine/cpp/Runtime/Core/Math/Transform.cppmEngine/cpp/Runtime/Core/Math/TypesCommon.cppmEngine/cpp/Runtime/Core/Math/Vector2.cppmEngine/cpp/Runtime/Core/Math/Vector3.cppmEngine/cpp/Runtime/Core/Math/Vector4.cppmEngine/cpp/Runtime/Rendering/Renderer/Renderer.cppSamples/cpp/Render/RenderSample.cpp
💤 Files with no reviewable changes (1)
- Engine/cpp/Runtime/Core/Math/Transform.cpp
| [[nodiscard]] static Vector3 cylindrical(f32 angle, f32 radius = 1.0f, f32 height = 0.0f) noexcept; | ||
|
|
||
| // constant vectors (defined in Vector3.cppm) | ||
| static const Vector3 zero; |
There was a problem hiding this comment.
Can't make these inline constexpr at the declaration. they're static members of Vector3's own incomplete type, and its constexpr ctor lives in Vector3.cppm which imports this partition, so it isn't reachable here. The out-of-line inline constexpr definition already makes them usable in constexpr contexts; Transform now uses direct construction regardless.
There was a problem hiding this comment.
This is a non-conformant behavior of clang. This code does not compile on g++. The one and zero you declared are runtime statics - not compile time. It's true that you cannot simply declare them static constexpr here, but promoting qualifier from static const in the declaration to inline constexpr in the definition is incorrect behavior, and clang should not have accepted this.
Declare them as functions instead.
static constexpr Vector3 zero();
static constexpr Vector3 one();
Introduce core.math.matrix4 (4x4 row-major, row-vector convention: identity/ translation/scale/rotation, perspective/ortho/lookAt RH, multiply, transpose, determinant, transformPoint/Direction/Point2D, and a scale-aware tryInverse/ inverse) and core.math.quaternion (fromAxisAngle, Hamilton product, conjugate/ dot/inverse/normalize/rotateVector/slerp, rotationMatrix), with the Matrix4 inverse bodies kept in Matrix4.cpp so the interface stays light. Rewrite Transform to position / rotation (Quaternion) / scale with an inline toMatrix() returning Matrix4 and a lerp helper, removing the last bx dependency from Core. Update its two callers (Renderer, RenderSample) to the new API. Add the math helpers these types need (sin/cos/tan/acos/sqrt, scalar and vector nearlyEqual/nearlyZero) and unit tests for Matrix4/Quaternion/Transform. Review follow-ups: expose singular inverses through tryInverse with a Hadamard row-norm threshold instead of a silent identity; guard a zero-length axis in fromAxisAngle; treat antipodal quaternions as equal; make the near-equal helpers constexpr and drop the redundant approx_eq; use the multidimensional subscript operator on Matrix4.
| import core.stdtypes; | ||
| import core.math.transform; | ||
| import core.math.matrix4; |
There was a problem hiding this comment.
| import core.stdtypes; | |
| import core.math.transform; | |
| import core.math.matrix4; | |
| import core; |
| const draco::math::Matrix4 model = transform.toMatrix(); | ||
| std::memcpy(p.model, model.data(), sizeof(p.model)); |
There was a problem hiding this comment.
Requiring a construction of a temporary is a step backwards compared to the previous API. Either allow assigning a Matrix4 to p.model, or restore the array filling function as an overload.
There was a problem hiding this comment.
It's ok to do using namespace draco; and/or using namespace draco::math; inside the body of main.
| [[nodiscard]] static Vector3 cylindrical(f32 angle, f32 radius = 1.0f, f32 height = 0.0f) noexcept; | ||
|
|
||
| // constant vectors (defined in Vector3.cppm) | ||
| static const Vector3 zero; |
There was a problem hiding this comment.
This is a non-conformant behavior of clang. This code does not compile on g++. The one and zero you declared are runtime statics - not compile time. It's true that you cannot simply declare them static constexpr here, but promoting qualifier from static const in the declaration to inline constexpr in the definition is incorrect behavior, and clang should not have accepted this.
Declare them as functions instead.
static constexpr Vector3 zero();
static constexpr Vector3 one();
| // ======================================================================= | ||
| // Transform - position / rotation / scale, composed as S * R * T. | ||
| // ======================================================================= | ||
| struct Transform |
There was a problem hiding this comment.
It's easier to declare the struct itself struct [[nodiscard]] Transform. Then any function returning this type is nodiscard. Same for any of the math types.
|
|
||
| // Identity transform (position 0, rotation identity, scale 1) - the | ||
| // default-constructed value. | ||
| inline constexpr Transform identityTransform{}; |
There was a problem hiding this comment.
Please declare static constexpr, or as a function identity() (no repetition of Transform, right now one has to type Transform::identityTransform).
This case worked, because this type is aggregate and complete at this point, unlike what happened with the Vector types.
Vector3::zero/one and Quaternion::identity declare as static constexpr functions instead, and give Transform the same treatment: identityTransform -> Transform::identity(). Mark Matrix4, Quaternion and Transform [[nodiscard]] on the struct itself, so every function returning one of them inherits it. Restore an array-filling toMatrix(f32[16]) overload on Transform use a using-declaration in RenderSample's main rather than repeating draco::math.
2 participants
Introduce core.math.matrix4 (4x4 row-major, row-vector convention: identity/translation/scale/rotation, perspective/ortho/lookAt RH, multiply, transpose,determinant, transformPoint/Direction/Point2D, and a scale-aware tryInverse/inverse) and core.math.quaternion (fromAxisAngle, Hamilton product, conjugate/dot/inverse/normalize/rotateVector/slerp, rotationMatrix), with the Matrix4 inverse bodies kept in Matrix4.cpp so the interface stays light.
Rewrite Transform to position / rotation (Quaternion) / scale with an inline toMatrix() returning Matrix4 and a lerp helper, removing the last bx dependency from Core. Update its two callers (Renderer, RenderSample) to the new API.
Add the math helpers these types need (sin/cos/tan/acos/sqrt, scalar and vector nearlyEqual/nearlyZero) and unit tests for Matrix4/Quaternion/Transform.
Review follow-ups: expose singular inverses through tryInverse with a Hadamard row-norm threshold instead of a silent identity; guard a zero-length axis in fromAxisAngle; treat antipodal quaternions as equal; make the near-equal helpers constexpr and drop the redundant approx_eq; use the multidimensional subscript operator on Matrix4.
TypesCommon zero/one: left as-is - they're static members; the out-of-line Vector3::zero definition requires the in-class declaration
PIC: Runtime is the only shared library, and it links the whole Core closure (Definitions/Math/IO/Memory/Logging) transitively - so all of those objects end up in libRuntime.so and must be PIC, not just Math. the only consumers paying the PIC cost otherwise are the test exes, which aren't perf-sensitive.
Summary by CodeRabbit
New Features
Bug Fixes
Tests