Add more vector types, functions, and tests#18
Open
Apeirobyte wants to merge 8 commits into
Open
Conversation
Contributor
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR modernizes the 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}
Contributor
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
requested changes
May 17, 2026
Contributor
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 { |
Contributor
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.
Author
There was a problem hiding this comment.
Are you basically saying to just remove the runtime branch?
| // element access | ||
| [[nodiscard]] constexpr float& Vector2::operator[](const int i) noexcept { | ||
| if consteval { | ||
| return i ? y : x; |
Contributor
There was a problem hiding this comment.
Use select in the compile time case?
Author
There was a problem hiding this comment.
When I do, i get this error in the non-const case:
non-const lvalue reference to type 'float' cannot bind to a temporary of type 'float'
and this warning in the const case:
returning reference to local temporary object [-Wreturn-stack-address]
Author
There was a problem hiding this comment.
Also for the other places where I didn't use select, it's the same issue
| } | ||
|
|
||
| // Returns magnitude | ||
| [[nodiscard]] float length(const Vector2& v) noexcept { |
Contributor
There was a problem hiding this comment.
We'll make these constexpr as well soon, but that's coming next. :)
|
|
||
| // constructors | ||
| [[nodiscard]] constexpr Vector2() noexcept = default; | ||
| [[nodiscard]] constexpr Vector2(const float n) noexcept; |
Contributor
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.
| }; | ||
| } | ||
|
|
||
| template<typename T> consteval T select(const int i, const T v1, const T v2) { |
Contributor
There was a problem hiding this comment.
These compile time utilities are very useful for the vector types compile time branch index operators, and are underutilized in there.
Comment on lines
+452
to
+461
| export namespace std { | ||
| template<> struct formatter<draco::math::Vector2> : formatter<float> { | ||
| auto format(const draco::math::Vector2& v, format_context& ctx) const { | ||
| ctx.advance_to(format_to(ctx.out(), "{{")); | ||
| ctx.advance_to(formatter<float>::format(v.x, ctx)); | ||
| ctx.advance_to(format_to(ctx.out(), ", ")); | ||
| ctx.advance_to(formatter<float>::format(v.y, ctx)); | ||
| return format_to(ctx.out(), "}}"); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
We might need to reconsider the use of std::format at one point. Just to keep in mind. But for now, it's okay.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Vector2,Vector3, andVector4types with comprehensive operations including dot products, normalization, projection, reflection, interpolation, and component-wise functionsTests