`MjSpec` default assignment doesn't take effect

hartikainen commented 1 month ago

Intro

Hi!

I use MuJoCo for robotic manipulation.

My setup

MuJoCo:

$ 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?

Modifying MjSpec element's default values doesn't take effect as I would expect. For example, modifying the default contype value of an element that doesn't have contype explicitly defined, I would expect the default's contype value to take precedence. However, what I see is that MjSpec directly assigns the contype value to the element and thus the default contype is not taken into account.

See the test case below for reproduction.

Steps for reproduction

(This could be dropped directly into specs_test.py.)

  def test_modify_default_class(self):
    spec = mujoco.MjSpec()
    spec.from_string(textwrap.dedent("""
      <mujoco model="MuJoCo Model">
          <default>
              <default class="cube">
                  <geom type="box" size="1 1 1" rgba="1 0 0 1" contype="1" conaffinity="1"/>
              </default>
          </default>
          <worldbody>
              <body name="cube">
                  <geom name="cube" class="cube"/>
              </body>
          </worldbody>
      </mujoco>
    """))

    spec.compile()
    # Note how the `cube` `geom` doesn't have `con{type,affinity}` set.
    self.assertEqual(spec.to_xml().strip(), textwrap.dedent("""
      <mujoco model="MuJoCo Model">
        <compiler angle="radian"/>

        <default>
          <default class="cube">
            <geom size="1 1 1" type="box" rgba="1 0 0 1"/>
          </default>
        </default>

        <worldbody>
          <body name="cube">
            <geom name="cube" class="cube"/>
          </body>
        </worldbody>
      </mujoco>
    """).strip())

    self.assertEqual(spec.find_body("cube").first_geom().default().name, "cube")
    self.assertEqual(spec.find_body("cube").first_geom().default().geom.contype, 1)
    self.assertEqual(spec.find_body("cube").first_geom().default().geom.conaffinity, 1)
    self.assertEqual(spec.find_default("cube").geom.contype, 1)
    self.assertEqual(spec.find_default("cube").geom.conaffinity, 1)

    spec.find_default("cube").geom.contype = 0
    spec.find_default("cube").geom.conaffinity = 0

    self.assertEqual(spec.find_body("cube").first_geom().default().geom.contype, 0)
    self.assertEqual(spec.find_body("cube").first_geom().default().geom.conaffinity, 0)
    self.assertEqual(spec.find_default("cube").geom.contype, 0)
    self.assertEqual(spec.find_default("cube").geom.conaffinity, 0)

    mj_model = spec.compile()

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

        <default>
          <default class="cube">
            <geom size="1 1 1" type="box" contype="0" conaffinity="0" rgba="1 0 0 1"/>
          </default>
        </default>

        <worldbody>
          <body name="cube">
            <geom name="cube" class="cube"/>
          </body>
        </worldbody>
      </mujoco>
    """).strip())
    self.assertEqual(mj_model.geom("cube").contype.item(), 0)
    self.assertEqual(mj_model.geom("cube").conaffinity.item(), 0)

This one fails with:

Traceback (most recent call last):
  File "mujoco/python/mujoco/specs_test.py", line 619, in test_modify_default_class
    self.assertEqual(spec.to_xml().strip(), textwrap.dedent("""
AssertionError:
  <mujoco model="MuJoCo Model">
    <compiler angle="radian"/>

    <default>
      <default class="cube">
        <geom size="1 1 1" type="box" contype="0" conaffinity="0" rgba="1 0 0 1"/>
      </default>
    </default>

    <worldbody>
      <body name="cube">
-       <geom name="cube" class="cube" contype="1" conaffinity="1"/>
+       <geom name="cube" class="cube"/>
      </body>
    </worldbody>
  </mujoco>

As I didn't modify the con{type,affinity} values for geom directly, I'd expected them to not be set (note how they're set only if I modify the default values) and thus the default values to take precedence. Here, however, they're both set to 1 explicitly and the default values get ignored.

Minimal model for reproduction

See above.

Code required for reproduction

See above.

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

I would say this is more of a feature request than a bug. With the current design, you can't set defaults, create a geom, change the defaults, and expect that they propagate. Defaults are used once to create the geom and then only kept for deciding which attributes to write explicitly when saving an XML. Maybe a solution is to keep track if a default has been used and if so return an error when changing any of its attributes? Any better ideas?

hartikainen commented 1 month ago

Perhaps I misunderstand how the defaults work, but it was surprising to me that the contype="1" conaffinity="1" values get written to the geom element only when I manually change the geom's default values (and not when I haven't touched the default values; see the note and assert after the first compile() call above). I'll need to play around with this a bit more to understand what the semantics are before I can comment on what the good solution here would be.

yuvaltassa commented 1 month ago

Yes, I think this is basically a documentation issue. When using mjSpec, defaults are applied once, when creating the element. I will add a clarifying note.

google-deepmind / mujoco