Import Mujco mjcf files

[Giulero]

This PR introduces the possibility of loading mujoco files.

The handling of the tree and the loading of links and joints should be fine since the tests are passing (I'm generating the iCub XML).

I have some doubt.

1

There is a small API breaking when calling KinDynComputation:

Now instead of using urdfstring I'm using model_string, since it should be more general.

https://github.com/ami-iit/adam/blob/ef03b97ffe58270c70b196f7f6444e092c1b1f4f/src/adam/numpy/computations.py#L16-L25 Should I brake this?

2

The model factory is chosen via https://github.com/ami-iit/adam/blob/ef03b97ffe58270c70b196f7f6444e092c1b1f4f/src/adam/numpy/computations.py#L33

This function (https://github.com/ami-iit/adam/blob/mjcf-importer/src/adam/model/utils.py) should handle when a:

• mujoco file
• urdf file
• urdf XML string (see #74)

Is this covering the cases?

3

This PR should handle "standard" mujoco files having, for example, quat as orientation or diaginertia as inertia. I figured out that there are also other conventions (for example, expressing the axis with xyaxis). Btw, @giotherobot showed me that these "non -standard" mujoco files can be converted to "standard" ones via, for example:

import mujoco
from robot_descriptions import cassie_description, cassie_mj_description

model_path_xml = cassie_mj_description.MJCF_PATH

model = mujoco.MjModel.from_xml_path(model_path_xml)

mujoco.mj_saveLastXML("new_model_cassie.xml", model)

[traversaro]

Should I brake this?

Not an expert about this, but it is possible to leave urdfstring as a named argument and only use it if model_string is not set, perhaps printing a warning? A complext version of this in a form of a decorator is https://docs.astropy.org/en/latest/_modules/astropy/utils/decorators.html#deprecated_renamed_argument, but I guess we could just do something ad hoc. If that is too difficult, clearly explaining the change in the release notes and perhaps doing a bump in the minor release can help downstream users.

[traversaro]

The model factory is chosen via

https://github.com/ami-iit/adam/blob/ef03b97ffe58270c70b196f7f6444e092c1b1f4f/src/adam/numpy/computations.py#L33

This function (https://github.com/ami-iit/adam/blob/mjcf-importer/src/adam/model/utils.py) should handle when a:

• mujoco file
• urdf file
• urdf XML string (see Adding the possibility to load both from string and path #74)

Is this covering the cases?

To be honest, I find this a bit confusing:

• The detection of the type of the file is based on extensions, but it can happen that URDF files as well use .xml as extension. However, this can be overridden (if I am not wrong) so it may not be a big problem. However, I would document this logic in the user facing function that the users actually call, i.e. KinDynComputations's init, to avoid misunderstandings. More complex logic could be use (like checking if the file contains a mujoco or robot top-level tag to distinguish between the two, but perhaps the existing logic may work fine until we find some exceptions.
• Why is this supporting the use case of loading a urdf xml string, and not the a mjcf xml string? Probably as I user I would find confusing that one is supported and another not.

This PR should handle "standard" mujoco files having, for example, quat as orientation or diaginertia as inertia. I figured out that there are also other conventions (for example, expressing the axis with xyaxis). Btw, @giotherobot showed me that these "non -standard" mujoco files can be converted to "standard" ones via, for example:

What is the failure mode if one tries to load an unsupported model? It raises an error or silently load a wrong file? I am particularly scared of the second case as it can lead to hard to detect bugs. On a related note, perhaps we could add some helper functions to "normalize" mjcf files and/or string in adam itself? Perhaps those functions could lazy-load mujoco, to avoid adding a mujoco dependency to whole of adam, and only require mujoco if the conversion functions are used.

[traversaro]

Minor: tests have a lot of commonalities, but I am not sure if there is a way to reduce the code duplication without hurting too much readability.

[traversaro]

General comment (not something that needs to be done): one way of being sure to support the full mjcf spec would be to change the parser to use the official mujoco parser, and actually create the adam model from the mjModel object populated by the official mujoco parser. Obviously the downside of this is that you add mujoco as a strict dependency if you want to use mjcf files.

[traversaro]

Nice to have: many users using mujoco use https://github.com/google-deepmind/mujoco_menagerie to get their models, if it is easy it may be useful to have a snippet that show in the README or the docs how to get a mujoco_menagerie model loaded in adam.

[S-Dafarra]

Should I brake this?

Not an expert about this, but it is possible to leave urdfstring as a named argument and only use it if model_string is not set, perhaps printing a warning? A complext version of this in a form of a decorator is docs.astropy.org/en/latest/_modules/astropy/utils/decorators.html, but I guess we could just do something ad hoc. If that is too difficult, clearly explaining the change in the release notes and perhaps doing a bump in the minor release can help downstream users.

This is also something that popped out with https://github.com/ami-iit/adam/pull/74

[Giulero]

Thanks @traversaro for the review, it really helped to clarify some doubts.

The detection of the type of the file is based on extensions, but it can happen that URDF files as well use .xml as extension. However, this can be overridden (if I am not wrong) so it may not be a big problem. However, I would document this logic in the user facing function that the users actually call, i.e. KinDynComputations's init, to avoid misunderstandings. More complex logic could be use (like checking if the file contains a mujoco or robot top-level tag to distinguish between the two, but perhaps the existing logic may work fine until we find some exceptions.

and in particular

like checking if the file contains a mujoco or robot top-level tag to distinguish between the two

I thought the same. My doubt was about the fact that the tags mujoco or robot are an unambiguous format and are always present in the descriptions. If so this is a better logic. I'm gonna implement it.

Why is this supporting the use case of loading a urdf xml string, and not the a mjcf xml string? Probably as I user I would find confusing that one is supported and another not.

This is implementable as well!

What is the failure mode if one tries to load an unsupported model? It raises an error or silently load a wrong file? I am particularly scared of the second case as it can lead to hard to detect bugs.

My idea was to raise an error if a unsupported field (e.g. xyaxis, fullinertia) is present in the description.

On a related note, perhaps we could add some helper functions to "normalize" mjcf files and/or string in adam itself? Perhaps those functions could lazy-load mujoco, to avoid adding a mujoco dependency to whole of adam, and only require mujoco if the conversion functions are used.

Indeed. Without adding the direct dependency, most likely, the user that is interested in this importer has already installed mujco as a dependency.

General comment (not something that needs to be done): one way of being sure to support the full mjcf spec would be to change the parser to use the official mujoco parser, and actually create the adam model from the mjModel object populated by the official mujoco parser. Obviously the downside of this is that you add mujoco as a strict dependency if you want to use mjcf files.

Good idea! As in the previous comment, somehow the dependency can be handled supposing that the user needs to install mujoco anyway.

Nice to have: many users using mujoco use https://github.com/google-deepmind/mujoco_menagerie to get their models, if it is easy it may be useful to have a snippet that show in the README or the docs how to get a mujoco_menagerie model loaded in adam.

This is indeed really nice to have!