search

google-deepmind / mujoco

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

Model attach and MjSpec leads to broken xml #2162

Closed jjyyxx closed 1 month ago

jjyyxx commented 1 month ago

Intro

I am a graduate student at HKU, I use MuJoCo for my research on robotic manipulation. This issue is a follow-up of comment https://github.com/google-deepmind/mujoco/issues/2150#issuecomment-2426708998

My setup

MuJoCo 3.2.4

What's happening? What did you expect?

  1. the output xml contains two levels of nameless , leading to error when loaded back;
  2. the output xml is full of asset path inconsistencies (this may be harder to deal with)
    • meshdir would be overwritten, breaking either parent or child
    • If the child model is in another directory, its asset paths are just copied in, leading to failed asset lookup.
  3. MjSpec load error if the child model contains <custom> tags.

Steps for reproduction

Run snippet below

Minimal model for reproduction

parent.xml

<mujoco>
    <compiler meshdir="a" />
    <asset>
        <!-- <mesh name="a" file="a.obj" /> -->
        <model name="child" file="child/child.xml" />
    </asset>
    <worldbody>
        <attach model="child" body="child" />
    </worldbody>
</mujoco>

child/child.xml

<mujoco>
    <compiler meshdir="b" />
    <asset>
        <!-- <mesh name="b" file="b.obj" /> -->
    </asset>
    <worldbody>
        <body name="child" />
    </worldbody>
</mujoco>

Code required for reproduction

import mujoco
spec = mujoco.MjSpec.from_file('parent.xml')
spec.compile()
xml = spec.to_xml()
print(xml)
mujoco.MjModel.from_xml_string(xml)

Confirmations

quagla commented 1 month ago
  1. Should be solved by using a prefix, could you please confirm? It's likely that we will require a prefix for attach in the future.
  2. This is known and part of conflicting compiler options in general. We are working to keep track of attached options.
  3. This sounds like a genuine new bug, I'll have a look.
quagla commented 1 month ago

I can't reproduce 3 (I tried with custom numeric data and it works fine), could you please send a minimal example?

jjyyxx commented 1 month ago

@quagla Once again, with replicate 😂 With one level of replicate, the output model contains a lot of (seems combinatory?) numerics. With another nested level of replicate (uncomment below), MjSpec breaks internally with Error: repeated name 'a00' in numeric

parent.xml

<mujoco>
    <asset>
        <model name="child" file="child.xml" />
    </asset>
    <worldbody>
        <attach model="child" body="child" />
        <replicate count="12">
            <!-- <replicate count="12"> -->
                <body />
            <!-- </replicate> -->
        </replicate>
    </worldbody>
</mujoco>

child.xml

<mujoco>
    <custom>
        <numeric name="a" data="1"/>
    </custom>
    <worldbody>
        <body name="child" />
    </worldbody>
</mujoco>
jjyyxx commented 1 month ago

@quagla Thanks for the quick fix! But,

  1. Should be solved by using a prefix, could you please confirm? It's likely that we will require a prefix for attach in the future.

I could not confirm this workaround, because applying prefix when attaching a child model containing actuator with class leads to yet another segfault (in mjXWriter::OneActuator). 😂

a.xml

<mujoco>
    <asset>
        <model name="b" file="b.xml" />
    </asset>

    <worldbody>
        <attach model="b" body="b" prefix="b" />
    </worldbody>
</mujoco>

b.xml

<mujoco>
  <default>
    <default class="b"/>
  </default>

  <worldbody>
    <body name="b">
      <geom type="box" size="0.1 0.1 0.1"/>
      <joint name="b"/>
    </body>
  </worldbody>

  <actuator>
    <position joint="b" class="b"/>
  </actuator>
</mujoco>
jjyyxx commented 1 month ago

Besides,

This is known and part of conflicting compiler options in general. We are working to keep track of attached options.

I found that, currently, in order for an attached model (with assets) to work with MjSpec, the parent and child XML must be in the same level of directory. Otherwise, path inconsistency either prevents MjSpec.compile before to_xml, or produces a model with wrong relative path. I understand that completely resolving this issue may require some major changes, but this indeed complicates our model management (we would like to put thirdparty child models in a sub-directory, but currently could not.)

quagla commented 1 month ago

Besides,

This is known and part of conflicting compiler options in general. We are working to keep track of attached options.

I found that, currently, in order for an attached model (with assets) to work with MjSpec, the parent and child XML must be in the same level of directory. Otherwise, path inconsistency either prevents MjSpec.compile before to_xml, or produces a model with wrong relative path. I understand that completely resolving this issue may require some major changes, but this indeed complicates our model management (we would like to put thirdparty child models in a sub-directory, but currently could not.)

Yes, I know. I have a solution partially implemented but it's not ready to be submitted yet. Maybe in a week's time.

quagla commented 1 month ago

@quagla Thanks for the quick fix! But,

  1. Should be solved by using a prefix, could you please confirm? It's likely that we will require a prefix for attach in the future.

I could not confirm this workaround, because applying prefix when attaching a child model containing actuator with class leads to yet another segfault (in mjXWriter::OneActuator). 😂

a.xml

<mujoco>
    <asset>
        <model name="b" file="b.xml" />
    </asset>

    <worldbody>
        <attach model="b" body="b" prefix="b" />
    </worldbody>
</mujoco>

b.xml

<mujoco>
  <default>
    <default class="b"/>
  </default>

  <worldbody>
    <body name="b">
      <geom type="box" size="0.1 0.1 0.1"/>
      <joint name="b"/>
    </body>
  </worldbody>

  <actuator>
    <position joint="b" class="b"/>
  </actuator>
</mujoco>

That's very useful, thanks!