Fix inconsistent asset paths in mj_saveLastXML: canonicalize includes…

src/xml/xml.cc

@@ -154,18 +154,22 @@ 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()) {
     throw mjXError(elem, "Include element missing file attribute");
   }
   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<std::string> included = {filename};
+      // NOTE: canonicalize the initial included set entry so all entries
+      // use the same canonical form (FilePath(...).Str()).
+      std::unordered_set<std::string> included;
+      included.insert(FilePath(filename).Str());
+
       mjXReader parser;
       parser.SetModelFileDir(mjs_getString(spec->modelfiledir));
       IncludeXML(parser, root, FilePath(), vfs, included);

src/xml/xml_api.cc

@@ -49,23 +49,48 @@ class GlobalModel {
   void Set(mjSpec* spec = nullptr);
 
   // writes XML to string
-  std::optional<std::string> 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<std::string> ToXML(const mjModel* m, const char* out_dir,
+                                      char* error, int error_sz);
 
  private:
   // using raw pointers as GlobalModel needs to be trivially destructible
   std::mutex* mutex_ = new std::mutex();
   mjSpec* spec_ = nullptr;
 };
 
-std::optional<std::string> GlobalModel::ToXML(const mjModel* m, char* error,
-                                              int error_sz) {
+std::optional<std::string> GlobalModel::ToXML(const mjModel* m, const char* out_dir,
+                                              char* error, int error_sz) {
   std::lock_guard<std::mutex> 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 &nbsp;
 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;
 }
-