From c77babe0468cefeb91c5dfc71ff7db864ea7c5d1 Mon Sep 17 00:00:00 2001
From: Alessio Quaglino <quaglino@google.com>
Date: Mon, 16 Sep 2024 04:21:11 -0700
Subject: [PATCH] Fix a bug in MuJoCo's user_mesh.cc that caused meshdir_ to be
 overwritten when a model was loaded with a nested model.

PiperOrigin-RevId: 675092713
Change-Id: Iad6336141749510bf5544cab9c06533ee1589c08
---
 src/user/user_mesh.cc        | 16 +++++--
 src/user/user_model.cc       |  1 +
 src/user/user_objects.cc     | 16 +++++--
 src/xml/xml_native_reader.cc |  1 -
 test/user/user_model_test.cc | 91 ++++++++++++++++++++++++++++++++++++
 5 files changed, 116 insertions(+), 9 deletions(-)

diff --git a/src/user/user_mesh.cc b/src/user/user_mesh.cc
index 16b30961..28869b69 100644
--- a/src/user/user_mesh.cc
+++ b/src/user/user_mesh.cc
@@ -222,8 +222,12 @@ void mjCMesh::PointToLocal() {
 
 void mjCMesh::NameSpace(const mjCModel* m) {
   mjCBase::NameSpace(m);
-  modelfiledir_ = FilePath(m->spec_modelfiledir_);
-  meshdir_ = FilePath(m->spec_meshdir_);
+  if (modelfiledir_.empty()) {
+    modelfiledir_ = FilePath(m->spec_modelfiledir_);
+  }
+  if (meshdir_.empty()) {
+    meshdir_ = FilePath(m->spec_meshdir_);
+  }
 }
 
 
@@ -2034,8 +2038,12 @@ void mjCSkin::NameSpace(const mjCModel* m) {
   for (auto& name : spec_bodyname_) {
     name = m->prefix + name + m->suffix;
   }
-  modelfiledir_ = FilePath(m->spec_modelfiledir_);
-  meshdir_ = FilePath(m->spec_meshdir_);
+  if (modelfiledir_.empty()) {
+    modelfiledir_ = FilePath(m->spec_modelfiledir_);
+  }
+  if (meshdir_.empty()) {
+    meshdir_ = FilePath(m->spec_meshdir_);
+  }
 }
 
 
diff --git a/src/user/user_model.cc b/src/user/user_model.cc
index ea0c0028..b1049ff3 100644
--- a/src/user/user_model.cc
+++ b/src/user/user_model.cc
@@ -607,6 +607,7 @@ mjCModel::~mjCModel() {
   for (int i=0; i<tuples_.size(); i++) delete tuples_[i];
   for (int i=0; i<keys_.size(); i++) delete keys_[i];
   for (int i=0; i<defaults_.size(); i++) delete defaults_[i];
+  for (int i=0; i<specs_.size(); i++) mj_deleteSpec(specs_[i]);
 
   if (plugin_owner) {
     for (int i=0; i<plugins_.size(); i++) delete plugins_[i];
diff --git a/src/user/user_objects.cc b/src/user/user_objects.cc
index a2b0d852..6fdb782d 100644
--- a/src/user/user_objects.cc
+++ b/src/user/user_objects.cc
@@ -3405,8 +3405,12 @@ void mjCHField::CopyFromSpec() {
 
 void mjCHField::NameSpace(const mjCModel* m) {
   mjCBase::NameSpace(m);
-  modelfiledir_ = FilePath(m->spec_modelfiledir_);
-  meshdir_ = FilePath(m->spec_meshdir_);
+  if (modelfiledir_.empty()) {
+    modelfiledir_ = FilePath(m->spec_modelfiledir_);
+  }
+  if (meshdir_.empty()) {
+    meshdir_ = FilePath(m->spec_meshdir_);
+  }
 }
 
 
@@ -3650,8 +3654,12 @@ void mjCTexture::CopyFromSpec() {
 
 void mjCTexture::NameSpace(const mjCModel* m) {
   mjCBase::NameSpace(m);
-  modelfiledir_ = FilePath(m->spec_modelfiledir_);
-  texturedir_ = FilePath(m->spec_texturedir_);
+  if (modelfiledir_.empty()) {
+    modelfiledir_ = FilePath(m->spec_modelfiledir_);
+  }
+  if (texturedir_.empty()) {
+    texturedir_ = FilePath(m->spec_texturedir_);
+  }
 }
 
 
diff --git a/src/xml/xml_native_reader.cc b/src/xml/xml_native_reader.cc
index ba089c32..a5833cdb 100644
--- a/src/xml/xml_native_reader.cc
+++ b/src/xml/xml_native_reader.cc
@@ -3645,7 +3645,6 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
           throw mjXError(0, "could not find body '%s''%s'", body_name.c_str());
         }
         if (mjs_attachBody(pframe, child, prefix.c_str(), "") != 0) {
-          mj_deleteSpec(asset);
           throw mjXError(elem, mjs_getError(spec));
         }
       } else {
diff --git a/test/user/user_model_test.cc b/test/user/user_model_test.cc
index be6d9c4e..03b06985 100644
--- a/test/user/user_model_test.cc
+++ b/test/user/user_model_test.cc
@@ -545,5 +545,96 @@ TEST_F(MujocoTest, Modeldir) {
   mj_deleteVFS(vfs.get());
 }
 
+TEST_F(MujocoTest, NestedMeshDir) {
+  static constexpr char cube[] = R"(
+  v -1 -1  1
+  v  1 -1  1
+  v -1  1  1
+  v  1  1  1
+  v -1  1 -1
+  v  1  1 -1
+  v -1 -1 -1
+  v  1 -1 -1)";
+
+  static constexpr char child_xml[] = R"(
+  <mujoco>
+    <compiler meshdir="child_meshdir"/>
+
+    <asset>
+      <mesh name="m" file="child_mesh.obj"/>
+    </asset>
+
+    <worldbody>
+      <body name="child">
+        <geom type="mesh" mesh="m"/>
+      </body>
+    </worldbody>
+  </mujoco>
+  )";
+
+  static constexpr char parent_xml[] = R"(
+  <mujoco>
+    <compiler meshdir="parent_meshdir"/>
+
+    <asset>
+      <mesh name="m" file="parent_mesh.obj"/>
+      <model name="child" file="child.xml"/>
+    </asset>
+
+    <worldbody>
+      <body name="parent">
+        <geom type="mesh" mesh="m"/>
+        <attach model="child" body="child" prefix="child_"/>
+      </body>
+    </worldbody>
+  </mujoco>
+  )";
+
+  static constexpr char grandparent_xml[] = R"(
+  <mujoco>
+    <compiler meshdir="grandparent_meshdir"/>
+
+    <asset>
+      <mesh name="m" file="grandparent_mesh.obj"/>
+      <model name="parent" file="parent.xml"/>
+    </asset>
+
+    <worldbody>
+      <geom type="mesh" mesh="m"/>
+      <attach model="parent" body="parent" prefix="parent_"/>
+    </worldbody>
+  </mujoco>
+  )";
+
+  auto vfs = std::make_unique<mjVFS>();
+  mj_defaultVFS(vfs.get());
+  mj_addBufferVFS(vfs.get(), "child_meshdir/child_mesh.obj", cube,
+                  sizeof(cube));
+  mj_addBufferVFS(vfs.get(), "child.xml", child_xml, sizeof(child_xml));
+  mj_addBufferVFS(vfs.get(), "parent_meshdir/parent_mesh.obj", cube,
+                  sizeof(cube));
+  mj_addBufferVFS(vfs.get(), "parent.xml", parent_xml, sizeof(parent_xml));
+  mj_addBufferVFS(vfs.get(), "grandparent_meshdir/grandparent_mesh.obj", cube,
+                  sizeof(cube));
+
+  std::array<char, 1024> error;
+  mjModel* child_model = LoadModelFromString(child_xml, error.data(),
+                                             error.size(), vfs.get());
+  EXPECT_THAT(child_model, NotNull()) << error.data();
+  mj_deleteModel(child_model);
+
+  mjModel* parent_model = LoadModelFromString(parent_xml, error.data(),
+                                              error.size(), vfs.get());
+  EXPECT_THAT(parent_model, NotNull()) << error.data();
+  mj_deleteModel(parent_model);
+
+  mjModel* grandparent_model = LoadModelFromString(
+      grandparent_xml, error.data(), error.size(), vfs.get());
+  EXPECT_THAT(grandparent_model, NotNull()) << error.data();
+  mj_deleteModel(grandparent_model);
+
+  mj_deleteVFS(vfs.get());
+}
+
 }  // namespace
 }  // namespace mujoco