Fix MJScene body position pinned at reference pose on mass change#1477
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b42c3fa59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
schaubh
requested changes
Jul 12, 2026
8b42c3f to
14a273e
Compare
schaubh
approved these changes
Jul 12, 2026
schaubh
left a comment
Contributor
There was a problem hiding this comment.
Good to go.
Maybe update the PR description to reflect the final solution? The description still says “two behaviour-preserving performance improvements,” but only the pointer-caching improvement remains. Change “two … improvements” to “a … improvement.”
…ange mj_setConst leaves data->qpos at the model reference pose. Restore the integrator state (and clear the const-stale flag) after it, so position-dependent quantities use the current configuration.
Avoids re-fetching them through the recompile-checking accessor on every access. No behavioural change.
14a273e to
d472c07
Compare
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
MJScene::equationsOfMotioncallsmj_setConstwhenever a body's mass changes to refreshMuJoCo's cached "constant" quantities.
mj_setConstoverwritesdata->qposwith the modelreference pose and leaves it there, so every position-dependent quantity computed afterwards in
the same call — forward kinematics, position-dependent forces, and the site state-output
messages — is evaluated at the reference pose instead of the current configuration. Since the
branch fires on every step of a burn, the reported body position stays pinned at the initial
pose for the whole maneuver, even though velocity and mass integrate correctly.
The fix restores the integrator state via
updateMujocoArraysFromStates()immediately aftermj_setConst(which also re-flags the kinematics stale so they are recomputed), and clears themodel-const-stale flag now that the constants match the current mass properties.
Commits are organized so the fix is isolated from a behaviour-preserving performance
improvements in the same code path:
[#1476]themj_setConststate-restore fix plus a regression test (MJScene.cpp,test_mass_update.py).mjModel/mjDatapointers for the duration ofequationsOfMotioninstead ofre-fetching them through the recompile-checking accessor on every access.
Verification
test_mass_update.py::test_continuouslyChangingMassto also assert the reportedposition against the closed-form trajectory. The existing test only checked velocity, which
integrates correctly even with the bug present, so it could never have caught this. The new
assertion requires
scene.extraEoMCall = True— with the defaultextraEoMCall = False,postIntegrationrewrites the site message after the step and masks the mid-stepmj_setConstclobber, which is why no existing test surfaced it.
passes with the fix applied.
mujocoDynamicsgeneral-module unit tests pass with this branch applied.Documentation
mjscene-mass-change-and-perf.rst).