diff --git a/cmake/MujocoLinkOptions.cmake b/cmake/MujocoLinkOptions.cmake index 242767f9..a844ce45 100644 --- a/cmake/MujocoLinkOptions.cmake +++ b/cmake/MujocoLinkOptions.cmake @@ -18,11 +18,12 @@ include(CheckCSourceCompiles) # linker. function(get_mujoco_extra_link_options OUTPUT_VAR) if(MSVC) - set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5) + set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5 /STACK:16777216) else() set(EXTRA_LINK_OPTIONS) if(WIN32) + set(EXTRA_LINK_OPTIONS ${EXTRA_LINK_OPTIONS} -Wl,/STACK:16777216) set(CMAKE_REQUIRED_FLAGS "-fuse-ld=lld-link") check_c_source_compiles("int main() {}" SUPPORTS_LLD) if(SUPPORTS_LLD) diff --git a/doc/changelog.rst b/doc/changelog.rst index 312b24d7..bab8cc13 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -2,6 +2,15 @@ Changelog ========= +Upcoming version (not yet released) +----------------------------------- + +General +^^^^^^^^^ + +- Raise an error if there are name collisions also during parsing. +- Increase Windows stack size to 16MB to enable models with deep nested body hierarchies. + Version 3.3.7 (October 13, 2025) ----------------------------------- diff --git a/sample/cmake/MujocoLinkOptions.cmake b/sample/cmake/MujocoLinkOptions.cmake index 242767f9..a844ce45 100644 --- a/sample/cmake/MujocoLinkOptions.cmake +++ b/sample/cmake/MujocoLinkOptions.cmake @@ -18,11 +18,12 @@ include(CheckCSourceCompiles) # linker. function(get_mujoco_extra_link_options OUTPUT_VAR) if(MSVC) - set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5) + set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5 /STACK:16777216) else() set(EXTRA_LINK_OPTIONS) if(WIN32) + set(EXTRA_LINK_OPTIONS ${EXTRA_LINK_OPTIONS} -Wl,/STACK:16777216) set(CMAKE_REQUIRED_FLAGS "-fuse-ld=lld-link") check_c_source_compiles("int main() {}" SUPPORTS_LLD) if(SUPPORTS_LLD) diff --git a/simulate/cmake/MujocoLinkOptions.cmake b/simulate/cmake/MujocoLinkOptions.cmake index 242767f9..a844ce45 100644 --- a/simulate/cmake/MujocoLinkOptions.cmake +++ b/simulate/cmake/MujocoLinkOptions.cmake @@ -18,11 +18,12 @@ include(CheckCSourceCompiles) # linker. function(get_mujoco_extra_link_options OUTPUT_VAR) if(MSVC) - set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5) + set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5 /STACK:16777216) else() set(EXTRA_LINK_OPTIONS) if(WIN32) + set(EXTRA_LINK_OPTIONS ${EXTRA_LINK_OPTIONS} -Wl,/STACK:16777216) set(CMAKE_REQUIRED_FLAGS "-fuse-ld=lld-link") check_c_source_compiles("int main() {}" SUPPORTS_LLD) if(SUPPORTS_LLD) diff --git a/src/user/user_api.cc b/src/user/user_api.cc index 6e2e2ee3..98033998 100644 --- a/src/user/user_api.cc +++ b/src/user/user_api.cc @@ -279,6 +279,10 @@ mjsElement* mjs_attach(mjsElement* parent, const mjsElement* child, // get error message from model const char* mjs_getError(mjSpec* s) { + if (!s) { + mju_error("spec is null"); + return nullptr; + } mjCModel* modelC = static_cast(s->element); return modelC->GetError().message; } diff --git a/src/user/user_model.cc b/src/user/user_model.cc index c7a7fd61..d9d82a37 100644 --- a/src/user/user_model.cc +++ b/src/user/user_model.cc @@ -27,6 +27,7 @@ #include // NOLINT(build/c++17) #include #include +#include #include #include #include @@ -4999,78 +5000,98 @@ void mjCModel::TryCompile(mjModel*& m, mjData*& d, const mjVFS* vfs) { -std::string mjCModel::PrintTree(const mjCBody* body, std::string indent) { - std::string tree; - tree += indent + "\n"; - indent += " "; +static void PrintIndent(std::stringstream& ss, int depth) { + // A static string of spaces, created only once during the program's lifetime. + static const std::string spaces(1024, ' '); + + if (depth > 0) { + // Write 'depth * 2' spaces directly to the stringstream + // without creating any new std::string objects. + ss.write(spaces.c_str(), std::min((size_t)depth * 2, spaces.length())); + } +} + + + +void mjCModel::PrintTree(std::stringstream& tree, const mjCBody* body, int depth) { + if (depth == 1024) { + throw mjCError(body, "depth limit exceeded in signature computation"); + } + PrintIndent(tree, depth); + tree << "\n"; for (const auto& joint : body->joints) { - tree += indent + "" + std::to_string(joint->nq()) + "\n"; + PrintIndent(tree, depth + 1); + tree << "" << std::to_string(joint->nq()) << "\n"; } for (uint64_t i = 0; i < body->geoms.size(); ++i) { - tree += indent + "\n"; + PrintIndent(tree, depth + 1); + tree << "\n"; } for (uint64_t i = 0; i < body->sites.size(); ++i) { - tree += indent + "\n"; + PrintIndent(tree, depth + 1); + tree << "\n"; } for (uint64_t i = 0; i < body->cameras.size(); ++i) { - tree += indent + "\n"; + PrintIndent(tree, depth + 1); + tree << "\n"; } for (uint64_t i = 0; i < body->lights.size(); ++i) { - tree += indent + "\n"; + PrintIndent(tree, depth + 1); + tree << "\n"; } for (uint64_t i = 0; i < body->bodies.size(); ++i) { - tree += PrintTree(body->bodies[i], indent); + PrintTree(tree, body->bodies[i], depth + 1); } - indent.pop_back(); - indent.pop_back(); - tree += indent + "\n"; - return tree; + PrintIndent(tree, depth); + tree << "\n"; } uint64_t mjCModel::Signature() { - std::string tree = "\n" + PrintTree(bodies_[0]); + std::stringstream tree; + tree << "\n"; + PrintTree(tree, bodies_[0]); for (unsigned int i = 0; i < flexes_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < meshes_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < skins_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < hfields_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < textures_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < materials_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < pairs_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < excludes_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 1; i < equalities_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < tendons_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < actuators_.size(); ++i) { - tree += "\n"; + tree << "\n"; } for (unsigned int i = 0; i < sensors_.size(); ++i) { - tree += "" + std::to_string(sensors_[i]->spec.type) + "\n"; + tree << "" << std::to_string(sensors_[i]->spec.type) << "\n"; } for (unsigned int i = 0; i < keys_.size(); ++i) { - tree += "\n"; + tree << "\n"; } - return mj_hashString(tree.c_str(), UINT64_MAX); + return mj_hashString(tree.str().c_str(), UINT64_MAX); } diff --git a/src/user/user_model.h b/src/user/user_model.h index 725289eb..b0ab37c8 100644 --- a/src/user/user_model.h +++ b/src/user/user_model.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -456,7 +457,7 @@ class mjCModel : public mjCModel_, private mjSpec { const std::vector& list); // print the tree of a body - std::string PrintTree(const mjCBody* body, std::string indent = ""); + void PrintTree(std::stringstream& tree, const mjCBody* body, int depth = 0); // generate a signature for the model uint64_t Signature(); diff --git a/src/xml/xml_native_reader.cc b/src/xml/xml_native_reader.cc index 9bc462cb..0796a43f 100644 --- a/src/xml/xml_native_reader.cc +++ b/src/xml/xml_native_reader.cc @@ -1401,7 +1401,9 @@ void mjXReader::OneFlex(XMLElement* elem, mjsFlex* flex) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(flex->element, name.c_str()); + if (mjs_setName(flex->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "material", material)) { mjs_setString(flex->material, material.c_str()); @@ -1495,7 +1497,9 @@ void mjXReader::OneMesh(XMLElement* elem, mjsMesh* mesh, const mjVFS* vfs) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(mesh->element, name.c_str()); + if (mjs_setName(mesh->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "content_type", content_type)) { *mesh->content_type = content_type; @@ -1568,7 +1572,7 @@ void mjXReader::OneMesh(XMLElement* elem, mjsMesh* mesh, const mjVFS* vfs) { throw mjXError(elem, "builtin mesh cannot be used with user vertex data"); } if (mjs_makeMesh(mesh, (mjtMeshBuiltin)n, params.data(), nparams)) { - throw mjXError(elem, mjs_getError(spec)); + throw mjXError(elem, "%s", mjs_getError(spec)); } } @@ -1590,7 +1594,9 @@ void mjXReader::OneSkin(XMLElement* elem, mjsSkin* skin, const mjVFS* vfs) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(skin->element, name.c_str()); + if (mjs_setName(skin->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } auto file = ReadAttrFile(elem, "file", vfs, AssetDir()); if (file.has_value()) { @@ -1680,7 +1686,9 @@ void mjXReader::OneMaterial(XMLElement* elem, mjsMaterial* material) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(material->element, name.c_str()); + if (mjs_setName(material->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } bool tex_attributes_found = false; @@ -1729,7 +1737,9 @@ void mjXReader::OneJoint(XMLElement* elem, mjsJoint* joint) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(joint->element, name.c_str()); + if (mjs_setName(joint->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (MapValue(elem, "type", &n, joint_map, joint_sz)) { joint->type = (mjtJoint)n; @@ -1777,7 +1787,9 @@ void mjXReader::OneGeom(XMLElement* elem, mjsGeom* geom) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(geom->element, name.c_str()); + if (mjs_setName(geom->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (MapValue(elem, "type", &n, geom_map, mjNGEOMTYPES)) { geom->type = (mjtGeom)n; @@ -1849,7 +1861,9 @@ void mjXReader::OneSite(XMLElement* elem, mjsSite* site) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(site->element, name.c_str()); + if (mjs_setName(site->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (MapValue(elem, "type", &n, geom_map, mjNGEOMTYPES)) { site->type = (mjtGeom)n; @@ -1882,7 +1896,9 @@ void mjXReader::OneCamera(XMLElement* elem, mjsCamera* camera) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(camera->element, name.c_str()); + if (mjs_setName(camera->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "target", targetbody)) { mjs_setString(camera->targetbody, targetbody.c_str()); @@ -1937,7 +1953,9 @@ void mjXReader::OneLight(XMLElement* elem, mjsLight* light) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(light->element, name.c_str()); + if (mjs_setName(light->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "texture", texture)) { mjs_setString(light->texture, texture.c_str()); @@ -1998,7 +2016,9 @@ void mjXReader::OnePair(XMLElement* elem, mjsPair* pair) { // read other parameters if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(pair->element, name.c_str()); + if (mjs_setName(pair->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } ReadAttrInt(elem, "condim", &pair->condim); ReadAttr(elem, "solref", mjNREF, pair->solref, text, false, false); @@ -2026,7 +2046,9 @@ void mjXReader::OneEquality(XMLElement* elem, mjsEquality* equality) { // regular only if (!readingdefaults) { if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(equality->element, name.c_str()); + if (mjs_setName(equality->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } switch (equality->type) { @@ -2162,7 +2184,9 @@ void mjXReader::OneTendon(XMLElement* elem, mjsTendon* tendon) { // read attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(tendon->element, name.c_str()); + if (mjs_setName(tendon->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } ReadAttrInt(elem, "group", &tendon->group); if (ReadAttrTxt(elem, "material", material)) { @@ -2205,7 +2229,9 @@ void mjXReader::OneActuator(XMLElement* elem, mjsActuator* actuator) { // common attributes if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(actuator->element, name.c_str()); + if (mjs_setName(actuator->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } ReadAttrInt(elem, "group", &actuator->group); MapValue(elem, "ctrllimited", &actuator->ctrllimited, TFAuto_map, 3); @@ -2956,7 +2982,9 @@ void mjXReader::Custom(XMLElement* section) { // read attributes ReadAttrTxt(elem, "name", elname, true); - mjs_setName(numeric->element, elname.c_str()); + if (mjs_setName(numeric->element, elname.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } if (ReadAttrInt(elem, "size", &numeric->size)) { int sz = numeric->size < 500 ? numeric->size : 500; for (int i=0; i < sz; i++) { @@ -2987,7 +3015,9 @@ void mjXReader::Custom(XMLElement* section) { // read attributes ReadAttrTxt(elem, "name", elname, true); - mjs_setName(text->element, elname.c_str()); + if (mjs_setName(text->element, elname.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } ReadAttrTxt(elem, "data", str, true); if (str.empty()) { throw mjXError(elem, "text field cannot be empty"); @@ -3007,7 +3037,9 @@ void mjXReader::Custom(XMLElement* section) { // read attributes ReadAttrTxt(elem, "name", elname, true); - mjs_setName(tuple->element, elname.c_str()); + if (mjs_setName(tuple->element, elname.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } // read objects and add XMLElement* obj = FirstChildElement(elem); @@ -3223,7 +3255,9 @@ void mjXReader::Asset(XMLElement* section, const mjVFS* vfs) { texture->colorspace = (mjtColorSpace)n; } if (ReadAttrTxt(elem, "name", texname)) { - mjs_setName(texture->element, texname.c_str()); + if (mjs_setName(texture->element, texname.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "content_type", content_type)) { mjs_setString(texture->content_type, content_type.c_str()); @@ -3315,7 +3349,9 @@ void mjXReader::Asset(XMLElement* section, const mjVFS* vfs) { // read attributes string name, content_type; if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(hfield->element, name.c_str()); + if (mjs_setName(hfield->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "content_type", content_type)) { mjs_setString(hfield->content_type, content_type.c_str()); @@ -3482,7 +3518,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame, // read attributes string name; if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(joint->element, name.c_str()); + if (mjs_setName(joint->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } ReadAttrInt(elem, "group", &joint->group); MapValue(elem, "align", &joint->align, TFAuto_map, 3); @@ -3554,7 +3592,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame, // read attributes string name, childclass; if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(pframe->element, name.c_str()); + if (mjs_setName(pframe->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "childclass", childclass)) { mjs_setString(pframe->childclass, childclass.c_str()); @@ -3626,13 +3666,13 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame, // attach to parent if (!mjs_attach(body->element, pframe->element, /*prefix=*/"", suffix.c_str())) { - throw mjXError(elem, mjs_getError(spec)); + throw mjXError(elem, "%s", mjs_getError(spec)); } } // delete subtree if (mjs_delete(spec, subtree->element)) { - throw mjXError(elem, mjs_getError(spec)); + throw mjXError(elem, "%s", mjs_getError(spec)); } } @@ -3655,7 +3695,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame, // read attributes string name, childclass; if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(child->element, name.c_str()); + if (mjs_setName(child->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } if (ReadAttrTxt(elem, "childclass", childclass)) { mjs_setString(child->childclass, childclass.c_str()); @@ -3708,12 +3750,12 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame, child = child_body->element; } if (!mjs_attach(pframe->element, child, prefix.c_str(), "")) { - throw mjXError(elem, mjs_getError(spec)); + throw mjXError(elem, "%s", mjs_getError(spec)); } } else { // only set frame to existing body if (mjs_setFrame(child_body->element, pframe)) { - throw mjXError(elem, mjs_getError(spec)); + throw mjXError(elem, "%s", mjs_getError(spec)); } } } @@ -3764,7 +3806,9 @@ void mjXReader::Contact(XMLElement* section) { // read name and body names if (ReadAttrTxt(elem, "name", exname)) { - mjs_setName(exclude->element, exname.c_str()); + if (mjs_setName(exclude->element, exname.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } ReadAttrTxt(elem, "body1", exbody1, true); mjs_setString(exclude->bodyname1, exbody1.c_str()); @@ -3946,7 +3990,9 @@ void mjXReader::Sensor(XMLElement* section) { // read name, noise, userdata if (ReadAttrTxt(elem, "name", name)) { - mjs_setName(sensor->element, name.c_str()); + if (mjs_setName(sensor->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } } ReadAttr(elem, "cutoff", 1, &sensor->cutoff, text); ReadAttr(elem, "noise", 1, &sensor->noise, text); @@ -4377,7 +4423,9 @@ void mjXReader::Keyframe(XMLElement* section) { // read name, time ReadAttrTxt(elem, "name", name); - mjs_setName(key->element, name.c_str()); + if (mjs_setName(key->element, name.c_str())) { + throw mjXError(elem, "%s", mjs_getError(spec)); + } ReadAttr(elem, "time", 1, &key->time, text); // read qpos diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a286a1c6..f459a6ca 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -42,6 +42,7 @@ macro(mujoco_test name) ) add_executable(${name} ${name}.cc) + target_link_options(${name} PRIVATE ${EXTRA_LINK_OPTIONS}) target_link_libraries(${name} mujoco fixture gmock) if(_ARGS_MAIN_TARGET) target_link_libraries(${name} ${_ARGS_MAIN_TARGET}) diff --git a/test/xml/xml_native_reader_test.cc b/test/xml/xml_native_reader_test.cc index b6c0779a..d150099e 100644 --- a/test/xml/xml_native_reader_test.cc +++ b/test/xml/xml_native_reader_test.cc @@ -1529,7 +1529,7 @@ TEST_F(XMLReaderTest, ParseReplicateRepeatedName) { mjSpec* spec = mj_parseXMLString(xml, 0, error.data(), error.size()); EXPECT_THAT(spec, IsNull()) << error.data(); EXPECT_THAT(error.data(), HasSubstr("repeated name 'b' in actuator")); - EXPECT_THAT(error.data(), HasSubstr("Element 'replicate'")); + EXPECT_THAT(error.data(), HasSubstr("Element 'position'")); } TEST_F(XMLReaderTest, RepeatedPrefix) { diff --git a/test/xml/xml_native_writer_test.cc b/test/xml/xml_native_writer_test.cc index aa89b647..c6148234 100644 --- a/test/xml/xml_native_writer_test.cc +++ b/test/xml/xml_native_writer_test.cc @@ -1417,6 +1417,8 @@ TEST_F(XMLWriterTest, WriteReadCompare) { auto abs_path = p.path(); mjSpec* stemp = mj_parseXMLString(SaveAndReadXml(s).c_str(), 0, error.data(), error.size()); + ASSERT_THAT(stemp, NotNull()) + << "Failed to load " << xml.c_str() << ": " << error.data(); mjs_setString(stemp->modelfiledir, abs_path.remove_filename().string().c_str()); mjModel* mtemp = mj_compile(stemp, nullptr);