Fix COM frame composition in mjCBody::AccumulateInertia#3250
Conversation
|
|
||
| // Merged COM must be correct when a fused-static child has a non-identity | ||
| // inertial-frame orientation (e.g. from a degenerate diaginertia). | ||
| TEST_F(UserObjectsTest, FuseStaticWithRotatedInertial) { |
There was a problem hiding this comment.
Hi I tried to run this test and it passes even before the bugfix, could we please modify the test such that it fails before the fix and it passes afterwards? Thanks!
There was a problem hiding this comment.
Hi @quagla thanks for the feedback.
I've re-checked on a clean worktree of current main (2b67fe3) with only the test cherry-picked in (no source change) -- test fails for me with body_ipos = {0, 0, 0.5} vs expected {0.5, 0, 0}. Cherry-picking the source change from this PR makes it pass.
Could the source change have still been applied when you tested? Or is there a different testing approach I should take?
For reference, I'm building and running with:
cmake -B build -DMUJOCO_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release .
cmake --build build --target user_objects_test -j$(nproc)
./build/bin/user_objects_test --gtest_filter="UserObjectsTest.FuseStaticWithRotatedInertial"Platform:
- Linux x86_64 (WSL2), gcc default, Release mode, googletest from the repo's
_deps/
There was a problem hiding this comment.
Something probably went wrong the previous time I tested it. I confirm it fails for me too!
There was a problem hiding this comment.
Awesome -- glad we're on same page, and thanks again for the timely review and approval.
At this point are we just waiting for review from @yuvaltassa? Just checking in to make sure y'all aren't waiting on me for something that I'm not realizing.
|
|
||
| // Merged COM must be correct when a fused-static child has a non-identity | ||
| // inertial-frame orientation (e.g. from a degenerate diaginertia). | ||
| TEST_F(UserObjectsTest, FuseStaticWithRotatedInertial) { |
There was a problem hiding this comment.
Something probably went wrong the previous time I tested it. I confirm it fails for me too!
|
Hi @yuvaltassa @quagla, thanks again for the reviews. I am noticing the |
3 participants
mjCBody::AccumulateInertiacallsmjuu_frameaccumwith the body pose and inertial offset arguments swapped, composingbody_ipose ∘ body_poseinstead of the documentedbody_pose ∘ body_ipose. The merged center-of-mass is wrong whenever the body pose and inertial offset don't commute as transforms.Most commonly triggered by URDF fixed-joint children with degenerate-eigenvalue inertia (typical of capsule/cylinder approximations), where
mjuu_fullInertiareturns a non-trivial principal-axes rotation.Distinct from #2982, which patched the inertia-tensor rotation in the same function. #2982's reproduction (child with
pos = 0) collapses both arg orders to the same answer, so it could not have caught this.Added test fails before the fix with merged
body_ipos = (0, 0, 0.5)and passes after with(0.5, 0, 0).