Require prefix for the attach tag.

For `mjs_attachFrame` and `mjs_attachBody`, require either a prefix or suffix.

It is tempting for users to not use a prefix (see #2162), but this causes issues. For example, it creates a duplicate "main" default class in the parent as a child of the already existing "main" default.

PiperOrigin-RevId: 688535755
Change-Id: I653181ca35943f1e4eb89c70fca8b5289818324e

doc/XMLreference.rst

@@ -3861,19 +3861,19 @@ all attachments will appear in the saved XML file.
 
 .. _body-attach-model:
 
-:at:`model`: :at-val:`string, optional`
+:at:`model`: :at-val:`string, required`
    The sub-model from which to attach a subtree.
 
 .. _body-attach-body:
 
-:at:`body`: :at-val:`string, optional`
+:at:`body`: :at-val:`string, required`
    Name of the body in the sub-model to attach here. The body and its subtree will be attached.
 
 .. _body-attach-prefix:
 
-:at:`prefix`: :at-val:`string, optional`
+:at:`prefix`: :at-val:`string, required`
-   Prefix to prepend to names of elements in the sub-model. If empty, the names are unchanged. This attribute is
+   Prefix to prepend to names of elements in the sub-model. This attribute is required to prevent name collisions with
-   required to prevent name collisions with the parent or when attaching the same sub-tree multiple times.
+   the parent or when attaching the same sub-tree multiple times.
 
 
 .. _body-frame:

python/mujoco/specs_test.py

@@ -850,7 +850,7 @@ class SpecsTest(absltest.TestCase):
     body = parent.worldbody.add_body()
     frame = child.worldbody.add_frame()
     with self.assertRaises(ValueError) as cm:
-      body.attach_frame(frame, '', '')
+      body.attach_frame(frame, '_', '')
     self.assertEqual(
         str(cm.exception),
         'Error: cannot attach mjSpecs with incompatible compiler/angle'
@@ -862,7 +862,7 @@ class SpecsTest(absltest.TestCase):
     parent = mujoco.MjSpec()
     site = parent.worldbody.add_site(pos=[1, 2, 3])
     body = child.worldbody.add_body()
-    self.assertIsNotNone(site.attach(body, '', ''))
+    self.assertIsNotNone(site.attach(body, '_', ''))
     model = parent.compile()
     np.testing.assert_array_equal(model.body_pos[1], [1, 2, 3])
 

src/user/user_objects.cc

@@ -850,6 +850,10 @@ mjCBody& mjCBody::operator+=(const mjCFrame& other) {
   other.model->suffix = other.suffix;
   other.model->StoreKeyframes(model);
 
+  if (other.prefix.empty() && other.suffix.empty()) {
+    throw mjCError(this, "either prefix or suffix must be non-empty");
+  }
+
   // attach defaults
   if (other.model != model) {
     mjCDef* subdef = new mjCDef(*other.model->Default());
@@ -1812,6 +1816,10 @@ mjCFrame& mjCFrame::operator+=(const mjCBody& other) {
   other.model->prefix = "";
   other.model->suffix = "";
 
+  if (other.prefix.empty() && other.suffix.empty()) {
+    throw mjCError(this, "either prefix or suffix must be non-empty");
+  }
+
   mjCBody* subtree = new mjCBody(other, model);
   other.ForgetKeyframes();
   other.model->prefix = subtree->prefix;

src/xml/xml_native_reader.cc

@@ -3674,9 +3674,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
     // attachment
     else if (name=="attach") {
       string model_name, body_name, prefix;
-      ReadAttrTxt(elem, "model", model_name);
+      ReadAttrTxt(elem, "model", model_name, /*required=*/true);
-      ReadAttrTxt(elem, "body", body_name);
+      ReadAttrTxt(elem, "body", body_name, /*required=*/true);
-      ReadAttrTxt(elem, "prefix", prefix);
+      ReadAttrTxt(elem, "prefix", prefix, /*required=*/true);
 
       mjsBody* child = mjs_findBody(spec, (prefix+body_name).c_str());
       mjsFrame* pframe = frame ? frame : mjs_addFrame(body, nullptr);
@@ -3684,11 +3684,11 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
       if (!child) {
         mjSpec* asset = mjs_findSpec(spec, model_name.c_str());
         if (!asset) {
-          throw mjXError(0, "could not find model '%s'", model_name.c_str());
+          throw mjXError(elem, "could not find model '%s'", model_name.c_str());
         }
         child = mjs_findBody(asset, body_name.c_str());
         if (!child) {
-          throw mjXError(0, "could not find body '%s''%s'", body_name.c_str());
+          throw mjXError(elem, "could not find body '%s''%s'", body_name.c_str());
         }
         if (!mjs_attachBody(pframe, child, prefix.c_str(), "")) {
           throw mjXError(elem, mjs_getError(spec));

test/xml/xml_native_reader_test.cc

@@ -1544,12 +1544,12 @@ TEST_F(XMLReaderTest, InvalidAttach) {
       <body name="parent">
         <joint name="joint1"/>
         <geom size="2"/>
-        <attach model="other" body="body"/>
+        <attach model="other" body="body" prefix="_"/>
       </body>
     </worldbody>
 
     <actuator>
-      <motor name="actuator" joint="joint1"/>
+      <motor name="_actuator" joint="joint1"/>
     </actuator>
   </mujoco>
   )";
@@ -1578,7 +1578,7 @@ TEST_F(XMLReaderTest, InvalidAttach) {
       LoadModelFromString(xml_parent, er.data(), er.size(), vfs.get());
 
   EXPECT_THAT(model, IsNull()) << er.data();
-  EXPECT_THAT(er.data(), HasSubstr("repeated name 'actuator' in actuator"));
+  EXPECT_THAT(er.data(), HasSubstr("repeated name '_actuator' in actuator"));
   EXPECT_THAT(er.data(), HasSubstr("Element 'attach'"));
   mj_deleteVFS(vfs.get());
 }