Informative error if mis-naming the top-level default class.

Also: - Move the the top-level default class name into the mjCModel constructor. - Fix bug in default class propagation in frame elements. PiperOrigin-RevId: 646384669 Change-Id: Icc6154124361a234b84a07e2b5ecaa7c8b18ebf7
google-deepmind · Jun 25, 2024 · d8b8524 · d8b8524
1 parent 9e9a0c6
commit d8b8524
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 8 deletions.
diff --git a/src/user/user_model.cc b/src/user/user_model.cc
@@ -97,6 +97,7 @@ mjCModel::mjCModel() {
 
   //------------------------ master default set
   defaults_.push_back(new mjCDef);
+  defaults_.back()->name = "main";
 
   // world body
   mjCBody* world = new mjCBody(this);

diff --git a/src/xml/xml_native_reader.cc b/src/xml/xml_native_reader.cc
@@ -2673,25 +2673,25 @@ void mjXReader::Default(XMLElement* section, int parentid) {
   mjsDefault* def;
   int thisid;
 
-  // create new default, except at top level (already added in mjCModel ctor)
+  // create new default, except at top level (already added in mjCModel constructor)
   text.clear();
   ReadAttrTxt(section, "class", text);
   if (text.empty()) {
-    if (parentid>=0) {
+    if (parentid >= 0) {
       throw mjXError(section, "empty class name");
-    } else {
-      text = "main";
     }
   }
-  if (parentid>=0) {
+  if (parentid >= 0) {
     def = mjs_addDefault(model, text.c_str(), parentid, &thisid);
     if (!def) {
       throw mjXError(section, "repeated default class name");
     }
   } else {
     thisid = 0;
     def = mjs_getSpecDefault(model);
-    mjs_setString(def->name, text.c_str());
+    if (!text.empty() && text != "main") {
+      throw mjXError(section, "top-level default class 'main' cannot be renamed");
+    }
   }
 
   // iterate over elements other than nested defaults
@@ -3497,6 +3497,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* pbody, mjsFrame* frame) {
       mjs_setString(pchild->info,
                     std::string("line " + std::to_string(elem->GetLineNum())).c_str());
 
+      // set default from class or childclass
+      mjs_setDefault(pchild->element, childdef ? childdef : def);
+
       // read attributes
       std::string name, childclass;
       if (ReadAttrTxt(elem, "name", name)) {

diff --git a/test/xml/xml_native_reader_test.cc b/test/xml/xml_native_reader_test.cc
@@ -464,6 +464,39 @@ TEST_F(XMLReaderTest, InvalidDoubleOrientation) {
   }
 }
 
+TEST_F(XMLReaderTest, ClassOverridesChildclass) {
+  static constexpr char xml[] = R"(
+  <mujoco>
+    <default>
+      <default class="size2">
+        <geom size="2"/>
+      </default>
+      <default class="size3">
+        <geom size="3"/>
+      </default>
+    </default>
+    <worldbody>
+      <frame childclass="size2">
+        <geom/>
+        <geom class="size3"/>
+        <body childclass="size2">
+          <geom/>
+          <geom class="size3"/>
+        </body>
+      </frame>
+    </worldbody>
+  </mujoco>
+  )";
+  std::array<char, 1024> error;
+  mjModel* model = LoadModelFromString(xml, error.data(), error.size());
+  ASSERT_THAT(model, NotNull()) << error.data();
+  EXPECT_EQ(model->geom_size[3*0], 2);
+  EXPECT_EQ(model->geom_size[3*1], 3);
+  EXPECT_EQ(model->geom_size[3*2], 2);
+  EXPECT_EQ(model->geom_size[3*3], 3);
+  mj_deleteModel(model);
+}
+
 TEST_F(XMLReaderTest, RepeatedDefaultName) {
   static constexpr char xml[] = R"(
   <mujoco>
@@ -511,6 +544,45 @@ TEST_F(XMLReaderTest, InvalidDefaultClassName) {
                     HasSubstr("Element 'geom'"), HasSubstr("line 10")));
 }
 
+TEST_F(XMLReaderTest, InvalidTopDefaultClassName) {
+  static constexpr char xml[] = R"(
+  <mujoco>
+    <default class="sphere">
+      <geom type="sphere" size="1"/>
+    </default>
+    <worldbody>
+      <body>
+        <geom class="sphere"/>
+      </body>
+    </worldbody>
+  </mujoco>
+  )";
+  std::array<char, 1024> error;
+  mjModel* model = LoadModelFromString(xml, error.data(), error.size());
+  ASSERT_THAT(model, IsNull()) << error.data();
+  EXPECT_THAT(error.data(),
+              HasSubstr("top-level default class 'main' cannot be renamed"));
+}
+
+TEST_F(XMLReaderTest, ValidTopDefaultClassName) {
+  static constexpr char xml[] = R"(
+  <mujoco>
+    <default class="main">
+      <geom type="sphere" size="1"/>
+    </default>
+    <worldbody>
+      <body>
+        <geom class="main"/>
+      </body>
+    </worldbody>
+  </mujoco>
+  )";
+  std::array<char, 1024> error;
+  mjModel* model = LoadModelFromString(xml, error.data(), error.size());
+  ASSERT_THAT(model, NotNull()) << error.data();
+  mj_deleteModel(model);
+}
+
 // ------------------------ test including -------------------------------------
 
 // tiny RGB 2 x 3 PNG file

diff --git a/test/xml/xml_native_writer_test.cc b/test/xml/xml_native_writer_test.cc
@@ -727,8 +727,10 @@ TEST_F(XMLWriterTest, WritesActuatorDefaults) {
 TEST_F(XMLWriterTest, WritesFrameDefaults) {
   static constexpr char xml[] = R"(
   <mujoco>
-    <default class="dframe">
-      <geom size=".1"/>
+    <default>
+      <default class="dframe">
+        <geom size=".1"/>
+      </default>
     </default>
 
     <worldbody>