add remap_default (issue 1157)#1216
Conversation
…d've done a division by zero.
WalkthroughAdds a new VariantUtilityFunctions::remap_default(value, istart, istop, ostart, ostop, default_value) that remaps a value between ranges and returns the provided default when the result is infinite or NaN. Changes include implementation, header declaration, binding registration, documentation, and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/variant/variant_utility.cpp (1)
559-564: Add targeted tests for bothremap_defaultbranches.This introduces new edge-case behavior; please add coverage for (1)
istart == istopfallback and (2) normal remap passthrough to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 559 - 564, Add unit tests covering VariantUtilityFunctions::remap_default for both branches: one test where istart == istop verifies the function returns the provided default_value for representative inputs, and another test where istart != istop verifies it delegates to Math::remap by asserting the expected remapped numeric result (include multiple cases like values inside and outside the input range). Place tests in the existing variant utility test suite and use the same numeric precision/assertion helpers used elsewhere for floating comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/variant/variant_utility.cpp`:
- Around line 559-564: Add unit tests covering
VariantUtilityFunctions::remap_default for both branches: one test where istart
== istop verifies the function returns the provided default_value for
representative inputs, and another test where istart != istop verifies it
delegates to Math::remap by asserting the expected remapped numeric result
(include multiple cases like values inside and outside the input range). Place
tests in the existing variant utility test suite and use the same numeric
precision/assertion helpers used elsewhere for floating comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd3518ed-e105-487d-bf3a-56cdccf4f01b
📒 Files selected for processing (3)
core/variant/variant_utility.cppcore/variant/variant_utility.hdoc/classes/@GlobalScope.xml
|
@Tekkitslime Would you mind adding in some unit tests for this new function? You can add the test to You have to compile the project with unit tests enabled to test it by doing |
Arctis-Fireblight
left a comment
Arctis-Fireblight
left a comment
There was a problem hiding this comment.
@Tekkitslime I agree with Jon, can you add unit tests please?
Also, I know that in the original issue, istart and istop were equal, but I am not sure if this is the only potential error condition for remap(). We might want to either add additional guards / checks, or just run the remap() function and just return the default value if we happen to hit an error condition. Thoughts?
|
There are more error conditions? like what? Maybe passing one of the arguments as NaN or Inf? Yeah i think you have a point, i should just check if remap gives a bad result and only then return the default value! |
…te, changed the documentation to reflect this change, and added a few test cases to test_variant_utility, and they work on my machine
Basically, i messed up the branches, this is the remap_default feature branch :P
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/classes/`@GlobalScope.xml:
- Around line 1117-1123: The docs for remap_default currently only mention INF
and NAN, but the implementation uses Math::is_finite() to catch any non-finite
results; update the description of remap_default to indicate it returns
default_value for any non-finite result (including -INF, +INF, and NAN) rather
than only INF/NAN, and optionally use the term "non-finite" for clarity; refer
to remap_default, remap, and Math::is_finite() when editing the text so the
behavior matches the core/variant handling.
In `@tests/core/variant/test_variant_utility.h`:
- Around line 134-147: Run clang-format on the modified test block for the
TEST_CASE_TEMPLATE named "[VariantUtility] remap_default" (the checks calling
VariantUtilityFunctions::remap_default) and commit the resulting formatted
changes so pre-commit no longer rewrites this section; ensure the committed
change exactly matches the CI's expected formatting output for that test block.
- Line 35: tests/core/variant/test_variant_utility.h now includes
"core/math/math_defs.h" which the build cannot resolve; either remove the
unnecessary include from test_variant_utility.h (if math_defs.h is unused) or
correct the include to the project's canonical header path and/or add the
directory to the test target's include paths so the compiler can find
core/math/math_defs.h; locate the include line in test_variant_utility.h and
either delete it or update it and the target's include settings so the header
resolves.
- Around line 139-146: Add a test for the collapsed-range NaN branch where value
== istart == istop so the fallback default is returned: add a CHECK calling
VariantUtilityFunctions::remap_default(100.0, 100.0, 100.0, 0.0, 1000.0, -99.0)
and assert it equals doctest::Approx(-99.0); this covers the
core/math/math_funcs.h collapsed-range (istart==istop) path referenced around
lines 500-512.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7490bf0c-d174-41da-8316-a66bd4baddad
📒 Files selected for processing (3)
core/variant/variant_utility.cppdoc/classes/@GlobalScope.xmltests/core/variant/test_variant_utility.h
🚧 Files skipped from review as they are similar to previous changes (1)
- core/variant/variant_utility.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/core/variant/test_variant_utility.h (3)
134-147:⚠️ Potential issue | 🟡 MinorCommit the clang-format output for this block.
CI is still reporting that pre-commit rewrites this test case, so the branch will keep failing until the formatted result is committed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/variant/test_variant_utility.h` around lines 134 - 147, Run clang-format on the test block in tests/core/variant/test_variant_utility.h (the TEST_CASE_TEMPLATE "[VariantUtility] remap_default" that calls VariantUtilityFunctions::remap_default) and commit the formatted result so pre-commit no longer rewrites it; ensure the file is re-saved with clang-format changes applied and include that updated file in the commit.
35-35:⚠️ Potential issue | 🔴 CriticalFix the unresolved include before merging.
"core/math/math_defs.h"is still not resolvable from this test target, so the file does not compile as patched. Either remove this dependency or switch to a header path that is actually available to the tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/variant/test_variant_utility.h` at line 35, The test header tests/core/variant/test_variant_utility.h currently includes the unresolved header "core/math/math_defs.h"; either remove that include if the math definitions are not needed, replace it with a header that is actually exported to the test target (e.g., the public math header used elsewhere in tests), or add the appropriate dependency so "core/math/math_defs.h" is available to the test target; update the include line or test target configuration accordingly and re-run compilation to confirm the unresolved include is resolved.
139-139:⚠️ Potential issue | 🟡 MinorAdd the
value == istart == istopcollapsed-range case.Line 139 covers the
value != istart && istart == istopbranch, butcore/math/math_funcs.h:500-512also producesNaNfor the0/0case whenvalue == istart == istop.core/variant/variant_utility.cpp:560-567should return the fallback there as well, so this path still needs explicit coverage.Proposed test addition
CHECK(VariantUtilityFunctions::remap_default(150.0, 100.0, 100.0, 0.0, 1000.0, -99.0) == doctest::Approx(-99.0)); + CHECK(VariantUtilityFunctions::remap_default(100.0, 100.0, 100.0, 0.0, 1000.0, -99.0) == doctest::Approx(-99.0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/variant/test_variant_utility.h` at line 139, The remap_default logic must handle the collapsed-range 0/0 case when value == istart == istop to avoid NaN: update VariantUtilityFunctions::remap_default (in variant_utility.cpp around the existing istart==istop handling at lines ~560-567) to explicitly detect istart == istop and value == istart and return the provided fallback value, and add a unit test in tests/core/variant/test_variant_utility.h mirroring the existing check (e.g., call remap_default with value, istart, istop all equal and expect the fallback) so the collapsed-range equal-value path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/core/variant/test_variant_utility.h`:
- Around line 134-147: Run clang-format on the test block in
tests/core/variant/test_variant_utility.h (the TEST_CASE_TEMPLATE
"[VariantUtility] remap_default" that calls
VariantUtilityFunctions::remap_default) and commit the formatted result so
pre-commit no longer rewrites it; ensure the file is re-saved with clang-format
changes applied and include that updated file in the commit.
- Line 35: The test header tests/core/variant/test_variant_utility.h currently
includes the unresolved header "core/math/math_defs.h"; either remove that
include if the math definitions are not needed, replace it with a header that is
actually exported to the test target (e.g., the public math header used
elsewhere in tests), or add the appropriate dependency so
"core/math/math_defs.h" is available to the test target; update the include line
or test target configuration accordingly and re-run compilation to confirm the
unresolved include is resolved.
- Line 139: The remap_default logic must handle the collapsed-range 0/0 case
when value == istart == istop to avoid NaN: update
VariantUtilityFunctions::remap_default (in variant_utility.cpp around the
existing istart==istop handling at lines ~560-567) to explicitly detect istart
== istop and value == istart and return the provided fallback value, and add a
unit test in tests/core/variant/test_variant_utility.h mirroring the existing
check (e.g., call remap_default with value, istart, istop all equal and expect
the fallback) so the collapsed-range equal-value path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 102310bf-c258-4293-b5ca-2fcb2798881d
📒 Files selected for processing (1)
tests/core/variant/test_variant_utility.h
There was a problem hiding this comment.
♻️ Duplicate comments (3)
doc/classes/@GlobalScope.xml (1)
1117-1122:⚠️ Potential issue | 🟡 MinorDocument this as “non-finite,” not just
INF/NAN.The implementation falls back on any non-finite result, so this description is still too narrow:
-INFalso returnsdefault_value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/classes/`@GlobalScope.xml around lines 1117 - 1122, Update the documentation for remap_default to state that it returns default_value when the remap would produce a non-finite result (including INF, -INF, or NAN) instead of only mentioning INF/NAN; keep the existing examples and cross-references to remap, lerp, inverse_lerp and clamp, but replace the narrow wording with the term “non-finite” so readers understand negative infinity is also covered.tests/core/variant/test_variant_utility.h (2)
35-35:⚠️ Potential issue | 🔴 CriticalFix the unresolved
math_defs.hinclude.Line 35 still adds a header this test target can't resolve, so the suite does not compile. Remove it if it's unnecessary here, or switch to a header that is already available on this target's include path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/variant/test_variant_utility.h` at line 35, The test target fails to compile because tests/core/variant/test_variant_utility.h includes an unresolved header "core/math/math_defs.h"; remove that include from test_variant_utility.h if the test does not need it, or replace it with a header that is present on this test target's include path (i.e., use an existing public math header already available to the tests) so the unresolved symbol math_defs.h is no longer referenced.
139-146:⚠️ Potential issue | 🟡 MinorAdd the collapsed-range
NaNcase too.The current block covers the
istart == istop && value != istartpath, but it still missesvalue == istart == istop, which goes through the0 / 0branch and should also return the fallback.➕ Suggested test
CHECK(VariantUtilityFunctions::remap_default(150.0, 100.0, 100.0, 0.0, 1000.0, -99.0) == doctest::Approx(-99.0)); + CHECK(VariantUtilityFunctions::remap_default(100.0, 100.0, 100.0, 0.0, 1000.0, -99.0) == doctest::Approx(-99.0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/variant/test_variant_utility.h` around lines 139 - 146, Add a test covering the collapsed-range where value == istart == istop and those are NaN so the function hits the 0/0 branch and should return the fallback; update tests in tests/core/variant/test_variant_utility.h by adding a CHECK calling VariantUtilityFunctions::remap_default with value, istart and istop all set to NAN (and the same fallback -99.0) and assert it equals doctest::Approx(-99.0).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/classes/`@GlobalScope.xml:
- Around line 1117-1122: Update the documentation for remap_default to state
that it returns default_value when the remap would produce a non-finite result
(including INF, -INF, or NAN) instead of only mentioning INF/NAN; keep the
existing examples and cross-references to remap, lerp, inverse_lerp and clamp,
but replace the narrow wording with the term “non-finite” so readers understand
negative infinity is also covered.
In `@tests/core/variant/test_variant_utility.h`:
- Line 35: The test target fails to compile because
tests/core/variant/test_variant_utility.h includes an unresolved header
"core/math/math_defs.h"; remove that include from test_variant_utility.h if the
test does not need it, or replace it with a header that is present on this test
target's include path (i.e., use an existing public math header already
available to the tests) so the unresolved symbol math_defs.h is no longer
referenced.
- Around line 139-146: Add a test covering the collapsed-range where value ==
istart == istop and those are NaN so the function hits the 0/0 branch and should
return the fallback; update tests in tests/core/variant/test_variant_utility.h
by adding a CHECK calling VariantUtilityFunctions::remap_default with value,
istart and istop all set to NAN (and the same fallback -99.0) and assert it
equals doctest::Approx(-99.0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a3ffce3-1f10-4cd5-96b6-5c20f155c1a2
📒 Files selected for processing (4)
core/variant/variant_utility.cppcore/variant/variant_utility.hdoc/classes/@GlobalScope.xmltests/core/variant/test_variant_utility.h
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/variant/test_variant_utility.h (1)
134-134: Unused template parameterT.The template parameter
Tis declared but never used in the test body—all literals aredouble. This causes the test to run twice with identical behavior. Sinceremap_defaultacceptsdoubleparameters (per implementation invariant_utility.cpp), consider simplifying to a plainTEST_CASE.♻️ Suggested simplification
-TEST_CASE_TEMPLATE("[VariantUtility] remap_default", T, float, double) { +TEST_CASE("[VariantUtility] remap_default") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/variant/test_variant_utility.h` at line 134, The TEST_CASE_TEMPLATE declaration uses an unused template parameter T causing duplicate identical tests; change the test from TEST_CASE_TEMPLATE("[VariantUtility] remap_default", T, float, double) to a non-templated TEST_CASE so it only runs once and matches remap_default's double parameters (update the test declaration in tests/core/variant/test_variant_utility.h and remove any unused T references so the test body calls remap_default(double) as implemented in variant_utility.cpp).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/variant/test_variant_utility.h`:
- Line 134: The TEST_CASE_TEMPLATE declaration uses an unused template parameter
T causing duplicate identical tests; change the test from
TEST_CASE_TEMPLATE("[VariantUtility] remap_default", T, float, double) to a
non-templated TEST_CASE so it only runs once and matches remap_default's double
parameters (update the test declaration in
tests/core/variant/test_variant_utility.h and remove any unused T references so
the test body calls remap_default(double) as implemented in
variant_utility.cpp).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0f7e46b-5e52-4e78-8c66-447021d01b07
📒 Files selected for processing (1)
tests/core/variant/test_variant_utility.h
3 participants
This commit adds
remap_default, which has an extra argument that is returned ifistart == istop, that is, if there is a division by zero inremap, it returns the default argument.Summary by CodeRabbit
New Features
Documentation
Tests