diff --git a/python/mujoco/specs.cc b/python/mujoco/specs.cc index 82d43057..e58d66cc 100644 --- a/python/mujoco/specs.cc +++ b/python/mujoco/specs.cc @@ -450,7 +450,9 @@ PYBIND11_MODULE(_specs, m) { }, py::return_value_policy::reference_internal); mjSpec.def("delete", [](MjSpec& self, raw::MjsBody& body) { - mjs_delete(self.ptr, body.element); + if (mjs_delete(self.ptr, body.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjSpec.def( "attach", @@ -867,7 +869,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSFRAME ==================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsFrame& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsFrame.def("set_frame", [](raw::MjsFrame& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { @@ -906,7 +910,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSGEOM ===================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsGeom& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsGeom.def("set_frame", [](raw::MjsGeom& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { @@ -937,7 +943,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSJOINT ==================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsJoint& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsJoint.def("set_frame", [](raw::MjsJoint& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { @@ -968,7 +976,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSSITE ===================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsSite& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsSite.def("set_frame", [](raw::MjsSite& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { @@ -1016,7 +1026,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSCAMERA =================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsCamera& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsCamera.def("set_frame", [](raw::MjsCamera& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { @@ -1047,7 +1059,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSLIGHT ==================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsLight& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsLight.def("set_frame", [](raw::MjsLight& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { @@ -1078,7 +1092,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSMATERIAL ================================= mjSpec.def("delete", [](MjSpec& self, raw::MjsMaterial& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsMaterial.def_property( "classname", @@ -1092,7 +1108,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSMESH ===================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsMesh& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); mjsMesh.def_property( "classname", @@ -1452,47 +1470,65 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSFLEX ===================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsFlex& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSHFIELD =================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsHField& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSSKIN ===================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsSkin& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSTEXTURE ================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsTexture& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSKEY ====================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsKey& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSTEXT ===================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsText& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSNUMERIC ================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsNumeric& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSEXCLUDE ================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsExclude& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSTUPLE ==================================== mjSpec.def("delete", [](MjSpec& self, raw::MjsTuple& obj) { - mjs_delete(self.ptr, obj.element); + if (mjs_delete(self.ptr, obj.element) != 0) { + throw pybind11::value_error(mjs_getError(self.ptr)); + } }); // ============================= MJSPLUGIN =================================== diff --git a/python/mujoco/specs_test.py b/python/mujoco/specs_test.py index a82646dd..8bb9360e 100644 --- a/python/mujoco/specs_test.py +++ b/python/mujoco/specs_test.py @@ -979,7 +979,6 @@ class SpecsTest(absltest.TestCase): self.assertEqual(mesh.plugin.name, 'inst') self.assertEqual(mesh.plugin.plugin_name, 'mujoco.sdf.torus') - def test_duplicate_name_error(self): main_xml = """ @@ -1391,6 +1390,21 @@ class SpecsTest(absltest.TestCase): with self.assertRaisesRegex(ValueError, 'Frame not found.'): parent.attach(child4, frame='invalid_frame', prefix='child3-') + def test_delete_from_attached_spec_error(self): + parent = mujoco.MjSpec() + child = mujoco.MjSpec() + body = child.worldbody.add_body(name='child_body') + geom = body.add_geom(name='child_geom') + + frame = parent.worldbody.add_frame() + parent.attach(child, frame=frame, prefix='child_') + + # Now child spec is attached. Deleting from it should raise ValueError. + with self.assertRaisesRegex( + ValueError, 'Cannot delete element from an attached mjSpec.' + ): + child.delete(geom) + def test_attach_valid_child_lists(self): xml1 = """ diff --git a/src/user/user_api.cc b/src/user/user_api.cc index e2277bfd..13b9168c 100644 --- a/src/user/user_api.cc +++ b/src/user/user_api.cc @@ -474,8 +474,12 @@ int mj_copyBack(mjSpec* s, const mjModel* m) { // remove body from mjSpec, return 0 on success int mjs_delete(mjSpec* s, mjsElement* element) { mjCModel* model = static_cast(s->element); + if (model->IsAttached()) { + model->SetError(mjCError(nullptr, "Cannot delete element from an attached mjSpec.")); + return -1; + } if (!element) { - model->SetError(mjCError(0, "Element is null.")); + model->SetError(mjCError(nullptr, "Element is null.")); return -1; } try { diff --git a/src/user/user_model.h b/src/user/user_model.h index 025f8097..90feb1ed 100644 --- a/src/user/user_model.h +++ b/src/user/user_model.h @@ -327,6 +327,9 @@ class mjCModel : public mjCModel_, private mjSpec { // set attached flag void SetAttached(bool deepcopy) { attached_ |= !deepcopy; } + // check if model is attached + bool IsAttached() const { return attached_; } + // check for repeated names in list void CheckRepeat(mjtObj type); diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 2a7d8a53..3c7af426 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -173,6 +173,36 @@ TEST_F(MujocoTest, TreeTraversal) { mj_deleteSpec(spec); } +TEST_F(MujocoTest, AttachAndChildDeletion) { + mjSpec* child_spec = mj_makeSpec(); + mjsBody* child_world = mjs_findBody(child_spec, "world"); + mjsBody* child_body = mjs_addBody(child_world, 0); + mjsJoint* freejoint = mjs_addJoint(child_body, 0); + freejoint->type = mjJNT_FREE; + mjs_setName(freejoint->element, "child_freejoint"); + + mjSpec* parent_spec = mj_makeSpec(); + mjsBody* parent_world = mjs_findBody(parent_spec, "world"); + mjsBody* parent_body = mjs_addBody(parent_world, 0); + + // Attach child spec to parent_body + mjsElement* attached = + mjs_attach(parent_body->element, child_spec->element, "pre_", ""); + ASSERT_THAT(attached, NotNull()); + + // Delete freejoint from child_spec, should fail because it is attached + int result = mjs_delete(child_spec, freejoint->element); + EXPECT_EQ(result, -1); + + // The freejoint should still be in parent_spec because deletion failed + mjsElement* found_joint = + mjs_findElement(parent_spec, mjOBJ_JOINT, "pre_child_freejoint"); + EXPECT_THAT(found_joint, NotNull()); + + mj_deleteSpec(child_spec); + mj_deleteSpec(parent_spec); +} + TEST_F(MujocoTest, ActivatePlugin) { mjSpec* spec = mj_makeSpec(); mjs_activatePlugin(spec, "mujoco.elasticity.cable");