Initial implementation of mujoco texture support #423

RussTedrake · 2025-01-14T22:26:58Z

It works for a handful of the cases I've tried, but I'm sure there will be more improvements coming. I've not added additional tests, since I'm mostly checking things visually for now. I will add tests that at least check the texture path in the mtl file soon.

For instance, I've found that it works for Cassie's legs, but not yet for the pelvis:

Towards #419

This change is

RussTedrake · 2025-01-15T04:50:20Z

adding the textures does make some models / model parts disappear. I would rather land this with it's documented (in menagerie.ipynb) deficiencies, and then continue to resolve them. Probably one or two small fixes will make the rest of them work.

nepfaff

All the logic looks good! The comments are mainly nitpicking.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @RussTedrake)

book/robot/inspector.ipynb line 155 at r1 (raw file):

    "# Robots description files from other repositories\n",
    "\n",
    "We've tried to make it very easy to pull in resources from other repositories.  Here's an example of pulling in files directly from github. Drake's MuJoCo parser is the least used/complete of our parsers (I only added a minimal version of it quite recently), so many of these don't parse beautifully yet -- I'm working on it! But it's a good example of how to pull in resources from other repositories, and if there is a particular model you need that doesn't parse well for you, you can let me know."

Does this need updating? I remember you saying that the parser is getting close to feature-complete. I guess it's still the only one that is not 100% complete yet?

Maybe it is better to move this comment to the menagerie.ipynb, which will make it easier to remember to update it. The current place seems a bit weird as it's the header of a list of external places and not just MuJoCo.

book/robot/inspector.ipynb line 202 at r1 (raw file):

   "metadata": {},
   "source": [
    "Sometimes it's useful to run a short simulation.  Here's an example of how to do that."

I'm wondering if this is still useful for the other examples in this notebook. It might fit in well at the very end of this notebook. Getting a minimal simulation working without HardwareStation seems like something that students would struggle with.

manipulation/utils.py line 22 at r1 (raw file):

from pydrake.geometry import RenderLabel
from pydrake.multibody.parsing import PackageMap, Parser
from pydrake.systems.framework import DiagramBuilder, System

Most of the manipulation repo seems to import from pydrake.all, but some files like this one are more explicit. Consistency could be nice, especially for student notebooks. How do you choose which path to take?

manipulation/test/test_make_drake_compatible_model.py line 182 at r1 (raw file):

                                    msg_pattern, str(e)
                                ):
                                    results += f"FAIL: {os.path.join(rel_path, file)}: {note}\n"

This should be split into multiple lines as it violates the black max line length

manipulation/test/test_make_drake_compatible_model.py line 186 at r1 (raw file):

                                    break
                            if not known_failure:
                                results += f"FAIL: {os.path.join(rel_path, file)}: Unregistered exception\n"

This should be split into multiple lines as it violates the black max line length

manipulation/test/test_make_drake_compatible_model.py line 188 at r1 (raw file):

                                results += f"FAIL: {os.path.join(rel_path, file)}: Unregistered exception\n"
                                raise  # Re-raise if not a known exception
        print(results)

The print in the test seems a bit odd. Why is this desirable? Would it be better to raise with the text if there are failures?

manipulation/make_drake_compatible_model.py line 12 at r1 (raw file):

def _convert_mesh(

A docstring would make this much easier to parse. It's not strictly required, as it's a private method, but it would still be nice for maintainability.

The same holds for the other _convert functions in this file, though a bit less important for these.

manipulation/make_drake_compatible_model.py line 16 at r1 (raw file):

    path: str,
    scale: Optional[List[float]] = None,
    texture_path: Optional[str] = None,

It might make sense for us to start moving towards str | None over Optional[str] based on the discussion here.

manipulation/make_drake_compatible_model.py line 18 at r1 (raw file):

    texture_path: Optional[str] = None,
    overwrite: bool = False,
) -> Tuple[str, str]:

The second return variable isn't used anywhere apart from the test. Should this return URL only?

manipulation/make_drake_compatible_model.py line 46 at r1 (raw file):

        # Apply the texture to the mesh
        print(f"applying texture {texture_path} to mesh {path}")

Most print statements in this file start with a capital letter. It would be nice to change this one to maintain consistency.

manipulation/make_drake_compatible_model.py line 51 at r1 (raw file):

    if scale is not None:
        scale = [float(s) for s in scale]
        assert len(scale) == 3, "Scale must be a 3-element array."

Raising an error would be more appropriate as this is a user input. Would also make this consistent with the other input validation in this function.

manipulation/make_drake_compatible_model.py line 193 at r1 (raw file):

    Args:
        element: The XML element to apply defaults to
        defaults_dict: Dictionary mapping (class_name, element_type) to default elements

Docstring is incomplete, and class_name_override isn't obvious from its name.

manipulation/make_drake_compatible_model.py line 231 at r1 (raw file):

def _convert_mjcf(input_filename, output_filename, package_map, overwrite):

package_map argument is unused

manipulation/make_drake_compatible_model.py line 246 at r1 (raw file):

        if "assetdir" in compiler_element.attrib:
            meshdir = compiler_element.attrib["assetdir"]
            compiler_element.attrib["assetdir"]

Accessed but not assigned. Looks like this can be removed without effect.

manipulation/make_drake_compatible_model.py line 251 at r1 (raw file):

            meshdir = compiler_element.attrib["meshdir"]
        if "texturedir" in compiler_element.attrib:
            compiler_element.attrib["texturedir"]

Accessed but not assigned. Looks like this can be removed without effect.

manipulation/make_drake_compatible_model.py line 283 at r1 (raw file):

                    except:
                        print(f"Failed to create default element for {key}")
                        raise

Raising an error with text would be cleaner and more consistent with the other instances in this file.

manipulation/make_drake_compatible_model.py line 333 at r1 (raw file):

    material_paths = {}
    for material_element in root.findall(".//asset/material"):
        assert (

Raise would be more consistent

manipulation/make_drake_compatible_model.py line 359 at r1 (raw file):

            ):
                raise AssertionError(
                    f"Mesh {mesh_name} was already associated with {mesh_to_material[mesh_name]}. We don't handle multiple materials assigned to the same mesh yet."

This should be split into multiple lines as it violates the black max line length

manipulation/make_drake_compatible_model.py line 387 at r1 (raw file):

            mesh_url = os.path.basename(mesh_url)

        # Check if mesh_url is already an absolute path

Mujoco seems to have general rules that apply to both meshes and textures. It could make sense to factor this out into a helper method to avoid code duplication and improve readability

manipulation/make_drake_compatible_model.py line 452 at r1 (raw file):

    input_filename: str,
    output_filename: str,
    package_map: PackageMap = None,

This is now discouraged (see discussion) and makes

if package_map is None:
        package_map = PackageMap()

displayed at unreachable by pylance.

book/robot/menagerie.ipynb line 66 at r1 (raw file):

    "\n",
    "\n",
    "def show_model(model):\n",

type hints would be great

book/robot/menagerie.ipynb line 86 at r1 (raw file):

    "    simulator = Simulator(diagram)\n",
    "\n",
    "    display(Image(filename=image_path))\n",

Should this be optional? This looks like a development feature.

book/robot/menagerie.ipynb line 97 at r1 (raw file):

    "            )\n",
    "\n",
    "    # simulator.AdvanceTo(0.01)\n",

Should this be removed or uncommented?

This represents a substantial upgrade in our mujoco processing power. There is a new script in inspector which does a parade of the menagerie assets. Many of them visualize well... but not all. There will certainly be more improvements coming. I've not added additional tests, since I'm mostly checking things visually for now. (but all menagerie assets do run successfully through make_drake_compatible_model, which was no small feat)

RussTedrake

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @nepfaff)

book/robot/inspector.ipynb line 155 at r1 (raw file):