Conversation
WalkthroughRefactors the color subsystem: replaces the single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant RGBAColor
participant ColorBase
participant OKLABColor
Note right of RGBAColor #DDEEFF: Construction & access
User->>RGBAColor: construct(from floats/ints/hex/Vec4)
RGBAColor->>ColorBase: use common ops (tuple, math, stream)
User->>RGBAColor: access/set components / format (argb/bgra/abgr)
User->>RGBAColor: request conversion to OKLAB
RGBAColor->>OKLABColor: convert<RGBAColor,OKLABColor>(rgba)
OKLABColor-->>User: OKLABColor instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
mr-math.natvis (1)
107-116: 🧹 Nitpick (assertive)Spurious closing brace in the display string
R={r()}, G={g()}, B={b()}, A={a()}}has one brace too many, so Visual Studio will show an Invalid Expression error for the whole visualiser.-<DisplayString>R={r()}, G={g()}, B={b()}, A={a()}}</DisplayString> +<DisplayString>R={r()}, G={g()}, B={b()}, A={a()}</DisplayString>USAGE.md (1)
220-220: 🧹 Nitpick (assertive)Typo:
epsion→epsilon-float eps = mr::epsion<float>(); +float eps = mr::epsilon<float>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
USAGE.md(2 hunks)include/mr-math/color.hpp(3 hunks)mr-math.natvis(1 hunks)tests/misc.cpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
include/mr-math/color.hpp (3)
include/mr-math/matr.hpp (18)
other(117-126)other(117-117)other(128-133)other(128-128)other(135-140)other(135-135)other(142-150)other(142-142)other(152-158)other(152-152)other(160-166)other(160-160)nodiscard(169-171)nodiscard(173-175)nodiscard(178-191)_data(321-324)_data(383-386)_data(455-458)include/mr-math/operators.hpp (2)
s(133-141)s(134-134)include/mr-math/def.hpp (2)
epsilon(35-37)epsilon(35-35)
tests/misc.cpp (1)
include/mr-math/color.hpp (6)
RGBAColor(76-76)RGBAColor(79-80)RGBAColor(82-83)RGBAColor(86-93)RGBAColor(95-101)RGBAColor(95-95)
🪛 markdownlint-cli2 (0.17.2)
USAGE.md
190-190: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
192-192: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
197-197: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
199-199: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
199-199: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
200-200: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
205-205: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
206-206: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
206-206: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
207-207: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🪛 GitHub Actions: Benchmarks
include/mr-math/color.hpp
[error] 29-54: Syntax errors due to invalid use of 'this' keyword in method parameters and undeclared identifiers 'self', 'other', and 'eps'.
[error] 213-213: No matching function for call to 'mr::math::Vec<float, 3>::Vec(mr::math::RGBAColor&)'. Invalid static_cast from RGBAColor to Vec3f.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (ubuntu-24.04, Debug, clang)
🔇 Additional comments (5)
include/mr-math/color.hpp (1)
213-230:convert<RGBAColor, OKLABColor>still fails without the implicit Vec3f conversion
After replacing the “deducing this” operators you must adjust this line:auto v = static_cast<Vec3f>(rgba) * Matr3f{ … };Because
static_cast<Vec3f>(rgba)now works (see earlier fix) this part will compile, but note:
- You need
<cmath>forcbrtf.Vec3f * Matr3flooks backwards – shouldn’t it beMatr3f * Vec3f? Double-check the intended order.No code suggestion here, only a heads-up to verify the math once the build is green.
🧰 Tools
🪛 GitHub Actions: Benchmarks
[error] 213-213: No matching function for call to 'mr::math::Vec<float, 3>::Vec(mr::math::RGBAColor&)'. Invalid static_cast from RGBAColor to Vec3f.
tests/misc.cpp (4)
34-45: Tests updated to usemr::RGBAColorAll constructor tests have been correctly updated to reference the new
RGBAColortype, including float, integer, and packed‐integer overloads and the user‐defined literal. The assertions align with the implementations ininclude/mr-math/color.hpp.
35-35: Verify default‐constructor behaviorThe test assumes
RGBAColor()yields(0,0,0,0). Please confirm that the default constructor zero‐initializes all channels (including alpha) rather than defaulting alpha to1.0or leaving uninitialized memory.
77-77: Setters test updated toRGBAColorThe setter test now correctly asserts the updated RGBA channel values against the
RGBAColorinstance. Good use ofset(index, value)followingr(…).
94-95: Clarify addition semantics and potential clampingThe test allows channel values to exceed
1.0(e.g., alpha sums to2.0). Please verify whetherRGBAColor::operator+is intended to produce unclamped results or should clamp channels to[0,1]. If clamping is desired, consider adding a saturating‐add function or adjust operator behavior.
| template <typename InputT, typename OutputT> | ||
| OutputT convert(InputT input) { | ||
| static_assert(false, "You have to specify this function for your particular types"); | ||
| } |
There was a problem hiding this comment.
convert is currently unusable – the unconditional static_assert(false) aborts every build
With the present implementation the primary template can never be instantiated, so even a valid call (one that has a full specialization) will trigger the assertion before the compiler has a chance to pick the specialization. Make the assertion dependent on the template parameters:
-template <typename InputT, typename OutputT>
- OutputT convert(InputT input) {
- static_assert(false, "You have to specify this function for your particular types");
- }
+template <typename...>
+inline constexpr bool _always_false_v = false;
+
+template <typename InputT, typename OutputT>
+constexpr OutputT convert(InputT /*unused*/) {
+ static_assert(_always_false_v<InputT, OutputT>,
+ "No `convert` specialization found for the supplied types");
+}This stops the hard error, allows the compiler to look for specializations, and still provides a clear diagnostic when none are found.
📝 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.
| template <typename InputT, typename OutputT> | |
| OutputT convert(InputT input) { | |
| static_assert(false, "You have to specify this function for your particular types"); | |
| } | |
| template <typename...> | |
| inline constexpr bool _always_false_v = false; | |
| template <typename InputT, typename OutputT> | |
| constexpr OutputT convert(InputT /*unused*/) { | |
| static_assert(_always_false_v<InputT, OutputT>, | |
| "No `convert` specialization found for the supplied types"); | |
| } |
| bool operator==(this const ColorDerived &self, const ColorDerived &other) noexcept { | ||
| return self._data == other._data; | ||
| } | ||
|
|
||
| bool equal(this const ColorDerived &self, const ColorDerived &other, ValueT eps = epsilon<ValueT>()) noexcept { | ||
| return self._data.equal(other._data, eps); | ||
| } |
There was a problem hiding this comment.
operator== / equal suffer from the same “deducing this” issue
Rewrite using the conventional form and preserve const-correctness:
-bool operator==(this const ColorDerived &self, const ColorDerived &other) noexcept {
- return self._data == other._data;
-}
-
-bool equal(this const ColorDerived &self, const ColorDerived &other, ValueT eps = epsilon<ValueT>()) noexcept {
- return self._data.equal(other._data, eps);
-}
+constexpr bool operator==(const ColorDerived &other) const noexcept {
+ return static_cast<const ColorDerived &>(*this)._data == other._data;
+}
+
+constexpr bool equal(const ColorDerived &other,
+ ValueT eps = epsilon<ValueT>()) const noexcept {
+ return static_cast<const ColorDerived &>(*this)._data.equal(other._data, eps);
+}This removes the compilation errors and keeps behaviour identical.
📝 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.
| bool operator==(this const ColorDerived &self, const ColorDerived &other) noexcept { | |
| return self._data == other._data; | |
| } | |
| bool equal(this const ColorDerived &self, const ColorDerived &other, ValueT eps = epsilon<ValueT>()) noexcept { | |
| return self._data.equal(other._data, eps); | |
| } | |
| constexpr bool operator==(const ColorDerived &other) const noexcept { | |
| return static_cast<const ColorDerived &>(*this)._data == other._data; | |
| } | |
| constexpr bool equal(const ColorDerived &other, | |
| ValueT eps = epsilon<ValueT>()) const noexcept { | |
| return static_cast<const ColorDerived &>(*this)._data.equal(other._data, eps); | |
| } |
| // structured binding support | ||
| template <size_t I> requires (I < 4) | ||
| ValueT get(this const ColorDerived &self) { return self._data[I]; } | ||
|
|
||
| operator Vec3f(this ColorDerived &self) noexcept { | ||
| return (Vec3f)self._data; | ||
| } | ||
|
|
||
| operator Vec4f(this ColorDerived &self) noexcept { | ||
| return self._data; | ||
| } | ||
|
|
There was a problem hiding this comment.
deducing this syntax isn’t yet supported by our tool-chain – replace it with ordinary methods
All the functions that use
operator Vec3f(this ColorDerived &self) / ValueT get(this const ColorDerived &self) etc. are rejected by both GCC 13 and Clang 17 (see CI errors 29-54). A drop-in, C++20-compatible rewrite is straightforward:
-// structured binding support
-template <size_t I> requires (I < 4)
- ValueT get(this const ColorDerived &self) { return self._data[I]; }
-
-operator Vec3f(this ColorDerived &self) noexcept {
- return (Vec3f)self._data;
-}
-
-operator Vec4f(this ColorDerived &self) noexcept {
- return self._data;
-}
+// structured binding support
+template <size_t I> requires (I < 4)
+constexpr ValueT get() const noexcept {
+ static_assert(I < 4);
+ return static_cast<const ColorDerived &>(*this)._data[I];
+}
+
+/* implicit conversions */
+constexpr operator Vec3f() const noexcept {
+ const auto &d = static_cast<const ColorDerived &>(*this)._data;
+ return Vec3f{d.x(), d.y(), d.z()}; // avoid the “no Vec3f(Vec4f) ctor” error
+}
+constexpr operator Vec4f() const noexcept {
+ return static_cast<const ColorDerived &>(*this)._data;
+}Apply the same pattern to every other member that currently relies on the P0847 “deducing this” feature.
📝 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.
| // structured binding support | |
| template <size_t I> requires (I < 4) | |
| ValueT get(this const ColorDerived &self) { return self._data[I]; } | |
| operator Vec3f(this ColorDerived &self) noexcept { | |
| return (Vec3f)self._data; | |
| } | |
| operator Vec4f(this ColorDerived &self) noexcept { | |
| return self._data; | |
| } | |
| // structured binding support | |
| template <size_t I> requires (I < 4) | |
| constexpr ValueT get() const noexcept { | |
| static_assert(I < 4); | |
| return static_cast<const ColorDerived &>(*this)._data[I]; | |
| } | |
| /* implicit conversions */ | |
| constexpr operator Vec3f() const noexcept { | |
| const auto &d = static_cast<const ColorDerived &>(*this)._data; | |
| return Vec3f{d.x(), d.y(), d.z()}; // avoid the “no Vec3f(Vec4f) ctor” error | |
| } | |
| constexpr operator Vec4f() const noexcept { | |
| return static_cast<const ColorDerived &>(*this)._data; | |
| } |
| friend std::ostream & operator<<(std::ostream &s, const ColorDerived &color) noexcept { | ||
| Vec4u comps = color._data * 255; | ||
| comps.clamp(0, 256); | ||
| s << '#' << std::hex << std::uppercase << comps[0] << comps[1] << comps[2] << comps[3] << std::nouppercase << std::dec; | ||
| return s; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
operator<< prints incorrect hex and risks narrowing
- Multiplying a
Vec4fby255gives floats; converting them to aVec4uvia implicit ctor may silently floor or truncate. << std::hexstreams the decimal value of every component, not two-digit padded bytes, so#3F3F3F3Fmight appear as#3F3F3F3For#3f3f3f3fdepending on the values ‑ but e.g. component10becomes justA, losing the leading zero.
Consider formatting each channel explicitly:
-Vec4u comps = color._data * 255;
-comps.clamp(0, 256);
-s << '#' << std::hex << std::uppercase << comps[0] << comps[1] << comps[2] << comps[3] << std::nouppercase << std::dec;
+const auto comps = (color._data * 255.0f).clamped(0.0f, 255.0f).round(); // or explicit cast
+s << '#' << std::uppercase << std::hex
+ << std::setw(2) << std::setfill('0') << uint32_t(comps[0])
+ << std::setw(2) << std::setfill('0') << uint32_t(comps[1])
+ << std::setw(2) << std::setfill('0') << uint32_t(comps[2])
+ << std::setw(2) << std::setfill('0') << uint32_t(comps[3])
+ << std::dec << std::nouppercase;Requires <iomanip> and a round()/clamped helper, but produces predictable 8-digit ARGB output.
Committable suggestion skipped: line range outside the PR's diff.
| ColorDerived &operator+=(const ColorDerived &other) noexcept { | ||
| static_cast<ColorDerived*>(this)->_data += other._data; | ||
| return *(ColorDerived*)this; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsafe pointer casts & missing const qualification in operator+=
static_cast<ColorDerived*>(this)->_data += other._data; forces a mutable pointer even when *this is const.
Use a reference, keep qualifiers, and avoid C-style casts:
-ColorDerived &operator+=(const ColorDerived &other) noexcept {
- static_cast<ColorDerived*>(this)->_data += other._data;
- return *(ColorDerived*)this;
-}
+ColorDerived &operator+=(const ColorDerived &other) noexcept {
+ auto &self = static_cast<ColorDerived &>(*this);
+ self._data += other._data;
+ return self;
+}This addresses UB and removes the noisy casts.
📝 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.
| ColorDerived &operator+=(const ColorDerived &other) noexcept { | |
| static_cast<ColorDerived*>(this)->_data += other._data; | |
| return *(ColorDerived*)this; | |
| } | |
| ColorDerived &operator+=(const ColorDerived &other) noexcept { | |
| auto &self = static_cast<ColorDerived &>(*this); | |
| self._data += other._data; | |
| return self; | |
| } |
| mr::RGBAColor c1; // same as Vec4f(0, 0, 0, 0) | ||
| mr::RGBAColor c2 = mr::Color(0.3, 0.47, 0.8); // == (0.3, 0.47, 0.8, 1.0) | ||
| mr::RGBAColor c3 = mr::Color(76, 119, 204, 255); // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | ||
| mr::RGBAColor c4 = mr::Color(0x4C'77'CC'FF); // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | ||
| mr::RGBAColor c5 = 0x4C'77'CC'FF_rgba; // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Examples still reference the removed mr::Color type
The refactor renamed the type to mr::RGBAColor, yet the docs mix both names:
-mr::RGBAColor c2 = mr::Color(0.3, 0.47, 0.8);
-mr::RGBAColor c3 = mr::Color(76, 119, 204, 255);
-mr::RGBAColor c4 = mr::Color(0x4C'77'CC'FF);
+mr::RGBAColor c2{0.3, 0.47, 0.8};
+mr::RGBAColor c3{76, 119, 204, 255};
+mr::RGBAColor c4{0x4C'77'CC'FF};Do the same for the arithmetic example further down (mr::Color(1.0, …) ➜ mr::RGBAColor{…}).
Keeping the docs in sync prevents confusion for consumers.
📝 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.
| mr::RGBAColor c1; // same as Vec4f(0, 0, 0, 0) | |
| mr::RGBAColor c2 = mr::Color(0.3, 0.47, 0.8); // == (0.3, 0.47, 0.8, 1.0) | |
| mr::RGBAColor c3 = mr::Color(76, 119, 204, 255); // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | |
| mr::RGBAColor c4 = mr::Color(0x4C'77'CC'FF); // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | |
| mr::RGBAColor c5 = 0x4C'77'CC'FF_rgba; // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | |
| ``` | |
| mr::RGBAColor c1; // same as Vec4f(0, 0, 0, 0) | |
| mr::RGBAColor c2{0.3, 0.47, 0.8}; // == (0.3, 0.47, 0.8, 1.0) | |
| mr::RGBAColor c3{76, 119, 204, 255}; // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | |
| mr::RGBAColor c4{0x4C'77'CC'FF}; // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) | |
| mr::RGBAColor c5 = 0x4C'77'CC'FF_rgba; // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
190-190: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
| const mr::RGBAColor expected1{0.3, 0.47, 0.8, 1.0}; | ||
| EXPECT_EQ(mr::RGBAColor(0.3, 0.47, 0.8), expected1); | ||
| EXPECT_EQ(mr::RGBAColor(mr::Vec4f(0.3, 0.47, 0.8, 1)), expected1); | ||
|
|
||
| const mr::Color expected2{0.2980392156862745, 0.4666666666666667, 0.8, 1.0}; | ||
| EXPECT_EQ(mr::Color(76, 119, 204, 255), expected2); | ||
| EXPECT_EQ(mr::Color(0x4C'77'CC'FF), expected2); | ||
| const mr::RGBAColor expected2{0.2980392156862745, 0.4666666666666667, 0.8, 1.0}; | ||
| EXPECT_EQ(mr::RGBAColor(76, 119, 204, 255), expected2); | ||
| EXPECT_EQ(mr::RGBAColor(0x4C'77'CC'FF), expected2); | ||
| EXPECT_EQ(0x4C'77'CC'FF_rgba, expected2); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using EXPECT_FLOAT_EQ for floating‐point constructor tests
The constructors accepting floats and vectors compare RGBAColor instances via EXPECT_EQ, which relies on operator==. If operator== uses exact float comparison, tiny rounding differences could cause spurious failures. Consider switching to EXPECT_TRUE(equal(...)) or EXPECT_FLOAT_EQ on each component for these tests to tolerate floating‐point precision.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
include/mr-math/color.hpp (5)
17-20: Unconditionalstatic_assertstill prevents all instantiationsThis exact issue was raised in the previous review: the primary template aborts every build even when a matching specialization exists. Make the assertion dependent on the template parameters.
27-38:deducing thisis the cause of the current CI syntax errorsGCC 13 / Clang 17 (our tool-chain) do not support the P0847 “deducing-this” syntax yet. Every use (
get, the two conversion operators) fails to compile (self/other/epsundeclared).
44-48:operator+=still does UB-prone pointer castsCasting away
constand going through a pointer was already flagged; the unsafe pattern is unchanged.
49-55: Equality helpers repeat the unsupported “deducing-this” styleAgain identical to the previously reported issue; the build stops here.
57-62:operator<<prints wrong hex & silently truncates ‑ nothing changedThe earlier suggestions (use
std::setw, explicit rounding, avoid<< std::hexonfloats) are still applicable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
include/mr-math/color.hpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
include/mr-math/color.hpp (4)
include/mr-math/matr.hpp (8)
nodiscard(169-171)nodiscard(173-175)nodiscard(178-191)_data(321-324)_data(383-386)_data(455-458)i(169-169)i(173-173)include/mr-math/operators.hpp (2)
s(133-141)s(134-134)include/mr-math/def.hpp (6)
epsilon(35-37)epsilon(35-35)value(93-93)value(93-93)value(116-116)value(116-116)include/mr-math/vec.hpp (7)
l(165-169)l(172-175)i(84-84)i(84-84)i(95-95)y(86-86)y(86-86)
🪛 GitHub Actions: Benchmarks
include/mr-math/color.hpp
[error] 29-54: Syntax errors due to incorrect use of 'this' keyword in method parameters and undeclared identifiers 'self', 'other', and 'eps'.
[error] 371-371: No matching function for call to 'mr::math::Vec<float, 3>::Vec(mr::math::RGBAColor&)'. Invalid static_cast from RGBAColor to Vec3f.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (ubuntu-24.04, Release, clang)
| struct HSLAColor { | ||
| friend class ColorBase<HSLAColor>; | ||
|
|
||
| private: | ||
| Vec4f _data; | ||
|
|
||
| public: | ||
| using ValueT = float; | ||
|
|
||
| HSLAColor() = default; | ||
|
|
||
| template <std::floating_point T> | ||
| HSLAColor(T h, T s, T l, T a = 1) noexcept | ||
| : _data(h, s, l, a) {} | ||
|
|
||
| HSLAColor(Vec4f hsla) noexcept | ||
| : _data(hsla) {} | ||
|
|
||
| template <std::integral T> |
There was a problem hiding this comment.
All new colour structs forget to inherit from ColorBase
HSLAColor, HSVAColor, CMYKColor, and OKLABColor only friend the base but do not derive from it, so they do not get:
operator+,operator+=- equality helpers
- structured-binding support
- stream output
…and any future generic utilities.
This is a functional break (e.g. HSLAColor{} can’t be streamed, added, or compared).
Apply the pattern already used for RGBAColor:
-struct HSLAColor {
- friend class ColorBase<HSLAColor>;
+struct HSLAColor : ColorBase<HSLAColor> {
+ friend class ColorBase<HSLAColor>;Repeat the same one-liner change for HSVAColor, CMYKColor, and OKLABColor.
Also applies to: 196-214, 254-272, 312-330
cone-forest
force-pushed
the
feature/color-spaces
branch
from
c4ddbf9 to
2640318
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mr-math.natvis (1)
107-116: Fix natvis: extra brace and better hex packing.
- Remove the stray ‘}’ in DisplayString.
- Prefer rounded bytes and bitwise OR.
- <Type Name="mr::math::RGBAColor"> - <DisplayString>R={r()}, G={g()}, B={b()}, A={a()}}</DisplayString> + <Type Name="mr::math::RGBAColor"> + <DisplayString>R={r()}, G={g()}, B={b()}, A={a()}</DisplayString> <Expand> <Item Name="R">r()</Item> <Item Name="G">g()</Item> <Item Name="B">b()</Item> <Item Name="A">a()</Item> - <Item Name="Hex">(uint32_t(r() * 255) << 24) + (uint32_t(g() * 255) << 16) + (uint32_t(b() * 255) << 8) + (uint32_t(a() * 255)), Xb</Item> + <Item Name="Hex">(uint32_t(r()*255 + 0.5f) << 24) | (uint32_t(g()*255 + 0.5f) << 16) | (uint32_t(b()*255 + 0.5f) << 8) | uint32_t(a()*255 + 0.5f), Xb</Item> </Expand> </Type>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
USAGE.md(2 hunks)include/mr-math/color.hpp(3 hunks)mr-math.natvis(1 hunks)tests/misc.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Benchmarks
include/mr-math/color.hpp
[error] 29-54: Compilation errors in ColorBase definitions: use of 'this' in member function signatures appears invalid (e.g., 'ValueT get(this const ColorDerived &self)', 'operator Vec3f(this ColorDerived &self)'), plus missing declarations of 'self', 'other', and 'eps' in several methods.
🪛 markdownlint-cli2 (0.18.1)
USAGE.md
218-218: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
220-220: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
225-225: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
228-228: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
233-233: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
235-235: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
⏰ 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). (1)
- GitHub Check: build (windows-latest, Release, cl)
🔇 Additional comments (12)
tests/misc.cpp (3)
442-447: Setters test aligns with the API and expected semantics.
463-465: Define addition semantics (clamp/saturate vs. unbounded).Test asserts unbounded sums; ensure this matches intended API (document or clamp in impl).
403-414: Constructors tests look good.Consider
EXPECT_TRUE(equal(lhs, rhs))or component-wiseEXPECT_FLOAT_EQto avoid brittle exact-float compares.include/mr-math/color.hpp (7)
17-21: Fix: make the primaryconvertSFINAE-friendly (current code hard-errors every instantiation).The unconditional
static_assert(false)prevents valid specializations from being found.Apply:
-template <typename InputT, typename OutputT> - OutputT convert(InputT input) { - static_assert(false, "You have to specify this function for your particular types"); - } +template <typename...> +inline constexpr bool _mr_convert_always_false_v = false; + +template <typename InputT, typename OutputT> +constexpr OutputT convert(InputT /*unused*/) { + static_assert(_mr_convert_always_false_v<InputT, OutputT>, + "No `convert` specialization found for the supplied types"); +}
27-37: Replace unsupported “deducing this” with standard C++20 members.These signatures won’t compile on our toolchain and are the source of the pipeline errors.
Apply:
-// structured binding support -template <size_t I> requires (I < 4) - ValueT get(this const ColorDerived &self) { return self._data[I]; } - -operator Vec3f(this ColorDerived &self) noexcept { - return (Vec3f)self._data; -} - -operator Vec4f(this ColorDerived &self) noexcept { - return self._data; -} +// structured binding support +template <size_t I> + requires (I < 4) +constexpr ValueT get() const noexcept { + static_assert(I < 4); + return static_cast<const ColorDerived &>(*this)._data[I]; +} + +// implicit conversions +constexpr operator Vec3f() const noexcept { + const auto &d = static_cast<const ColorDerived &>(*this)._data; + return Vec3f{d.x(), d.y(), d.z()}; +} +constexpr operator Vec4f() const noexcept { + return static_cast<const ColorDerived &>(*this)._data; +}Additionally (outside this hunk), expose ADL
getfor structured bindings:namespace mr { inline namespace math { template <size_t I> constexpr auto get(const RGBAColor &c) noexcept -> RGBAColor::ValueT { return c.template get<I>(); } }}
44-47: Fix UB-prone casts inoperator+=(use a reference, preserve cv-qualifiers).-ColorDerived &operator+=(const ColorDerived &other) noexcept { - static_cast<ColorDerived*>(this)->_data += other._data; - return *(ColorDerived*)this; -} +ColorDerived &operator+=(const ColorDerived &other) noexcept { + auto &self = static_cast<ColorDerived &>(*this); + self._data += other._data; + return self; +}
49-55: Rewrite equality helpers without “deducing this” and keep const-correctness.-bool operator==(this const ColorDerived &self, const ColorDerived &other) noexcept { - return self._data == other._data; -} - -bool equal(this const ColorDerived &self, const ColorDerived &other, ValueT eps = epsilon<ValueT>()) noexcept { - return self._data.equal(other._data, eps); -} +constexpr bool operator==(const ColorDerived &other) const noexcept { + return static_cast<const ColorDerived &>(*this)._data == other._data; +} +constexpr bool equal(const ColorDerived &other, + ValueT eps = epsilon<ValueT>()) const noexcept { + return static_cast<const ColorDerived &>(*this)._data.equal(other._data, eps); +}
57-62: Hex stream formatting is incorrect; also risks truncation.Print zero‑padded bytes with rounding; avoid relying on implicit narrowing.
-friend std::ostream & operator<<(std::ostream &s, const ColorDerived &color) noexcept { - Vec4u comps = color._data * 255; - comps.clamp(0, 256); - s << '#' << std::hex << std::uppercase << comps[0] << comps[1] << comps[2] << comps[3] << std::nouppercase << std::dec; - return s; -} +friend std::ostream & operator<<(std::ostream &s, const ColorDerived &color) noexcept { + auto tmp = color._data * 255.0f; + tmp.clamp(0.0f, 255.0f); + const uint32_t r = static_cast<uint32_t>(tmp[0] + 0.5f); + const uint32_t g = static_cast<uint32_t>(tmp[1] + 0.5f); + const uint32_t b = static_cast<uint32_t>(tmp[2] + 0.5f); + const uint32_t a = static_cast<uint32_t>(tmp[3] + 0.5f); + s << '#' << std::uppercase << std::hex + << std::setw(2) << std::setfill('0') << r + << std::setw(2) << std::setfill('0') << g + << std::setw(2) << std::setfill('0') << b + << std::setw(2) << std::setfill('0') << a + << std::dec << std::nouppercase; + return s; +}Also add (outside this hunk):
#include <iomanip>
142-160: All non-RGBA colour structs don’t inherit fromColorBase(missing operators, streaming, etc.).This breaks arithmetic, equality, and output for these types.
Apply the one-liner on each:
-struct HSLAColor { +struct HSLAColor : ColorBase<HSLAColor> { friend class ColorBase<HSLAColor>;-struct HSVAColor { +struct HSVAColor : ColorBase<HSVAColor> { friend class ColorBase<HSVAColor>;-struct CMYKColor { +struct CMYKColor : ColorBase<CMYKColor> { friend class ColorBase<CMYKColor>;-struct OKLABColor { +struct OKLABColor : ColorBase<OKLABColor> { friend class ColorBase<OKLABColor>;Also applies to: 200-256, 258-314, 316-371
373-392: Fix RGBA→OKLAB conversion: avoid invalid cast, ensure matrix×vector, use std::cbrtf
- Change signature to
convert<RGBAColor,OKLABColor>(const RGBAColor& rgba)and extractVec3f rgb{rgba.r(),rgba.g(),rgba.b()}- Compute
auto v = Matr3f{…} * rgb(avoidstatic_cast<Vec3f>(rgba)and reversed operand)- Replace
cbrtfwithstd::cbrtfand add#include <cmath>- Confirm a
Matr3f*Vec3foverload exists and thatRGBAColoris linear‐RGB (add gamma→linear if needed)USAGE.md (2)
213-218: Docs still reference removedmr::Color– switch tomr::RGBAColorwith brace-init.-mr::RGBAColor c1; // same as Vec4f(0, 0, 0, 0) -mr::RGBAColor c2 = mr::Color(0.3, 0.47, 0.8); // == (0.3, 0.47, 0.8, 1.0) -mr::RGBAColor c3 = mr::Color(76, 119, 204, 255); // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) -mr::RGBAColor c4 = mr::Color(0x4C'77'CC'FF); // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) -mr::RGBAColor c5 = 0x4C'77'CC'FF_rgba; // == (0.2980392156862745, 0.4666666666666667, 0.8, 1.0) +mr::RGBAColor c1; // same as Vec4f(0, 0, 0, 0) +mr::RGBAColor c2{0.3, 0.47, 0.8}; // == (0.3, 0.47, 0.8, 1.0) +mr::RGBAColor c3{76, 119, 204, 255}; // == (0.298..., 0.466..., 0.8, 1.0) +mr::RGBAColor c4{0x4C'77'CC'FF}; // == (0.298..., 0.466..., 0.8, 1.0) +mr::RGBAColor c5 = 0x4C'77'CC'FF_rgba; // == (0.298..., 0.466..., 0.8, 1.0)Also add blank lines around fenced code blocks to satisfy MD031.
236-236: UseRGBAColorin arithmetic example.-mr::RGBAColor c11 = mr::Color(1.0, 0.0, 0.5, 1.0) + mr::Color(0.0, 1.0, 0.5, 1.0); // == mr::Color(1.0, 1.0, 1.0, 2.0)); +mr::RGBAColor c11 = mr::RGBAColor{1.0, 0.0, 0.5, 1.0} + mr::RGBAColor{0.0, 1.0, 0.5, 1.0}; // == mr::RGBAColor{1.0, 1.0, 1.0, 2.0}
| struct alignas(float) [[nodiscard]] RGBAColor : ColorBase<RGBAColor> { | ||
| friend class ColorBase<RGBAColor>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop redundant alignas(float) on RGBAColor.
It’s unnecessary and potentially misleading; member alignment dictates the type’s alignment anyway.
-struct alignas(float) [[nodiscard]] RGBAColor : ColorBase<RGBAColor> {
+struct [[nodiscard]] RGBAColor : ColorBase<RGBAColor> {📝 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.
| struct alignas(float) [[nodiscard]] RGBAColor : ColorBase<RGBAColor> { | |
| friend class ColorBase<RGBAColor>; | |
| struct [[nodiscard]] RGBAColor : ColorBase<RGBAColor> { | |
| friend class ColorBase<RGBAColor>; |
🤖 Prompt for AI Agents
In include/mr-math/color.hpp around lines 67 to 68, the struct declaration uses
a redundant alignas(float) on RGBAColor; remove the alignas(float) specifier
from the RGBAColor declaration so the type uses natural member-driven alignment
instead, leaving the struct declaration as a plain struct alignement (and run a
quick build to ensure no intended ABI assumptions relied on that attribute).
| struct tuple_size<mr::RGBAColor> | ||
| : std::integral_constant<size_t, 4> {}; | ||
|
|
||
| template <size_t I> | ||
| struct tuple_element<I, mr::Color> { | ||
| using type = mr::Color::ValueT; | ||
| struct tuple_element<I, mr::RGBAColor> { | ||
| using type = mr::RGBAColor::ValueT; | ||
| }; |
There was a problem hiding this comment.
Structured bindings need ADL get – provide free functions.
tuple_size/tuple_element isn’t enough by itself.
Add (outside this hunk):
namespace mr { inline namespace math {
template <size_t I>
constexpr auto get(const RGBAColor &c) noexcept -> RGBAColor::ValueT {
return c.template get<I>();
}
}}🤖 Prompt for AI Agents
In include/mr-math/color.hpp around lines 410–416, tuple_size/tuple_element are
defined but structured bindings also require free ADL get functions; add inline
free get overloads in namespace mr (e.g., inline namespace math) that forward to
the member template get<I>() — provide at least a constexpr const& (or by-value
if ValueT is a scalar) noexcept template<size_t I> returning RGBAColor::ValueT
using c.template get<I>(), and also add non-const and rvalue overloads so
structured bindings and std::get work for const, mutable and rvalue RGBAColor
objects.
| mr::RGBAColor c6 = 0x4C'77'CC'FF_rgba; | ||
| mr::RGBAColor c7 = color.argb(); // == 0xFF'4C'77'CC_rgba | ||
| mr::RGBAColor c8 = color.bgra(); // == 0xCC'77'4c'FF_rgba | ||
| mr::RGBAColor c9 = color.abgr(); // == 0xFF'CC'77'4c_rgba | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type of color.argb()/bgra()/abgr() in examples.
They return Vec4f; relying on the RGBAColor(Vec4f) ctor is fine, but prefer clarity.
-mr::RGBAColor c7 = color.argb(); // == 0xFF'4C'77'CC_rgba
-mr::RGBAColor c8 = color.bgra(); // == 0xCC'77'4c'FF_rgba
-mr::RGBAColor c9 = color.abgr(); // == 0xFF'CC'77'4c_rgba
+mr::RGBAColor c7{color.argb()}; // == 0xFF'4C'77'CC_rgba
+mr::RGBAColor c8{color.bgra()}; // == 0xCC'77'4c'FF_rgba
+mr::RGBAColor c9{color.abgr()}; // == 0xFF'CC'77'4c_rgba📝 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.
| mr::RGBAColor c6 = 0x4C'77'CC'FF_rgba; | |
| mr::RGBAColor c7 = color.argb(); // == 0xFF'4C'77'CC_rgba | |
| mr::RGBAColor c8 = color.bgra(); // == 0xCC'77'4c'FF_rgba | |
| mr::RGBAColor c9 = color.abgr(); // == 0xFF'CC'77'4c_rgba | |
| ``` | |
| mr::RGBAColor c6 = 0x4C'77'CC'FF_rgba; | |
| mr::RGBAColor c7{color.argb()}; // == 0xFF'4C'77'CC_rgba | |
| mr::RGBAColor c8{color.bgra()}; // == 0xCC'77'4c'FF_rgba | |
| mr::RGBAColor c9{color.abgr()}; // == 0xFF'CC'77'4c_rgba |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
225-225: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In USAGE.md around lines 221 to 225, the examples call
color.argb()/bgra()/abgr() which return Vec4f; update the examples to explicitly
construct an mr::RGBAColor from those Vec4f results for clarity. Replace
assignments like "mr::RGBAColor c7 = color.argb();" with an explicit constructor
call such as "mr::RGBAColor c7 = mr::RGBAColor(color.argb());" and do the same
for c8 and c9 so the type conversion is obvious.
1 participant
Summary by CodeRabbit
New Features
Documentation
Style
Tests