diff --git a/doc/APIreference/functions.rst b/doc/APIreference/functions.rst index 53c616a8..abe36ac8 100644 --- a/doc/APIreference/functions.rst +++ b/doc/APIreference/functions.rst @@ -4235,7 +4235,7 @@ Attribute setters .. mujoco-include:: mjs_setName -Set element's name. +Set element's name, return 0 on success. .. _mjs_setBuffer: diff --git a/doc/changelog.rst b/doc/changelog.rst index 2f8e4375..2fdd87d5 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -17,6 +17,9 @@ General :ref:`visual/global/cameraid`. - Added support to only sync the state in the Python :ref:`passive viewer`'s ``Sync`` method, this is useful to improve performance. The default behavior is unchanged and copies the entire model and data. +- In the mjSpec C API, directly setting an element's name using :ref:`mjs_setString` has been replaced with a new + function :ref:`mjs_setName` which allows checking for naming collisions at set-time rather than compile-time, for + earlier catching of errors. Bug fixes ^^^^^^^^^ diff --git a/doc/includes/references.h b/doc/includes/references.h index c5d868be..d107fd60 100644 --- a/doc/includes/references.h +++ b/doc/includes/references.h @@ -3456,7 +3456,7 @@ mjsElement* mjs_firstChild(mjsBody* body, mjtObj type, int recurse); mjsElement* mjs_nextChild(mjsBody* body, mjsElement* child, int recurse); mjsElement* mjs_firstElement(mjSpec* s, mjtObj type); mjsElement* mjs_nextElement(mjSpec* s, mjsElement* element); -void mjs_setName(mjsElement* element, const char* name); +int mjs_setName(mjsElement* element, const char* name); void mjs_setBuffer(mjByteVec* dest, const void* array, int size); void mjs_setString(mjString* dest, const char* text); void mjs_setStringVec(mjStringVec* dest, const char* text); diff --git a/include/mujoco/mujoco.h b/include/mujoco/mujoco.h index 295d9a19..9291935f 100644 --- a/include/mujoco/mujoco.h +++ b/include/mujoco/mujoco.h @@ -1600,8 +1600,8 @@ MJAPI mjsElement* mjs_nextElement(mjSpec* s, mjsElement* element); //---------------------------------- Attribute setters --------------------------------------------- -// Set element's name. -MJAPI void mjs_setName(mjsElement* element, const char* name); +// Set element's name, return 0 on success. +MJAPI int mjs_setName(mjsElement* element, const char* name); // Copy buffer. MJAPI void mjs_setBuffer(mjByteVec* dest, const void* array, int size); diff --git a/python/mujoco/codegen/generate_spec_bindings.py b/python/mujoco/codegen/generate_spec_bindings.py index d94c06b9..e7b13691 100644 --- a/python/mujoco/codegen/generate_spec_bindings.py +++ b/python/mujoco/codegen/generate_spec_bindings.py @@ -677,7 +677,9 @@ def generate_name() -> None: return mjs_getName(self.element); }}, [](raw::{titlecase}& self, std::string& name) -> void {{ - mjs_setName(self.element, name.c_str()); + if (mjs_setName(self.element, name.c_str())) {{ + throw pybind11::value_error(mjs_getError(mjs_getSpec(self.element))); + }} }}, py::return_value_policy::reference_internal); """ print(code) diff --git a/python/mujoco/introspect/functions.py b/python/mujoco/introspect/functions.py index 987a4ce8..27ec5c2e 100644 --- a/python/mujoco/introspect/functions.py +++ b/python/mujoco/introspect/functions.py @@ -10194,7 +10194,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([ ('mjs_setName', FunctionDecl( name='mjs_setName', - return_type=ValueType(name='void'), + return_type=ValueType(name='int'), parameters=( FunctionParameterDecl( name='element', @@ -10209,7 +10209,7 @@ FUNCTIONS: Mapping[str, FunctionDecl] = dict([ ), ), ), - doc="Set element's name.", + doc="Set element's name, return 0 on success.", )), ('mjs_setBuffer', FunctionDecl( diff --git a/python/mujoco/specs_test.py b/python/mujoco/specs_test.py index 7f444c73..257bdd6b 100644 --- a/python/mujoco/specs_test.py +++ b/python/mujoco/specs_test.py @@ -887,7 +887,7 @@ class SpecsTest(absltest.TestCase): ): s.compile() - def test_recompile_error(self): + def test_duplicate_name_error(self): main_xml = """ @@ -899,16 +899,11 @@ class SpecsTest(absltest.TestCase): """ spec = mujoco.MjSpec.from_string(main_xml) - model = spec.compile() - data = mujoco.MjData(model) - spec.add_material().name = 'yellow' - spec.add_material().name = 'yellow' - with self.assertRaisesRegex( ValueError, "Error: repeated name 'yellow' in material" ): - spec.recompile(model, data) + spec.add_material().name = 'yellow' def test_delete_unused_plugin(self): spec = mujoco.MjSpec.from_string(""" diff --git a/src/user/user_api.cc b/src/user/user_api.cc index 512f752a..4fcc9c1a 100644 --- a/src/user/user_api.cc +++ b/src/user/user_api.cc @@ -1342,14 +1342,21 @@ mjsPlugin* mjs_asPlugin(mjsElement* element) { // set element name -void mjs_setName(mjsElement* element, const char* name) { +int mjs_setName(mjsElement* element, const char* name) { if (element->elemtype == mjOBJ_DEFAULT) { mjCDef* def = static_cast(element); def->name = std::string(name); - return; + return 0; } mjCBase* baseC = static_cast(element); baseC->name = std::string(name); + try { + baseC->model->CheckRepeat(element->elemtype); + } catch (mjCError& e) { + baseC->model->SetError(e); + return -1; + } + return 0; } diff --git a/src/user/user_api.h b/src/user/user_api.h index 658ae980..d112fc8e 100644 --- a/src/user/user_api.h +++ b/src/user/user_api.h @@ -339,8 +339,8 @@ MJAPI mjsPlugin* mjs_asPlugin(mjsElement* element); //---------------------------------- Attribute setters --------------------------------------------- -// Set element's name. -MJAPI void mjs_setName(mjsElement* element, const char* name); +// Set element's name, return 0 on success. +MJAPI int mjs_setName(mjsElement* element, const char* name); // Copy buffer. MJAPI void mjs_setBuffer(mjByteVec* dest, const void* array, int size); diff --git a/src/user/user_model.cc b/src/user/user_model.cc index 25931205..cd765679 100644 --- a/src/user/user_model.cc +++ b/src/user/user_model.cc @@ -116,50 +116,6 @@ bool IsNullPose(const T pos[3], const T quat[4]) { return IsSamePose(pos, zero, quat, qunit); } - -// set ids, check for repeated names -template -static void processlist(mjListKeyMap& ids, vector& list, - mjtObj type, bool checkrepeat = true) { - // assign ids for regular elements - if (type < mjNOBJECT) { - for (size_t i=0; i < list.size(); i++) { - // check for incompatible id setting; SHOULD NOT OCCUR - if (list[i]->id != -1 && list[i]->id != i) { - throw mjCError(list[i], "incompatible id in %s array, position %d", mju_type2Str(type), i); - } - - // id equals position in array - list[i]->id = i; - - // add to ids map - ids[type][list[i]->name] = i; - } - } - - // check for repeated names - if (checkrepeat) { - // created vectors with all names - vector allnames; - for (size_t i=0; i < list.size(); i++) { - if (!list[i]->name.empty()) { - allnames.push_back(list[i]->name); - } - } - - // sort and check for duplicates - if (allnames.size() > 1) { - std::sort(allnames.begin(), allnames.end()); - auto adjacent = std::adjacent_find(allnames.begin(), allnames.end()); - if (adjacent != allnames.end()) { - string msg = "repeated name '" + *adjacent + "' in " + mju_type2Str(type); - throw mjCError(nullptr, "%s", msg.c_str()); - } - } - } -} - - } // namespace //---------------------------------- CONSTRUCTOR AND DESTRUCTOR ------------------------------------ @@ -308,7 +264,7 @@ void mjCModel::CopyList(std::vector& dest, dest.back()->CopyPlugin(); } if (!dest.empty()) { - processlist(ids, dest, dest[0]->elemtype); + ProcessList_(ids, dest, dest[0]->elemtype); } } @@ -548,7 +504,7 @@ void mjCModel::RemoveFromList(std::vector& list, const mjCModel& other) { } if (removed > 0 && !list.empty()) { // if any elements were removed, update ids using processlist - processlist(ids, list, list[0]->elemtype, /*checkrepeat=*/false); + ProcessList_(ids, list, list[0]->elemtype, /*checkrepeat=*/false); } } @@ -604,7 +560,7 @@ void mjCModel::RemovePlugins() { // if any elements were removed, update ids using processlist if (removed > 0 && !plugins_.empty()) { - processlist(ids, plugins_, plugins_[0]->elemtype, /*checkrepeat=*/false); + ProcessList_(ids, plugins_, plugins_[0]->elemtype, /*checkrepeat=*/false); } } @@ -4052,7 +4008,7 @@ void mjCModel::FuseStatic(void) { } // remove empty names - processlist(ids, bodies_, mjOBJ_BODY, true); + ProcessList_(ids, bodies_, mjOBJ_BODY, /*checkrepeat=*/true); } @@ -4083,12 +4039,70 @@ void mjCModel::ProcessLists(bool checkrepeat) { for (int i = 0; i < mjNOBJECT; i++) { if (i != mjOBJ_XBODY && object_lists_[i]) { ids[i].clear(); - processlist(ids, *object_lists_[i], (mjtObj) i, checkrepeat); + ProcessList_(ids, *object_lists_[i], (mjtObj) i, checkrepeat); } } // check repeated names in meta elements - processlist(ids, frames_, mjOBJ_FRAME, checkrepeat); + ProcessList_(ids, frames_, mjOBJ_FRAME, checkrepeat); +} + + + +// set ids, check for repeated names +template +void mjCModel::ProcessList_(mjListKeyMap& ids, vector& list, + mjtObj type, bool checkrepeat) { + // assign ids for regular elements + if (type < mjNOBJECT) { + for (size_t i=0; i < list.size(); i++) { + // check for incompatible id setting; SHOULD NOT OCCUR + if (list[i]->id != -1 && list[i]->id != i) { + throw mjCError(list[i], "incompatible id in %s array, position %d", mju_type2Str(type), i); + } + + // id equals position in array + list[i]->id = i; + + // add to ids map + ids[type][list[i]->name] = i; + } + } + + // check for repeated names + if (checkrepeat) { + CheckRepeat(type); + } +} + + + +// check for repeated names in list +void mjCModel::CheckRepeat(mjtObj type) { + std::vector* list = nullptr; + if (type < mjNOBJECT) { + list = object_lists_[type]; + } else if (type == mjOBJ_FRAME) { + list = (std::vector*) &frames_; + } + + // created vectors with all names + vector allnames; + for (size_t i=0; i < list->size(); i++) { + if (!(*list)[i]->name.empty()) { + allnames.push_back((*list)[i]->name); + } + } + + // sort and check for duplicates + if (allnames.size() > 1) { + std::sort(allnames.begin(), allnames.end()); + auto adjacent = std::adjacent_find(allnames.begin(), allnames.end()); + if (adjacent != allnames.end()) { + string msg = "repeated name '" + *adjacent + "' in " + mju_type2Str(type); + throw mjCError(nullptr, "%s", msg.c_str()); + } + } } diff --git a/src/user/user_model.h b/src/user/user_model.h index b5e937bd..288446ec 100644 --- a/src/user/user_model.h +++ b/src/user/user_model.h @@ -323,6 +323,9 @@ class mjCModel : public mjCModel_, private mjSpec { // set attached flag void SetAttached(bool deepcopy) { attached_ |= !deepcopy; } + // check for repeated names in list + void CheckRepeat(mjtObj type); + private: // settings for each defaults class std::vector defaults_; @@ -411,6 +414,10 @@ class mjCModel : public mjCModel_, private mjSpec { // populate objects ids void ProcessLists(bool checkrepeat = true); + // process list of objects + template void ProcessList_(mjListKeyMap& ids, std::vector& list, + mjtObj type, bool checkrepeat = true); + // reset lists of kinematic tree void ResetTreeLists(); diff --git a/src/user/user_objects.cc b/src/user/user_objects.cc index e1ff3a88..d43a2f49 100644 --- a/src/user/user_objects.cc +++ b/src/user/user_objects.cc @@ -1403,7 +1403,6 @@ mjCJoint* mjCBody::AddFreeJoint() { model->ResetTreeLists(); model->MakeTreeLists(); - // update signature model->spec.element->signature = model->Signature(); return obj; @@ -1425,7 +1424,6 @@ mjCJoint* mjCBody::AddJoint(mjCDef* _def) { model->ResetTreeLists(); model->MakeTreeLists(); - // update signature model->spec.element->signature = model->Signature(); return obj; @@ -1447,7 +1445,6 @@ mjCGeom* mjCBody::AddGeom(mjCDef* _def) { model->ResetTreeLists(); model->MakeTreeLists(); - // update signature model->spec.element->signature = model->Signature(); return obj; @@ -1469,7 +1466,6 @@ mjCSite* mjCBody::AddSite(mjCDef* _def) { model->ResetTreeLists(); model->MakeTreeLists(); - // update signature model->spec.element->signature = model->Signature(); return obj; @@ -1491,7 +1487,6 @@ mjCCamera* mjCBody::AddCamera(mjCDef* _def) { model->ResetTreeLists(); model->MakeTreeLists(); - // update signature model->spec.element->signature = model->Signature(); return obj; @@ -1513,7 +1508,6 @@ mjCLight* mjCBody::AddLight(mjCDef* _def) { model->ResetTreeLists(); model->MakeTreeLists(); - // update signature model->spec.element->signature = model->Signature(); return obj;