Added quick scalar implementations of distance, distance_sq, lerp, min, max, clamp, and abs to vec4.#17
Closed
mcdubhghlas wants to merge 3 commits into
Closed
Added quick scalar implementations of distance, distance_sq, lerp, min, max, clamp, and abs to vec4.#17mcdubhghlas wants to merge 3 commits into
mcdubhghlas wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdded seven exported Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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: 1
🧹 Nitpick comments (1)
engine/native/core/math/vector4.cppm (1)
173-221: Please add tests for the new exported helpers.Since these are new public math APIs, add unit tests for nominal and edge cases (
t=0/1, negative components forabs, and clamp boundaries) before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/math/vector4.cppm` around lines 173 - 221, Add unit tests covering the new public Vector4 helpers: distance, distance_sq, lerp, min, max, clamp, and abs; for each function include nominal cases and edge cases (e.g., distance/distance_sq for identical and different vectors, lerp with t=0 and t=1 and an intermediate t, min/max with mixed-sign components, clamp where components equal x_min and x_max and x outside bounds, and abs with negative/zero/positive components). Place tests in the math/vector4 test suite and use the Vector4 constructor and equality/approx helpers already in test infra to assert expected results and floating-point tolerances. Ensure tests cover constexpr-friendly functions where possible and include negative-component scenarios and boundary values to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/native/core/math/vector4.cppm`:
- Around line 199-206: The return statement in the constexpr Vector4 max(const
Vector4& a, const Vector4& b) noexcept is missing the terminating semicolon
after the braced initializer; edit the Vector4 max function so the return {
std::max(...), ... } expression ends with a semicolon to fix the compilation
error.
---
Nitpick comments:
In `@engine/native/core/math/vector4.cppm`:
- Around line 173-221: Add unit tests covering the new public Vector4 helpers:
distance, distance_sq, lerp, min, max, clamp, and abs; for each function include
nominal cases and edge cases (e.g., distance/distance_sq for identical and
different vectors, lerp with t=0 and t=1 and an intermediate t, min/max with
mixed-sign components, clamp where components equal x_min and x_max and x
outside bounds, and abs with negative/zero/positive components). Place tests in
the math/vector4 test suite and use the Vector4 constructor and equality/approx
helpers already in test infra to assert expected results and floating-point
tolerances. Ensure tests cover constexpr-friendly functions where possible and
include negative-component scenarios and boundary values to prevent regressions.
🪄 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: bd0c3d94-7b79-4572-8a41-b1580e128259
📒 Files selected for processing (1)
engine/native/core/math/vector4.cppm
Comment on lines
+199
to
+206
| [[nodiscard]] constexpr Vector4 max(const Vector4& a, const Vector4& b) noexcept { | ||
| return { | ||
| std::max(a.x, b.x), | ||
| std::max(a.y, b.y), | ||
| std::max(a.z, b.z), | ||
| std::max(a.w, b.w), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix compile error in max return statement.
Line 205 is missing the semicolon after the braced return value, which breaks compilation.
🛠️ Minimal fix
[[nodiscard]] constexpr Vector4 max(const Vector4& a, const Vector4& b) noexcept {
return {
std::max(a.x, b.x),
std::max(a.y, b.y),
std::max(a.z, b.z),
std::max(a.w, b.w),
- }
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [[nodiscard]] constexpr Vector4 max(const Vector4& a, const Vector4& b) noexcept { | |
| return { | |
| std::max(a.x, b.x), | |
| std::max(a.y, b.y), | |
| std::max(a.z, b.z), | |
| std::max(a.w, b.w), | |
| } | |
| } | |
| [[nodiscard]] constexpr Vector4 max(const Vector4& a, const Vector4& b) noexcept { | |
| return { | |
| std::max(a.x, b.x), | |
| std::max(a.y, b.y), | |
| std::max(a.z, b.z), | |
| std::max(a.w, b.w), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/native/core/math/vector4.cppm` around lines 199 - 206, The return
statement in the constexpr Vector4 max(const Vector4& a, const Vector4& b)
noexcept is missing the terminating semicolon after the braced initializer; edit
the Vector4 max function so the return { std::max(...), ... } expression ends
with a semicolon to fix the compilation error.
…n, max, clamp, and abs to vec4.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
engine/native/core/math/vector4.cppm (2)
188-196: Consider clarifying the comment.The comment "Returns smallest vector" could be misread as returning the vector with smallest magnitude. Consider "Returns component-wise minimum" to be precise.
📝 Suggested comment improvement
- // Returns smallest vector between a and b. + // Returns component-wise minimum of a and b. [[nodiscard]] constexpr Vector4 min(const Vector4& a, const Vector4& b) noexcept {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/math/vector4.cppm` around lines 188 - 196, The comment for the function Vector4 min is ambiguous; update it to read something like "Returns component-wise minimum of two vectors" so it's clear the function compares each component separately (x, y, z, w) rather than computing the vector with the smallest magnitude; change the comment above the constexpr Vector4 min(const Vector4& a, const Vector4& b) noexcept accordingly.
178-181: Minor inconsistency: argument order differs fromdistance.
distancecomputeslength(b - a)whiledistance_sqcomputeslength_sq(a - b). While mathematically equivalent (squared magnitude is symmetric), aligning the argument order improves readability.♻️ Suggested fix for consistency
// Return squared distance between two vectors. [[nodiscard]] FORCEINLINE float distance_sq(const Vector4& a, const Vector4& b) noexcept { - return length_sq(a - b); + return length_sq(b - a); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/native/core/math/vector4.cppm` around lines 178 - 181, The distance_sq implementation uses a - b while distance uses b - a; make them consistent by changing Vector4::distance_sq to call length_sq(b - a) so its operand order matches distance (functions: distance and distance_sq, and helper length_sq). Update the expression in distance_sq accordingly to improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@engine/native/core/math/vector4.cppm`:
- Around line 188-196: The comment for the function Vector4 min is ambiguous;
update it to read something like "Returns component-wise minimum of two vectors"
so it's clear the function compares each component separately (x, y, z, w)
rather than computing the vector with the smallest magnitude; change the comment
above the constexpr Vector4 min(const Vector4& a, const Vector4& b) noexcept
accordingly.
- Around line 178-181: The distance_sq implementation uses a - b while distance
uses b - a; make them consistent by changing Vector4::distance_sq to call
length_sq(b - a) so its operand order matches distance (functions: distance and
distance_sq, and helper length_sq). Update the expression in distance_sq
accordingly to improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0dfe1de-954c-4785-a3dc-7938a2b882fb
📒 Files selected for processing (1)
engine/native/core/math/vector4.cppm
Member
Author
|
Closing, waiting for APEIRO's PR first. |
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
1 participant
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.
Title.
We will still need to make tests, but these are just generic implementations of them. I will go back over them later, as needed, to improve performance via simd intrinsics and such. But I'll keep these as defaults/scalar.
Summary by CodeRabbit