From 0488d9f4ebde88161f09f98f0db81a540cff89bc Mon Sep 17 00:00:00 2001 From: Alessio Quaglino Date: Fri, 20 Jun 2025 04:11:14 -0700 Subject: [PATCH] Replace mjs_detachBody and mjs_detachDefault with mjs_detach. In a follow-up, mjs_detach will be changed to only remove without deleting an element. PiperOrigin-RevId: 773637351 Change-Id: I9ee4d3584ba88ccc225591337cf036bab806e25c --- doc/APIreference/functions.rst | 19 ++--- doc/changelog.rst | 7 ++ doc/includes/references.h | 5 +- doc/python.rst | 2 +- include/mujoco/mujoco.h | 9 +-- python/mjspec.ipynb | 4 +- python/mujoco/introspect/functions.py | 38 +++------ python/mujoco/specs.cc | 107 +++++++++++++++++--------- python/mujoco/specs_test.py | 8 +- src/user/user_api.cc | 58 ++++++-------- src/user/user_api.h | 9 +-- src/user/user_model.cc | 54 +++++++------ src/user/user_model.h | 3 - src/xml/xml_native_reader.cc | 2 +- test/user/user_api_test.cc | 31 +++----- 15 files changed, 177 insertions(+), 179 deletions(-) diff --git a/doc/APIreference/functions.rst b/doc/APIreference/functions.rst index 2bba2a3c..8c252a0f 100644 --- a/doc/APIreference/functions.rst +++ b/doc/APIreference/functions.rst @@ -3783,23 +3783,14 @@ Attachment Attach child to a parent, return the attached element if success or NULL otherwise. -.. _mjs_detachBody: +.. _mjs_detach: -`mjs_detachBody <#mjs_detachBody>`__ -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +`mjs_detach <#mjs_detach>`__ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -.. mujoco-include:: mjs_detachBody +.. mujoco-include:: mjs_detach -Delete body and descendants from mjSpec, remove all references, return 0 on success. - -.. _mjs_detachDefault: - -`mjs_detachDefault <#mjs_detachDefault>`__ -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -.. mujoco-include:: mjs_detachDefault - -Delete default class and descendants from mjSpec, remove all references, return 0 on success. +Detach but not delete object corresponding to the given element, return 0 on success. .. _AddTreeElements: diff --git a/doc/changelog.rst b/doc/changelog.rst index dec65676..02d2cacf 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -5,6 +5,13 @@ Changelog Upcoming version (not yet released) ----------------------------------- +.. admonition:: Breaking API changes + :class: attention + + - The functions ``mjs_detachBody`` and ``mjs_detachDefault`` have been replaced by :ref:`mjs_delete`. Additionally, + `:ref:mjs_detach` was added in order to remove an element from the spec without deleting it. + - The Python functions ``element.delete`` have been replaced by ``spec.delete(element)``. + Bug fixes ^^^^^^^^^ - Inverse dynamics were not being computed correctly when :ref:`tendon armature` was present, diff --git a/doc/includes/references.h b/doc/includes/references.h index 1d539d56..2e1ef149 100644 --- a/doc/includes/references.h +++ b/doc/includes/references.h @@ -3410,8 +3410,7 @@ void mju_defaultTask(mjTask* task); void mju_taskJoin(mjTask* task); mjsElement* mjs_attach(mjsElement* parent, const mjsElement* child, const char* prefix, const char* suffix); -int mjs_detachBody(mjSpec* s, mjsBody* b); -int mjs_detachDefault(mjSpec* s, mjsDefault* d); +int mjs_detach(mjSpec* spec, mjsElement* element); mjsBody* mjs_addBody(mjsBody* body, const mjsDefault* def); mjsSite* mjs_addSite(mjsBody* body, const mjsDefault* def); mjsJoint* mjs_addJoint(mjsBody* body, const mjsDefault* def); @@ -3420,7 +3419,7 @@ mjsGeom* mjs_addGeom(mjsBody* body, const mjsDefault* def); mjsCamera* mjs_addCamera(mjsBody* body, const mjsDefault* def); mjsLight* mjs_addLight(mjsBody* body, const mjsDefault* def); mjsFrame* mjs_addFrame(mjsBody* body, mjsFrame* parentframe); -int mjs_delete(mjsElement* element); +int mjs_delete(mjSpec* spec, mjsElement* element); mjsActuator* mjs_addActuator(mjSpec* s, const mjsDefault* def); mjsSensor* mjs_addSensor(mjSpec* s); mjsFlex* mjs_addFlex(mjSpec* s); diff --git a/doc/python.rst b/doc/python.rst index 61ebcdc8..251cce46 100644 --- a/doc/python.rst +++ b/doc/python.rst @@ -607,7 +607,7 @@ For elements that can have children (bodies and defaults), the methods ``spec.de ``spec.detach_default(def)`` remove, respectively, ``body`` and ``def`` from the spec, together with all of their children. When detaching body subtrees, all elements which reference elements in the subtree, will also be removed. For all other elements, the method ``delete()`` removes the corresponding element from the spec, e.g. -``spec.geom('my_geom').delete()`` will remove the geom named "my_geom" and all of the elements that reference it. +``spec.delete(spec.geom('my_geom'))`` will remove the geom named "my_geom" and all of the elements that reference it. Tree traversal ^^^^^^^^^^^^^^ diff --git a/include/mujoco/mujoco.h b/include/mujoco/mujoco.h index 944e5b21..b824e9f7 100644 --- a/include/mujoco/mujoco.h +++ b/include/mujoco/mujoco.h @@ -1411,11 +1411,8 @@ MJAPI void mju_taskJoin(mjTask* task); MJAPI mjsElement* mjs_attach(mjsElement* parent, const mjsElement* child, const char* prefix, const char* suffix); -// Delete body and descendants from mjSpec, remove all references, return 0 on success. -MJAPI int mjs_detachBody(mjSpec* s, mjsBody* b); - -// Delete default class and descendants from mjSpec, remove all references, return 0 on success. -MJAPI int mjs_detachDefault(mjSpec* s, mjsDefault* d); +// Detach but not delete object corresponding to the given element, return 0 on success. +MJAPI int mjs_detach(mjSpec* spec, mjsElement* element); //---------------------------------- Tree elements ------------------------------------------------- @@ -1444,7 +1441,7 @@ MJAPI mjsLight* mjs_addLight(mjsBody* body, const mjsDefault* def); MJAPI mjsFrame* mjs_addFrame(mjsBody* body, mjsFrame* parentframe); // Delete object corresponding to the given element, return 0 on success. -MJAPI int mjs_delete(mjsElement* element); +MJAPI int mjs_delete(mjSpec* spec, mjsElement* element); //---------------------------------- Non-tree elements --------------------------------------------- diff --git a/python/mjspec.ipynb b/python/mjspec.ipynb index ff4b997e..49b1f54f 100644 --- a/python/mjspec.ipynb +++ b/python/mjspec.ipynb @@ -2000,7 +2000,7 @@ "\n", "# Delete all key frames to avoid name conflicts\n", "while humanoid.keys:\n", - " humanoid.keys[-1].delete()\n", + " humanoid.delete(keys[-1])\n", "\n", "# Create a grid of humanoids by attaching humanoid to spec multiple times\n", "for i in range(4):\n", @@ -2335,7 +2335,7 @@ "fly = mj.MjSpec.from_file(fly_file)\n", "# Remove lights from fly so they are not duplicated in line-up\n", "for light in fly.lights:\n", - " light.delete()\n", + " fly.delete(light)\n", "\n", "small_fly = scale_spec(fly, 1.25)\n", "large_fly = scale_spec(fly, 2)\n", diff --git a/python/mujoco/introspect/functions.py b/python/mujoco/introspect/functions.py index 680122a1..fe713dc6 100644 --- a/python/mujoco/introspect/functions.py +++ b/python/mujoco/introspect/functions.py @@ -8987,45 +8987,25 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([ ), doc='Attach child to a parent, return the attached element if success or NULL otherwise.', # pylint: disable=line-too-long )), - ('mjs_detachBody', + ('mjs_detach', FunctionDecl( - name='mjs_detachBody', + name='mjs_detach', return_type=ValueType(name='int'), parameters=( FunctionParameterDecl( - name='s', + name='spec', type=PointerType( inner_type=ValueType(name='mjSpec'), ), ), FunctionParameterDecl( - name='b', + name='element', type=PointerType( - inner_type=ValueType(name='mjsBody'), + inner_type=ValueType(name='mjsElement'), ), ), ), - doc='Delete body and descendants from mjSpec, remove all references, return 0 on success.', # pylint: disable=line-too-long - )), - ('mjs_detachDefault', - FunctionDecl( - name='mjs_detachDefault', - return_type=ValueType(name='int'), - parameters=( - FunctionParameterDecl( - name='s', - type=PointerType( - inner_type=ValueType(name='mjSpec'), - ), - ), - FunctionParameterDecl( - name='d', - type=PointerType( - inner_type=ValueType(name='mjsDefault'), - ), - ), - ), - doc='Delete default class and descendants from mjSpec, remove all references, return 0 on success.', # pylint: disable=line-too-long + doc='Detach but not delete object corresponding to the given element, return 0 on success.', # pylint: disable=line-too-long )), ('mjs_addBody', FunctionDecl( @@ -9202,6 +9182,12 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([ name='mjs_delete', return_type=ValueType(name='int'), parameters=( + FunctionParameterDecl( + name='spec', + type=PointerType( + inner_type=ValueType(name='mjSpec'), + ), + ), FunctionParameterDecl( name='element', type=PointerType( diff --git a/python/mujoco/specs.cc b/python/mujoco/specs.cc index b2d081e6..a6a9139e 100644 --- a/python/mujoco/specs.cc +++ b/python/mujoco/specs.cc @@ -397,7 +397,7 @@ PYBIND11_MODULE(_specs, m) { }, py::return_value_policy::reference_internal); mjSpec.def("detach_default", [](MjSpec& self, raw::MjsDefault& def) { - if (mjs_detachDefault(self.ptr, &def) != 0) { + if (mjs_delete(self.ptr, def.element) != 0) { throw pybind11::value_error(mjs_getError(self.ptr)); } }); @@ -408,7 +408,7 @@ PYBIND11_MODULE(_specs, m) { }, py::return_value_policy::reference_internal); mjSpec.def("detach_body", [](MjSpec& self, raw::MjsBody& body) { - mjs_detachBody(self.ptr, &body); + mjs_delete(self.ptr, body.element); }); mjSpec.def( "attach", @@ -759,7 +759,9 @@ PYBIND11_MODULE(_specs, m) { // ============================= MJSFRAME ==================================== - mjsFrame.def("delete", [](raw::MjsFrame& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsFrame& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsFrame.def("set_frame", [](raw::MjsFrame& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); @@ -796,7 +798,9 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSGEOM ===================================== - mjsGeom.def("delete", [](raw::MjsGeom& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsGeom& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsGeom.def("set_frame", [](raw::MjsGeom& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); @@ -824,7 +828,9 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSJOINT ==================================== - mjsJoint.def("delete", [](raw::MjsJoint& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsJoint& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsJoint.def("set_frame", [](raw::MjsJoint& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); @@ -852,7 +858,9 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSSITE ===================================== - mjsSite.def("delete", [](raw::MjsSite& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsSite& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsSite.def("set_frame", [](raw::MjsSite& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); @@ -897,8 +905,9 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSCAMERA =================================== - mjsCamera.def("delete", - [](raw::MjsCamera& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsCamera& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsCamera.def("set_frame", [](raw::MjsCamera& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); @@ -926,7 +935,9 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSLIGHT ==================================== - mjsLight.def("delete", [](raw::MjsLight& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsLight& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsLight.def("set_frame", [](raw::MjsLight& self, raw::MjsFrame& frame) { if (mjs_setFrame(self.element, &frame) != 0) { throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); @@ -954,8 +965,9 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSMATERIAL ================================= - mjsMaterial.def("delete", - [](raw::MjsMaterial& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsMaterial& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsMaterial.def_property( "classname", [](raw::MjsMaterial& self) -> raw::MjsDefault* { @@ -966,7 +978,9 @@ PYBIND11_MODULE(_specs, m) { }); // ============================= MJSMESH ===================================== - mjsMesh.def("delete", [](raw::MjsMesh& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsMesh& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsMesh.def_property( "classname", [](raw::MjsMesh& self) -> raw::MjsDefault* { @@ -977,7 +991,9 @@ PYBIND11_MODULE(_specs, m) { }); // ============================= MJSPAIR ===================================== - mjsPair.def("delete", [](raw::MjsPair& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsPair& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsPair.def_property( "classname", [](raw::MjsPair& self) -> raw::MjsDefault* { @@ -988,8 +1004,9 @@ PYBIND11_MODULE(_specs, m) { }); // ============================= MJSEQUAL ==================================== - mjsEquality.def("delete", - [](raw::MjsEquality& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsEquality& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsEquality.def_property( "classname", [](raw::MjsEquality& self) -> raw::MjsDefault* { @@ -1000,8 +1017,9 @@ PYBIND11_MODULE(_specs, m) { }); // ============================= MJSACTUATOR ================================= - mjsActuator.def("delete", - [](raw::MjsActuator& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsActuator& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsActuator.def_property( "classname", [](raw::MjsActuator& self) -> raw::MjsDefault* { @@ -1101,8 +1119,9 @@ PYBIND11_MODULE(_specs, m) { py::arg("gain")); // ============================= MJSTENDON =================================== - mjsTendon.def("delete", - [](raw::MjsTendon& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsTendon& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsTendon.def( "default", [](raw::MjsTendon& self) -> raw::MjsDefault* { @@ -1135,39 +1154,54 @@ PYBIND11_MODULE(_specs, m) { py::return_value_policy::reference_internal); // ============================= MJSSENSOR =================================== - mjsSensor.def("delete", - [](raw::MjsSensor& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsSensor& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSFLEX ===================================== - mjsFlex.def("delete", [](raw::MjsFlex& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsFlex& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSHFIELD =================================== - mjsHField.def("delete", - [](raw::MjsHField& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsHField& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSSKIN ===================================== - mjsSkin.def("delete", [](raw::MjsSkin& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsSkin& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSTEXTURE ================================== - mjsTexture.def("delete", - [](raw::MjsTexture& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsTexture& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSKEY ====================================== - mjsKey.def("delete", [](raw::MjsKey& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsKey& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSTEXT ===================================== - mjsText.def("delete", [](raw::MjsText& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsText& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSNUMERIC ================================== - mjsNumeric.def("delete", - [](raw::MjsNumeric& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsNumeric& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSEXCLUDE ================================== - mjsExclude.def("delete", - [](raw::MjsExclude& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsExclude& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSTUPLE ==================================== - mjsTuple.def("delete", [](raw::MjsTuple& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsTuple& obj) { + mjs_delete(self.ptr, obj.element); + }); // ============================= MJSPLUGIN =================================== mjsPlugin.def_property( @@ -1176,8 +1210,9 @@ PYBIND11_MODULE(_specs, m) { [](raw::MjsPlugin& self, raw::MjsPlugin* other) { self.element = other->element; }); - mjsPlugin.def("delete", - [](raw::MjsPlugin& self) { mjs_delete(self.element); }); + mjSpec.def("delete", [](MjSpec& self, raw::MjsPlugin& obj) { + mjs_delete(self.ptr, obj.element); + }); mjsPlugin.def_property( "config", [](raw::MjsPlugin& self) -> py::dict { diff --git a/python/mujoco/specs_test.py b/python/mujoco/specs_test.py index 82e05e4a..4020d4f2 100644 --- a/python/mujoco/specs_test.py +++ b/python/mujoco/specs_test.py @@ -838,9 +838,9 @@ class SpecsTest(absltest.TestCase): self.assertIsNotNone(site) self.assertEqual(site, spec.site('head')) - site.delete() - spec.sensors[-1].delete() - spec.sensors[-1].delete() + spec.delete(site) + spec.delete(spec.sensors[-1]) + spec.delete(spec.sensors[-1]) model = spec.compile() self.assertIsNotNone(model) @@ -930,7 +930,7 @@ class SpecsTest(absltest.TestCase): """) plugin = spec.plugins[0] self.assertIsNotNone(plugin) - plugin.delete() + spec.delete(plugin) model = spec.compile() self.assertIsNotNone(model) diff --git a/src/user/user_api.cc b/src/user/user_api.cc index c8bde69c..a823573a 100644 --- a/src/user/user_api.cc +++ b/src/user/user_api.cc @@ -284,35 +284,18 @@ const char* mjs_getError(mjSpec* s) { // detach body from mjSpec, return 0 on success -int mjs_detachBody(mjSpec* s, mjsBody* b) { +int mjs_detach(mjSpec* s, mjsElement* element) { mjCModel* model = static_cast(s->element); - mjCBody* body = static_cast(b->element); - try { - *model -= *body; - } catch (mjCError& e) { - model->SetError(e); + if (!element) { + model->SetError(mjCError(0, "Element is null.")); return -1; } - model->Detach(body); - return 0; + mjCError e(0, "Detach is not implemented yet."); + model->SetError(e); + return -1; } -// detach default from mjSpec, return 0 on success -int mjs_detachDefault(mjSpec* s, mjsDefault* def) { - mjCModel* modelC = static_cast(s->element); - if (!def) { - modelC->SetError(mjCError(0, "Cannot detach, default is null")); - return -1; - } - mjCDef* defC = static_cast(def->element); - try { - *modelC -= *defC; - } catch (mjCError& e) { - modelC->SetError(e); - return -1; - } - return 0; -} + // check if model has warnings int mjs_isWarning(mjSpec* s) { @@ -372,15 +355,24 @@ int mj_copyBack(mjSpec* s, const mjModel* m) { // delete object, return 0 on success -int mjs_delete(mjsElement* element) { - mjCModel* model; - if (element->elemtype == mjOBJ_DEFAULT) - model = static_cast(element)->model; - else - model = static_cast(element)->model; +int mjs_delete(mjSpec* s, mjsElement* element) { + mjCModel* model = static_cast(s->element); + if (!element) { + model->SetError(mjCError(0, "Element is null.")); + return -1; + } try { - // it will call the appropriate destructor since ~mjCBase is virtual - model->DeleteElement(element); + if (element->elemtype == mjOBJ_BODY) { + mjCBody* body = static_cast(element); + *model -= *body; + model->DeleteElement(body); + } else if (element->elemtype == mjOBJ_DEFAULT) { + mjCDef* def = static_cast(element); + *model -= *def; + } else { + // it will call the appropriate destructor since ~mjCBase is virtual + model->DeleteElement(element); + } return 0; } catch (mjCError& e) { model->SetError(e); @@ -1035,7 +1027,7 @@ const char* mjs_resolveOrientation(double quat[4], mjtByte degree, const char* s mjsFrame* mjs_bodyToFrame(mjsBody** body) { mjCBody* bodyC = static_cast((*body)->element); mjCFrame* frameC = bodyC->ToFrame(); - bodyC->model->Detach(bodyC); + bodyC->model->DeleteElement((*body)->element); *body = nullptr; return &frameC->spec; } diff --git a/src/user/user_api.h b/src/user/user_api.h index 37825a76..89863d8f 100644 --- a/src/user/user_api.h +++ b/src/user/user_api.h @@ -76,11 +76,8 @@ MJAPI int mj_copyBack(mjSpec* s, const mjModel* m); MJAPI mjsElement* mjs_attach(mjsElement* parent, const mjsElement* child, const char* prefix, const char* suffix); -// Detach body from mjSpec, remove all references and delete the body, return 0 on success. -MJAPI int mjs_detachBody(mjSpec* s, mjsBody* b); - -// Detach default from mjSpec, remove all references and delete the default, return 0 on success. -MJAPI int mjs_detachDefault(mjSpec* s, mjsDefault* d); +// Detach but not delete object corresponding to the given element, return 0 on success. +MJAPI int mjs_detach(mjSpec* s, mjsElement* element); //---------------------------------- Add tree elements --------------------------------------------- @@ -109,7 +106,7 @@ MJAPI mjsLight* mjs_addLight(mjsBody* body, const mjsDefault* def); MJAPI mjsFrame* mjs_addFrame(mjsBody* body, mjsFrame* parentframe); // Delete object corresponding to the given element, return 0 on success. -MJAPI int mjs_delete(mjsElement* element); +MJAPI int mjs_delete(mjSpec* s, mjsElement* element); //---------------------------------- Add non-tree elements ----------------------------------------- diff --git a/src/user/user_model.cc b/src/user/user_model.cc index a02ad654..cc22560e 100644 --- a/src/user/user_model.cc +++ b/src/user/user_model.cc @@ -742,15 +742,44 @@ void deletefromlist(std::vector* list, mjsElement* element) { +// recursively delete all plugins in the subtree +static void deletesubtreeplugin(mjCBody* subtree, mjCModel* model) { + mjsPlugin* plugin = &(subtree->spec.plugin); + if (plugin->active && plugin->name->empty()) { + model->DeleteElement(plugin->element); + } + for (auto* body : subtree->Bodies()) { + deletesubtreeplugin(body, model); + } +} + + + // discard all invalid elements from all lists void mjCModel::DeleteElement(mjsElement* el) { ResetTreeLists(); + if (el->elemtype != mjOBJ_DEFAULT) { + if (static_cast(el)->model != this) { + throw mjCError(nullptr, "element is not in this model"); + } + } else { + if (static_cast(el)->model != this) { + throw mjCError(nullptr, "default is not in this model"); + } + } + switch (el->elemtype) { case mjOBJ_BODY: + { MakeTreeLists(); // rebuild lists that were reset at the beginning of the function - throw mjCError(nullptr, "bodies cannot be deleted, use detach instead"); + mjCBody* subtree = static_cast(el); + if (subtree->GetRef() == 1) { + deletesubtreeplugin(subtree, this); + } + subtree->Release(); break; + } case mjOBJ_DEFAULT: MakeTreeLists(); // rebuild lists that were reset at the beginning of the function @@ -828,29 +857,6 @@ void mjCModel::DeleteElement(mjsElement* el) { -// recursively delete all plugins in the subtree -void deletesubtreeplugin(mjCBody* subtree, mjCModel* model) { - mjsPlugin* plugin = &(subtree->spec.plugin); - if (plugin->active && plugin->name->empty()) { - model->DeleteElement(plugin->element); - } - for (auto* body : subtree->Bodies()) { - deletesubtreeplugin(body, model); - } -} - - - -// deletes all plugins in the subtree and then the subtree itself -void mjCModel::Detach(mjCBody* subtree) { - if (subtree->GetRef() == 1) { - deletesubtreeplugin(subtree, this); - } - subtree->Release(); -} - - - // TODO: we should not use C-type casting with multiple C++ inheritance void mjCModel::CreateObjectLists() { for (int i = 0; i < mjNOBJECT; ++i) { diff --git a/src/user/user_model.h b/src/user/user_model.h index cd03914d..e3f1d9c0 100644 --- a/src/user/user_model.h +++ b/src/user/user_model.h @@ -232,9 +232,6 @@ class mjCModel : public mjCModel_, private mjSpec { // delete default and all descendants void RemoveDefault(mjCDef* def); - // detach subtree from model - void Detach(mjCBody* subtree); - // API for access to model elements (outside tree) int NumObjects(mjtObj type); // number of objects in specified list mjCBase* GetObject(mjtObj type, int id); // pointer to specified object diff --git a/src/xml/xml_native_reader.cc b/src/xml/xml_native_reader.cc index e8a7dc13..f5994e0a 100644 --- a/src/xml/xml_native_reader.cc +++ b/src/xml/xml_native_reader.cc @@ -3554,7 +3554,7 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame, } // delete subtree - if (mjs_detachBody(spec, subtree)) { + if (mjs_delete(spec, subtree->element)) { throw mjXError(elem, mjs_getError(spec)); } } diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 52df9153..d0fc8e6e 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -219,7 +219,7 @@ TEST_F(PluginTest, DeletePlugin) { EXPECT_THAT(model->actuator_plugin[0], 0); // delete actuator - mjs_delete(actuator->element); + mjs_delete(spec, actuator->element); // recompile and check that the plugin is not present mjModel* newmodel = mj_compile(spec, NULL); @@ -326,7 +326,7 @@ TEST_F(PluginTest, DetachPlugin) { // detach the body and compile mjsBody* body_to_detach = mjs_findBody(parent, "child-body"); EXPECT_THAT(body_to_detach, NotNull()); - EXPECT_THAT(mjs_detachBody(parent, body_to_detach), 0); + EXPECT_THAT(mjs_delete(parent, body_to_detach->element), 0); mjModel* model = mj_compile(parent, nullptr); EXPECT_THAT(model, NotNull()); EXPECT_THAT(model->nbody, 2); @@ -1368,12 +1368,8 @@ void TestDetachBody(bool compile) { mjsBody* body = mjs_findBody(child, "body"); EXPECT_THAT(body, NotNull()); - // get an error if trying to delete the body - EXPECT_EQ(mjs_delete(body->element), -1); - EXPECT_THAT(mjs_getError(child), HasSubstr("use detach instead")); - - // detach subtree - EXPECT_THAT(mjs_detachBody(child, body), 0); + // delete subtree + EXPECT_THAT(mjs_delete(child, body->element), 0); // try saving to XML before compiling again std::array e; @@ -1898,12 +1894,12 @@ TEST_F(MujocoTest, PreserveState) { // detach subtree mjsBody* body = mjs_findBody(spec, "detachable"); EXPECT_THAT(body, NotNull()); - EXPECT_THAT(mjs_detachBody(spec, body), 0); + EXPECT_THAT(mjs_delete(spec, body->element), 0); // detach mocap mjsBody* mocap_body = mjs_findBody(spec, "mocap_detach"); EXPECT_THAT(mocap_body, NotNull()); - EXPECT_THAT(mjs_detachBody(spec, mocap_body), 0); + EXPECT_THAT(mjs_delete(spec, mocap_body->element), 0); // add body mjsBody* newbody = mjs_addBody(mjs_findBody(spec, "world"), 0); @@ -2787,25 +2783,20 @@ TEST_F(MujocoTest, DetachDefault) { mjsDefault* child = mjs_findDefault(spec, "child1"); EXPECT_THAT(child, NotNull()); - // try using mjs_delete to remove default, should fail - EXPECT_EQ(mjs_delete(child->element), -1); - - // detach default - EXPECT_EQ(mjs_detachDefault(spec, child), 0); + // delete default + EXPECT_EQ(mjs_delete(spec, child->element), 0); child = mjs_findDefault(spec, "child1"); EXPECT_THAT(child, IsNull()); // try and detach previously detached default, should fail - EXPECT_EQ(mjs_detachDefault(spec, child), -1); + EXPECT_EQ(mjs_delete(spec, nullptr), -1); child = mjs_findDefault(spec, "child1"); EXPECT_THAT(child, IsNull()); - EXPECT_THAT(mjs_getError(spec), - HasSubstr("Cannot detach, default is null")); // detach parent mjsDefault* parent = mjs_findDefault(spec, "parent"); EXPECT_THAT(parent, NotNull()); - mjs_detachDefault(spec, parent); + mjs_delete(spec, parent->element); // both parent and remaining child should be removed parent = mjs_findDefault(spec, "parent"); @@ -2816,7 +2807,7 @@ TEST_F(MujocoTest, DetachDefault) { // error when trying to detach the 'main' default mjsDefault* main = mjs_findDefault(spec, "main"); EXPECT_THAT(main, NotNull()); - EXPECT_EQ(mjs_detachDefault(spec, main), -1); + EXPECT_EQ(mjs_delete(spec, main->element), -1); EXPECT_THAT(mjs_getError(spec), HasSubstr("cannot remove the global default ('main')"));