From 3a12db9ad2c395eeb3a736a23a957234788c0815 Mon Sep 17 00:00:00 2001 From: Alessio Quaglino Date: Fri, 22 Nov 2024 11:04:20 -0800 Subject: [PATCH] Remove id attribute from mjSpec objects in Python bindings. Using ids is error prone in scenarios of repeated attachment and detachment. Python users are encouraged to use names for unique identification of model elements. PiperOrigin-RevId: 699227286 Change-Id: Ifd83e6d85d36ff72ea43caf8b82eab9e4d552440 --- doc/changelog.rst | 5 ++++ python/mjspec.ipynb | 20 +++++++------ python/mujoco/specs.cc | 57 ------------------------------------- python/mujoco/specs_test.py | 11 ++----- 4 files changed, 18 insertions(+), 75 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 917e2e63..e448e8bf 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -6,6 +6,11 @@ Changelog Upcoming version (not yet released) ----------------------------------- +General +^^^^^^^ +- Removed id attribute from :ref:`mjSpec` objects in Python bindings. Using ids is error prone in scenarios of repeated + attachment and detachment. Python users are encouraged to use names for unique identification of model elements. + MJX ^^^ - Added muscle actuators. diff --git a/python/mjspec.ipynb b/python/mjspec.ipynb index 772faf30..886032f4 100644 --- a/python/mjspec.ipynb +++ b/python/mjspec.ipynb @@ -300,8 +300,8 @@ " \n", "\n", " \n", - " \n", - " \n", + " \n", + " \n", " \n", "\n", "\"\"\"\n", @@ -450,14 +450,16 @@ "source": [ "#@title Video of the movement{vertical-output: true}\n", "\n", + "data = mj.MjData(model)\n", "duration = 10 # (Seconds)\n", "framerate = 30 # (Hz)\n", "video = []\n", "pos_x = []\n", "pos_y = []\n", "geoms = arena.worldbody.find_all(mj.mjtObj.mjOBJ_GEOM)\n", - "torsos = [geom.id for geom in geoms if 'torso' in geom.name]\n", - "actuators = [actuator.id for actuator in arena.actuators]\n", + "torsos_data = [data.geom(geom.name) for geom in geoms if 'torso' in geom.name]\n", + "torsos_model = [model.geom(geom.name) for geom in geoms if 'torso' in geom.name]\n", + "actuators = [data.actuator(actuator.name) for actuator in arena.actuators]\n", "\n", "# Control signal frequency, phase, amplitude.\n", "freq = 5\n", @@ -465,17 +467,17 @@ "amp = 0.9\n", "\n", "# Simulate, saving video frames and torso locations.\n", - "data = mj.MjData(model)\n", "mj.mj_resetData(model, data)\n", "with mj.Renderer(model) as renderer:\n", " while data.time < duration:\n", " # Inject controls and step the physics.\n", - " data.ctrl[actuators] = amp * np.sin(freq * data.time + phase)\n", + " for i, actuator in enumerate(actuators):\n", + " actuator.ctrl = amp * np.sin(freq * data.time + phase[i])\n", " mj.mj_step(model, data)\n", "\n", " # Save torso horizontal positions using name indexing.\n", - " pos_x.append(data.geom_xpos[torsos, 0].copy())\n", - " pos_y.append(data.geom_xpos[torsos, 1].copy())\n", + " pos_x.append([torso.xpos[0] for torso in torsos_data])\n", + " pos_y.append([torso.xpos[1] for torso in torsos_data])\n", "\n", " # Save video frames.\n", " if len(video) < data.time * framerate:\n", @@ -496,7 +498,7 @@ "source": [ "#@title Movement trajectories{vertical-output: true}\n", "\n", - "creature_colors = model.geom_rgba[torsos][:, :3]\n", + "creature_colors = [torso.rgba[:3] for torso in torsos_model]\n", "fig, ax = plt.subplots(figsize=(4, 4))\n", "ax.set_prop_cycle(color=creature_colors)\n", "_ = ax.plot(pos_x, pos_y, linewidth=4)" diff --git a/python/mujoco/specs.cc b/python/mujoco/specs.cc index d88a0544..725894c6 100644 --- a/python/mujoco/specs.cc +++ b/python/mujoco/specs.cc @@ -407,8 +407,6 @@ PYBIND11_MODULE(_specs, m) { }); // ============================= MJSBODY ===================================== - mjsBody.def_property_readonly( - "id", [](raw::MjsBody& self) -> int { return mjs_getId(self.element); }); mjsBody.def( "add_freejoint", [](raw::MjsBody& self, py::kwargs kwargs) -> raw::MjsJoint* { @@ -650,8 +648,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSFRAME ==================================== - mjsFrame.def_property_readonly( - "id", [](raw::MjsFrame& self) -> int { return mjs_getId(self.element); }); mjsFrame.def("delete", [](raw::MjsFrame& self) { mjs_delete(self.element); }); mjsFrame.def("set_frame", [](raw::MjsFrame& self, raw::MjsFrame& frame) { mjs_setFrame(self.element, &frame); @@ -671,8 +667,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSGEOM ===================================== - mjsGeom.def_property_readonly( - "id", [](raw::MjsGeom& self) -> int { return mjs_getId(self.element); }); mjsGeom.def("delete", [](raw::MjsGeom& self) { mjs_delete(self.element); }); mjsGeom.def("set_frame", [](raw::MjsGeom& self, raw::MjsFrame& frame) { mjs_setFrame(self.element, &frame); @@ -688,8 +682,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSJOINT ==================================== - mjsJoint.def_property_readonly( - "id", [](raw::MjsJoint& self) -> int { return mjs_getId(self.element); }); mjsJoint.def("delete", [](raw::MjsJoint& self) { mjs_delete(self.element); }); mjsJoint.def("set_frame", [](raw::MjsJoint& self, raw::MjsFrame& frame) { mjs_setFrame(self.element, &frame); @@ -705,8 +697,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSSITE ===================================== - mjsSite.def_property_readonly( - "id", [](raw::MjsSite& self) -> int { return mjs_getId(self.element); }); mjsSite.def("delete", [](raw::MjsSite& self) { mjs_delete(self.element); }); mjsSite.def("set_frame", [](raw::MjsSite& self, raw::MjsFrame& frame) { mjs_setFrame(self.element, &frame); @@ -735,9 +725,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSCAMERA =================================== - mjsCamera.def_property_readonly("id", [](raw::MjsCamera& self) -> int { - return mjs_getId(self.element); - }); mjsCamera.def("delete", [](raw::MjsCamera& self) { mjs_delete(self.element); }); mjsCamera.def("set_frame", [](raw::MjsCamera& self, raw::MjsFrame& frame) { @@ -754,8 +741,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSLIGHT ==================================== - mjsLight.def_property_readonly( - "id", [](raw::MjsLight& self) -> int { return mjs_getId(self.element); }); mjsLight.def("delete", [](raw::MjsLight& self) { mjs_delete(self.element); }); mjsLight.def("set_frame", [](raw::MjsLight& self, raw::MjsFrame& frame) { mjs_setFrame(self.element, &frame); @@ -771,9 +756,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSMATERIAL ================================= - mjsMaterial.def_property_readonly("id", [](raw::MjsMaterial& self) -> int { - return mjs_getId(self.element); - }); mjsMaterial.def("delete", [](raw::MjsMaterial& self) { mjs_delete(self.element); }); mjsMaterial.def("set_default", @@ -788,8 +770,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSMESH ===================================== - mjsMesh.def_property_readonly( - "id", [](raw::MjsMesh& self) -> int { return mjs_getId(self.element); }); mjsMesh.def("delete", [](raw::MjsMesh& self) { mjs_delete(self.element); }); mjsMesh.def("set_default", [](raw::MjsMesh& self, raw::MjsDefault& def) { mjs_setDefault(self.element, &def); @@ -802,8 +782,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSPAIR ===================================== - mjsPair.def_property_readonly( - "id", [](raw::MjsPair& self) -> int { return mjs_getId(self.element); }); mjsPair.def("delete", [](raw::MjsPair& self) { mjs_delete(self.element); }); mjsPair.def("set_default", [](raw::MjsPair& self, raw::MjsDefault& def) { mjs_setDefault(self.element, &def); @@ -816,9 +794,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSEQUAL ==================================== - mjsEquality.def_property_readonly("id", [](raw::MjsEquality& self) -> int { - return mjs_getId(self.element); - }); mjsEquality.def("delete", [](raw::MjsEquality& self) { mjs_delete(self.element); }); mjsEquality.def("set_default", @@ -833,9 +808,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSACTUATOR ================================= - mjsActuator.def_property_readonly("id", [](raw::MjsActuator& self) -> int { - return mjs_getId(self.element); - }); mjsActuator.def("delete", [](raw::MjsActuator& self) { mjs_delete(self.element); }); mjsActuator.def("set_default", @@ -850,9 +822,6 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSTENDON =================================== - mjsTendon.def_property_readonly("id", [](raw::MjsTendon& self) -> int { - return mjs_getId(self.element); - }); mjsTendon.def("delete", [](raw::MjsTendon& self) { mjs_delete(self.element); }); mjsTendon.def("set_default", [](raw::MjsTendon& self, raw::MjsDefault& def) { @@ -890,64 +859,38 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSSENSOR =================================== - mjsSensor.def_property_readonly("id", [](raw::MjsSensor& self) -> int { - return mjs_getId(self.element); - }); mjsSensor.def("delete", [](raw::MjsSensor& self) { mjs_delete(self.element); }); // ============================= MJSFLEX ===================================== - mjsFlex.def_property_readonly( - "id", [](raw::MjsFlex& self) -> int { return mjs_getId(self.element); }); mjsFlex.def("delete", [](raw::MjsFlex& self) { mjs_delete(self.element); }); // ============================= MJSHFIELD =================================== - mjsHField.def_property_readonly("id", [](raw::MjsHField& self) -> int { - return mjs_getId(self.element); - }); mjsHField.def("delete", [](raw::MjsHField& self) { mjs_delete(self.element); }); // ============================= MJSSKIN ===================================== - mjsSkin.def_property_readonly( - "id", [](raw::MjsSkin& self) -> int { return mjs_getId(self.element); }, - py::return_value_policy::reference_internal); mjsSkin.def("delete", [](raw::MjsSkin& self) { mjs_delete(self.element); }); // ============================= MJSTEXTURE ================================== - mjsTexture.def_property_readonly("id", [](raw::MjsTexture& self) -> int { - return mjs_getId(self.element); - }); mjsTexture.def("delete", [](raw::MjsTexture& self) { mjs_delete(self.element); }); // ============================= MJSKEY ====================================== - mjsKey.def_property_readonly( - "id", [](raw::MjsKey& self) -> int { return mjs_getId(self.element); }); mjsKey.def("delete", [](raw::MjsKey& self) { mjs_delete(self.element); }); // ============================= MJSTEXT ===================================== - mjsText.def_property_readonly( - "id", [](raw::MjsText& self) -> int { return mjs_getId(self.element); }); mjsText.def("delete", [](raw::MjsText& self) { mjs_delete(self.element); }); // ============================= MJSNUMERIC ================================== - mjsNumeric.def_property_readonly("id", [](raw::MjsNumeric& self) -> int { - return mjs_getId(self.element); - }); mjsNumeric.def("delete", [](raw::MjsNumeric& self) { mjs_delete(self.element); }); // ============================= MJSEXCLUDE ================================== - mjsExclude.def_property_readonly("id", [](raw::MjsExclude& self) -> int { - return mjs_getId(self.element); - }); mjsExclude.def("delete", [](raw::MjsExclude& self) { mjs_delete(self.element); }); // ============================= MJSTUPLE ==================================== - mjsTuple.def_property_readonly( - "id", [](raw::MjsTuple& self) -> int { return mjs_getId(self.element); }); mjsTuple.def("delete", [](raw::MjsTuple& self) { mjs_delete(self.element); }); // ============================= MJSPLUGIN =================================== diff --git a/python/mujoco/specs_test.py b/python/mujoco/specs_test.py index 7bca48f0..74a34648 100644 --- a/python/mujoco/specs_test.py +++ b/python/mujoco/specs_test.py @@ -92,18 +92,11 @@ class SpecsTest(absltest.TestCase): self.assertEqual(site.type, mujoco.mjtGeom.mjGEOM_BOX) np.testing.assert_array_equal(site.userdata, [1, 2, 3, 4, 5, 6]) - # Check that the site and body have no id before compilation. - self.assertEqual(body.id, -1) - self.assertEqual(site.id, -1) - # Compile the spec and check for expected values in the model. model = spec.compile() - self.assertEqual(spec.worldbody.id, 0) - self.assertEqual(body.id, 1) - self.assertEqual(site.id, 0) self.assertEqual(model.nbody, 2) # 2 bodies, including the world body - np.testing.assert_array_equal(model.body_pos[1], [1, 2, 3]) - np.testing.assert_array_equal(model.body_quat[1], [0, 1, 0, 0]) + np.testing.assert_array_equal(model.body('baz').pos, [1, 2, 3]) + np.testing.assert_array_equal(model.body('baz').quat, [0, 1, 0, 0]) self.assertEqual(model.nsite, 1) self.assertEqual(model.nuser_site, 6) np.testing.assert_array_equal(model.site_user[0], [1, 2, 3, 4, 5, 6])