search

google-deepmind / mujoco

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

`mj_copyData` not copying contact details? #1710

Closed marioprats closed 2 months ago

marioprats commented 3 months ago

Hi, I have an issue with mj_copyData not copying contact details. Not sure if this is a bug or something intended.

Here is a minimal model:

minimal XML ```XML ```

It has a sphere in contact with a ground plane. I found the issue with mj_copyData in C++, but it can be reproduced in python too:

minimal Python reproduction case ```python import mujoco import copy model = mujoco.MjModel.from_xml_path("scene.xml") data = mujoco.MjData(model) mujoco.mj_forward(model,data) print("Original data:") print(" n_contacts: ", data.ncon) print(" colliding geoms: ", data.contact.geom) data2 = copy.copy(data) print("Data copy:") print(" n_contacts: ", data2.ncon) print(" colliding geoms: ", data2.contact.geom) ```

The output is:

Original data:
  n_contacts:  1
  colliding geoms:  [[0 1]]
Data copy:
  n_contacts:  1
  colliding geoms:  [[0 0]]

So the copy preserves ncon, but doesn't seem to copy contact details over (e.g. colliding geoms).

yuvaltassa commented 3 months ago

Hmmm, this should work. (That said, there is no test for this function, so ya this could be a bug)

@saran-t is there a bug in this line?

saran-t commented 3 months ago

It's certainly not deep-copying the arena. I can't remember whether that was a deliberate choice though, for MJPC?

yuvaltassa commented 3 months ago

Oh damn you're right. So the comment // copy arena memory is basically a lie eh? It should either be // set arena pointers, don't copy memory or we should actually copy the memory...

I have no recollection of this being a deliberate choice, seems like a strange thing to do. @nimrod-gileadi, @erez-tom, @thowell, any such recollection?

Marking this as a bug, even if we end up deciding that it's deliberate, the inline and API docs should be updated.

nimrod-gileadi commented 2 months ago

We do have tests for this function in the Python bindings, but indeed we don't check for contacts: https://github.com/google-deepmind/mujoco/blob/14a3ce5ffac0550a645ca391119014f489f70b55/python/mujoco/bindings_test.py#L430

I think that copying the arena content is the right thing to do. Can't see why not.