constexpr math functions#1305
Open
OldDev78 wants to merge 8 commits into
Open
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 PR modernizes math and color APIs with C++20 templates, ChangesMath modernization and image loading
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ImageLoaderTinyEXR
participant FileAccess
participant TinyEXRDecoder
ImageLoaderTinyEXR->>FileAccess: read file bytes
ImageLoaderTinyEXR->>TinyEXRDecoder: pass std::span buffer
TinyEXRDecoder->>ImageLoaderTinyEXR: return decoded image result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
c005cb0 to
8f3de80
Compare
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
f1dd03c to
1da8121
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
modules/tinyexr/image_loader_tinyexr.h (1)
48-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the new public buffer-entry precondition.
The core dispatcher validates
p_imagebefore invoking loaders, but direct callers ofload_image_from_buffer()bypass that check. A nullRef<Image>will later be dereferenced byset_data(); add the same invalid-parameter guard here.🤖 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 `@modules/tinyexr/image_loader_tinyexr.h` at line 48, Add an invalid-parameter guard at the start of load_image_from_buffer() that rejects a null p_image before any buffer processing or set_data() call, matching the core dispatcher’s validation and returning the established invalid-parameter error.core/math/math_funcs.h (2)
46-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstrain
sintostd::floating_pointto matchcos.
cos(Line 51) is constrained tostd::floating_point, butsinaccepts anytypename T. An integer instantiationsin(int)would callstd::sin(int)and truncate the double result back toint. Aligning the constraint prevents accidental integer instantiation and keeps the pair consistent.♻️ Proposed change
-template <typename T> -_ALWAYS_INLINE_ T sin(T p_x) { +template <std::floating_point T> +_ALWAYS_INLINE_ T sin(T p_x) { return std::sin(p_x); }🤖 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 46 - 52, Constrain the `sin` template in `math_funcs.h` with `std::floating_point`, matching the existing constraint on `cos`. Leave its implementation and return behavior unchanged while preventing integer instantiations.
604-614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMisleading
constexpron helpers that depend on non-constexprfunctions.
linear_to_db/db_to_linearare markedconstexprbut calllog/exp, which forward tostd::log/std::exp(notconstexprin C++20). The same applies to otherconstexprhelpers here that rely onfloor/fmod/exp(e.g.fract,wrapf,sigmoid_affine). These can never be constant-evaluated, so theconstexpris aspirational and technically IFNDR. Consider droppingconstexpruntil the underlying functions areconstexpr-capable, or gate the constexpr path.Based on learnings: don't leave
constexprhelpers that depend on non-constexprfunctions; either avoid the misleadingconstexpror add a safe alternative/gating.🤖 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 604 - 614, Remove the misleading constexpr qualifiers from linear_to_db, db_to_linear, and the nearby helpers that call non-constexpr math functions such as log, exp, floor, or fmod, including fract, wrapf, and sigmoid_affine. Keep their existing runtime behavior and noexcept/template interfaces unchanged unless required by the qualifier removal.Source: Learnings
🤖 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/basis.cpp`:
- Line 128: Update the determinant comparison in is_rotation() to explicitly use
UNIT_EPSILON rather than the default Math::is_equal_approx tolerance, preserving
acceptance of slightly drifted valid rotation bases while retaining the existing
conformality check.
In `@core/math/color.h`:
- Line 608: Rename the public Colors constant PaleTorquoise to PaleTurquoise
while preserving its existing hex value and constexpr definition.
In `@core/math/math_funcs.h`:
- Around line 197-226: Update ipow() to handle negative exponents before
converting exp to unsigned long long: return the reciprocal of ipow(base, -exp),
or otherwise ensure negative values follow the correct pow() semantics. Preserve
the existing zero, one, two, three, and exponentiation-by-squaring fast paths
for non-negative exponents, while safely avoiding overflow when negating the
minimum long long value.
- Around line 386-392: Update the std::is_constant_evaluated() branch to apply
the T(0.5) factor to the entire sum of 2*p_from, a*p_weight, b*w2, and c*w3,
matching the runtime return expression while leaving the runtime path unchanged.
- Around line 103-107: Add the standard <algorithm> header to math_funcs.h so
the clamp template can directly use std::min and std::max. Keep the existing
clamp implementation and behavior unchanged.
In `@core/math/transform_interpolator.cpp`:
- Line 295: Update the determinant validation in the transform interpolation
logic around the is_equal_approx call to retain the previous explicit 0.01f
tolerance, rather than using the default CMP_EPSILON-based overload. Preserve
the existing slerp-versus-INTERP_LERP behavior while allowing the same small
determinant drift as before.
In `@core/typedefs.h`:
- Around line 402-411: Add direct standard-library includes for the concepts and
type-traits facilities used by the unsigned_integral, signed_integral, and
arithmetic concepts in core/typedefs.h. Include <concepts> for std::integral and
<type_traits> for the type-trait checks, without relying on platform_config.h or
other transitive includes.
In `@editor/scene/3d/node_3d_editor_plugin.cpp`:
- Around line 7959-7961: Update the fade-limit calculations near fade_size to
cast division_level_min and division_level_max with static_cast<real_t> before
passing them to Math::pow, matching fsteps and preserving double-precision
builds.
In `@editor/scene/canvas_item_editor_plugin.cpp`:
- Around line 486-487: Update both Math::pow calls in the grid_output.x and
grid_output.y assignments to use real_t for the base as well as the exponent,
replacing 2.f with a real_t-compatible value such as static_cast<real_t>(2).
Preserve the existing snapping calculations and offsets.
In `@modules/tinyexr/image_loader_tinyexr.cpp`:
- Around line 292-300: In ImageLoaderTinyEXR::load_image, capture the byte count
returned by f->get_buffer and reject the input with ERR_FILE_CORRUPT unless it
equals src_image_len. Perform this validation before constructing the buffer
span or invoking TinyEXR decoding, so truncated reads never expose uninitialized
Vector data.
In `@scene/resources/animation.cpp`:
- Line 4579: Update the Math::pow call in the precision calculation to cast the
base literal 0.1 to real_t, matching the existing cast of p_precision and
ensuring both arguments use the same type.
---
Nitpick comments:
In `@core/math/math_funcs.h`:
- Around line 46-52: Constrain the `sin` template in `math_funcs.h` with
`std::floating_point`, matching the existing constraint on `cos`. Leave its
implementation and return behavior unchanged while preventing integer
instantiations.
- Around line 604-614: Remove the misleading constexpr qualifiers from
linear_to_db, db_to_linear, and the nearby helpers that call non-constexpr math
functions such as log, exp, floor, or fmod, including fract, wrapf, and
sigmoid_affine. Keep their existing runtime behavior and noexcept/template
interfaces unchanged unless required by the qualifier removal.
In `@modules/tinyexr/image_loader_tinyexr.h`:
- Line 48: Add an invalid-parameter guard at the start of
load_image_from_buffer() that rejects a null p_image before any buffer
processing or set_data() call, matching the core dispatcher’s validation and
returning the established invalid-parameter error.
🪄 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: 5f925578-f9aa-4c09-b933-26f948548805
📒 Files selected for processing (37)
.gitignorecore/math/basis.cppcore/math/color.cppcore/math/color.hcore/math/geometry_2d.cppcore/math/math_defs.hcore/math/math_funcs.cppcore/math/math_funcs.hcore/math/projection.cppcore/math/quaternion.cppcore/math/transform_interpolator.cppcore/math/vector2.cppcore/math/vector3.hcore/os/time.cppcore/typedefs.heditor/animation/animation_track_editor_plugins.cppeditor/audio/editor_audio_buses.cppeditor/gui/editor_toaster.cppeditor/import/3d/resource_importer_scene.heditor/scene/2d/tiles/tile_map_layer_editor.cppeditor/scene/3d/node_3d_editor_plugin.cppeditor/scene/canvas_item_editor_plugin.cppeditor/settings/editor_autoload_settings.cppmodules/gridmap/editor/grid_map_editor_plugin.cppmodules/tinyexr/image_loader_tinyexr.cppmodules/tinyexr/image_loader_tinyexr.hscene/3d/spline_ik_3d.cppscene/gui/color_picker.cppscene/gui/color_picker_shape.cppscene/gui/graph_edit.cppscene/gui/range.cppscene/gui/rich_text_label.cppscene/main/canvas_item.cppscene/resources/3d/primitive_meshes.cppscene/resources/animation.cppscene/resources/packed_scene.cppservers/rendering/renderer_rd/effects/copy_effects.cpp
Comment on lines
+197
to
226
| constexpr T ipow(T base, long long exp) noexcept { | ||
| // TODO: Should we handle negatives? | ||
| T res = T(1); | ||
| T b = base; | ||
| unsigned long long e = static_cast<unsigned long long>(exp); | ||
|
|
||
| if (exp == 0) { | ||
| return res; | ||
| } | ||
| if (exp == 1) { | ||
| return b; | ||
| } | ||
| if (exp == 2) { | ||
| return b * b; | ||
| } | ||
| if (exp == 3) { | ||
| return b * b * b; | ||
| } | ||
|
|
||
| // exponentiation by squaring. | ||
| while (e != 0) { | ||
| if (e & 1ULL) { | ||
| res *= b; | ||
| } | ||
| b *= b; | ||
| e >>= 1ULL; | ||
| } | ||
|
|
||
| return res; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the header.
FILE="core/math/math_funcs.h"
wc -l "$FILE"
sed -n '170,260p' "$FILE"
# Find the pow overloads/usages nearby.
rg -n "constexpr .*pow|ipow\(" "$FILE"Repository: Redot-Engine/redot-engine
Length of output: 2165
Guard the constexpr integer fast-path against negative exponents
pow() sends any integral y into ipow(), including negative values like -2.0. ipow() only handles non-negative exponents, so the cast to unsigned long long turns that into a huge power and produces the wrong constexpr result. Add a y >= 0 check here or handle negatives via 1 / ipow(x, -exp).
🤖 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 197 - 226, Update ipow() to handle
negative exponents before converting exp to unsigned long long: return the
reciprocal of ipow(base, -exp), or otherwise ensure negative values follow the
correct pow() semantics. Preserve the existing zero, one, two, three, and
exponentiation-by-squaring fast paths for non-negative exponents, while safely
avoiding overflow when negating the minimum long long value.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/math/math_funcs.h (1)
48-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConstrain
Math::sinto floating-point types.
Match thestd::floating_pointconstraint already used byMath::cos; otherwiseMath::sin(1)deducesT = intand returns0after truncating thestd::sinresult.🤖 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 48 - 50, Constrain the Math::sin template to std::floating_point types, matching the existing constraint on Math::cos, so integral arguments such as Math::sin(1) are rejected instead of producing a truncated result.
🤖 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.
Outside diff comments:
In `@core/math/math_funcs.h`:
- Around line 48-50: Constrain the Math::sin template to std::floating_point
types, matching the existing constraint on Math::cos, so integral arguments such
as Math::sin(1) are rejected instead of producing a truncated result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54adeb9f-cfed-4565-b0ab-7cfa16e5f294
📒 Files selected for processing (2)
core/math/math_funcs.hcore/typedefs.h
🚧 Files skipped from review as they are similar to previous changes (1)
- core/typedefs.h
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
1da8121 to
0b59167
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/core/math/test_math_funcs.h`:
- Line 562: Update the Math::wrapi test case to use the fixed-width int64_t
template type and an LL-suffixed 300-billion input literal, preserving the
existing bounds and expected result.
🪄 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: 8c691266-d65b-48dc-9b07-6ce62ac194a0
📒 Files selected for processing (7)
core/math/color.hcore/math/math_funcs.hcore/typedefs.hplatform/macos/tts_macos.mmtests/core/math/test_math_funcs.htests/core/templates/test_list.htests/scene/test_code_edit.h
🚧 Files skipped from review as they are similar to previous changes (3)
- core/typedefs.h
- core/math/color.h
- core/math/math_funcs.h
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
0b59167 to
b7cf6d9
Compare
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
ffe36a1 to
e161094
Compare
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
e161094 to
807034d
Compare
mcdubhghlas
previously approved these changes
Jul 14, 2026
Shakai-Dev
requested changes
Jul 14, 2026
Shakai-Dev
left a comment
Shakai-Dev
left a comment
Member
There was a problem hiding this comment.
Overall LGTM, only these need to be addressed
Closed
OldDev78
force-pushed
the
constexpr-math-funcs
branch
from
4097d48 to
b41268c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes some math functions to templates, and a few of them
constexpras well.Includes some changes, such as making Color
constexprand support for TinyEXR read into a std::span buffer, both of which are requirements for the terrain editor module.Superseeds #1297
Summary by CodeRabbit
constexpr/noexceptbehavior.ColorAPI by removing multiple packing/HSV/utility helpers and updating related conversions.uv.lock).