mujoco: Add support for Universal and Screw Joints#980
Conversation
- Support universal joints in the MuJoCo plugin by creating two hinge joints in series and two actuators. - Add basic state integration tests and out-of-bounds DOF safety checks across MuJoCo, Dartsim, and Bullet-Featherstone wrappers. Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
- Support screw joints in the MuJoCo physics engine plugin using coupled slide/hinge joints and equality constraints. - Make the rotational hinge joint the primary axis in the MuJoCo plugin, fully aligning its coordinate units (radians) with DARTsim. - Add a new anchored screw joint model with static damping in the test world. - Add a unified screw joint integration test suite inside joint_features, validating kinematic coupling and damping decay. Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
…DF strings - Revert test.world back to its pristine state to ensure all other engine tests (dartsim, tpe) expecting exactly 9 models pass cleanly. - Migrate Universal joint checks in SDFFeatures_TEST to load a self-contained SDF string. - Migrate Screw joint coupling and damping checks in joint_features to load a self-contained SDF string. Generated-by: Gemini 3.0 Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
- Verify screw joint position limits enforcement across DARTsim and MuJoCo plugins. - Enforce position limits purely on the translational Slide joint to allow exact kinematic constraint coupling without solver damping. Generated-by: Gemini 3.0 Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
caguero
left a comment
There was a problem hiding this comment.
Nice videos! I left a few comments to consider.
| const Identity &_id, const std::size_t _dof) const | ||
| { | ||
| const auto *joint = this->ReferenceInterface<JointInfo>(_id); | ||
| if (_dof >= this->GetJointDegreesOfFreedom(_id)) |
There was a problem hiding this comment.
I just noticed that this pattern is slightly different than the one in GetJointVelocity(), where you're checking the identifier before this block. Here, you're checking it after this block.
I think a fixed joint will return NAN pose and 0 velocity. Maybe that's the intention but just checking in.
Also, line 104-105 comment implies that a fixed joint will return 0. I think now it'll return NAN.
There was a problem hiding this comment.
Okay, there seems to be a mismatch between what GetJointDegreesOfFreedom returns and what is an acceptable input value for _dof in bullet-featherstone. GetJointDegreesOfFreedom returns zero for RootJoint and FixedConstraintJoint joint types, but GetJointVelocity returns a valid value for RootJoint types and nan for FixedConstraintJoint while GetJointPosition returns 0 for both. In dartsim, the free joint at the root of the kinematic tree (model) is not exposed to the gz-physics API. Thus, there no way GetJointPosition or GetJointVelocity would be called on a free joint, but this doesn't appear to be the case in bullet-featherstone. I don't want to go down a rabbit hole in this PR so I'm inclined to revert all the changes in bullet-featherstone and skip it in the test.
| joint->limited = false; | ||
| } | ||
|
|
||
| joint2 = mjs_addJoint(child, nullptr); |
There was a problem hiding this comment.
Just checking: Adding the two joints together will make GetDegreesOfFreedom() return 2 instead of 1?
There was a problem hiding this comment.
Good catch! I added a test in ca30b39 and it failed. I ended up storing the number of equality constraints in JointInfo as you suggested in your review below and used that to subtract from the DOF computed by MuJoCo.
| { | ||
| auto *m = jointInfo->worldInfo->mjModelObj; | ||
| int jntId = m->dof_jntid[jointInfo->nv_index]; | ||
| for (int i = 0; i < m->neq; ++i) |
There was a problem hiding this comment.
I wonder if we could get rid of this loop by catching the information that you need in JointInfo and reading it from here directly instead of iterating. Besides the performance gain, we'd get rid of almost the same block in SetJointVelocity().
| const auto *identifier = std::get_if<InternalJoint>(&joint->identifier); | ||
| if (identifier) | ||
| { | ||
| if (_dof >= this->GetJointDegreesOfFreedom(_id)) |
There was a problem hiding this comment.
This block is repeated many times. Could we perhaps move it to its own function similar to Mujoco's ValidateDofParam()? Same for DART.
| // data[1] * (hinge_pos - hinge_ref) | ||
| // where data[1] = pitch (meters/rad) = ScrewThreadPitch/2pi | ||
| eq->data[0] = 0.0; | ||
| eq->data[1] = sdfJoint->ScrewThreadPitch() / (2.0 * GZ_PI); |
There was a problem hiding this comment.
This expression appears in the same function above. I wonder if you could declare a single const and use it in both places.
There was a problem hiding this comment.
Since the function is valid only when the joint is a screw joint, I've instead created a converter function that deduplicates the unit conversion. 8f4e576
Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
- Add an eqIndices vector to JointInfo to track equality constraint indices compiled in the mjModel. - Resolve and store associated equality constraint indices during model compilation in resolveJointIndices. - Dynamically resolve active degrees of freedom by subtracting eqIndices.size() from the physical joints compiled on the child body in GetJointDegreesOfFreedom. - Refactor simultaneous position and velocity updates to utilize eqIndices and updateCoupledSecondaryJoint helper to avoid code duplication. - Safely ensure Gazebo API queries are always backed by the actual compiled MuJoCo solver state, preventing out-of-bounds coordinate access. Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
- Add convertScrewThreadPitch helper function to convert SDFormat screw thread pitch (m/rev) to MuJoCo coordinate ratio (m/rad). - Document screw thread pitch conversion helper along with physical units. - Deduplicate pitch conversion logic in Slide limits and equality constraint ratio calculations. Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
- Replace manual index assignments with robust C++ std::fill zero-initialization. Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
caguero
left a comment
There was a problem hiding this comment.
Minor comment, ready to approve after it.
| for (int eqId : jointInfo->eqIndices) | ||
| { | ||
| double pitch = m->eq_data[eqId * mjNEQDATA + 1]; | ||
| _mjDataArray[_baseIndex + 1] = _value * pitch; |
There was a problem hiding this comment.
Just checking: Is it OK to update the same _mjDataArray index at every pass of the loop?
There was a problem hiding this comment.
No. Honestly, I missed this in my review of AI generated code. The original idea was, since we potentially have multiple equality constraints for implementing additional joint types (e.g. mimic joints), we'd put the constraints in a vector. But for that, the baseindex would need to be updated for each time in the loop.
After going ahead and implementing mimic joints, it actually seems better to track the equality constraints separately. So in 8c9388a, a couple of variables are added to JointInfo for this purpose. I've also renamed this function to updateScrewJointFollower to be more explicit that this is just for screw joints only.
looks like the DOF checks were reverted in bullet-featherstone and handled differently in test. Curious, what's the reason? |
ah just saw the reason in #980 (comment) |
Replace implicit body ID comparisons and search loops with a high-performance pointer-based constraint tracking scheme that stores direct mjsEquality* pointers and caches the resolved screwEqIndex in O(1) time. Generated-by: Gemini 3.0 Pro Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
iche033
left a comment
There was a problem hiding this comment.
looks good to me, just minor suggestions
Signed-off-by: Addisu Z. Taddese <addisuzt@intrinsic.ai>
🎉 New feature
Part of #299
Summary
This pull request adds support for Universal and Screw joint types inside the MuJoCo physics engine plugin
Changes Made:
Video of a universal joint
Screen.Recording.2026-05-12.at.15.31.28.mov
SDF file: https://gist.github.com/azeey/ec80bf54f326becd1869400bf1d7edf6#file-universal_joint_demo-sdf
Video of Screw Joint
Screen.Recording.2026-05-13.at.15.33.20.mov
SDF file: https://gist.github.com/azeey/ec80bf54f326becd1869400bf1d7edf6#file-screw_joint_demo-sdf
Test it
Checklist
codecheckpassed (See contributing)Generated-by: Gemini 3.0
Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.Backports: If this is a backport, please use Rebase and Merge instead.