google-deepmind / mujoco

Multi-Joint dynamics with Contact. A general purpose physics simulator.
https://mujoco.org
Apache License 2.0
8.27k stars 827 forks source link

`MjSpec`: Deleting a body unexpectedly resets keyframe values #2085

Closed hartikainen closed 2 months ago

hartikainen commented 2 months ago

Intro

Hi!

I am a MuJoCo user working on manipulation.

My setup

MuJoCo: (built from https://github.com/google-deepmind/mujoco/commit/3a17be2d24ad8b89272115ef5027b799f233ba19)

$ python -c "import mujoco; print(mujoco.__version__)"
3.2.3

API: Python

OS:

$ python -c "import platform; print(f'{platform.system()=}, {platform.release()=}, {platform.machine()=}')"
platform.system()='Darwin', platform.release()='24.0.0', platform.machine()='arm64'

What's happening? What did you expect?

I think the test case and its comments below describes the issue quite clearly. To summarize: deleting a body a body independent of a key resets the values in the key. I would expect the keyframe values to not be modified when deleting the body.

Steps for reproduction

See the simpler test case in the https://github.com/google-deepmind/mujoco/issues/2085#issuecomment-2370880734 below.

Try 1 with an unnecessary joint. ```python def test_body_deletion_should_not_reset_key(self): spec = mujoco.MjSpec() spec.from_string(textwrap.dedent(""" """)) # 1. We delete the `joint-1`. joint_1 = spec.find_body("body-1").first_joint() self.assertEqual(joint_1.name, "joint-1") joint_1.delete() # 2. Add key for `joint-2` home_key = spec.add_key() home_key.name = "home" home_key.qpos = np.array([16.0]) # 3. Now, detach `body-1`, which has nothing to do with `body-2` or `joint-2` and # the key gets reset! spec.detach_body(spec.find_body("body-1")) model = spec.compile() np.testing.assert_array_equal(model.key("home").qpos, 16.0) # Is actually 0.0 np.testing.assert_array_equal(spec.key[0].qpos, 16.0) # Is actually 0.0 # Here's what the XML looks like. Note that the `spec.key[0].qpos` still exists, # even though it's not written in the xml. The value is just incorrect (`0.0` # instead of `16.0`). self.assertEqual(textwrap.dedent(""" """).strip(), spec.to_xml().strip()) ```

Minimal model for reproduction

See above.

Code required for reproduction

See above.

Confirmations

hartikainen commented 2 months ago

I was actually able to simplify this even further. The key gets reset even without the joint-1 under body-1 like in my initial test case above:

  def test_body_deletion_should_not_reset_key(self):
    spec = mujoco.MjSpec()
    spec.from_string(textwrap.dedent("""
      <mujoco model="MuJoCo Model">
        <worldbody>
          <body name="body-1"/>
          <body name="body-2">
            <joint name="joint" />
            <geom size="1"/>
          </body>
        </worldbody>
      </mujoco>
    """))

    # 1. Add key for `joint`
    home_key = spec.add_key()
    home_key.name = "home"
    home_key.qpos = np.array([16.0])
    model = spec.compile()

    np.testing.assert_array_equal(model.key("home").qpos, 16.0)  # Passes
    np.testing.assert_array_equal(spec.key[0].qpos, 16.0)  # Passes

    # 2. Detach `body-1`, which has nothing to do with `body-2` or `joint` and
    # the key gets reset!
    spec.detach_body(spec.find_body("body-1"))
    model = spec.compile()

    np.testing.assert_array_equal(model.key("home").qpos, 16.0)  # Fails: is actually 0.0
    np.testing.assert_array_equal(spec.key[0].qpos, 16.0)  # Fails: is actually 0.0

    # Here's what the XML looks like. Note that the `spec.key[0].qpos` still exists,
    # even though it's not written in the xml. The value is just incorrect (`0.0`
    # instead of `16.0`).
    self.assertEqual(textwrap.dedent("""
      <mujoco model="MuJoCo Model">
        <compiler angle="radian"/>

        <size nkey="1"/>

        <worldbody>
          <body name="body-2">
            <joint name="joint" />
            <geom size="1"/>
          </body>
        </worldbody>

        <keyframe>
          <key name="home"/>
        </keyframe>
      </mujoco>
    """).strip(), spec.to_xml().strip())