From d8b85245a77b88b24811fa0f3ceb4e2c7afdc10e Mon Sep 17 00:00:00 2001 From: Yuval Tassa Date: Tue, 25 Jun 2024 01:45:10 -0700 Subject: [PATCH] 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 --- src/user/user_model.cc | 1 + src/xml/xml_native_reader.cc | 15 ++++--- test/xml/xml_native_reader_test.cc | 72 ++++++++++++++++++++++++++++++ test/xml/xml_native_writer_test.cc | 6 ++- 4 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/user/user_model.cc b/src/user/user_model.cc index 7bd1ebbe0e..23f946348a 100644 --- 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 index c28cc35f3a..250f2bfe17 100644 --- a/src/xml/xml_native_reader.cc +++ b/src/xml/xml_native_reader.cc @@ -2673,17 +2673,15 @@ 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"); @@ -2691,7 +2689,9 @@ void mjXReader::Default(XMLElement* section, int parentid) { } 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 index 1d13f7d1ef..f71e66fa69 100644 --- 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"( + + + + + + + + + + + + + + + + + + + + + )"; + std::array 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"( @@ -511,6 +544,45 @@ TEST_F(XMLReaderTest, InvalidDefaultClassName) { HasSubstr("Element 'geom'"), HasSubstr("line 10"))); } +TEST_F(XMLReaderTest, InvalidTopDefaultClassName) { + static constexpr char xml[] = R"( + + + + + + + + + + + )"; + std::array 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"( + + + + + + + + + + + )"; + std::array 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 index 09176bd51a..96134397a8 100644 --- 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"( - - + + + +