Check for duplicated names when setting element names.

PiperOrigin-RevId: 778827590
Change-Id: I77a208c1571cf735cae80840ea830aa347681f71

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:
 

doc/changelog.rst

@@ -17,6 +17,9 @@ General
   :ref:`visual/global/cameraid<visual-global-cameraid>`.
 - Added support to only sync the state in the Python :ref:`passive viewer<PyViewerPassive>`'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
 ^^^^^^^^^

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);

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);

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)

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(

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 = """
     <mujoco>
       <worldbody>
@@ -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("""

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<mjCDef*>(element);
     def->name = std::string(name);
-    return;
+    return 0;
   }
   mjCBase* baseC = static_cast<mjCBase*>(element);
   baseC->name = std::string(name);
+  try {
+    baseC->model->CheckRepeat(element->elemtype);
+  } catch (mjCError& e) {
+    baseC->model->SetError(e);
+    return -1;
+  }
+  return 0;
 }
 
 

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);

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 <class T>
-static void processlist(mjListKeyMap& ids, vector<T*>& 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<string> 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<T*>& 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<T*>& 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 <class T>
+void mjCModel::ProcessList_(mjListKeyMap& ids, vector<T*>& 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<mjCBase*>* list = nullptr;
+  if (type < mjNOBJECT) {
+    list = object_lists_[type];
+  } else if (type == mjOBJ_FRAME) {
+    list = (std::vector<mjCBase*>*) &frames_;
+  }
+
+  // created vectors with all names
+  vector<string> 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());
+    }
+  }
 }
 
 

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<mjCDef*> defaults_;
@@ -411,6 +414,10 @@ class mjCModel : public mjCModel_, private mjSpec {
   // populate objects ids
   void ProcessLists(bool checkrepeat = true);
 
+  // process list of objects
+  template <class T> void ProcessList_(mjListKeyMap& ids, std::vector<T*>& list,
+                                       mjtObj type, bool checkrepeat = true);
+
   // reset lists of kinematic tree
   void ResetTreeLists();
 

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;