Conversation
WalkthroughThis update refactors internal data member names and access patterns throughout the vector, matrix, quaternion, row, and unit types for clarity and consistency. It introduces new accessor methods, updates documentation, adds a direction namespace, and increments the project version. Several constructors and function signatures are improved for const-correctness and type precision. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Camera
participant Vec
participant Norm
participant Rotation
User->>Camera: Construct with (position, direction, up)
Camera->>Norm: Accepts direction/up as NormT (normalized)
Camera->>Rotation: Initializes rotation with NormT.as_row()
Note right of Camera: New constructor allows (position, target, up)
User->>Vec: Create vector, access row via as_underlying()
User->>Norm: Create normalized vector, access via as_vec()/as_row()
User->>Rotation: Construct with normalized vectors
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code Graph Analysis (1)include/mr-math/vec.hpp (3)
🪛 LanguageToolREADME.md[grammar] ~42-~42: Use correct spacing (QB_NEW_EN_OTHER_ERROR_IDS_5) [grammar] ~65-~65: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) [grammar] ~66-~66: Use hyphens correctly (QB_NEW_EN_OTHER_ERROR_IDS_29) [grammar] ~66-~66: There might be a problem here. (QB_NEW_EN_MERGED_MATCH) 🪛 markdownlint-cli2 (0.17.2)README.md66-66: Unordered list indentation (MD007, ul-indent) 66-66: Lists should be surrounded by blank lines (MD032, blanks-around-lists) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (12)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
include/mr-math/rot.hpp (2)
77-87: Consider updating getters for consistency.The getters still use direct
_dataaccess, which is inconsistent with the accessor pattern used elsewhere in the refactoring. Consider updating them to use proper accessor methods.For consistency with the broader refactoring effort, consider updating the getters:
constexpr NormT direction() const noexcept { - return {unchecked, VecT{_data[0]}}; + return {unchecked, VecT{_data.as_underlying()[0]}}; }
73-73: Replace directNormdata access inRotation::directionsetterThe
directionsetter currently does:_data[0] = dir._data;But
Normdoes not expose a_datamember—it stores its values in_vecand provides anas_row()accessor. Update the setter to use that public API:--- a/include/mr-math/rot.hpp +++ b/include/mr-math/rot.hpp @@ -72,1 +72,1 @@ - _data[0] = dir._data; + _data[0] = dir.as_row();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
CMakeLists.txt(1 hunks)README.md(1 hunks)USAGE.md(1 hunks)conanfile.py(1 hunks)include/mr-math/camera.hpp(2 hunks)include/mr-math/def.hpp(1 hunks)include/mr-math/matr.hpp(13 hunks)include/mr-math/norm.hpp(4 hunks)include/mr-math/operators.hpp(1 hunks)include/mr-math/packing.hpp(2 hunks)include/mr-math/quat.hpp(2 hunks)include/mr-math/rot.hpp(1 hunks)include/mr-math/row.hpp(4 hunks)include/mr-math/units.hpp(3 hunks)include/mr-math/vec.hpp(7 hunks)tests/vec.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
include/mr-math/vec.hpp (3)
include/mr-math/norm.hpp (30)
N(59-59)N(60-60)N(61-61)N(62-62)N(109-118)N(109-109)i(63-63)nodiscard(59-59)nodiscard(60-60)nodiscard(61-61)nodiscard(62-62)nodiscard(63-63)nodiscard(90-92)nodiscard(94-96)other(69-83)other(69-69)other(85-87)other(85-85)other(90-90)other(94-94)other(100-106)other(100-100)other(133-135)other(133-133)other(136-138)other(136-136)other(140-142)other(140-140)other(143-145)other(143-143)include/mr-math/row.hpp (4)
i(57-57)i(61-61)simd(54-54)simd(55-55)include/mr-math/matr.hpp (4)
i(169-169)i(173-173)res(362-365)res(377-381)
include/mr-math/norm.hpp (4)
include/mr-math/vec.hpp (35)
N(91-91)N(92-92)N(93-93)N(94-94)N(191-200)N(191-191)N(213-223)N(213-213)nodiscard(91-91)nodiscard(92-92)nodiscard(93-93)nodiscard(94-94)nodiscard(95-95)nodiscard(110-112)nodiscard(114-116)nodiscard(119-121)nodiscard(177-179)i(84-84)i(84-84)i(95-95)other(104-107)other(104-104)other(177-177)other(182-188)other(182-182)other(203-210)other(203-203)other(255-257)other(255-255)other(259-261)other(259-259)n(226-228)n(226-226)n(230-233)n(230-230)include/mr-math/def.hpp (6)
equal(42-44)equal(42-42)equal(53-55)equal(53-53)epsilon(37-39)epsilon(37-37)include/mr-math/matr.hpp (6)
nodiscard(169-171)nodiscard(173-175)nodiscard(178-191)i(169-169)i(173-173)other(117-126)include/mr-math/row.hpp (2)
i(57-57)i(61-61)
🪛 LanguageTool
README.md
[grammar] ~42-~42: Use correct spacing
Context: ...ndex Add `"mr-math/1.1.1"` to your conanfile. ## Building Build with Conan:bash cona...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (macos-latest, Debug, clang)
- GitHub Check: build (macos-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (ubuntu-24.04, Release, gcc)
- GitHub Check: build (ubuntu-24.04, Debug, gcc)
🔇 Additional comments (25)
conanfile.py (1)
14-14: Version update looks good.The version bump from "1.1.0" to "1.1.1" is consistent with the changes in other files.
include/mr-math/def.hpp (1)
11-11: Good addition of standard headers.The inclusion of
<cstdint>and<array>provides necessary support for fixed-width integer types and array containers used throughout the refactored codebase.Also applies to: 15-15
include/mr-math/packing.hpp (2)
4-4: Good addition of def.hpp include.Including "def.hpp" provides access to the standard headers and type definitions needed for consistent packing operations.
95-95: Explicit cast improves type safety.The explicit cast to
PackedNorm16ensures the bitwise combined result is properly typed, which is good practice for type safety.CMakeLists.txt (1)
10-10: Version update is consistent.The project version bump to 1.1.1 aligns with the coordinated version updates across the codebase.
README.md (1)
42-42: Documentation update reflects the new version correctly.The installation instructions are properly updated to reference version 1.1.1, maintaining consistency with the project version bump.
tests/vec.cpp (1)
32-32: Test addition looks good.The new test correctly validates that the
rowmember contains the expected data, ensuring the refactoring from_datatorowworks as intended.USAGE.md (1)
272-274: Documentation improvement looks good.The addition clearly informs users about the new direction vectors in the
mr::directionnamespace, complementing the existing axis directions documentation.include/mr-math/row.hpp (2)
20-20: Improved semantic naming of internal storage.Renaming
_datatosimdbetter reflects the underlying SIMD nature of the storage, improving code clarity.
54-55: Good encapsulation with accessor methods.The new
as_underlying()methods provide proper encapsulated access to the internal SIMD data, supporting both const and non-const access patterns.include/mr-math/operators.hpp (1)
12-159: Systematic refactoring to use accessor methods.All operators have been consistently updated to use
as_underlying()instead of direct_dataaccess, improving encapsulation while maintaining identical operator semantics.include/mr-math/rot.hpp (1)
32-40: Constructor refactoring looks good.The updated constructor properly accepts
NormTparameters and uses the newas_row()accessor method, maintaining consistency with the broader refactoring effort.include/mr-math/units.hpp (1)
69-70: LGTM! Consistent accessor pattern.The
as_underlying()methods follow the same pattern as other types in the codebase and provide proper const-correctness.Also applies to: 102-103
include/mr-math/quat.hpp (1)
78-80: Good precision improvements for quaternion operations.Using
doublefor length calculations and spherical interpolation improves numerical accuracy, which is particularly important for quaternion operations that are sensitive to precision errors.Also applies to: 178-182
include/mr-math/matr.hpp (1)
86-87: Excellent refactoring for improved code clarity.The renaming from generic
_datato more descriptive names (rows,_scale,_offset,_rotation) significantly improves code readability and makes the purpose of each member clearer.Also applies to: 97-103, 117-126, 142-150, 153-158, 161-166, 169-175, 179-180, 196-196, 301-301, 315-315, 319-319, 322-322, 326-326, 332-332, 338-338, 345-345, 349-349, 354-354, 358-358, 371-371, 375-375, 379-379, 384-384, 388-388, 393-393, 397-397, 403-403, 407-407, 412-412, 416-416, 422-422, 426-426, 434-434, 438-438, 441-441, 456-456, 460-460, 465-465, 469-469, 475-475, 484-484
include/mr-math/vec.hpp (3)
55-55: Clean refactoring with consistent accessor pattern.The renaming from
_datatorowand the addition ofas_underlying()methods improve consistency across the codebase.Also applies to: 60-61, 65-65, 72-72, 78-79, 81-81, 84-84, 91-95, 97-98, 101-101, 111-112, 177-179, 185-186, 196-197, 206-207, 217-220, 236-237, 246-247, 255-257, 259-261
114-114: Good precision improvements consistent with quaternion changes.Changing
length()to returndoubleimproves precision for length calculations, which is especially important for normalization operations.Also applies to: 119-119
105-106: Ensure SIMD operation consistency in cross product implementationsThe
Vec::crossininclude/mr-math/vec.hpp(lines 105–106) usesrow.simd.rotated(…)whereas
VecT::crossininclude/mr-math/norm.hpp(lines 69–71) usessimd.shifted(…)These two lane-wise operations may not be equivalent. Please verify:
- That
rotatedandshiftedperform the same circular shuffle for computing each component of the 3D cross product.- That
Norm::crossproduces identical results toVec::crosson representative vectors (e.g. unit basis vectors).If there’s a discrepancy, unify both implementations to use the same SIMD operation and add tests covering
Norm::cross.include/mr-math/norm.hpp (1)
40-40: Good refactoring with improved encapsulation.The renaming to
_vecand addition of explicit accessor methods (as_vec(),as_row()) improve clarity. Note that the conversion operator now returns a const reference for efficiency, but users should be aware of potential lifetime issues.Also applies to: 45-51, 53-56, 59-63, 65-66, 91-91, 95-96, 130-131, 134-135, 137-138, 141-142, 144-145, 151-151
include/mr-math/camera.hpp (6)
55-55: LGTM: Improved const-correctness in constructor parameter.The constructor now properly takes a const reference parameter, which is more efficient and follows best practices.
57-59: LGTM: Well-designed constructor for direction-based initialization.The constructor properly takes normalized vectors (
NormT) for direction and up parameters and initializes the rotation matrix directly using the cross product. This is efficient and mathematically sound.
61-62: LGTM: Elegant target-based constructor with proper error handling.The constructor correctly computes the normalized direction vector from position and target, with proper fallback handling using
value_or()for degenerate cases where the direction cannot be normalized.
66-70: LGTM: Improved copy constructor with member initializer lists.The copy constructor now uses member initializer lists which is more efficient than assignment in the constructor body. Correctly resets the cache flag.
85-89: LGTM: Improved move constructor with member initializer lists.The move constructor properly uses member initializer lists and
std::move()for efficient resource transfer. Cache invalidation is handled correctly.
92-93: LGTM: Correct self-assignment check in move assignment operator.The self-assignment check correctly compares pointer addresses to avoid self-move operations.
- use mr::Norm instead of mr::Vec for direction & up vectors
- improve copy & move semantics
- fix silent error when direction vector is colinear to up vector (by
using default value {1, 0, 0})
- add as_row() cast methods to mr::Norm & mr::Vec
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
include/mr-math/units.hpp (1)
158-166: Useful direction constants with proper const declaration.The new
directionnamespace provides commonly used direction vectors, and they are properly declared asconstto prevent accidental modification. This addresses the concern from the previous review comment.include/mr-math/norm.hpp (1)
103-103: Past issue fixed: Correct matrix member access.The code now correctly accesses
other.rows[i]instead of the incorrectother._vec[i]mentioned in the past review comment.include/mr-math/camera.hpp (1)
73-74: Past issue fixed: Correct self-assignment check.The self-assignment check has been corrected from
*this == &othertothis == &other, properly comparing pointer addresses as noted in the past review comment.Also applies to: 92-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
CMakeLists.txt(1 hunks)README.md(1 hunks)USAGE.md(1 hunks)conanfile.py(1 hunks)include/mr-math/camera.hpp(2 hunks)include/mr-math/matr.hpp(13 hunks)include/mr-math/norm.hpp(4 hunks)include/mr-math/operators.hpp(1 hunks)include/mr-math/packing.hpp(2 hunks)include/mr-math/quat.hpp(2 hunks)include/mr-math/rot.hpp(1 hunks)include/mr-math/row.hpp(4 hunks)include/mr-math/units.hpp(3 hunks)include/mr-math/vec.hpp(7 hunks)tests/vec.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
include/mr-math/row.hpp (3)
include/mr-math/operators.hpp (4)
rhs(81-84)rhs(82-82)rhs(156-159)rhs(157-157)include/mr-math/vec.hpp (11)
i(84-84)i(84-84)i(95-95)other(104-107)other(104-104)other(177-177)other(182-188)other(182-182)other(203-210)other(203-203)other(255-257)include/mr-math/def.hpp (10)
epsilon(37-39)epsilon(37-37)equal(42-44)equal(42-42)equal(53-55)equal(53-53)value(95-95)value(95-95)value(118-118)value(118-118)
include/mr-math/norm.hpp (4)
include/mr-math/vec.hpp (35)
N(91-91)N(92-92)N(93-93)N(94-94)N(191-200)N(191-191)N(213-223)N(213-213)nodiscard(91-91)nodiscard(92-92)nodiscard(93-93)nodiscard(94-94)nodiscard(95-95)nodiscard(110-112)nodiscard(114-116)nodiscard(119-121)nodiscard(177-179)i(84-84)i(84-84)i(95-95)other(104-107)other(104-104)other(177-177)other(182-188)other(182-182)other(203-210)other(203-203)other(255-257)other(255-255)other(259-261)other(259-259)n(226-228)n(226-226)n(230-233)n(230-230)include/mr-math/def.hpp (6)
equal(42-44)equal(42-42)equal(53-55)equal(53-53)epsilon(37-39)epsilon(37-37)include/mr-math/matr.hpp (6)
nodiscard(169-171)nodiscard(173-175)nodiscard(178-191)i(169-169)i(173-173)other(117-126)include/mr-math/row.hpp (2)
i(57-57)i(61-61)
include/mr-math/camera.hpp (2)
include/mr-math/norm.hpp (12)
other(69-83)other(69-69)other(85-87)other(85-85)other(90-90)other(94-94)other(100-106)other(100-100)other(133-135)other(133-133)other(136-138)other(136-136)include/mr-math/rot.hpp (4)
other(44-44)other(44-44)other(48-48)other(48-48)
include/mr-math/matr.hpp (3)
include/mr-math/vec.hpp (18)
N(91-91)N(92-92)N(93-93)N(94-94)N(191-200)N(191-191)N(213-223)N(213-213)i(84-84)i(84-84)i(95-95)nodiscard(91-91)nodiscard(92-92)nodiscard(93-93)nodiscard(94-94)nodiscard(95-95)nodiscard(110-112)nodiscard(114-116)include/mr-math/row.hpp (2)
i(57-57)i(61-61)include/mr-math/quat.hpp (14)
lhs(103-103)lhs(108-108)lhs(112-116)lhs(113-113)lhs(118-122)lhs(119-119)lhs(124-124)lhs(130-133)lhs(130-130)lhs(135-135)lhs(141-144)lhs(141-141)lhs(151-154)lhs(151-151)
🪛 LanguageTool
README.md
[grammar] ~42-~42: Use correct spacing
Context: ...ndex Add `"mr-math/1.1.1"` to your conanfile. ## Building Build with Conan:bash cona...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (macos-latest, Debug, clang)
- GitHub Check: build (macos-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (ubuntu-24.04, Debug, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (ubuntu-24.04, Release, gcc)
- GitHub Check: build (ubuntu-24.04, Release, clang)
- GitHub Check: build
🔇 Additional comments (28)
include/mr-math/packing.hpp (2)
4-4: Good addition of dependency inclusion.Adding the
def.hppinclude ensures proper access to necessary type definitions and standard headers used throughout the file.
95-95: Excellent type safety improvement.The explicit cast to
PackedNorm16ensures the bitwise operation result is correctly typed, preventing potential compiler warnings and making the intent clear.README.md (1)
42-42: Version update looks good, but fix the spacing issue.The version bump to 1.1.1 is consistent with other project files. However, there's a spacing issue around this line as flagged by static analysis.
Apply this diff to fix the spacing:
-Add `"mr-math/1.1.1"` to your conanfile. +Add `"mr-math/1.1.1"` to your conanfile.Likely an incorrect or invalid review comment.
CMakeLists.txt (1)
10-10: Version update is consistent and appropriate.The version bump to 1.1.1 aligns with the coordinated version updates across the project files.
conanfile.py (1)
14-14: Version update maintains consistency across build files.The version bump to 1.1.1 properly aligns with the CMake project version and documentation.
tests/vec.cpp (1)
32-32: Good test addition for the refactored data member.The test correctly verifies that the
rowmember provides access to the vector's internal data representation, which aligns with the refactoring from_datatorowmentioned in the broader changes.USAGE.md (1)
272-273: Documentation update looks good.The addition of the
mr::directionnamespace documentation is well-placed and appropriately brief, following the existing pattern for documenting axis directions.include/mr-math/row.hpp (3)
20-20: Good refactoring: descriptive member name.Renaming
_datatosimdimproves code readability by making the purpose of the member variable more explicit.
54-55: Excellent addition of accessor methods.The
as_underlying()methods provide proper encapsulation for accessing the underlying SIMD data, following the same pattern used in the operator overloads.
25-87: Consistent updates throughout the class.All references to the internal data member have been systematically updated to use the new
simdname, maintaining consistency across constructors, accessors, and operations.include/mr-math/operators.hpp (2)
12-159: Excellent encapsulation improvement.The systematic replacement of direct
_dataaccess withas_underlying()calls throughout all operator overloads improves encapsulation and maintains consistency with the new accessor pattern introduced across the math library.
166-205: Consistent application in RowOperators.The
RowOperatorstemplate correctly follows the same pattern asUnitOperators, usingas_underlying()for all element-wise operations while maintaining the existing operator semantics.include/mr-math/rot.hpp (1)
32-40: Improved type safety with normalized vectors.The constructor signature change from
VecTtoNormTparameters ensures that only normalized vectors are used for rotation construction, which is mathematically correct. The use ofas_row()instead of direct_dataaccess maintains consistency with the encapsulation pattern.include/mr-math/units.hpp (2)
69-70: Consistent accessor pattern for Radians.The addition of
as_underlying()methods to theRadiansstruct maintains consistency with the accessor pattern established across the math library.
102-103: Consistent accessor pattern for Degrees.The addition of
as_underlying()methods to theDegreesstruct maintains consistency with the accessor pattern established across the math library.include/mr-math/quat.hpp (2)
78-80: Good improvement to numerical precision.Changing the return types of
length()andinversed_length()todoubleenhances precision for length-related computations, which is particularly beneficial for normalization operations and numerical stability.
178-182: Consistent precision improvement in slerp interpolation.The changes to use
doublefor the interpolation parameter and intermediate calculations improve precision, while the explicit casts back toTmaintain type consistency. This is especially important for smooth quaternion interpolation.include/mr-math/matr.hpp (4)
86-90: Excellent refactoring of member names.The renaming from
_datatorowssignificantly improves code readability by making it immediately clear that this member stores the matrix rows. All references have been consistently updated throughout theMatrclass.Also applies to: 97-104, 121-122, 130-131, 137-138, 146-147, 155-156, 163-164, 170-171, 174-175, 179-180, 185-186, 196-197, 206-207, 217-218, 262-263, 271-272, 301-301
315-366: Good naming improvement for ScaleMatr.The renaming from
_datato_scaleclearly indicates the purpose of this member.
371-429: Clear naming for TranslateMatr members.The renaming to
_offsetmakes the purpose of this member immediately apparent. The matrix multiplication withrhs._offset.rowcorrectly uses the row accessor.
434-491: Consistent naming for RotateMatr.The
_rotationmember name clearly indicates it stores a quaternion rotation.include/mr-math/norm.hpp (2)
40-56: Well-structured refactoring with improved encapsulation.The changes effectively rename
_datato_vecand introduce proper accessor methodsas_vec()andas_row(). This improves the API design and encapsulation.
70-72: Good SIMD optimization for cross product.The implementation now uses SIMD shifted operations for better performance. The
Normcross product correctly returns a normalized result using the unchecked constructor.Also applies to: 85-87
include/mr-math/vec.hpp (3)
55-55: Consistent and clear refactoring from _data to row.The renaming to
rowbetter describes the underlying SIMD row storage. The addition ofas_underlying()accessor methods provides controlled access to the internal representation while maintaining encapsulation.Also applies to: 60-60, 65-65, 72-73, 78-79, 81-81, 84-84, 91-95, 97-98, 101-101
114-114: Appropriate precision improvements for length calculations.Changing
length()to returndoubleimproves precision for normalization and distance calculations. Usingautoforinversed_length()allows the compiler to optimize the return type.Also applies to: 119-119
105-106: Thorough update of all vector operations.All operations have been consistently updated to use
rowandrow.simdwhere appropriate, maintaining the benefits of SIMD operations while improving code clarity.Also applies to: 111-111, 178-178, 185-186, 196-197, 206-207, 217-218, 236-237, 246-247, 256-256, 260-260
include/mr-math/camera.hpp (2)
55-63: Improved constructor design with better const-correctness.The constructors now properly accept parameters by const reference, avoiding unnecessary copies. The new constructor accepting a normalized direction vector (
NormT) aligns well with the overall refactoring and provides a cleaner API.
66-70: Efficient implementation using member initializer lists.The copy and move constructors now use member initializer lists, which is more efficient and follows best practices. The explicit reset of
_perspective_calculatedtofalseensures proper cache invalidation.Also applies to: 85-89
| constexpr Norm(UncheckedTag, const VecT &v) noexcept : _data(v) { | ||
| assert(mr::equal(v.length(), 1, 0.1f)); | ||
| constexpr Norm(UncheckedTag, const VecT &v) noexcept : _vec(v) { | ||
| assert(mr::equal(v.length(), 1.0, 0.1)); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a smaller epsilon for unit vector assertion.
The epsilon value of 0.1 for checking unit vector length seems quite large. For normalized vectors, a tighter tolerance would be more appropriate.
-assert(mr::equal(v.length(), 1.0, 0.1));
+assert(mr::equal(v.length(), 1.0, 1e-6));📝 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.
| assert(mr::equal(v.length(), 1.0, 0.1)); | |
| assert(mr::equal(v.length(), 1.0, 1e-6)); |
🤖 Prompt for AI Agents
In include/mr-math/norm.hpp at line 50, the assertion checking if the vector
length is 1.0 uses an epsilon of 0.1, which is too large for unit vectors.
Reduce the epsilon value to a smaller number, such as 0.01 or less, to enforce a
tighter tolerance for normalized vectors and improve accuracy.
Also, add as_underlying() methods that comes handy in generic code, such as mr::UnitOperators CRTP class
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
include/mr-math/units.hpp (1)
158-166: Well-designed direction namespace with proper const declarations.The new
directionnamespace provides convenient access to common directional vectors. The vectors are properly declared asconstto prevent accidental modification, which addresses potential safety concerns.include/mr-math/norm.hpp (1)
50-50: Use a smaller epsilon for unit vector assertion.The epsilon value of 0.1 for checking unit vector length is too large. For normalized vectors, a tighter tolerance ensures better precision.
-assert(mr::equal(v.length(), 1.0, 0.1)); +assert(mr::equal(v.length(), 1.0, 1e-6));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
CMakeLists.txt(1 hunks)README.md(1 hunks)USAGE.md(1 hunks)conanfile.py(1 hunks)include/mr-math/matr.hpp(13 hunks)include/mr-math/norm.hpp(4 hunks)include/mr-math/operators.hpp(1 hunks)include/mr-math/packing.hpp(2 hunks)include/mr-math/quat.hpp(2 hunks)include/mr-math/row.hpp(4 hunks)include/mr-math/units.hpp(3 hunks)include/mr-math/vec.hpp(7 hunks)tests/vec.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
include/mr-math/row.hpp (3)
include/mr-math/operators.hpp (4)
rhs(81-84)rhs(82-82)rhs(156-159)rhs(157-157)include/mr-math/vec.hpp (11)
i(84-84)i(84-84)i(95-95)other(104-107)other(104-104)other(177-177)other(182-188)other(182-182)other(203-210)other(203-203)other(255-257)include/mr-math/def.hpp (10)
epsilon(37-39)epsilon(37-37)equal(42-44)equal(42-42)equal(53-55)equal(53-53)value(95-95)value(95-95)value(118-118)value(118-118)
include/mr-math/norm.hpp (4)
include/mr-math/vec.hpp (35)
N(91-91)N(92-92)N(93-93)N(94-94)N(191-200)N(191-191)N(213-223)N(213-213)nodiscard(91-91)nodiscard(92-92)nodiscard(93-93)nodiscard(94-94)nodiscard(95-95)nodiscard(110-112)nodiscard(114-116)nodiscard(119-121)nodiscard(177-179)i(84-84)i(84-84)i(95-95)other(104-107)other(104-104)other(177-177)other(182-188)other(182-182)other(203-210)other(203-203)other(255-257)other(255-255)other(259-261)other(259-259)n(226-228)n(226-226)n(230-233)n(230-230)include/mr-math/def.hpp (6)
equal(42-44)equal(42-42)equal(53-55)equal(53-53)epsilon(37-39)epsilon(37-37)include/mr-math/matr.hpp (6)
nodiscard(169-171)nodiscard(173-175)nodiscard(178-191)i(169-169)i(173-173)other(117-126)include/mr-math/row.hpp (2)
i(57-57)i(61-61)
🪛 LanguageTool
README.md
[grammar] ~42-~42: Use correct spacing
Context: ...ndex Add `"mr-math/1.1.1"` to your conanfile. ## Building Build with Conan:bash cona...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (ubuntu-24.04, Release, clang)
- GitHub Check: build (ubuntu-24.04, Release, gcc)
- GitHub Check: build (ubuntu-24.04, Debug, gcc)
- GitHub Check: build
🔇 Additional comments (20)
include/mr-math/packing.hpp (2)
4-4: LGTM! Include necessary for type definitions.The inclusion of "def.hpp" provides the required standard headers (
<cstdint>,<array>) needed for the types used throughout this file, such asuint32_t,uint16_t,uint8_t, andstd::array<uint8_t, 3>inPackedNorm24.
95-95: LGTM! Explicit cast improves type safety and consistency.The explicit cast to
PackedNorm16enhances type safety by making the return type conversion explicit and aligns with the pattern used in similar functions likequantize24which usesPackedNorm24(...)constructor. This makes the code more readable and prevents potential implicit conversion issues.CMakeLists.txt (1)
10-10: Project version updated – no further action requiredThe bump to
1.1.1is consistent with the rest of the build script;${PROJECT_VERSION}is already wired everywhere relevant.conanfile.py (1)
14-14: Version bump looks good and stays in sync with CMake/READMENothing else to flag.
USAGE.md (1)
272-274: Newmr::directionhelpers documented – looks greatThe extra examples slot nicely after the axis section.
include/mr-math/operators.hpp (1)
12-205: Excellent systematic refactoring for improved encapsulation.The replacement of direct
_datamember access withas_underlying()method calls throughout all operator overloads is a well-executed refactoring that improves encapsulation and aligns with the accessor pattern being introduced across the math library. The changes are consistent and systematic across bothUnitOperatorsandRowOperatorstemplates.include/mr-math/row.hpp (3)
20-20: Improved naming with more descriptive member variable.The renaming from
_datatosimdmakes the code more self-documenting and clearly indicates the underlying SIMD implementation.
54-55: Well-implemented accessor methods for encapsulation.The addition of both const and non-const
as_underlying()methods provides controlled access to the underlying SIMD data, supporting the operator overloads while maintaining proper encapsulation.
25-87: Consistent refactoring throughout the class.All references to the internal data member have been systematically updated to use the new
simdmember name, maintaining consistency across constructors, operators, and member functions.include/mr-math/units.hpp (2)
69-70: Consistent accessor method implementation.The addition of
as_underlying()methods to theRadiansclass aligns with the encapsulation improvements throughout the math library, providing controlled access to the underlying data.
102-103: Consistent accessor method implementation.The addition of
as_underlying()methods to theDegreesclass maintains consistency with the encapsulation pattern being applied across the math library.include/mr-math/matr.hpp (4)
86-301: Improved naming enhances code readability.The renaming of
_datatorowsin theMatrclass and the consistent updates throughout all matrix operations make the code more self-documenting and easier to understand. The changes maintain all functionality while improving clarity.
315-366: Clear member naming in ScaleMatr class.The
_scalemember name is appropriate and descriptive. All operations correctly use the member variable consistently throughout the class.
371-429: Descriptive member naming in TranslateMatr class.The
_offsetmember name clearly indicates the purpose of the variable. All operations correctly reference this member throughout the class implementation.
434-491: Appropriate member naming in RotateMatr class.The
_rotationmember name is descriptive and self-documenting. All operations correctly use this member variable consistently throughout the class.include/mr-math/quat.hpp (2)
78-80: Improved precision for length calculations.Changing the return types of
length()andinversed_length()fromTtodoubleenhances numerical precision for length-related computations. This is particularly beneficial for applications requiring high precision in quaternion normalization and distance calculations.
178-183: Enhanced precision in spherical interpolation.The changes to use
doublefor the interpolation parametertand intermediate calculations (angle,denom) improve numerical accuracy in quaternion interpolation. The explicit casts back toTfor sine values maintain type consistency while benefiting from higher precision during computation.include/mr-math/norm.hpp (1)
55-56: Good addition of accessor methods.The new
as_vec()andas_row()methods provide clean, const-correct access to the internal data, improving encapsulation and API consistency.include/mr-math/vec.hpp (2)
97-98: Well-designed accessor methods.The
as_underlying()methods with const and non-const overloads provide proper encapsulation while maintaining flexibility for both read and write access when needed.
114-114: Breaking Change:length()Now Returnsdouble
This modifies the API return type fromTtodouble, which—while improving precision—can affect any client code that:
- Uses
decltype(v.length())or SFINAE based on the formerTreturn type- Expects an exact
Ttype (e.g. in template specializations)- Relies on implicit conversions without truncation warnings
In this repo, the only occurrences of
length()are in:
- USAGE.md (assigning to
float l1)- mr-math.natvis (debug visualizer)
- tests/vec.cpp & tests/quat.cpp (assigning to
floator passing toEXPECT_NEAR)- include/mr-math/norm.hpp (calling in
mr::equal)- include/mr-math/debug.hpp (instantiation helpers)
None of these will fail to compile (implicit narrowing from
doubletofloat/Tis allowed outside braced initializers), but downstream users should:
- Update documentation (e.g. use
autoordoubleforlength())- Audit any code that inspects or overloads on the old
Treturn type- Verify third-party or client code where
length()’s type mattersPlease review client-facing code and docs to ensure this change is handled correctly.
| EXPECT_EQ(v1[0], 1.0); | ||
| EXPECT_EQ(v1[1], 2.0); | ||
| EXPECT_EQ(v1[2], 3.0); | ||
| EXPECT_EQ(v1.row, mr::Vec3f::RowT(1, 2, 3)); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Public-member access in tests may leak implementation details
v1.row is now part of the test surface. If row is meant to stay a public data member, no problem; otherwise consider exposing it through a row() accessor so tests (and users) don’t couple to the storage layout.
// if an accessor exists / will be added:
EXPECT_EQ(v1.row(), mr::Vec3f::RowT(1, 2, 3));🤖 Prompt for AI Agents
In tests/vec.cpp at line 32, the test accesses the public data member 'row' of
'v1', which may expose internal implementation details. To fix this, replace
direct access to 'v1.row' with a call to a 'row()' accessor method, such as
'v1.row()', ensuring the test does not depend on the internal storage layout. If
the accessor does not exist, add a public 'row()' method to provide read-only
access to the row data.
introduce macro to disable multi-threded checks for performance
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
include/mr-math/camera.hpp (1)
82-83: Self-assignment check correctly fixed.The previous review comment about incorrect self-assignment check has been properly addressed by comparing pointer addresses (
this == &other) instead of dereferencing.README.md (1)
42-42: Version update looks good, but formatting could be improved.The version has been properly updated to 1.1.1. However, the previous review comment about formatting the package name as code and adding an example snippet would improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)include/mr-math/camera.hpp(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
include/mr-math/camera.hpp (2)
include/mr-math/norm.hpp (12)
other(69-83)other(69-69)other(85-87)other(85-85)other(90-90)other(94-94)other(100-106)other(100-100)other(133-135)other(133-133)other(136-138)other(136-136)include/mr-math/rot.hpp (4)
other(44-44)other(44-44)other(48-48)other(48-48)
🪛 LanguageTool
README.md
[grammar] ~42-~42: Use correct spacing
Context: ...ndex Add `"mr-math/1.1.1"` to your conanfile. ## Building Build with Conan:bash cona...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~65-~65: Use articles correctly
Context: ...list-presets ``` ## Configuration Use following macros to change library behaivour: - ...
(QB_NEW_EN_OTHER_ERROR_IDS_11)
[grammar] ~65-~65: Ensure spelling is correct
Context: ... Use following macros to change library behaivour: - MR_MATH_SINGLE_THREDED (not defin...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~66-~66: Use articles correctly
Context: ...NGLE_THREDED` (not defined by default): library is used in a single threded environment...
(QB_NEW_EN_OTHER_ERROR_IDS_11)
[grammar] ~66-~66: There might be a mistake here.
Context: ...fined by default): library is used in a single threded environment, so race-condition-p...
(QB_NEW_EN_OTHER)
[grammar] ~66-~66: There might be a problem here.
Context: ...o race-condition-preventing code can be omitted. ## Benchmarking #### With Bash & Python: ```bash bash run-ben...
(QB_NEW_EN_MERGED_MATCH)
🪛 markdownlint-cli2 (0.17.2)
README.md
66-66: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
66-66: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (macos-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (ubuntu-24.04, Release, clang)
- GitHub Check: build (macos-latest, Debug, clang)
- GitHub Check: build (ubuntu-24.04, Release, gcc)
🔇 Additional comments (8)
include/mr-math/camera.hpp (8)
12-18: Good conditional compilation pattern for thread safety.The
CacheFlagtype alias properly switches betweenboolandstd::atomic_boolbased on the threading macro, which is a clean way to handle single vs multi-threaded builds.
64-71: Improved constructor const-correctness and added convenience constructor.The constructors now properly take const references, and the new position-target constructor is a useful addition that internally computes the normalized direction.
74-99: Well-implemented copy constructor and move constructor with proper member initialization.The constructors use member initializer lists and properly reset the
_perspective_calculatedflag to maintain cache consistency.
101-102: Consistent self-assignment check in move assignment operator.The move assignment operator correctly uses the same self-assignment check pattern as the copy assignment operator.
201-203: Conditional mutex locking appropriately implemented.The mutex locking is correctly conditionally compiled only for multi-threaded builds, which should improve performance in single-threaded environments.
218-220: Consistent conditional compilation pattern.The orthographic calculation method uses the same conditional mutex locking pattern as the perspective calculation.
239-241: Consistent conditional compilation pattern.The frustum calculation method maintains the same conditional mutex locking pattern for consistency.
265-268: Proper use of conditional cache flag type.The
_perspective_calculatedmember correctly uses the newCacheFlagtype alias, maintaining consistency with the other cache flags.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
include/mr-math/norm.hpp (1)
50-50: Consider using a smaller epsilon for unit vector assertion.The epsilon value of 0.1 for checking unit vector length seems quite large. For normalized vectors, a tighter tolerance would be more appropriate.
Apply this diff to use a more appropriate epsilon:
-assert(mr::equal(v.length(), 1.0, 0.1)); +assert(mr::equal(v.length(), 1.0, 1e-6));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/mr-math/norm.hpp(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
include/mr-math/norm.hpp (4)
include/mr-math/vec.hpp (35)
N(91-91)N(92-92)N(93-93)N(94-94)N(191-200)N(191-191)N(213-223)N(213-213)nodiscard(91-91)nodiscard(92-92)nodiscard(93-93)nodiscard(94-94)nodiscard(95-95)nodiscard(110-112)nodiscard(114-116)nodiscard(119-121)nodiscard(177-179)i(84-84)i(84-84)i(95-95)other(104-107)other(104-104)other(177-177)other(182-188)other(182-182)other(203-210)other(203-203)other(255-257)other(255-255)other(259-261)other(259-259)n(226-228)n(226-226)n(230-233)n(230-230)include/mr-math/def.hpp (6)
equal(42-44)equal(42-42)equal(53-55)equal(53-53)epsilon(37-39)epsilon(37-37)include/mr-math/matr.hpp (6)
nodiscard(169-171)nodiscard(173-175)nodiscard(178-191)i(169-169)i(173-173)other(117-126)include/mr-math/row.hpp (2)
i(57-57)i(61-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build
- GitHub Check: build (ubuntu-24.04, Debug, gcc)
- GitHub Check: build (ubuntu-24.04, Release, clang)
- GitHub Check: build (macos-latest, Debug, clang)
- GitHub Check: build (macos-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (ubuntu-24.04, Release, gcc)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
include/mr-math/norm.hpp (4)
70-71: Good fix for the cross product implementation.The cross product now correctly uses
rotated()instead ofshifted(), which aligns with theVecclass implementation.
85-87: Good addition of Norm-specific cross product overload.This overload properly maintains the normalized property when computing cross products between normalized vectors.
103-103: Matrix multiplication correctly accesses matrix rows.Good fix - the code now properly uses
other.rows[i]to access matrix elements during multiplication.
40-40: Well-executed refactoring with improved encapsulation.The refactoring from
_datato_vecwith the addition of accessor methodsas_vec()andas_row()improves code clarity and maintains consistency with related vector/matrix types.Also applies to: 55-57, 151-151
include/mr-math/quat.hpp
Outdated
| [[nodiscard]] constexpr T length() const noexcept { return std::sqrt(length2()); } | ||
| [[nodiscard]] constexpr T inversed_length() const noexcept { return fast_rsqrt(length2()); } | ||
| [[nodiscard]] constexpr T length2() const noexcept { return w() * w() + vec().length2(); } | ||
| [[nodiscard]] constexpr double length() const noexcept { return std::sqrt(length2()); } |
There was a problem hiding this comment.
Why not any floating point? Maybe some std::floating_point usage is better?
There was a problem hiding this comment.
Now it's DefaultRealT<T> which is either float or double
include/mr-math/quat.hpp
Outdated
| float denom = std::sin(angle); | ||
| [[nodiscard]] constexpr Quat<T> slerp(Quat<T> q1, Quat<T> q2, double t) noexcept { | ||
| double angle = std::acos(q1.w() * q2.w() + q1.vec().dot(q2.vec())); | ||
| double denom = std::sin(angle); |
There was a problem hiding this comment.
There sure has to be performance penalty for using higher precision. This might not be apparent on x86 but some RISC processors will definitely suffer
There was a problem hiding this comment.
DefaultRealT<T> is used here as well
|
|
||
| namespace literals { | ||
| namespace direction | ||
| { |
There was a problem hiding this comment.
Should be on the same line I believe
include/mr-math/vec.hpp
Outdated
|
|
||
| // use 1 / length() for higher precision | ||
| [[nodiscard]] constexpr T inversed_length() const { | ||
| [[nodiscard]] constexpr auto inversed_length() const { |
There was a problem hiding this comment.
Why use supposedly correct return type here but force double everywhere else?
There was a problem hiding this comment.
Now it's also DefaultRealT<T>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
include/mr-math/camera.hpp (1)
75-110: Copy/move semantics correctly implemented.The constructors now use member initializer lists for efficiency, and the self-assignment checks have been fixed as requested in the previous review. The perspective cache is properly invalidated in all cases.
README.md (1)
65-66: Fix spelling and hyphenation issues.-Use the following macros to change library behaivour: - - `MR_MATH_SINGLE_THREADED` (not defined by default): the library is used in a single threaded environment, so race-condition-preventing code can be omitted. +Use the following macros to change library behaviour: + - `MR_MATH_SINGLE_THREADED` (not defined by default): the library is used in a single-threaded environment, so race-condition-preventing code can be omitted.include/mr-math/quat.hpp (1)
78-80: Approve the consistent return type changes for length methods.Using
DefaultRealT<T>for length calculations is appropriate as these methods should return floating-point types regardless of the input typeT. This aligns with mathematical conventions where lengths are real numbers.Note: This change addresses the past review comment about using appropriate floating-point types, though there may still be performance considerations on RISC processors as previously mentioned.
include/mr-math/vec.hpp (1)
84-84: Address the operator[] compilation issue.The comment indicates a known issue where
T & RowT::operator[]doesn't compile, forcing the use ofrow._set_ind(i, value). This suggests theRowclass lacks a non-constoperator[]overload.As mentioned in past reviews, the
Rowclass needs a non-constoperator[]returningT&. Would you like me to generate the implementation for this fix?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)include/mr-math/camera.hpp(7 hunks)include/mr-math/def.hpp(2 hunks)include/mr-math/quat.hpp(2 hunks)include/mr-math/vec.hpp(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
include/mr-math/quat.hpp (3)
include/mr-math/norm.hpp (5)
nodiscard(59-59)nodiscard(60-60)nodiscard(61-61)nodiscard(62-62)nodiscard(63-63)include/mr-math/vec.hpp (11)
nodiscard(91-91)nodiscard(92-92)nodiscard(93-93)nodiscard(94-94)nodiscard(95-95)nodiscard(110-112)nodiscard(114-116)nodiscard(119-121)nodiscard(177-179)w(88-88)w(88-88)include/mr-math/def.hpp (6)
fast_rsqrt(70-74)fast_rsqrt(70-70)fast_rsqrt(79-83)fast_rsqrt(79-79)fast_rsqrt(89-91)fast_rsqrt(89-89)
include/mr-math/vec.hpp (3)
include/mr-math/norm.hpp (23)
N(59-59)N(60-60)N(61-61)N(62-62)N(109-118)N(109-109)i(63-63)other(69-83)other(69-69)other(85-87)other(85-85)other(90-90)other(94-94)other(100-106)other(100-100)other(133-135)other(133-133)other(136-138)other(136-136)other(140-142)other(140-140)other(143-145)other(143-143)include/mr-math/matr.hpp (4)
i(169-169)i(173-173)res(362-365)res(377-381)include/mr-math/row.hpp (4)
i(57-57)i(61-61)simd(54-54)simd(55-55)
🪛 LanguageTool
README.md
[grammar] ~42-~42: Use correct spacing
Context: ...ndex Add `"mr-math/1.1.1"` to your conanfile. ## Building Build with Conan:bash cona...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~65-~65: Ensure spelling is correct
Context: ... the following macros to change library behaivour: - MR_MATH_SINGLE_THREADED (not defi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~66-~66: Use hyphens correctly
Context: ...d by default): the library is used in a single threaded environment, so race-condition-...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~66-~66: There might be a problem here.
Context: ...o race-condition-preventing code can be omitted. ## Benchmarking #### With Bash & Python: ```bash bash run-ben...
(QB_NEW_EN_MERGED_MATCH)
🪛 markdownlint-cli2 (0.17.2)
README.md
66-66: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
66-66: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🪛 GitHub Actions: Tests
include/mr-math/def.hpp
[error] 39-39: Error C3864: 'mr::math::DefaultReal': requires clause is incompatible with the declaration.
include/mr-math/quat.hpp
[error] 79-80: Multiple errors including C2062: type '' unexpected, C2334: unexpected token(s) preceding '{'; skipping apparent function body, and C2938: 'mr::math::DefaultRealT' failed to specialize alias template.
include/mr-math/vec.hpp
[error] 114-119: Multiple errors including C2062: type '' unexpected, C2334: unexpected token(s) preceding '{'; skipping apparent function body, and C2938: 'mr::math::DefaultRealT' failed to specialize alias template.
🪛 GitHub Actions: Benchmarks
include/mr-math/def.hpp
[error] 37-40: Redeclaration of template 'DefaultReal' with different constraints. Original declaration at line 37 uses std::type_identity, redeclared at line 40 uses std::type_identity.
🔇 Additional comments (10)
include/mr-math/camera.hpp (3)
12-18: Good abstraction for thread-safe cache flags.The conditional type alias pattern is well-designed and provides a clean abstraction for switching between single-threaded and multi-threaded modes.
64-72: Well-designed constructor improvements.The constructors now properly take const references for efficiency, and the new position-target constructor provides a convenient API. The defensive programming with
value_orensures valid direction vectors even when the position and target are identical.
201-203: Thread safety correctly handled with conditional compilation.The mutex operations are properly omitted in single-threaded builds, which eliminates unnecessary synchronization overhead while maintaining thread safety in multi-threaded environments.
Also applies to: 218-220, 239-241, 265-267
include/mr-math/def.hpp (1)
11-11: LGTM on the new includes.The addition of
<cstdint>and<array>is appropriate for the new functionality being introduced.Also applies to: 15-15
include/mr-math/quat.hpp (1)
178-183: Approve the slerp function improvements.The changes improve type consistency:
- Parameter
tnow usesDefaultRealT<T>instead of hardcodedfloat- Intermediate calculations use
autofor type deduction- Explicit casts to
Tmaintain compatibility with quaternion arithmeticThis is a well-designed approach that balances type safety with performance.
include/mr-math/vec.hpp (5)
55-55: Approve the descriptive member name change.Renaming
_datatorowimproves code readability and better reflects the underlying row-based storage.
97-98: Approve the new accessor methods.The
as_underlying()methods provide controlled access to the internalRowTobject, following good encapsulation practices with both const and non-const overloads.
114-114: Approve the consistent return type changes.Using
DefaultRealT<T>forlength()andinversed_length()methods aligns with the mathematical convention that lengths should be real numbers and maintains consistency with the quaternion implementation.Also applies to: 119-119
105-106: Approve the comprehensive member access updates.All references to the old
_datamember have been consistently updated to userowandrow.simdwhere appropriate. The changes maintain the same logical operations while using the new member naming convention.Also applies to: 111-111, 178-178, 185-185, 196-199, 206-206, 217-220, 236-236, 246-247, 256-256, 260-260
60-60: Rename the constructor parameter to avoid shadowing therowmemberTo keep parameter names distinct from member names and improve readability, update the overload in include/mr-math/vec.hpp.
• File: include/mr-math/vec.hpp
Line: 60Suggested diff:
- constexpr Vec(const RowT &row) noexcept : row(row.simd) {}; + constexpr Vec(const RowT &r) noexcept : row(r.simd) {};
Methods such as length() return float or double depending on the class template parameter
| friend constexpr DerivedT | ||
| operator+(const DerivedT &lhs, const DerivedT &rhs) noexcept { | ||
| return DerivedT{lhs._data + rhs._data}; | ||
| return DerivedT{lhs.as_underlying() + rhs.as_underlying()}; |
There was a problem hiding this comment.
Are you absolutely sure this additional function call gets inlined and doesn't hurt performance?
There was a problem hiding this comment.
I haven't checked, but there's no way it's not inlined. I can try to check with compiler explorer though
There was a problem hiding this comment.
You may run benchmarks in vtune. You would see "call ...::as_underlying" in there
There was a problem hiding this comment.
on what class and in what function?
There was a problem hiding this comment.
In flame graph after running "hotspots" routine
2 participants
Summary by CodeRabbit
New Features
Documentation
Refactor
Bug Fixes
Tests