GH-854 Add Monotonic Cubic Interpolation#1224
Open
JoltedJon wants to merge 9 commits intoRedot-Engine:masterfrom
Open
GH-854 Add Monotonic Cubic Interpolation#1224JoltedJon wants to merge 9 commits intoRedot-Engine:masterfrom
JoltedJon wants to merge 9 commits intoRedot-Engine:masterfrom
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds monotonic Hermite-based cubic interpolation across Math, vector/quaternion types, Variant bindings, Animation runtime and editor UI; introduces INTERPOLATION_CUBIC_MONOTONIC and INTERPOLATION_CUBIC_MONOTONIC_ANGLE, time- and angle-aware variants, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor (Track UI)
participant Animation as Animation Runtime
participant Type as Vector/Quaternion
participant Math as Math::monotonic_cubic*
participant Variant as Variant Bindings
Editor->>Animation: select/insert key (MONOTONIC)
Animation->>Type: _interpolate(dispatch INTERPOLATION_CUBIC_MONOTONIC)
Type->>Math: call monotonic_cubic_interpolate(_in_time / _angle)
Math-->>Type: per-component interpolated values
Type-->>Animation: composed Vector/Quaternion result
Animation-->>Variant: expose result to scripting / export
Variant-->>Editor: script-accessible interpolation APIs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/animation/animation_track_editor.cpp (1)
3092-3117:⚠️ Potential issue | 🔴 CriticalAdd icon handling for the new interpolation enums.
These modes are now selectable, but the draw path at Lines 2315-2321 and Line 2395 still uses a 5-entry
interp_iconlookup. That no longer matches the interpolation enum, so monotonic tracks will show the wrong icon or read past the array during redraw.Suggested fix
- Ref<Texture2D> interp_icon[5] = { - get_editor_theme_icon(SNAME("InterpRaw")), - get_editor_theme_icon(SNAME("InterpLinear")), - get_editor_theme_icon(SNAME("InterpCubic")), - get_editor_theme_icon(SNAME("InterpLinearAngle")), - get_editor_theme_icon(SNAME("InterpCubicAngle")), - }; @@ - Ref<Texture2D> icon = interp_icon[interp_mode]; + Ref<Texture2D> icon; + switch (interp_mode) { + case Animation::INTERPOLATION_NEAREST: + icon = get_editor_theme_icon(SNAME("InterpRaw")); + break; + case Animation::INTERPOLATION_LINEAR: + icon = get_editor_theme_icon(SNAME("InterpLinear")); + break; + case Animation::INTERPOLATION_CUBIC: + case Animation::INTERPOLATION_CUBIC_MONOTONIC: + icon = get_editor_theme_icon(SNAME("InterpCubic")); + break; + case Animation::INTERPOLATION_LINEAR_ANGLE: + icon = get_editor_theme_icon(SNAME("InterpLinearAngle")); + break; + case Animation::INTERPOLATION_CUBIC_ANGLE: + case Animation::INTERPOLATION_CUBIC_MONOTONIC_ANGLE: + icon = get_editor_theme_icon(SNAME("InterpCubicAngle")); + break; + default: + icon = get_editor_theme_icon(SNAME("InterpRaw")); + break; + }Also applies to: 3568-3571
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/animation/animation_track_editor.cpp` around lines 3092 - 3117, The draw code still uses a 5-entry interp_icon lookup which no longer matches the expanded interpolation enum (new monotonic entries), causing wrong icons/UB; update the interp_icon array and any switch/lookup in the drawing code to include icons for MENU_INTERPOLATION_CUBIC_MONOTONIC and MENU_INTERPOLATION_CUBIC_MONOTONIC_ANGLE (and their angle/non-angle counterparts), and adjust any index mapping logic so each interpolation enum value maps to the correct icon (update references to interp_icon and the drawing functions that read interpolation enum values). Ensure you add the corresponding get_editor_theme_icon calls for the new monotonic variants and verify the lookup size matches the enum range so no out-of-bounds access occurs.scene/resources/animation.h (1)
62-70:⚠️ Potential issue | 🔴 CriticalDo not renumber existing
InterpolationTypeenum values.Adding
INTERPOLATION_CUBIC_MONOTONICbefore existing entries changes legacy numeric IDs (notablyINTERPOLATION_LINEAR_ANGLEandINTERPOLATION_CUBIC_ANGLE). That can break backward compatibility for serialized animation tracks and runtime interpolation selection.🔧 Proposed fix
enum InterpolationType : uint8_t { INTERPOLATION_NEAREST, INTERPOLATION_LINEAR, INTERPOLATION_CUBIC, - INTERPOLATION_CUBIC_MONOTONIC, INTERPOLATION_LINEAR_ANGLE, INTERPOLATION_CUBIC_ANGLE, + INTERPOLATION_CUBIC_MONOTONIC, INTERPOLATION_CUBIC_MONOTONIC_ANGLE, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scene/resources/animation.h` around lines 62 - 70, The enum InterpolationType was modified in a way that shifts numeric IDs (specifically INTERPOLATION_LINEAR_ANGLE and INTERPOLATION_CUBIC_ANGLE); restore backward compatibility by ensuring existing values retain their original numeric IDs—either move INTERPOLATION_CUBIC_MONOTONIC to the end of the enum list or add explicit integer assignments for each enum entry so INTERPOLATION_NEAREST, INTERPOLATION_LINEAR, INTERPOLATION_CUBIC, INTERPOLATION_LINEAR_ANGLE, and INTERPOLATION_CUBIC_ANGLE keep their previous numeric values; update the InterpolationType definition accordingly to avoid changing legacy serialized IDs.
🧹 Nitpick comments (2)
core/math/quaternion.cpp (1)
269-368: Extract the shared spherical-cubic core.These two overloads duplicate the full phase-alignment and dual-expmap flow from
spherical_cubic_interpolate{_in_time}. A small helper that takes the scalar interpolation op would keep future fixes to the quaternion math in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/math/quaternion.cpp` around lines 269 - 368, Both spherical_monotonic_cubic_interpolate and spherical_monotonic_cubic_interpolate_in_time duplicate the phase-alignment, flip handling, dual ExpMap and slerp blending logic; extract that shared flow into a single helper (e.g., spherical_monotonic_cubic_interpolate_core) that accepts a scalar interpolation callable (std::function or template parameter) and any optional time arguments, so the two public methods call the core with either Math::monotonic_cubic_interpolate or Math::monotonic_cubic_interpolate_in_time; update the core to perform Basis conversions, flip adjustments (from_q, pre_q, to_q, post_q), compute ln vectors in both from_q and to_q spaces, call the provided scalar interpolator for x/y/z, build q1/q2 and return q1.slerp(q2, p_weight), keeping the public function names spherical_monotonic_cubic_interpolate and spherical_monotonic_cubic_interpolate_in_time as thin wrappers.scene/resources/animation.cpp (1)
6566-6754: Extract the shared Variant cubic dispatcher before these two implementations drift.
Animation::monotonic_cubic_interpolate_in_time_variantis effectively a second copy ofAnimation::cubic_interpolate_in_time_variant. Any future type fix now has to be mirrored across another very large switch and array-walking block. I'd pull the shared traversal into a helper and inject the cubic vs monotonic primitives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scene/resources/animation.cpp` around lines 6566 - 6754, The two large switch+array traversal blocks in Animation::monotonic_cubic_interpolate_in_time_variant and Animation::cubic_interpolate_in_time_variant should be refactored into a single helper (e.g., Animation::variant_cubic_dispatcher) that accepts a strategy flag or function pointers for the primitive scalar/vector interpolation routines (e.g., Math::cubic_interpolate_in_time vs Math::monotonic_cubic_interpolate_in_time and the instance methods like Vector2::monotonic_cubic_interpolate_in_time / cubic_interpolate_in_time, Quaternion::spherical_monotonic_cubic_interpolate_in_time / spherical_cubic_interpolate_in_time). Move the shared array handling and type-switch logic into that helper and have the two public methods call it with the appropriate primitive functions; ensure you preserve the special-casing (numeric cast_to_blendwise/cast_from_blendwise, string fallback, PACKED_BYTE_ARRAY skip, p_snap_array_element behavior) by passing any needed flags through the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/math/math_funcs.h`:
- Around line 947-960: The function monotonic_cubic_interpolate_angle_in_time
has the timestamp parameters in the wrong order (p_pre_t, p_to_t, p_post_t)
which mismatches the established _in_time API; change the signature to accept (T
p_to_t, T p_pre_t, T p_post_t) and update the call to
monotonic_cubic_interpolate_in_time(...) to pass the timestamps in the same
order expected by that function (p_weight, p_to_t, p_pre_t, p_post_t) so pre/to
timestamps are not swapped; ensure the parameter names in the function
declaration/definition and any callers use the corrected order to preserve API
consistency with cubic_interpolate_angle_in_time and VariantUtilityFunctions.
In `@core/variant/variant_utility.cpp`:
- Around line 548-550: The wrapper
VariantUtilityFunctions::monotonic_cubic_interpolate_angle_in_time forwards time
arguments to Math::monotonic_cubic_interpolate_angle_in_time in the wrong order,
swapping to_t and pre_t; fix it by calling
Math::monotonic_cubic_interpolate_angle_in_time(from, to, pre, post, weight,
pre_t, to_t, post_t) so that the forwarded times match Math's expected (p_pre_t,
p_to_t, p_post_t) parameter order.
In `@tests/core/math/test_math_funcs.h`:
- Around line 694-700: The test block is calling
Math::cubic_interpolate_angle_in_time instead of the intended
Math::monotonic_cubic_interpolate_angle_in_time; update all five CHECK lines to
call Math::monotonic_cubic_interpolate_angle_in_time (keeping the same argument
order) so the monotonic helper is exercised, then re-run tests and update the
expected doctest::Approx values to the correct baselines produced by the
monotonic implementation.
---
Outside diff comments:
In `@editor/animation/animation_track_editor.cpp`:
- Around line 3092-3117: The draw code still uses a 5-entry interp_icon lookup
which no longer matches the expanded interpolation enum (new monotonic entries),
causing wrong icons/UB; update the interp_icon array and any switch/lookup in
the drawing code to include icons for MENU_INTERPOLATION_CUBIC_MONOTONIC and
MENU_INTERPOLATION_CUBIC_MONOTONIC_ANGLE (and their angle/non-angle
counterparts), and adjust any index mapping logic so each interpolation enum
value maps to the correct icon (update references to interp_icon and the drawing
functions that read interpolation enum values). Ensure you add the corresponding
get_editor_theme_icon calls for the new monotonic variants and verify the lookup
size matches the enum range so no out-of-bounds access occurs.
In `@scene/resources/animation.h`:
- Around line 62-70: The enum InterpolationType was modified in a way that
shifts numeric IDs (specifically INTERPOLATION_LINEAR_ANGLE and
INTERPOLATION_CUBIC_ANGLE); restore backward compatibility by ensuring existing
values retain their original numeric IDs—either move
INTERPOLATION_CUBIC_MONOTONIC to the end of the enum list or add explicit
integer assignments for each enum entry so INTERPOLATION_NEAREST,
INTERPOLATION_LINEAR, INTERPOLATION_CUBIC, INTERPOLATION_LINEAR_ANGLE, and
INTERPOLATION_CUBIC_ANGLE keep their previous numeric values; update the
InterpolationType definition accordingly to avoid changing legacy serialized
IDs.
---
Nitpick comments:
In `@core/math/quaternion.cpp`:
- Around line 269-368: Both spherical_monotonic_cubic_interpolate and
spherical_monotonic_cubic_interpolate_in_time duplicate the phase-alignment,
flip handling, dual ExpMap and slerp blending logic; extract that shared flow
into a single helper (e.g., spherical_monotonic_cubic_interpolate_core) that
accepts a scalar interpolation callable (std::function or template parameter)
and any optional time arguments, so the two public methods call the core with
either Math::monotonic_cubic_interpolate or
Math::monotonic_cubic_interpolate_in_time; update the core to perform Basis
conversions, flip adjustments (from_q, pre_q, to_q, post_q), compute ln vectors
in both from_q and to_q spaces, call the provided scalar interpolator for x/y/z,
build q1/q2 and return q1.slerp(q2, p_weight), keeping the public function names
spherical_monotonic_cubic_interpolate and
spherical_monotonic_cubic_interpolate_in_time as thin wrappers.
In `@scene/resources/animation.cpp`:
- Around line 6566-6754: The two large switch+array traversal blocks in
Animation::monotonic_cubic_interpolate_in_time_variant and
Animation::cubic_interpolate_in_time_variant should be refactored into a single
helper (e.g., Animation::variant_cubic_dispatcher) that accepts a strategy flag
or function pointers for the primitive scalar/vector interpolation routines
(e.g., Math::cubic_interpolate_in_time vs
Math::monotonic_cubic_interpolate_in_time and the instance methods like
Vector2::monotonic_cubic_interpolate_in_time / cubic_interpolate_in_time,
Quaternion::spherical_monotonic_cubic_interpolate_in_time /
spherical_cubic_interpolate_in_time). Move the shared array handling and
type-switch logic into that helper and have the two public methods call it with
the appropriate primitive functions; ensure you preserve the special-casing
(numeric cast_to_blendwise/cast_from_blendwise, string fallback,
PACKED_BYTE_ARRAY skip, p_snap_array_element behavior) by passing any needed
flags through the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a60ef8ef-5981-48e7-9a61-c26f77ab2797
📒 Files selected for processing (15)
core/math/math_funcs.hcore/math/quaternion.cppcore/math/quaternion.hcore/math/vector2.hcore/math/vector3.hcore/math/vector4.cppcore/math/vector4.hcore/variant/variant_call.cppcore/variant/variant_utility.cppcore/variant/variant_utility.heditor/animation/animation_track_editor.cppeditor/animation/animation_track_editor.hscene/resources/animation.cppscene/resources/animation.htests/core/math/test_math_funcs.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
doc/classes/Vector4.xml (1)
99-122: Optional: clarify monotonicity is per-component.This avoids readers assuming path-level monotonicity for the whole vector.
Wording tweak
- Performs a monotonic cubic interpolation between this vector and [param b] using [param pre_a] and [param post_b] as handles, and returns the result at position [param weight]. + Performs a per-component monotonic cubic interpolation between this vector and [param b] using [param pre_a] and [param post_b] as handles, and returns the result at position [param weight].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/classes/Vector4.xml` around lines 99 - 122, Update the descriptions for the methods monotonic_cubic_interpolate and monotonic_cubic_interpolate_in_time in Vector4 documentation to explicitly state that "monotonic" is applied per-component (each vector component is interpolated independently), so readers don't assume the interpolation guarantees monotonicity along the whole vector path; mention that the monotonicity guarantee applies component-wise rather than across the combined vector trajectory.tests/core/math/test_math_funcs.h (1)
672-700: Add an invariant-style regression for “no overshoot”.The current assertions are mostly snapshot values. Adding a boundedness check against keyframe endpoints would better lock in the issue
#854guarantee.Proposed test addition
+TEST_CASE_TEMPLATE("[Math] monotonic_cubic_interpolate no_overshoot_regression", T, float, double) { + const T v1 = Math::monotonic_cubic_interpolate((T)90.0, (T)0.0, (T)0.0, (T)0.0, (T)0.25); + const T v2 = Math::monotonic_cubic_interpolate((T)90.0, (T)0.0, (T)0.0, (T)0.0, (T)0.5); + const T v3 = Math::monotonic_cubic_interpolate((T)90.0, (T)0.0, (T)0.0, (T)0.0, (T)0.75); + + CHECK(v1 <= (T)90.0); + CHECK(v1 >= (T)0.0); + CHECK(v2 <= (T)90.0); + CHECK(v2 >= (T)0.0); + CHECK(v3 <= (T)90.0); + CHECK(v3 >= (T)0.0); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/math/test_math_funcs.h` around lines 672 - 700, Add invariant "no overshoot" checks to the existing tests for monotonic_cubic_interpolate_angle, monotonic_cubic_interpolate_in_time, and monotonic_cubic_interpolate_angle_in_time: for each call already made in those TEST_CASE_TEMPLATE blocks, add an assertion that the returned value is bounded between the start and end keyframe values (use min(start,end) and max(start,end)); for the angle variants compute the shortest-interval normalized endpoints (or unwrap so start/end define the intended interval) and assert the interpolated angle lies within that unwrapped interval; use the existing doctest CHECK macros (e.g. CHECK(result >= min && result <= max)) so the tests enforce no-overshoot invariants in addition to the snapshot assertions.
🤖 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/Quaternion.xml`:
- Around line 219-233: Update the method descriptions to use quaternion
terminology: in the <method name="spherical_monotonic_cubic_interpolate"> and
<method name="spherical_monotonic_cubic_interpolate_in_time"> descriptions,
replace the phrase “this vector” with “this quaternion” so the docs consistently
refer to quaternions (e.g., change both occurrences inside those <description>
tags).
---
Nitpick comments:
In `@doc/classes/Vector4.xml`:
- Around line 99-122: Update the descriptions for the methods
monotonic_cubic_interpolate and monotonic_cubic_interpolate_in_time in Vector4
documentation to explicitly state that "monotonic" is applied per-component
(each vector component is interpolated independently), so readers don't assume
the interpolation guarantees monotonicity along the whole vector path; mention
that the monotonicity guarantee applies component-wise rather than across the
combined vector trajectory.
In `@tests/core/math/test_math_funcs.h`:
- Around line 672-700: Add invariant "no overshoot" checks to the existing tests
for monotonic_cubic_interpolate_angle, monotonic_cubic_interpolate_in_time, and
monotonic_cubic_interpolate_angle_in_time: for each call already made in those
TEST_CASE_TEMPLATE blocks, add an assertion that the returned value is bounded
between the start and end keyframe values (use min(start,end) and
max(start,end)); for the angle variants compute the shortest-interval normalized
endpoints (or unwrap so start/end define the intended interval) and assert the
interpolated angle lies within that unwrapped interval; use the existing doctest
CHECK macros (e.g. CHECK(result >= min && result <= max)) so the tests enforce
no-overshoot invariants in addition to the snapshot assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f79e7c0-d82b-4f2c-b2ff-3c76cf1f03ee
📒 Files selected for processing (7)
core/math/math_funcs.hdoc/classes/@GlobalScope.xmldoc/classes/Quaternion.xmldoc/classes/Vector2.xmldoc/classes/Vector3.xmldoc/classes/Vector4.xmltests/core/math/test_math_funcs.h
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/core/math/test_math_funcs.h (2)
658-670: Add a held-segment regression to the scalar helper.The monotonic logic in
core/math/math_funcs.h:832-878matters most when a flatp_from == p_tosegment would otherwise inherit slope from its neighbors. This block still only covers a rising segment plus extrapolation, so it doesn't lock the no-overshoot case behind GH-854.Example regression to add
+ CHECK(Math::monotonic_cubic_interpolate((T)0.0, (T)0.0, (T)1.0, (T)1.0, (T)0.25) == doctest::Approx((T)0.0)); + CHECK(Math::monotonic_cubic_interpolate((T)0.0, (T)0.0, (T)1.0, (T)1.0, (T)0.5) == doctest::Approx((T)0.0)); + CHECK(Math::monotonic_cubic_interpolate((T)0.0, (T)0.0, (T)1.0, (T)1.0, (T)0.75) == doctest::Approx((T)0.0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/math/test_math_funcs.h` around lines 658 - 670, The test suite needs a held-segment regression: add cases to the TEST_CASE_TEMPLATE for Math::monotonic_cubic_interpolate that exercise a flat segment (p_from == p_to) so the scalar helper locks a zero slope instead of inheriting neighbor slopes; then update the monotonic cubic logic in core/math/math_funcs.h (function Math::monotonic_cubic_interpolate / the scalar helper in that file) to detect p_from == p_to and set the segment derivatives to zero (and handle extrapolation correctly) so flat segments do not acquire nonzero slopes from neighbors or produce overshoot.
694-700: Mirror the held-angle case in_angle_in_time.The plain
_angleblock above already protects a flat0 -> 0segment with non-zero neighbors, but this time-aware variant only checks a smooth increasing arc. Since the animation regression is about playback on flat segments, adding the same plateau case here would make this the real regression guard.Example regression to add
+ CHECK(Math::monotonic_cubic_interpolate_angle_in_time((T)0.0, (T)0.0, (T)(Math::PI / 2.0), (T)(Math::PI / 2.0), (T)0.25, (T)0.5, (T)0.0, (T)1.0) == doctest::Approx((T)0.0)); + CHECK(Math::monotonic_cubic_interpolate_angle_in_time((T)0.0, (T)0.0, (T)(Math::PI / 2.0), (T)(Math::PI / 2.0), (T)0.5, (T)0.5, (T)0.0, (T)1.0) == doctest::Approx((T)0.0)); + CHECK(Math::monotonic_cubic_interpolate_angle_in_time((T)0.0, (T)0.0, (T)(Math::PI / 2.0), (T)(Math::PI / 2.0), (T)0.75, (T)0.5, (T)0.0, (T)1.0) == doctest::Approx((T)0.0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/math/test_math_funcs.h` around lines 694 - 700, monotonic_cubic_interpolate_angle_in_time is missing the plateau/held-angle guard that _angle_in_time has; add the same early-return case to detect a flat segment (e.g., when start and end angles produce a zero delta across the interval while neighbors are non-zero) and return the held angle so playback on flat segments remains stable; update the logic in monotonic_cubic_interpolate_angle_in_time (the time-aware interpolation routine) to mirror the held-angle protection present in _angle_in_time so 0->0 segments with non-zero neighbors are preserved.
🤖 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/math/test_math_funcs.h`:
- Around line 658-670: The test suite needs a held-segment regression: add cases
to the TEST_CASE_TEMPLATE for Math::monotonic_cubic_interpolate that exercise a
flat segment (p_from == p_to) so the scalar helper locks a zero slope instead of
inheriting neighbor slopes; then update the monotonic cubic logic in
core/math/math_funcs.h (function Math::monotonic_cubic_interpolate / the scalar
helper in that file) to detect p_from == p_to and set the segment derivatives to
zero (and handle extrapolation correctly) so flat segments do not acquire
nonzero slopes from neighbors or produce overshoot.
- Around line 694-700: monotonic_cubic_interpolate_angle_in_time is missing the
plateau/held-angle guard that _angle_in_time has; add the same early-return case
to detect a flat segment (e.g., when start and end angles produce a zero delta
across the interval while neighbors are non-zero) and return the held angle so
playback on flat segments remains stable; update the logic in
monotonic_cubic_interpolate_angle_in_time (the time-aware interpolation routine)
to mirror the held-angle protection present in _angle_in_time so 0->0 segments
with non-zero neighbors are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85dee939-3bf7-45ee-8c46-9a8ad30cfec3
⛔ Files ignored due to path filters (1)
editor/icons/InterpMonotonic.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
editor/animation/animation_track_editor.cpptests/core/math/test_math_funcs.h
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/animation/animation_track_editor.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
doc/classes/Animation.xml (1)
744-746: Add the angle-mode normalization note for consistency.Line 744 introduces another
*_ANGLEinterpolation type, but unlike Line 733-740 it doesn’t mention normalization behavior. If behavior matches other angle interpolations, documenting that caveat here avoids confusion.📘 Suggested doc tweak
<constant name="INTERPOLATION_CUBIC_MONOTONIC_ANGLE" value="6" enum="InterpolationType"> Monotonic Cubic interpolation with shortest path rotation. + [b]Note:[/b] The result value is always normalized and may not match the key value. </constant>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/classes/Animation.xml` around lines 744 - 746, Add the same angle-normalization note to the INTERPOLATION_CUBIC_MONOTONIC_ANGLE constant documentation: state that this interpolation uses shortest-path rotation and input/output angles are normalized (e.g., wrapped to -180..180 or consistent internal angle range) prior to interpolation, mirroring the wording used for the other *_ANGLE entries; update the <constant name="INTERPOLATION_CUBIC_MONOTONIC_ANGLE"> description to include that caveat so behavior is consistent with the other angle interpolation docs.
🤖 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 792-829: The descriptions for
monotonic_cubic_interpolate_angle_in_time and
monotonic_cubic_interpolate_in_time incorrectly imply the interpolation
factor/parameter is derived from the time arguments; update both method
descriptions to state that the interpolation factor ("weight") is provided by
the caller and that the time parameters (to_t, pre_t, post_t) are used only to
normalize/shape the interpolation and compute time-aware tangents to preserve
monotonicity and prevent overshoot when keyframes are unevenly spaced.
---
Nitpick comments:
In `@doc/classes/Animation.xml`:
- Around line 744-746: Add the same angle-normalization note to the
INTERPOLATION_CUBIC_MONOTONIC_ANGLE constant documentation: state that this
interpolation uses shortest-path rotation and input/output angles are normalized
(e.g., wrapped to -180..180 or consistent internal angle range) prior to
interpolation, mirroring the wording used for the other *_ANGLE entries; update
the <constant name="INTERPOLATION_CUBIC_MONOTONIC_ANGLE"> description to include
that caveat so behavior is consistent with the other angle interpolation docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13f324dc-6a25-4f72-8c00-274cb643f332
⛔ Files ignored due to path filters (1)
editor/icons/InterpMonotonic.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
doc/classes/@GlobalScope.xmldoc/classes/Animation.xmldoc/classes/Vector2.xmldoc/classes/Vector3.xmldoc/classes/Vector4.xmlscene/resources/animation.h
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
2 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.
Adds a Monotonic version of Cubic Interpolation. resolves #854
Cubic Interpolation is prone to overshooting during animations. This new feature allows for animations to be smooth but also doesn't overshoot the key frames.
The new function keeps the curve monotonic which means that the first derivative of the curve keeps the same sign, which cubic interpolation does not honor.
Here is the difference between Cubic and Monotonic Cubic for a curve going from 0 -> 90 -> 0 -> 0 -> 90.
screenrecording-2026-03-13_00-31-00.mp4
Summary by CodeRabbit