fix(mjcf): propagate fixed-base root body pos/quat and inertia - #2934
Open
isaka1022 wants to merge 2 commits into
Open
fix(mjcf): propagate fixed-base root body pos/quat and inertia#2934isaka1022 wants to merge 2 commits into
isaka1022 wants to merge 2 commits into
Conversation
MjcfGraph::parseRootTree calls MjcfVisitor::addRootJoint without the root body's placement when the base is fixed (no <joint> on the root body). The underlying UrdfVisitor::addRootJoint always places the root frame at the identity, which is correct for URDF (whose root link has no pos/quat) but drops the MJCF root body's own pos/quat. Since child bodies are placed relative to their parent frame's placement, this also silently offset the whole kinematic tree. Base::addRootJoint's frame creation also folds the root body's inertia into the universe inertia at that same identity placement, so the frame fix alone left the inertia stranded at the origin while the frame moved to its real pose. Fix is confined to the MJCF layer: MjcfVisitor::addRootJoint now takes the root body's placement and, only on the fixed-base path (no user-supplied root_joint), overwrites the frame Base::addRootJoint just created and re-applies its inertia contribution transformed by that placement, the same way appendBodyToJoint does for other fixed bodies. The root_joint-override path is untouched. Fixes stack-of-tasks#2782.
Contributor
There was a problem hiding this comment.
👋 Hi,
This is a reminder message to assign an extra build label to this Pull Request if needed.
By default, this PR will be build with minimal build options (URDF support and Python bindings)
The possible extra labels are:
- build_collision (build Pinocchio with coal support)
- build_casadi (build Pinocchio with CasADi support)
- build_autodiff (build Pinocchio with CppAD support)
- build_codegen (build Pinocchio with CppADCodeGen support)
- build_extra (build Pinocchio with extra algorithms)
- build_mpfr (build Pinocchio with Boost.Multiprecision support)
- build_sdf (build Pinocchio with SDF parser)
- build_accelerate (build Pinocchio with APPLE Accelerate framework support)
- build_all (build Pinocchio with ALL the options stated above)
Thanks.
The Pinocchio development team.
Restoring the fixed root body's placement exposed that sites were placed with the body's pose relative to its parent body, while frames are registered relative to their supporting joint. The two only agree when the body hangs off that joint's origin, which used to be guaranteed for a fixed root because its frame was the identity. Sites of a jointless body nested under another jointless body were already misplaced before this series. Build the root body frame directly on the fixed-base path rather than letting Base::addRootJoint create it and patching it afterwards. This keeps the placement and the inertia consistent and leaves the mass > 0 guard in Model::addFrame in charge of folding the inertia in, which a zero-mass root body relies on.
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.
Description
Fixes #2782.
When the first body under
<worldbody>has no<joint>(fixed base), its ownpos/quatwas dropped, so the whole kinematic tree ended up at the wrong place.MjcfGraph::parseRootTreecallsMjcfVisitor::addRootJointwithout the root body's placement, and that call delegates toUrdfVisitor::addRootJoint, which places the root body frame at the identity and folds its inertia into the universe inertia untransformed. That is correct for URDF, where the root link has no pos/quat of its own, but MJCF lets the root body carry one.MjcfVisitor::addRootJointnow takes the root body's placement and, on the fixed-base path, builds the body frame itself instead of calling the base and patching the result afterwards. That keeps the placement and the inertia consistent from the start, and leaves themass > 0guard insideModel::addFramein charge of whether the inertia is folded in at all. The path taken when aroot_jointis supplied is untouched.UrdfVisitor::addRootJointis shared with the URDF and SDF parsers and is not modified.Sites of jointless bodies
Moving the root body exposed a second bug, which this PR also fixes. Sites were placed with
SE3 placement = bodyPose * site.sitePlacement;where
bodyPoseis the body's placement relative to its parent body, while the frame is registered relative to its supporting joint. Those two agree only when the body it hangs off sits at that joint's origin, which used to be guaranteed for a fixed root because the root frame was the identity.This is reachable without any of the above: a jointless body nested under another jointless body already misplaces its sites today. Using the body frame's placement fixes both cases.
Known limitations, not addressed here
root_jointis supplied, the root body'spos/quatis still ignored. Neither path honoured it before, so this PR makes the two asymmetric. For a floating base the placement arguably belongs in the reference configuration rather than in the frame, and I did not want to guess at that inside a bug fix.parseContactInformationhas the same convention mismatch as the site code:equality/connectandequality/weldanchors are body-local in MJCF but are used directly as joint-relative (mjcf-graph.cpp:1400and:1408). Aweldwhosebody2is a fixed root at a non-zeropostherefore gets an anchor off by thatposonce this PR takes effect, and a jointless body nested under another one is already affected today. I left it alone because the fix has to decide what orientation the constraint frame should take, and unlike the placements above I could not find a way to check that against MuJoCo directly. Happy to take it on here, or in a follow-up, if you tell me which convention you want.How this was tested
Reproduced against the released 4.1.0, then rebuilt from source with the fix. Values are
data.oMf[...].translationafterframesForwardKinematicsat the neutral configuration, andmodel.inertias[0].lever, for a root body atpos="1 2 3"rotated 90° about z with one child atpos="0 0 0.5".baseframelink1frame[0, 0, 0][0, 0, 0.5][0, 0, 0][1, 2, 3][1, 2, 3.5][1, 2, 3]mj_forward,xpos/xipos)[1, 2, 3][1, 2, 3.5][1, 2, 3]Rotation is dropped the same way before the fix (identity instead of the 90° z rotation) and matches after it. Collision geometry placements follow the corrected frames.
For the site fix, a jointless child at
pos="0 0 0.4"under that root, carrying a site atpos="0 0.1 0": MuJoCo puts the site at[0.9, 2, 3.4], and its position expressed in the parent body is[0, 0.1, 0]. The released 4.1.0 gets the relative position right only because it puts the whole subtree at the origin; with the root placement restored and the site code left alone it becomes[0.346, -1, 0.2]. With this PR both the absolute and the relative position match MuJoCo.A root body with
mass="0"and a non-zerodiaginertialeaves the universe inertia at zero, matching whatModel::addFramedoes elsewhere.Every expected value in the new test was cross-checked against
mujoco'smj_forward. Full suite:ctest200/200 passed.pixi run lintpasses with no changes.computeTotalMassstill excludes the fixed root body's mass, since the universe inertia is not part of the total. That behaviour is unchanged by this PR.Checklist
pre-commit run --all-filesorpixi run lint