From f5b6752454b72b2c1105cbaf75be519d39e3426a Mon Sep 17 00:00:00 2001 From: Aditya kumar singh <143548997+Adityakk9031@users.noreply.github.com> Date: Tue, 25 Nov 2025 06:20:22 +0530 Subject: [PATCH] Fix inconsistent asset paths in mj_saveLastXML: canonicalize includes and set output dir during write (fixes #2941) --- src/xml/xml.cc | 24 ++++++++++++++------- src/xml/xml_api.cc | 54 ++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/src/xml/xml.cc b/src/xml/xml.cc index e7d9f58117..a206a7d6aa 100644 --- a/src/xml/xml.cc +++ b/src/xml/xml.cc @@ -154,7 +154,7 @@ void IncludeXML(mjXReader& reader, XMLElement* elem, throw mjXError(elem, "Include element cannot have children"); } - // get filename + // get filename (may be relative) auto file_attr = mjXUtil::ReadAttrFile(elem, "file", vfs, reader.ModelFileDir(), true); if (!file_attr.has_value()) { @@ -162,10 +162,14 @@ void IncludeXML(mjXReader& reader, XMLElement* elem, } FilePath filename = file_attr.value(); + // Compute a canonical fullname (dir + filename) early and use it for checks/insertion. + // This prevents inconsistent checks where included is compared against one form but the inserted + // value is another (causing duplicates or missed duplicates and eventually double-prefixing). + FilePath fullname = dir + filename; - // block repeated include files - if (included.find(filename.Str()) != included.end()) { - throw mjXError(elem, "File '%s' already included", filename.c_str()); + // block repeated include files using canonical fullname + if (included.find(fullname.Str()) != included.end()) { + throw mjXError(elem, "File '%s' already included", fullname.c_str()); } // TODO: b/325905702 - We have a messy wrapper here to remain backwards @@ -178,7 +182,6 @@ void IncludeXML(mjXReader& reader, XMLElement* elem, if (resource == nullptr) { // new behavior: try to load in relative directory if (!filename.IsAbs()) { - FilePath fullname = dir + filename; resource = mju_openResource(reader.ModelFileDir().c_str(), fullname.c_str(), vfs, error.data(), error.size()); } @@ -188,7 +191,8 @@ void IncludeXML(mjXReader& reader, XMLElement* elem, throw mjXError(elem, "%s", error.data()); } - filename = dir + filename; + // Use the canonical fullname for further processing/storage so we keep a single form. + filename = fullname; const char* include_dir = nullptr; int ninclude_dir = 0; @@ -220,7 +224,7 @@ void IncludeXML(mjXReader& reader, XMLElement* elem, throw mjXError(elem, "Include error: '%s'", err); } - // remember that file was included + // remember that file was included using canonical fullname included.insert(filename.Str()); // get and check root element @@ -347,7 +351,11 @@ mjSpec* ParseXML(const char* filename, const mjVFS* vfs, try { if (!strcasecmp(root->Value(), "mujoco")) { // find include elements, replace them with subtree from xml file - std::unordered_set included = {filename}; + // NOTE: canonicalize the initial included set entry so all entries + // use the same canonical form (FilePath(...).Str()). + std::unordered_set included; + included.insert(FilePath(filename).Str()); + mjXReader parser; parser.SetModelFileDir(mjs_getString(spec->modelfiledir)); IncludeXML(parser, root, FilePath(), vfs, included); diff --git a/src/xml/xml_api.cc b/src/xml/xml_api.cc index b722ceaf99..662bad07fb 100644 --- a/src/xml/xml_api.cc +++ b/src/xml/xml_api.cc @@ -49,8 +49,10 @@ class GlobalModel { void Set(mjSpec* spec = nullptr); // writes XML to string - std::optional ToXML(const mjModel* m, char* error, - int error_sz); + // out_dir: optional. If non-null, temporarily set spec->modelfiledir to out_dir + // so the writer can emit asset paths relative to the output directory. + std::optional ToXML(const mjModel* m, const char* out_dir, + char* error, int error_sz); private: // using raw pointers as GlobalModel needs to be trivially destructible @@ -58,14 +60,37 @@ class GlobalModel { mjSpec* spec_ = nullptr; }; -std::optional GlobalModel::ToXML(const mjModel* m, char* error, - int error_sz) { +std::optional GlobalModel::ToXML(const mjModel* m, const char* out_dir, + char* error, int error_sz) { std::lock_guard lock(*mutex_); if (!spec_) { mjCopyError(error, "No XML model loaded", error_sz); return std::nullopt; } + + // If out_dir is provided, temporarily override spec->modelfiledir so + // the writer can compute relative paths against the output file location. + std::string old_modelfiledir; + bool changed_modelfiledir = false; + if (out_dir && out_dir[0] != '\0') { + // Save old value + const char* old = mjs_getString(spec_->modelfiledir); + if (old) old_modelfiledir = old; + else old_modelfiledir.clear(); + + // Set to requested out_dir + mjs_setString(spec_->modelfiledir, out_dir); + changed_modelfiledir = true; + } + + // Call the writer (WriteXML relies on spec_->modelfiledir when emitting paths). std::string result = WriteXML(m, spec_, error, error_sz); + + // Restore old modelfiledir if we changed it + if (changed_modelfiledir) { + mjs_setString(spec_->modelfiledir, old_modelfiledir.empty() ? "" : old_modelfiledir.c_str()); + } + if (result.empty()) { return std::nullopt; } @@ -160,6 +185,16 @@ mjModel* mj_loadUSD(const char* filename, const mjVFS* vfs, char* error, int err } #endif +// helper: compute dirname of path (returns empty string if no dir component) +static std::string DirnameOfPath(const char* path) { + if (!path) return std::string(); + std::string s(path); + // find last slash or backslash + size_t pos = s.find_last_of("/\\"); + if (pos == std::string::npos) return std::string(); + return s.substr(0, pos); +} + // update XML data structures with info from low-level model, save as MJCF // returns 1 if successful, 0 otherwise // error can be NULL; otherwise assumed to have size error_sz @@ -173,7 +208,12 @@ int mj_saveLastXML(const char* filename, const mjModel* m, char* error, int erro } } - auto result = GetGlobalModel().ToXML(m, error, error_sz); + // compute output directory (if any) so writer can emit relative paths w.r.t. it + std::string out_dir = DirnameOfPath(filename ? filename : ""); + const char* out_dir_ptr = nullptr; + if (!out_dir.empty()) out_dir_ptr = out_dir.c_str(); + + auto result = GetGlobalModel().ToXML(m, out_dir_ptr, error, error_sz); if (result.has_value()) { fprintf(fp, "%s", result->c_str()); } @@ -195,6 +235,7 @@ void mj_freeLastXML(void) { + // print internal XML schema as plain text or HTML, with style-padding or   int mj_printSchema(const char* filename, char* buffer, int buffer_sz, int flg_html, int flg_pad) { // print to stringstream @@ -253,7 +294,7 @@ mjSpec* mj_parseXML(const char* filename, const mjVFS* vfs, char* error, int err -// parse spec from string + // parse spec from string mjSpec* mj_parseXMLString(const char* xml, const mjVFS* vfs, char* error, int error_sz) { return ParseSpecFromString(xml, vfs, error, error_sz); } @@ -294,4 +335,3 @@ int mj_saveXMLString(const mjSpec* s, char* xml, int xml_sz, char* error, int er xml[result.size()] = 0; return 0; } -