Add more vector types, functions, and tests#18
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSplit the monolithic draco::math into modules: float-based constants, a new core.math.functions module, a core.math.types aggregator with common/vector modules implementing Vector2/Vector3/Vector4, and extensive doctest coverage. Math Module Refactoring and Vector Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Apeirobyte
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
engine/native/core/math/functions.cppm (1)
104-112: ⚡ Quick winPrecision loss for
doublespecializations.
PIis afloatconstant, so whenTisdouble,T{PI}widens the float to double but retains only float precision (~7 digits instead of ~15). Usestd::numbers::pi_v<T>for type-appropriate precision:Proposed fix
template <std::floating_point T> constexpr T deg_to_rad(T y) noexcept { - return y * (T{PI} / T{180.}); + return y * (std::numbers::pi_v<T> / T{180}); } template <std::floating_point T> constexpr T rad_to_deg(T y) noexcept { - return y * (T{180.} / T{PI}); + return y * (T{180} / std::numbers::pi_v<T>); }🤖 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/native/core/math/functions.cppm` around lines 104 - 112, deg_to_rad and rad_to_deg currently use the float PI constant causing precision loss for double specializations; replace T{PI} with std::numbers::pi_v<T> so the pi value is generated at the correct floating-point type for both template functions (deg_to_rad and rad_to_deg) and ensure you include <numbers> where these templates are defined.engine/native/core/math/constants.cppm (2)
27-27: 💤 Low valueClarify the reciprocal naming.
UINT32_MAX_Fsuggests it holds the float representation ofUINT32_MAX, but it actually stores the reciprocal (1.f / max). Consider a clearer name likeINV_UINT32_MAX_ForUINT32_MAX_RECIP.🤖 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/native/core/math/constants.cppm` at line 27, The constant UINT32_MAX_F is misnamed because it stores the reciprocal (1.f / std::numeric_limits<uint32_t>::max()); rename it to a clearer identifier such as INV_UINT32_MAX_F (or UINT32_MAX_RECIP) and update all references to UINT32_MAX_F to the new name (e.g., any uses in functions or files referencing UINT32_MAX_F). Keep it constexpr float = 1.f / std::numeric_limits<uint32_t>::max(); and optionally add a short comment like "reciprocal of UINT32_MAX" to prevent future confusion; if backward compatibility is needed, add a deprecated alias constexpr float UINT32_MAX_F = INV_UINT32_MAX_F and mark it with a comment.
15-15: 💤 Low valueUse float literals for consistency with float constants.
Lines 15 and 21 use double literals (
1.,2.) which get implicitly narrowed to float. For clarity and consistency with the float type, use1.fand2.f.Suggested fix
- constexpr float SQRT12 = 1. / SQRT2; + constexpr float SQRT12 = 1.f / SQRT2;- constexpr float TAU = 2. * PI; + constexpr float TAU = 2.f * PI;Also applies to: 21-21
🤖 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/native/core/math/constants.cppm` at line 15, The float constants use double literals that get narrowed — update the numeric literals to float by changing occurrences like in the SQRT12 definition (and the other constant on line 21) from 1. and 2. to 1.f and 2.f respectively so the expressions (e.g., SQRT12 = 1.f / SQRT2) use float literals consistently with the float-typed constants.engine/native/core/math/math.test.cpp (1)
36-36: 💤 Low valueUse float literals for consistency.
The
powtest uses double literals (2.,.5) while the rest of the test suite consistently uses float literals (e.g.,1.0f). For consistency with the float-based math migration described in the PR summary, consider using2.0fand0.5f.Suggested fix
- float result = draco::math::pow(2., .5); + float result = draco::math::pow(2.0f, 0.5f);🤖 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/native/core/math/math.test.cpp` at line 36, The test uses double literals for draco::math::pow which conflicts with the float-based math migration; update the call in math.test.cpp so the literals are float types (e.g., change 2. and .5 to 2.0f and 0.5f) so the computed value assigned to result (float result) uses float operands consistently.
🤖 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 `@CMakeLists.txt`:
- Around line 20-21: The CMake file incorrectly sets CMAKE_CXX_MODULE_STD to
"libc++" (an invalid boolean); remove that line and either enable standard
library modules by setting CMAKE_CXX_MODULE_STD ON (to allow import std; C++23
modules) or, if the goal is to use libc++ as the standard library
implementation, append the flag -stdlib=libc++ to CMAKE_CXX_FLAGS (e.g., update
CMAKE_CXX_FLAGS to include -stdlib=libc++). Ensure you reference and modify the
CMake variable CMAKE_CXX_MODULE_STD or CMAKE_CXX_FLAGS accordingly.
In `@engine/native/core/math/types_common.cppm`:
- Around line 37-38: The unary operator declarations for Vector2, Vector3, and
Vector4 (operator+() and operator-()) must be made const so they can be used on
const instances; update the signatures in types_common.cppm to add the trailing
const to each declaration for Vector2::operator+(), Vector2::operator-(),
Vector3::operator+(), Vector3::operator-(), Vector4::operator+(), and
Vector4::operator-(), and then update the corresponding out-of-line
definitions/implementations to match the new const-qualified signatures.
In `@engine/native/core/math/vector2.cppm`:
- Around line 358-366: Guard against zero-length vectors before dividing by
sqrt(len_sq) in both max_length and clamp_length: compute len_sq via
length_sq(a), and if len_sq == 0.0f and the target magnitude b (or min/max
threshold) is > 0, return a canonical unit-direction times b (e.g. Vector2{b,0})
instead of performing the division; otherwise proceed with the existing scaling
logic. Update the implementations of max_length and clamp_length to check len_sq
== 0.0f early and handle that case explicitly to avoid NaNs.
In `@engine/native/core/math/vector3.cppm`:
- Around line 390-398: The functions max_length and clamp_length can divide by
zero for a zero vector; add a guard after computing len_sq in both Vector3
max_length(const Vector3& a, float b) noexcept and clamp_length(...) to handle
len_sq == 0: if len_sq == 0.0f return a (do not attempt to scale) so you avoid
b/√len_sq producing NaN; otherwise keep the existing logic that computes scale =
b/√len_sq (or clamps appropriately) and returns a * scale.
In `@engine/native/core/math/vector4.cppm`:
- Around line 437-444: max_length and clamp_length can divide by zero when 'a'
is a zero vector; fix by checking for zero (or near-zero) length before
dividing: compute len_sq = length_sq(a), if len_sq <= tiny_eps (e.g.
std::numeric_limits<float>::epsilon() or a small constant) then return 'a'
unchanged (or for clamp_length, return 'a' when no meaningful direction exists)
else perform the existing sqrt/divide scaling; apply the same guard in both
Vector4 max_length and Vector4 clamp_length to avoid sqrt(0)/division-by-zero.
- Around line 300-302: Remove the compile-time hard error for ARCH_ARM64 inside
the dot implementation: replace the `#error "ARM64 NEON support not yet
implemented."` branch with the same scalar fallback used for other architectures
so ARM64 builds succeed until NEON is implemented; update the conditional in the
dot function (the ARCH_ARM64/#elif block) to call the existing scalar dot path
(or inline the scalar arithmetic) instead of aborting on ARM64, keeping
NEON-specific stubs commented or noted for future implementation.
---
Nitpick comments:
In `@engine/native/core/math/constants.cppm`:
- Line 27: The constant UINT32_MAX_F is misnamed because it stores the
reciprocal (1.f / std::numeric_limits<uint32_t>::max()); rename it to a clearer
identifier such as INV_UINT32_MAX_F (or UINT32_MAX_RECIP) and update all
references to UINT32_MAX_F to the new name (e.g., any uses in functions or files
referencing UINT32_MAX_F). Keep it constexpr float = 1.f /
std::numeric_limits<uint32_t>::max(); and optionally add a short comment like
"reciprocal of UINT32_MAX" to prevent future confusion; if backward
compatibility is needed, add a deprecated alias constexpr float UINT32_MAX_F =
INV_UINT32_MAX_F and mark it with a comment.
- Line 15: The float constants use double literals that get narrowed — update
the numeric literals to float by changing occurrences like in the SQRT12
definition (and the other constant on line 21) from 1. and 2. to 1.f and 2.f
respectively so the expressions (e.g., SQRT12 = 1.f / SQRT2) use float literals
consistently with the float-typed constants.
In `@engine/native/core/math/functions.cppm`:
- Around line 104-112: deg_to_rad and rad_to_deg currently use the float PI
constant causing precision loss for double specializations; replace T{PI} with
std::numbers::pi_v<T> so the pi value is generated at the correct floating-point
type for both template functions (deg_to_rad and rad_to_deg) and ensure you
include <numbers> where these templates are defined.
In `@engine/native/core/math/math.test.cpp`:
- Line 36: The test uses double literals for draco::math::pow which conflicts
with the float-based math migration; update the call in math.test.cpp so the
literals are float types (e.g., change 2. and .5 to 2.0f and 0.5f) so the
computed value assigned to result (float result) uses float operands
consistently.
🪄 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: fcbf5e3f-813b-42d9-96f2-a52a2f75f8dc
📒 Files selected for processing (10)
CMakeLists.txtengine/native/core/math/constants.cppmengine/native/core/math/functions.cppmengine/native/core/math/math.cppmengine/native/core/math/math.test.cppengine/native/core/math/types.cppmengine/native/core/math/types_common.cppmengine/native/core/math/vector2.cppmengine/native/core/math/vector3.cppmengine/native/core/math/vector4.cppm
OldDev78
left a comment
OldDev78
left a comment
There was a problem hiding this comment.
A few things to iron out, it's almost ready.
| } | ||
|
|
||
| constexpr float ceil(float value) noexcept { | ||
| if consteval { |
There was a problem hiding this comment.
I'm not sure we need a compile time vs runtime split for these cases. We need to look at reducing dependency on STL in any case.
There was a problem hiding this comment.
Are you basically saying to just remove the runtime branch?
There was a problem hiding this comment.
If the consteval branch is trivial enough to work just as well in runtime, it can be simplified to just that one branch. We don't have to always delegate to std.
|
|
||
| // constructors | ||
| [[nodiscard]] constexpr Vector2() noexcept = default; | ||
| [[nodiscard]] constexpr Vector2(const float n) noexcept; |
There was a problem hiding this comment.
const is redundant when declaring value arguments. In the implementation, it says that the value cannot be modified, but for the caller this is redundant.
More importantly, it is especially relevant for the vector types to declare the single-argument constructors explicit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/native/core/math/functions.cppm (1)
92-100: 💤 Low valueConsider renaming parameter
yto something descriptive.The parameter name
ydoesn't convey meaning. Names likedegrees/radiansoranglewould improve readability.Suggested rename
template <std::floating_point T> - constexpr T deg_to_rad(T y) noexcept { - return y * (T{PI} / T{180.}); + constexpr T deg_to_rad(T degrees) noexcept { + return degrees * (T{PI} / T{180.}); } template <std::floating_point T> - constexpr T rad_to_deg(T y) noexcept { - return y * (T{180.} / T{PI}); + constexpr T rad_to_deg(T radians) noexcept { + return radians * (T{180.} / T{PI}); }🤖 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/native/core/math/functions.cppm` around lines 92 - 100, Parameter name `y` in functions deg_to_rad and rad_to_deg is non-descriptive; rename it to something meaningful such as `degrees` in deg_to_rad and `radians` in rad_to_deg (or use a common name like `angle`) to improve readability. Update the template signatures and all uses inside template <std::floating_point T> constexpr T deg_to_rad(T y) noexcept and constexpr T rad_to_deg(T y) noexcept to use the new parameter names (e.g., T degrees / T radians) while preserving templates, constexpr, and noexcept specifiers and ensuring no other code depends on the old identifier.
🤖 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.
Nitpick comments:
In `@engine/native/core/math/functions.cppm`:
- Around line 92-100: Parameter name `y` in functions deg_to_rad and rad_to_deg
is non-descriptive; rename it to something meaningful such as `degrees` in
deg_to_rad and `radians` in rad_to_deg (or use a common name like `angle`) to
improve readability. Update the template signatures and all uses inside template
<std::floating_point T> constexpr T deg_to_rad(T y) noexcept and constexpr T
rad_to_deg(T y) noexcept to use the new parameter names (e.g., T degrees / T
radians) while preserving templates, constexpr, and noexcept specifiers and
ensuring no other code depends on the old identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f520867a-f19a-4bfe-81c4-e18a9c062072
📒 Files selected for processing (2)
engine/native/core/math/functions.cppmengine/native/core/math/types_common.cppm
🚧 Files skipped from review as they are similar to previous changes (1)
- engine/native/core/math/types_common.cppm
JoltedJon
left a comment
JoltedJon
left a comment
There was a problem hiding this comment.
Will wait on replies to my comments before approving otherwise looks good
| constexpr Vector2& operator=(float other) noexcept; | ||
| }; | ||
|
|
||
| struct alignas(16) Vector3 { |
There was a problem hiding this comment.
@AR-DEV-1 Can you comment on this alignment if it is needed (same with the other vector types)? The default alignment would be at 4 byte boundaries for these structs. I want the perspective of rendering side
There was a problem hiding this comment.
@AR-DEV-1 Can you comment on this alignment if it is needed (same with the other vector types)? The default alignment would be at 4 byte boundaries for these structs. I want the perspective of rendering side
The 16 byte alignment is definitely intentional for the rendering side! Even though a Vector3 is only 12 bytes by default, forcing it to 16 bytes does two big things for us:
-
SIMD Math: It lets the CPU load the vector into SSE/AVX registers in a single, fast instruction (
MOVAPS) instead of slower unaligned loads. -
GPU Layouts: It perfectly matches the 16-byte alignment rules (like std140) that graphics APIs use for uniform/constant buffers, so we don't get weird memory offsets when uploading data.
The only downside is that it adds 4 bytes of padding per vector, but for core math and rendering pipelines, the SIMD speedup and GPU compatibility are 100% worth the trade off
| } | ||
|
|
||
| // Faster normalize, it presupposes vector has non-zero length | ||
| [[nodiscard]] Vector2 normalize_fast(const Vector2& v) noexcept { |
There was a problem hiding this comment.
Should we have a debug build version that has a check to make sure v is non-zero? If so might be worth it to add a note for this in the future when we add that feature
There was a problem hiding this comment.
Should we have a debug build version that has a check to make sure v is non-zero? If so might be worth it to add a note for this in the future when we add that feature
We should but for now, adding a note is fine since I don't want to have this PR remain in the limbo
There was a problem hiding this comment.
You're stealing my terminology lol
There was a problem hiding this comment.
You're stealing my terminology lol
😆
4 participants
Summary by CodeRabbit
New Features
Refactor
Tests