constexpr Color #1297
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Color API is moved toward inline ChangesColor API modernization
Development artifact ignore rules
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/math/color.h`:
- Around line 286-307: `color.h` uses `std::bit_cast` in the color packing logic
but does not include the required header directly. Add the missing `<bit>`
include near the other includes in `core/math/color.h`; keep the existing
`MaxChannel`/packing code unchanged and ensure the header remains
self-contained.
- Around line 40-66: The constexpr packers in color_pack::floats_to_uint32 and
color_pack::floats_to_uint64 are not actually usable in constant-evaluated code
because they rely on Math::round/std::round; update these helpers to use a
constexpr-friendly rounding implementation if compile-time use is intended, and
ensure the to_*32/64 wrappers remain valid, or remove constexpr from these
functions until the rounding dependency is constexpr-capable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 054e211b-98e9-4269-abd4-f27265fee959
📒 Files selected for processing (2)
core/math/color.cppcore/math/color.h
| return 0.2126f*r + 0.7152f*g + 0.0722f*b; | ||
| } | ||
|
|
||
| Color apply_intensity(float p_intensity) const; |
There was a problem hiding this comment.
Looks like the doxygen documentation was removed from apply_intensity .
Can we add this back please?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.gitignore (1)
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not ignore
uv.lockunless it is intentionally disposable.
uv.lockis a dependency-resolution artifact, not a virtual environment. Ignoring it prevents reproducible UV installs and can hide dependency changes. Please verify the repository’s dependency-management policy; if this project expects committed lockfiles, remove this rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore at line 85, Verify the repository’s dependency-management policy for uv and, if uv.lock is expected to be committed, remove the uv.lock ignore rule from .gitignore. Keep the rule only if the lockfile is intentionally disposable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/math/math_funcs.h`:
- Around line 544-557: Update the three mixed-type is_equal_approx overloads
around the existing floating-point templates: constrain the conversion-only
overloads so C must not be floating-point, then replace the mirrored floating
overloads with a single mixed-floating overload using std::common_type_t for
both operands. Preserve the two-argument and tolerance-based behavior while
ensuring calls such as is_equal_approx(1.0f, 1.0) resolve unambiguously.
- Around line 641-643: Update Math::round to detect non-finite values and values
outside the representable long range before casting, preserving signed zero for
-0.0. Use a constexpr-safe manual path during constant evaluation and a runtime
std::round fallback for guarded cases, while retaining the existing rounding
behavior for ordinary finite in-range inputs.
In `@core/typedefs.h`:
- Around line 161-165: Update the SWAP template’s noexcept specification to be
conditional on whether std::swap(m_x, m_y) is non-throwing for T, rather than
unconditionally noexcept. Preserve the existing constexpr implementation and
std::swap call.
---
Nitpick comments:
In @.gitignore:
- Line 85: Verify the repository’s dependency-management policy for uv and, if
uv.lock is expected to be committed, remove the uv.lock ignore rule from
.gitignore. Keep the rule only if the lockfile is intentionally disposable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 514db922-bcfd-4ad5-ab57-439ac37bd98c
📒 Files selected for processing (5)
.gitignorecore/math/color.cppcore/math/color.hcore/math/math_funcs.hcore/typedefs.h
🚧 Files skipped from review as they are similar to previous changes (1)
- core/math/color.h
| template <std::floating_point T, std::convertible_to<T> C> | ||
| constexpr bool is_equal_approx(T p_left, C p_right) noexcept { | ||
| return is_equal_approx(p_left, static_cast<T>(p_right)); | ||
| } | ||
| _ALWAYS_INLINE_ bool is_equal_approx(float p_left, float p_right) { | ||
| // Check for exact equality first, required to handle "infinity" values. | ||
| if (p_left == p_right) { | ||
| return true; | ||
| } | ||
| // Then check for approximate equality. | ||
| float tolerance = (float)CMP_EPSILON * abs(p_left); | ||
| if (tolerance < (float)CMP_EPSILON) { | ||
| tolerance = (float)CMP_EPSILON; | ||
| } | ||
| return abs(p_left - p_right) < tolerance; | ||
|
|
||
| template <std::floating_point T, std::convertible_to<T> C> | ||
| constexpr bool is_equal_approx(T p_left, C p_right, T p_tolerance) noexcept { | ||
| return is_equal_approx(p_left, static_cast<T>(p_right), p_tolerance); | ||
| } | ||
|
|
||
| _ALWAYS_INLINE_ bool is_zero_approx(double p_value) { | ||
| return abs(p_value) < CMP_EPSILON; | ||
| template <std::floating_point T, std::convertible_to<T> C> | ||
| constexpr bool is_equal_approx(C p_left, T p_right) noexcept { | ||
| return is_equal_approx(static_cast<T>(p_left), p_right); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Enumerate two-argument call sites requiring mixed-type inspection.
ast-grep run \
--pattern 'Math::is_equal_approx($LEFT, $RIGHT)' \
--lang cpp .Repository: Redot-Engine/redot-engine
Length of output: 28058
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the relevant overload set in core/math/math_funcs.h.
sed -n '500,590p' core/math/math_funcs.h
printf '\n---\n'
# Find every is_equal_approx declaration in the header.
rg -n "is_equal_approx" core/math/math_funcs.hRepository: Redot-Engine/redot-engine
Length of output: 4256
Resolve the mixed-floating overload ambiguity. is_equal_approx(1.0f, 1.0) can match both mirrored templates; constrain those overloads to non-floating conversions and add one mixed-floating overload using std::common_type_t.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/math/math_funcs.h` around lines 544 - 557, Update the three mixed-type
is_equal_approx overloads around the existing floating-point templates:
constrain the conversion-only overloads so C must not be floating-point, then
replace the mirrored floating overloads with a single mixed-floating overload
using std::common_type_t for both operands. Preserve the two-argument and
tolerance-based behavior while ensuring calls such as is_equal_approx(1.0f, 1.0)
resolve unambiguously.
| template <std::floating_point T> | ||
| constexpr T round(T p_val) noexcept { | ||
| return p_val >= T{ 0 } ? static_cast<T>(static_cast<long>(p_val + T{ .5f })) : static_cast<T>(static_cast<long>(p_val - T{ .5f })); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect callers and existing boundary tests.
ast-grep run --pattern 'Math::round($VALUE)' --lang cpp .
rg -n -C2 'round.*(inf|nan|LONG|zero)|negative zero' . \
-g '*test*' -g '*.{h,cpp}' || trueRepository: Redot-Engine/redot-engine
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -eu
# Map the file and inspect the round implementation context.
wc -l core/math/math_funcs.h
ast-grep outline core/math/math_funcs.h --view expanded || true
sed -n '620,670p' core/math/math_funcs.h
# Find all round helpers/usages in the math headers/source.
rg -n "\bround\b|Math::round|std::round" core -g '*.{h,cpp,cc,hpp,mm}' || trueRepository: Redot-Engine/redot-engine
Length of output: 8784
🏁 Script executed:
#!/bin/bash
set -eu
# Check how constexpr round would behave for special values.
python3 - <<'PY'
import math
def emulate(p_val):
return (float(int(p_val + 0.5)) if p_val >= 0 else float(int(p_val - 0.5)))
tests = [0.0, -0.0, 0.4, -0.4, 0.5, -0.5, 1.5, -1.5, math.inf, -math.inf, math.nan]
for x in tests:
try:
y = emulate(x)
print(repr(x), "->", repr(y), "signbit_in:", math.copysign(1.0, x) < 0, "signbit_out:", math.copysign(1.0, y) < 0)
except Exception as e:
print(repr(x), "->", type(e).__name__, e)
PYRepository: Redot-Engine/redot-engine
Length of output: 726
🏁 Script executed:
#!/bin/bash
set -eu
# Read the exact implementation and nearby helpers.
sed -n '600,700p' core/math/math_funcs.h
# Look for dedicated round tests or special-value tests in math-related test files.
git ls-files '*test*' '*spec*' | rg 'math|round|float|double|numeric' || true
rg -n -C3 'round\(|nan|inf|infinity|negative zero|std::round|LONG_MAX|LONG_MIN' \
core test tests . -g '*.{h,cpp,cc,hpp,mm,ts,js}' || trueRepository: Redot-Engine/redot-engine
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -eu
# Compare the current implementation against std::round semantics on edge cases.
python3 - <<'PY'
import math
def impl(x):
return float(int(x + 0.5)) if x >= 0 else float(int(x - 0.5))
tests = [0.0, -0.0, 0.1, -0.1, 0.5, -0.5, 1.4, -1.4, 2.5, -2.5,
math.inf, -math.inf, math.nan, 1e20, -1e20]
for x in tests:
try:
y = impl(x)
print("impl", repr(x), "->", repr(y), "signbit:", math.copysign(1.0, y) < 0)
except Exception as e:
print("impl", repr(x), "->", type(e).__name__, e)
try:
z = math.copysign(1.0, math.round(x))
except Exception:
pass
PYRepository: Redot-Engine/redot-engine
Length of output: 743
Guard Math::round from non-finite and out-of-range inputs. Casting these values to long is UB here, and -0.0 loses its sign. Use a constexpr-safe implementation with a runtime std::round fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/math/math_funcs.h` around lines 641 - 643, Update Math::round to detect
non-finite values and values outside the representable long range before
casting, preserving signed zero for -0.0. Use a constexpr-safe manual path
during constant evaluation and a runtime std::round fallback for guarded cases,
while retaining the existing rounding behavior for ordinary finite in-range
inputs.
| // Generic swap template. | ||
| #ifndef SWAP | ||
| #define SWAP(m_x, m_y) std::swap((m_x), (m_y)) | ||
| #endif // SWAP | ||
|
|
||
| template <typename T> | ||
| constexpr void SWAP(T &m_x, T &m_y) noexcept { | ||
| std::swap(m_x, m_y); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect whether SWAP is used with nontrivial user-defined types.
ast-grep run --pattern 'SWAP($LEFT, $RIGHT)' --lang cpp .Repository: Redot-Engine/redot-engine
Length of output: 18469
🏁 Script executed:
#!/bin/bash
set -eu
# Show the SWAP definition and nearby helper context.
file=core/typedefs.h
nl -ba "$file" | sed -n '145,175p'
# Check for any custom swap overloads that SWAP might rely on.
rg -n "namespace std|using std::swap|swap\(" core scene servers editor modules drivers platform -g '!**/thirdparty/**' | sed -n '1,200p'Repository: Redot-Engine/redot-engine
Length of output: 1909
Make SWAP conditionally noexcept core/typedefs.h:161-165
std::swap is only non-throwing for some T; the unconditional spec can turn a throwing swap into termination.
Proposed fix
template <typename T>
-constexpr void SWAP(T &m_x, T &m_y) noexcept {
+constexpr void SWAP(T &m_x, T &m_y) noexcept(noexcept(std::swap(m_x, m_y))) {
std::swap(m_x, m_y);
}📝 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.
| // Generic swap template. | |
| #ifndef SWAP | |
| #define SWAP(m_x, m_y) std::swap((m_x), (m_y)) | |
| #endif // SWAP | |
| template <typename T> | |
| constexpr void SWAP(T &m_x, T &m_y) noexcept { | |
| std::swap(m_x, m_y); | |
| } | |
| // Generic swap template. | |
| template <typename T> | |
| constexpr void SWAP(T &m_x, T &m_y) noexcept(noexcept(std::swap(m_x, m_y))) { | |
| std::swap(m_x, m_y); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/typedefs.h` around lines 161 - 165, Update the SWAP template’s noexcept
specification to be conditional on whether std::swap(m_x, m_y) is non-throwing
for T, rather than unconditionally noexcept. Preserve the existing constexpr
implementation and std::swap call.
OldDev78
force-pushed
the
constexpr-color
branch
from
3014d75 to
da86813
Compare
OldDev78
force-pushed
the
constexpr-color
branch
2 times, most recently
from
1a5e869 to
530b061
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/math/color.h`:
- Around line 140-175: Replace the three-argument MIN/MAX calls in get_h(),
get_s(), and get_v() with nested two-argument calls, matching the established
pattern used elsewhere in the file. Preserve the existing hue, saturation, and
value calculations.
- Line 608: Rename the Color constant PaleTorquoise to PaleTurquoise, preserving
its existing hex value and declaration style so it matches color_names.inc, the
documentation, and the C# binding.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8501e81e-1475-4a8a-8d2a-e04bacc708b6
📒 Files selected for processing (3)
.gitignorecore/math/color.cppcore/math/color.h
🚧 Files skipped from review as they are similar to previous changes (1)
- core/math/color.cpp
| static constexpr auto Orchid = Color::hex(0xDA70D6FF); | ||
| static constexpr auto PaleGoldenrod = Color::hex(0xEEE8AAFF); | ||
| static constexpr auto PaleGreen = Color::hex(0x98FB98FF); | ||
| static constexpr auto PaleTorquoise = Color::hex(0xAFEEEEFF); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all occurrences of the identifier and nearby color-name definitions.
rg -n "PaleTorquoise|PaleTurquoise|PALE_TURQUOISE|PALE_TORQUOISE" .
# Show the surrounding context in the header.
python3 - <<'PY'
from pathlib import Path
path = Path("core/math/color.h")
text = path.read_text()
for needle in ["PaleTorquoise", "PaleTurquoise"]:
idx = text.find(needle)
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-500))
end = text.find('\n', idx+200)
print(f"\n--- context for {needle} ---")
print(text[start:end])
PY
# Summarize whether the identifier is used elsewhere in the repo.
python3 - <<'PY'
from pathlib import Path
import re
name = "PaleTorquoise"
hits = []
for p in Path(".").rglob("*"):
if p.is_file() and p.suffix in {".h", ".hpp", ".c", ".cc", ".cpp", ".inl", ".inc", ".gd", ".ts", ".js", ".py", ".md"}:
try:
txt = p.read_text(errors="ignore")
except Exception:
continue
if name in txt:
hits.append(str(p))
print("\nFiles containing PaleTorquoise:")
for h in hits:
print(h)
PYRepository: Redot-Engine/redot-engine
Length of output: 1908
🌐 Web query:
Godot Color named color table PaleTurquoise PALE_TURQUOISE constant spelling
💡 Result:
In the Godot Engine, the constant representing the color Pale Turquoise is named PALE_TURQUOISE [1]. It is defined in the Color class as a constant with the following value: PALE_TURQUOISE = Color(0.6862745, 0.93333334, 0.93333334, 1) [1] The naming convention for Color constants in Godot uses all uppercase letters with underscores separating words (e.g., PALE_TURQUOISE) [2][1].
Citations:
- 1: https://github.com/godotengine/godot-docs/blob/master/classes/class%5Fcolor.rst
- 2: https://github.com/godotengine/godot/blob/4a919adc/core/math/color_names.inc
Rename PaleTorquoise to PaleTurquoise
This matches core/math/color_names.inc, the docs, and the C# binding; the current name is misspelled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/math/color.h` at line 608, Rename the Color constant PaleTorquoise to
PaleTurquoise, preserving its existing hex value and declaration style so it
matches color_names.inc, the documentation, and the C# binding.
530b061 to
77386e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
core/math/color.h (3)
608-608: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
PaleTorquoiseis still misspelledThis was already flagged in a prior review (unresolved) — should be
PaleTurquoiseto matchcolor_names.inc, the docs, and the C# binding.✏️ Proposed fix
- static constexpr auto PaleTorquoise = Color::hex(0xAFEEEEFF); + static constexpr auto PaleTurquoise = Color::hex(0xAFEEEEFF);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/math/color.h` at line 608, Rename the Color constant PaleTorquoise to PaleTurquoise, matching the spelling used by color_names.inc, the documentation, and the C# binding; update any references to the old identifier accordingly.
166-175: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
get_s()/get_v()still use invalid 3-argumentMIN/MAX— confirmed CI failure
get_h()above was already fixed to use nestedMIN(r, MIN(g, b))/MAX(r, MAX(g, b)), butget_s()(lines 167-168) andget_v()(line 174) still pass three arguments toMIN/MAX, which only accept two. Static analysis confirms this is currently breaking every non-Windows build too ("no matching function for call to 'MAX'/'MIN'" at 167/168/174).🐛 Proposed fix
constexpr float get_s() const noexcept { - const float min = MIN(r, g, b); - const float max = MAX(r, g, b); + const float min = MIN(MIN(r, g), b); + const float max = MAX(MAX(r, g), b); const float delta = max - min; return (max != 0.0f) ? (delta / max) : 0.0f; } constexpr float get_v() const noexcept { - return MAX(r, g, b); + return MAX(MAX(r, g), b); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/math/color.h` around lines 166 - 175, Update get_s() and get_v() to use the same nested two-argument MIN/MAX pattern already applied in get_h(), replacing every three-argument invocation while preserving the existing saturation and value calculations.Source: Linters/SAST tools
40-65: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBuild-breaking:
color_packconstexpr helpers still depend on non-constexprMath::roundCI is currently failing on Windows builds exactly at these lines ("constexpr function 'color_pack::floats_to_uint32/64' cannot result in a constant expression"). Per
math_funcs.h,Math::round(float)/Math::round(double)are only_ALWAYS_INLINE_, notconstexpr, so anyconstexprfunction that unconditionally calls them is currently ill-formed under MSVC's stricter diagnostics. This is the exact scenario the retrieved learning warned about — the anticipated "Math::round becomes constexpr" companion PR hasn't landed yet, so this PR is not buildable on Windows as-is.The same pattern silently recurs elsewhere in this file and will likely surface once these lines are fixed:
get_r8/get_g8/get_b8/get_a8(lines 390-396) callMath::rounddirectly, andColor::lerp(line 255) callsMath::lerp, which is also notconstexprinmath_funcs.h.🛡️ Proposed interim fix
namespace color_pack { -constexpr uint32_t floats_to_uint32(const float f1, const float f2, const float f3, const float f4) noexcept { +// TODO(color-constexpr): restore `constexpr` once Math::round() is constexpr-capable. +uint32_t floats_to_uint32(const float f1, const float f2, const float f3, const float f4) noexcept { ... } -constexpr uint64_t floats_to_uint64(const float f1, const float f2, const float f3, const float f4) noexcept { +uint64_t floats_to_uint64(const float f1, const float f2, const float f3, const float f4) noexcept { ... } } //namespace color_pack(and drop
constexprfrom the dependentto_rgba32/argb32/abgr32/rgba64/argb64/abgr64()wrappers andget_r8/g8/b8/a8()until the upstream change lands)Based on learnings that
color_pack's helpers rely onMath::round(), which "is not yetconstexpr-capable under the project's C++20 build," and that the fix should either avoid the misleadingconstexpror gate it untilMath::round()is ready.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/math/color.h` around lines 40 - 65, Remove constexpr from floats_to_uint32, floats_to_uint64, and every dependent color conversion wrapper (to_rgba32, argb32, abgr32, rgba64, argb64, abgr64) plus get_r8, get_g8, get_b8, and get_a8 while they call non-constexpr Math::round. Also remove constexpr from Color::lerp because it depends on non-constexpr Math::lerp, preserving all runtime behavior until those Math helpers become constexpr.Sources: Learnings, Linters/SAST tools
🧹 Nitpick comments (1)
core/math/color.h (1)
503-650: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a single source of truth for named colors
Colorsduplicates the color table already maintained incolor_names.inc(used by the runtimenamed()lookup), by hand, as separate literal hex values. ThePaleTorquoise/PaleTurquoisedrift above is a concrete example of the risk: the two tables can silently diverge. Longer-term it may be worth generatingColorsfrom the same data drivingcolor_names.inc(or vice versa) to guarantee they stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/math/color.h` around lines 503 - 650, Update the Colors named-color definitions to use the existing color_names.inc data source, or generate both Colors and the runtime named() lookup from one shared table. Remove the duplicated hand-maintained literals while preserving the existing public constants and correct the PaleTorquoise/PaleTurquoise naming mismatch so both APIs remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/math/color.h`:
- Around line 184-189: Update both Color::operator[] overloads to avoid
constexpr evaluation through the inactive union-backed components array; either
remove constexpr from these accessors or route access through the actively
initialized {r,g,b,a} representation. Preserve mutable and const indexing
behavior.
- Around line 212-218: Remove constexpr from Color::is_equal_approx and
Color::is_same, since their Math::is_equal_approx and Math::is_same dependencies
are not constexpr. Keep both methods’ existing comparison behavior and noexcept
qualifiers unchanged.
- Around line 335-348: Remove constexpr from the srgb_to_linear() and
linear_to_srgb() methods in Color, leaving their runtime conversion logic
unchanged. Keep them non-constexpr because their Math::pow calls cannot be
evaluated in constant expressions.
---
Duplicate comments:
In `@core/math/color.h`:
- Line 608: Rename the Color constant PaleTorquoise to PaleTurquoise, matching
the spelling used by color_names.inc, the documentation, and the C# binding;
update any references to the old identifier accordingly.
- Around line 166-175: Update get_s() and get_v() to use the same nested
two-argument MIN/MAX pattern already applied in get_h(), replacing every
three-argument invocation while preserving the existing saturation and value
calculations.
- Around line 40-65: Remove constexpr from floats_to_uint32, floats_to_uint64,
and every dependent color conversion wrapper (to_rgba32, argb32, abgr32, rgba64,
argb64, abgr64) plus get_r8, get_g8, get_b8, and get_a8 while they call
non-constexpr Math::round. Also remove constexpr from Color::lerp because it
depends on non-constexpr Math::lerp, preserving all runtime behavior until those
Math helpers become constexpr.
---
Nitpick comments:
In `@core/math/color.h`:
- Around line 503-650: Update the Colors named-color definitions to use the
existing color_names.inc data source, or generate both Colors and the runtime
named() lookup from one shared table. Remove the duplicated hand-maintained
literals while preserving the existing public constants and correct the
PaleTorquoise/PaleTurquoise naming mismatch so both APIs remain synchronized.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43625570-6e2d-4709-b4b3-a484e3b71a41
📒 Files selected for processing (3)
.gitignorecore/math/color.cppcore/math/color.h
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- core/math/color.cpp
| constexpr float &operator[](const int p_idx) noexcept { | ||
| return components[p_idx]; | ||
| } | ||
| _FORCE_INLINE_ const float &operator[](int p_idx) const { | ||
| constexpr const float &operator[](const int p_idx) const noexcept { | ||
| return components[p_idx]; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Color::operator[] is unsafe in constexpr contexts for union-backed colors
operator[] reads components, but Color::hex()/Colors:: constants initialize the {r,g,b,a} member. That makes the array an inactive union member in constant evaluation, so static_assert(Colors::Orange[0] > 0.0f) will fail. Either drop constexpr here or change the storage/access pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/math/color.h` around lines 184 - 189, Update both Color::operator[]
overloads to avoid constexpr evaluation through the inactive union-backed
components array; either remove constexpr from these accessors or route access
through the actively initialized {r,g,b,a} representation. Preserve mutable and
const indexing behavior.
| constexpr bool is_equal_approx(const Color &p_color) const noexcept { | ||
| return Math::is_equal_approx(r, p_color.r) && Math::is_equal_approx(g, p_color.g) && Math::is_equal_approx(b, p_color.b) && Math::is_equal_approx(a, p_color.a); | ||
| } | ||
|
|
||
| constexpr bool is_same(const Color &p_color) const noexcept { | ||
| return Math::is_same(r, p_color.r) && Math::is_same(g, p_color.g) && Math::is_same(b, p_color.b) && Math::is_same(a, p_color.a); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant declarations/definitions.
git ls-files core/math/color.h core/math/math_funcs.h
echo
echo "== color.h (around the cited lines) =="
sed -n '200,230p' core/math/color.h
echo
echo "== math_funcs.h: is_equal_approx/is_same =="
rg -n "is_equal_approx|is_same" core/math/math_funcs.h
sed -n '1,260p' core/math/math_funcs.hRepository: Redot-Engine/redot-engine
Length of output: 9842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the macro definition and any constexpr-related config that may affect Math helpers.
rg -n "_ALWAYS_INLINE_|`#define` _ALWAYS_INLINE_|constexpr.*is_equal_approx|constexpr.*is_same" core include . -g '!**/third_party/**'
echo
echo "== likely macro definition files =="
fd -a "typedefs.h|math_defs.h|config.h|platform_config.h" core include . 2>/dev/null | head -n 20
echo
echo "== core/typedefs.h =="
sed -n '1,220p' core/typedefs.h
echo
echo "== core/math/math_defs.h =="
sed -n '1,220p' core/math/math_defs.hRepository: Redot-Engine/redot-engine
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find constexpr usages and call sites involving Color::is_equal_approx / is_same.
rg -n "is_equal_approx\s*\(|is_same\s*\(|Color::is_equal_approx|Color::is_same|constexpr.*Color" core modules scene tests . -g '!**/third_party/**' -g '!**/platform/**' | head -n 200
echo
echo "== color.h surrounding declarations =="
sed -n '1,260p' core/math/color.h
echo
echo "== math_funcs.h around the relevant helpers =="
sed -n '520,585p' core/math/math_funcs.hRepository: Redot-Engine/redot-engine
Length of output: 32381
Drop constexpr here or make the Math:: helpers constexpr first. These methods call non-constexpr helpers, so they won’t be usable in constant evaluation and can break stricter toolchains.
🧰 Tools
🪛 GitHub Check: 🏁 Windows / Editor (target=editor)
[failure] 216-216:
constexpr function 'Color::is_same' cannot result in a constant expression
[failure] 212-212:
constexpr function 'Color::is_equal_approx' cannot result in a constant expression
🪛 GitHub Check: 🏁 Windows / Template (target=template_release)
[failure] 216-216:
constexpr function 'Color::is_same' cannot result in a constant expression
[failure] 212-212:
constexpr function 'Color::is_equal_approx' cannot result in a constant expression
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/math/color.h` around lines 212 - 218, Remove constexpr from
Color::is_equal_approx and Color::is_same, since their Math::is_equal_approx and
Math::is_same dependencies are not constexpr. Keep both methods’ existing
comparison behavior and noexcept qualifiers unchanged.
Source: Linters/SAST tools
| constexpr Color srgb_to_linear() const noexcept { | ||
| return Color( | ||
| r < 0.04045f ? r * (1.0f / 12.92f) : Math::pow(float((r + 0.055) * (1.0 / (1.0 + 0.055))), 2.4f), | ||
| g < 0.04045f ? g * (1.0f / 12.92f) : Math::pow(float((g + 0.055) * (1.0 / (1.0 + 0.055))), 2.4f), | ||
| b < 0.04045f ? b * (1.0f / 12.92f) : Math::pow(float((b + 0.055) * (1.0 / (1.0 + 0.055))), 2.4f), | ||
| a); | ||
| } | ||
| _FORCE_INLINE_ Color linear_to_srgb() const { | ||
|
|
||
| constexpr Color linear_to_srgb() const noexcept { | ||
| return Color( | ||
| r < 0.0031308f ? 12.92f * r : (1.0 + 0.055) * Math::pow(r, 1.0f / 2.4f) - 0.055, | ||
| g < 0.0031308f ? 12.92f * g : (1.0 + 0.055) * Math::pow(g, 1.0f / 2.4f) - 0.055, | ||
| b < 0.0031308f ? 12.92f * b : (1.0 + 0.055) * Math::pow(b, 1.0f / 2.4f) - 0.055, a); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant declarations and implementations.
git ls-files | rg '(^|/)(color\.h|math\.(h|hpp|cpp)|core/math/|core/math)' || true
echo '--- Math::pow references ---'
rg -n --hidden --no-messages '\bMath::pow\b|\bpow\(' core include . -g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' || true
echo '--- color.h around the relevant lines ---'
file='core/math/color.h'
if [ -f "$file" ]; then
nl -ba "$file" | sed -n '300,380p'
fi
echo '--- Math header candidates ---'
for f in $(git ls-files | rg '(^|/)math\.(h|hpp|inl)$|(^|/)math/.*\.(h|hpp|inl)$'); do
echo "### $f"
nl -ba "$f" | sed -n '1,260p'
doneRepository: Redot-Engine/redot-engine
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- core/math/math_funcs.h: pow definitions ---'
nl -ba core/math/math_funcs.h | sed -n '170,215p'
echo '--- core/math/color.h: color conversion methods ---'
nl -ba core/math/color.h | sed -n '328,352p'
echo '--- any constexpr use of Math::pow in core/math ---'
rg -n --hidden --no-messages 'constexpr[^;\n]*Math::pow|Math::pow[^;\n]*constexpr' core/math tests -g '!**/build/**' || trueRepository: Redot-Engine/redot-engine
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- core/math/math_funcs.h: pow definitions ---'
awk 'NR>=170 && NR<=215 { printf "%d:%s\n", NR, $0 }' core/math/math_funcs.h
echo '--- core/math/color.h: color conversion methods ---'
awk 'NR>=328 && NR<=352 { printf "%d:%s\n", NR, $0 }' core/math/color.h
echo '--- occurrences of constexpr Math::pow ---'
rg -n --hidden --no-messages 'constexpr[^;\n]*Math::pow|Math::pow[^;\n]*constexpr' core/math tests -g '!**/build/**' || trueRepository: Redot-Engine/redot-engine
Length of output: 2557
🌐 Web query:
cppreference std::pow constexpr C++23 C++26
💡 Result:
According to official C++ standards documentation, std::pow is constexpr starting in C++26 [1][2][3]. It is not constexpr in C++23 [2][3]. While various math functions were granted constexpr support in C++23 via papers such as P0533R9, the specific expansion of constexpr support to include a broader range of mathematical functions—including std::pow—is a feature associated with C++26 [2][4]. Implementations are currently in the process of adding this support [5][6].
Citations:
- 1: https://en.cppreference.com/cpp/numeric/math/pow
- 2: https://cppreference.dev/w/cpp/numeric/math/pow
- 3: https://docs.w3cub.com/cpp/numeric/math/pow
- 4: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p1383r2.pdf
- 5: Tracking progress for constexpr cmath (P0533R9, P1383R2) llvm/llvm-project#139885
- 6: P0533R9:
constexprfor<cmath>and<cstdlib>llvm/llvm-project#105174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- C++ standard settings ---'
rg -n --hidden --no-messages 'cxxstd|c\+\+1[1-7]|c\+\+2[0-6]|std=c\+\+|std_cpp|cpp_std|CXXSTD|CXX_STANDARD' \
SConstruct SConscript *.py *.toml *.yaml *.yml *.json . -g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' | head -n 200 || true
echo '--- compiler flags around standard in build scripts ---'
for f in $(git ls-files | rg '(^|/)(SConstruct|SConscript|.*\.py|.*\.cmake|.*\.mk|.*\.ninja)$'); do
rg -n --no-messages 'c\+\+1[1-7]|c\+\+2[0-6]|std=c\+\+|CXXSTD|CXX_STANDARD|cxxstd' "$f" && echo "### $f"
done | head -n 200 || trueRepository: Redot-Engine/redot-engine
Length of output: 2729
Drop constexpr from srgb_to_linear() and linear_to_srgb()
Math::pow forwards to std::pow, and this C++20 build doesn’t allow std::pow in a constant expression. Keep these methods non-constexpr unless you replace the power calls with a compile-time implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/math/color.h` around lines 335 - 348, Remove constexpr from the
srgb_to_linear() and linear_to_srgb() methods in Color, leaving their runtime
conversion logic unchanged. Keep them non-constexpr because their Math::pow
calls cannot be evaluated in constant expressions.
|
Replaced and superseeded by #1305 |
3 participants
Make
Colorconstexpr.Declare named Colors as compile time constants (i.e.,
Colors::Orange, etc.).Used by upcoming terrain editor.
Summary by CodeRabbit
constexpr/noexceptoperations for arithmetic, conversions, blending, interpolation, and sRGB↔linear.Coloroperator!=overload.uv.lock.