mdl: make Joint::clamp terminate for any finite q - #105
Open
webertob wants to merge 1 commit into
Open
Conversation
Joint::clamp normalises a wraparound joint by repeated subtraction:
while (q(i) > this->max(i)) { q(i) -= range; }
That has no termination guarantee. Once |q| is large enough that q - range == q
in double precision the value stops changing and the loop never exits - on
finite input, with an ordinary range of 2*pi. A range of zero does the same
immediately.
This is reachable from the iterative solvers. JacobianInverseKinematics::solve
calls Metric::step, which calls Joint::step, which calls clamp, and a solve that
diverges can produce a large iterate. The loop sits below solve()'s own
iteration and duration checks, so neither bounds it: the calling thread spins
with no error and no way out. In an application embedding the library, this
presents as a freeze with one core pinned.
Normalise with fmod instead. It is one step for any magnitude and cannot spin.
Inputs fmod cannot express - a non-finite q, or a degenerate range - fall
through to the existing clamp, so an infinity lands on the limit and a NaN stays
a NaN for the caller to notice.
The result is unchanged for inputs where the old loop terminated: both map q
into [min, max] by whole multiples of the range.
There was a problem hiding this comment.
Pull request overview
This PR updates rl::mdl::Joint::clamp to avoid potential non-termination when normalizing wraparound joints by replacing repeated subtraction/addition loops with a std::fmod-based normalization, and falling back to rl::std17::clamp for non-finite inputs or degenerate ranges.
Changes:
- Add
<cmath>and switch wraparound normalization to usestd::fmodfor constant-time wrapping. - Add guards to avoid
fmodfor non-finiteqor zero/degenerate ranges, falling back torl::std17::clamp. - Add explanatory in-code documentation describing the previous infinite-loop failure mode and the new behavior.
Suppressed comments (1)
src/rl/mdl/Joint.cpp:95
fmod(q - min, range)maps exact multiples ofrangeto 0, which makesq(i) == max(i)(andq(i) == max(i) + k*range) normalize tomin(i)instead of staying atmax(i)as the previous while-loop implementation did. This is a behavioral change for continuous revolute joints (e.g., [-pi, pi]) and contradicts the PR description that results are unchanged when the old loop terminated.
::rl::math::Real wrapped = ::std::fmod(q(i) - this->min(i), range);
if (wrapped < 0)
{
wrapped += range;
}
q(i) = this->min(i) + wrapped;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+71
to
+73
| ::rl::math::Real range = this->wraparound(i) | ||
| ? ::std::abs(this->max(i) - this->min(i)) | ||
| : ::rl::math::Real(0); |
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
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.
Joint::clamp normalises a wraparound joint by repeated subtraction:
Neither loop has a termination guarantee. Once
|q|is large enough thatq - range == qin double precision the value stops changing and the loop never exits — on finite input, with an ordinary range of 2π. Arangeof zero does the same immediately.Why it is reachable
JacobianInverseKinematics::solve→Metric::step→Joint::step→Joint::clamp. A solve that diverges can produce a large iterate, and the loop sits belowsolve()s own iteration and duration checks, so neither bounds it. The calling thread spins with no error and no way to interrupt it.We hit this embedding the library in a Unity plugin: entering play mode froze the editor with one core pinned, no exception and nothing logged. A debugger put the thread here:
The change
Normalise with
fmod— one step for any magnitude, cannot spin. Inputsfmodcannot express, a non-finiteqor a degenerate range, fall through to the existingrl::std17::clamp, so an infinity lands on the limit and a NaN stays a NaN for the caller to notice.For inputs where the old loop terminated the result is unchanged: both map
qinto[min, max]by whole multiples of the range.Happy to adjust style or split the guard from the normalisation if you would prefer them separate.