`MjSpec`: Attaching multiple bodies from spec with a keyframe fails due to repeated name

hartikainen commented 1 month 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?

Attaching more than one body from a spec with keyframes to another spec's frame fails because both of the keyframes get named the same. I would expect this to succeed and the keyframes to be named uniquely according to e.g. the attachment prefix and suffix.

Steps for reproduction

Drop the code from below to spec_test.py

Minimal model for reproduction

See below.

Code required for reproduction

  def test_attach_multiple_bodies_from_spec_with_keyframe(self):
    spec_1 = mujoco.MjSpec()
    spec_1.from_string(textwrap.dedent("""
      <mujoco model="MuJoCo Model">
        <worldbody>
          <body name="body-1"/>
        </worldbody>
      </mujoco>
    """))
    spec_2 = mujoco.MjSpec()
    spec_2.from_string(textwrap.dedent("""
      <mujoco model="MuJoCo Model">
        <worldbody>
          <body name="body-2-1">
            <joint name="joint-2-1"/>
            <geom size="0.1"/>
          </body>
          <body name="body-2-2">
            <joint name="joint-2-2"/>
            <geom size="0.1"/>
          </body>
        </worldbody>
        <keyframe>
          <key name="home" qpos="0.1" />
        </keyframe>
      </mujoco>
    """))

    body_1 = spec_1.find_body("body-1")
    attachment_frame = body_1.add_frame()

    attachment_frame.attach_body(spec_2.find_body("body-2-1"), "body-2-1-", "")
    spec_1.compile()
    attachment_frame.attach_body(spec_2.find_body("body-2-2"), "body-2-2-", "")
    spec_1.compile()

Despite the different attachment prefixes, both of the keys seem to get the same name, and thus this fails with:

Traceback (most recent call last):
  File "mujoco/python/mujoco/specs_test.py", line 920, in test_attach_multiple_bodies_from_spec_with_keyframe
    model = spec_1.compile()
            ^^^^^^^^^^^^^^^^
ValueError: Error: repeated name 'body-2-1-home' in key

Confirmations

[X] I searched the latest documentation thoroughly before posting.
[X] I searched previous Issues and Discussions, I am certain this has not been raised before.

quagla commented 1 month ago

Hi, a current limitation is that if you want to attach multiple times when keyframes are present, you need to compile spec_1 after each attachment. You may get the same error as in #2063 when you do that but I'm about to push out a fix for that one.

hartikainen commented 1 month ago

I tried compiling them between the attachments too and that didn't make a difference. (I'll update the test above to compile the spec just in case.)

quagla commented 1 month ago

Let me know if 58a49bad96244ddbb69852497bd6ac2cf053a081 fixes this issue and #2063

Also, maybe you find the test in that commit useful to show the intended behavior.

hartikainen commented 1 month ago

This error persists when I compile from 58a49bad9624.

While testing this, I also noticed that something just broke the from_string-method of MjSpec.

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

    spec.compile()
    self.assertEqual(spec.to_xml().strip(), textwrap.dedent("""
      <mujoco model="MuJoCo Model">
        <compiler angle="radian"/>

        <worldbody>
          <body name="body-1"/>
        </worldbody>
      </mujoco>
    """).strip())

This one fails with

AssertionError:
  <mujoco model="MuJoCo Model">
    <compiler angle="radian"/>

-   <worldbody/>
?             -
+   <worldbody>
+     <body name="body-1"/>
+   </worldbody>
  </mujoco>

That is, the body disappears if you initialize the MjSpec and load the xml separate. It works, however, if you do them both at the same time with spec = mujoco.MjSpec.from_string.

quagla commented 1 month ago

Yes, this is a change of behavior. We decided to disallow creating and empty spec and then loading it to prevent the mistake of editing it before loading it. The methods from_file and from_string are now static.

hartikainen commented 1 month ago

Ah, that makes sense! I expected the original behavior to stay the same and the new constructor to be just a more convenient and pythonic shorthand. Would it make sense to error if the user loads the spec with from_string after instantiating the spec? Or what's the expected behavior in the case above?

quagla commented 1 month ago

Since it's a static method, we can't check if it's invoked on an already existing object.. Basically if you create an empty spec and then call from_string, what would happen is that the spec remains empty because the return value of from_string is ignored (as you already found out)

quagla commented 1 month ago

I found the original issue. I will send out a fix soon.

google-deepmind / mujoco