-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Factor out path joint-space jump check #2506
Conversation
e86c571
to
0ce7a22
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2506 +/- ##
==========================================
+ Coverage 50.32% 50.38% +0.07%
==========================================
Files 391 391
Lines 31983 31992 +9
==========================================
+ Hits 16091 16117 +26
+ Misses 15892 15875 -17
☔ View full report in Codecov by Sentry. |
Ah, I think MTC is failing in CI because of the API change. Will probably need to keep the old |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Just a couple of questions and nits and then this is ready to be merged
Description
This change factors out the path joint-space jump detector of
CartesianInterpolator
into a separate function calledhasJointSpaceJumps()
, so that it can be used in a different context.The new function returns an
std::optional<int>
with the path index where the jump happens, or nothing if there are no jumps.The
CartesianInterpolator::checkJointSpaceJump
function is then updated to use the free function underneath, and truncate the path, etc, i.e., it keeps the same behavior.In addition to this, there's two API-breaking changes:
CartesianInterpolator::checkRelativeJointSpaceJump
andCartesianInterpolator::checkAbsoluteJointSpaceJump
, since these can be easily achieved with the more generalCartesianInterpolator::checkJointSpaceJump
, therefore exposing a smaller API footprint.JumpThreshold
struct with named builder methods (disabled()
,relative()
andabsolute()
) instead of public constructors. This should be more readable and less error-prone.Migrating old code to the new API should be straight-forward:
JumpThreshold()
->JumpThreshold::disabled()
JumpThreshold(2.0)
->JumpThreshold::relative(2.0)
JumpThreshold(1.0, 2.0)
->JumpThreshold::absolute(1.0, 2.0)
Finally, there's a few cosmetic changes in documentation and a semantic rename
traj
->path
, since these methods operate on paths (sequence ofRobotState
, as opposed toRobotTrajectory
, which should take timing into account).Closes #2272.
Checklist