Skip to content

Stop link transform drift on repeated SetDOFValues with bodyTransform#1555

Open
Puttichai wants to merge 5 commits into
productionfrom
fix_link_drift_20260526
Open

Stop link transform drift on repeated SetDOFValues with bodyTransform#1555
Puttichai wants to merge 5 commits into
productionfrom
fix_link_drift_20260526

Conversation

@Puttichai

@Puttichai Puttichai commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

KinBody::SetDOFValues(joints, bodyTransform, checklimits) previously set link[0] to baseLinkTransform directly and then propagated a relative tbase = baseLinkTransform * link[0].inverse() to link[i > 0]. Because link[0] was always exact, tbase carried the same FP epsilon on every call and that epsilon was re-applied to link[i > 0] each time, so link transforms drift on repeated identical calls, including the no-op case where the same values are passed back in.

This affects callers that round-trip configuration values for a body, notably SetConfigurationValues and any environment-synchronization path that mirrors a body's state into a clone every cycle. The drift is small per call but compounds.

The fix delegates to _SetTransformNoPostProcess, which applies the same tapply to all links including link[0]. Link[0] then absorbs the same epsilon as the other links, so on the next call tapply ~ I − epsilon and the drift cancels rather than compounds.

Notes

  • For DOF > 0 bodies, link[0] is no longer set to baseLinkTransform exactly (it now goes through tapply * link[0]_old, picking up one ULP). The error is bounded (that is, it does not compound) and the subsequent SetDOFValues(vJointValues, checklimits) recomputes link[i>0] via forward kinematics.
  • No API change. No behavior change for single-link bodies beyond the bounded one-ULP shift noted above.

Related links

@Puttichai
Puttichai marked this pull request as ready for review May 27, 2026 02:08
Comment thread src/libopenrave/kinbody.cpp Outdated
// tapply to every link (including link[0]) lets link[0] absorb the same epsilon, so the next
// call's tapply is approximately (identity - epsilon) and the drift cancels rather than
// compounds. For DOF>0 bodies the subsequent SetDOFValues recomputes link transforms via FK
// and overwrites this loop's output, so the change only affects 0-DOF multi-link bodies.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// For DOF>0 bodies the subsequent SetDOFValues recomputes link transforms via FK and overwrites this loop's output, so the change only affects 0-DOF multi-link bodies.

I think "the change" may make sense in the PR context where we are seeing old and new code side-by-side. However, once merged into production, this comment may not be clear what "the cahnge" refers to. Can you rephrase to something which makes sense without knowing the old code?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for checking. I updated the comment.

@kanbouchou

Copy link
Copy Markdown
Collaborator

Thanks, looks good to me.

@rdiankov

Copy link
Copy Markdown
Owner

To tell you the truth, it looks like _SetTransformNoPostProcess has the drift problem since now links[0] essentially get assigned (baseLinkTransform * links[0].transform.inverse() * links[0].transform), rather than just baseLinkTransform.
Something is weird here.. do you have an example of this in action?

@Puttichai

Copy link
Copy Markdown
Collaborator Author

@rdiankov Yes, and that's also noted in the description.

do you have an example of this in action?

Yes. You can find an example on the issue page https://tiny.mujin.co.jp/iqggf
Another working example would be the test case itself (https://tiny.mujin.co.jp/e4vee)

@Puttichai
Puttichai requested a review from rdiankov July 7, 2026 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants