Fix bug in cached meshes where convex hull was missing. Previously if a mesh was copied from the cache it would not have its convex hull if the original mesh that was cached didn't previously compute it.

Fixes #2609

PiperOrigin-RevId: 756223337
Change-Id: I54705456636c270607a17340b249047e0df75d0b

src/user/user_cache.cc

@@ -102,7 +102,8 @@ bool mjCCache::Insert(const std::string& modelname, const mjResource *resource,
 
 
 
-// populate data from the cache into the given function
+// populate data from the cache into the given function, return true if data was
+// copied
 bool mjCCache::PopulateData(const mjResource* resource, mjCDataFunc fn) {
   std::lock_guard<std::mutex> lock(mutex_);
   auto it = lookup_.find(resource->name);
@@ -121,8 +122,7 @@ bool mjCCache::PopulateData(const mjResource* resource, mjCDataFunc fn) {
   entries_.erase(asset);
   entries_.insert(asset);
 
-  asset->PopulateData(fn);
+  return asset->PopulateData(fn);
-  return true;
 }
 
 

src/user/user_cache.h

@@ -28,7 +28,7 @@
 
 #include <mujoco/mjplugin.h>
 
-typedef std::function<void(const void*)> mjCDataFunc;
+typedef std::function<bool(const void*)> mjCDataFunc;
 typedef void (*mjCDeallocFunc)(const void*);
 
 // A class container for a thread-safe asset cache
@@ -57,9 +57,9 @@ class mjCAsset {
   std::size_t InsertNum() const { return insert_num_; }
   std::size_t AccessCount() const { return access_count_; }
 
-  // pass data in the cache to the given function
+  // pass data in the cache to the given function, return true if data was copied
-  void PopulateData(mjCDataFunc fn) const {
+  bool PopulateData(mjCDataFunc fn) const {
-    fn(data_.get());
+    return fn(data_.get());
   }
 
  private:

src/user/user_mesh.cc

@@ -1012,30 +1012,28 @@ void mjCMesh::LoadOBJ(mjResource* resource, bool remove_repeated) {
 
 // load mesh from cached asset, return true on success
 bool mjCMesh::LoadCachedMesh(mjCCache *cache, const mjResource* resource) {
-  // save previous mesh properties (in case different from cached mesh)
-  int maxhullvert = maxhullvert_;
-  mjtMeshInertia old_inertia = inertia;
-  double old_scale[3] = {scale[0], scale[1], scale[2]};
-
   auto process_mesh = [&](const void* data) {
     const mjCMesh* mesh = static_cast<const mjCMesh*>(data);
     // check if maxhullvert is different
-    maxhullvert_ = mesh->maxhullvert_;
+    if (maxhullvert_ != mesh->maxhullvert_) {
-    if (maxhullvert != mesh->maxhullvert_) {
+      return false;
-      return;
     }
 
     // check if inertia is different
-    inertia = mesh->inertia;
+    if (inertia != mesh->inertia) {
-    if (old_inertia != mesh->inertia) {
+      return false;
-      return;
     }
 
     // check if scale is different
-    memcpy(scale, mesh->scale, 3*sizeof(double));
+    if (scale[0] != mesh->scale[0] ||
-    if (old_scale[0] != mesh->scale[0] || old_scale[1] != mesh->scale[1] ||
+        scale[1] != mesh->scale[1] ||
-        old_scale[2] != mesh->scale[2]) {
+        scale[2] != mesh->scale[2]) {
-      return;
+      return false;
+    }
+
+    // check if need hull
+    if (needhull_ && !mesh->szgraph_) {
+      return false;
     }
 
     processed_ = mesh->processed_;
@@ -1047,11 +1045,14 @@ bool mjCMesh::LoadCachedMesh(mjCCache *cache, const mjResource* resource) {
     facetexcoord_ = mesh->facetexcoord_;
     halfedge_ = mesh->halfedge_;
 
-    szgraph_ = mesh->szgraph_;
+    // only copy graph if needed
-    graph_ = nullptr;
+    if (needhull_ || mesh->face_.empty()) {
-    if (szgraph_) {
+      szgraph_ = mesh->szgraph_;
-      graph_ = (int*)mju_malloc(szgraph_*sizeof(int));
+      graph_ = nullptr;
-      std::copy(mesh->graph_, mesh->graph_ + szgraph_, graph_);
+      if (szgraph_) {
+        graph_ = (int*)mju_malloc(szgraph_*sizeof(int));
+        std::copy(mesh->graph_, mesh->graph_ + szgraph_, graph_);
+      }
     }
 
     polygons_ = mesh->polygons_;
@@ -1072,29 +1073,11 @@ bool mjCMesh::LoadCachedMesh(mjCCache *cache, const mjResource* resource) {
     }
     tree_ = mesh->tree_;
     face_aabb_ = mesh->face_aabb_;
+    return true;
   };
 
-  // check that cached asset has all data, make sure no metadata has changed
+  // check that cached asset has all data
-  if (!cache->PopulateData(resource, process_mesh)) {
+  return cache->PopulateData(resource, process_mesh);
-    return false;
-  }
-
-  if (maxhullvert != maxhullvert_) {
-    maxhullvert_ = maxhullvert;
-    return false;
-  }
-  if (inertia != old_inertia) {
-    inertia = old_inertia;
-    return false;
-  }
-  if (scale[0] != old_scale[0] || scale[1] != old_scale[1] ||
-      scale[2] != old_scale[2]) {
-    scale[0] = old_scale[0];
-    scale[1] = old_scale[1];
-    scale[2] = old_scale[2];
-    return false;
-  }
-  return true;
 }
 
 // load STL binary mesh

src/user/user_objects.cc

@@ -75,14 +75,19 @@ PNGImage PNGImage::Load(const mjCBase* obj, mjResource* resource,
   image.color_type_ = color_type;
   mjCCache *cache = reinterpret_cast<mjCCache*>(mj_globalCache());
 
+  // cache callback
+  auto callback = [&image](const void* data) {
+    const PNGImage *cached_image = static_cast<const PNGImage*>(data);
+    if (cached_image->color_type_ == image.color_type_) {
+      image = *cached_image;
+      return true;
+    }
+    return false;
+  };
+
   // try loading from cache
-  if (cache && cache->PopulateData(resource, [&image](const void* data) {
+  if (cache && cache->PopulateData(resource, callback)) {
-      const PNGImage *cached_image = static_cast<const PNGImage*>(data);
+      return image;
-      if (cached_image->color_type_ == image.color_type_) {
-        image = *cached_image;
-      }
-    })) {
-    if (!image.data_.empty()) return image;
   }
 
   // open PNG resource

test/user/user_cache_test.cc

@@ -65,6 +65,7 @@ GetCachedText(mjCCache& cache, const std::string& model,
   bool inserted = cache.PopulateData(resource,
                                      [&cached_text](const void* data) {
     cached_text = *(static_cast<const std::string*>(data));
+    return true;
   });
   mju_closeResource(resource);
   mj_deleteVFS(&vfs);

test/user/user_mesh_test.cc

@@ -42,14 +42,14 @@ static const char* const kDuplicateVerticesPath =
     "user/testdata/duplicate_vertices.xml";
 static const char* const kCubePath =
     "user/testdata/cube.xml";
+static const char* const kCubeCompletePath =
+    "user/testdata/cube_complete.obj";
 static const char* const kTorusPath =
     "user/testdata/torus.xml";
 static const char* const kTorusMaxhullVertPath =
     "user/testdata/torus_maxhullvert.xml";
 static const char* const kTorusDefaultMaxhullVertPath =
     "user/testdata/torus_maxhullvert_default.xml";
-static const char* const kTorusShellPath =
-    "user/testdata/torus_shell.xml";
 static const char* const kCompareInertiaPath =
     "user/testdata/inertia_compare.xml";
 static const char* const kConvexInertiaPath =
@@ -1207,6 +1207,48 @@ TEST_F(MjCMeshTest, InvalidIndexInFace) {
   mj_deleteModel(model);
 }
 
+TEST_F(MjCMeshTest, QhullCache) {
+  static constexpr char xml1[] = R"(
+    <mujoco>
+      <asset>
+        <mesh name="box" file="cube_complete.obj"/>
+      </asset>
+      <worldbody>
+        <geom type="mesh" pos="0 0 2" mesh="box" contype="0" conaffinity="0"/>
+        <geom type="mesh" pos="0 0 0" mesh="box" contype="0" conaffinity="0"/>
+      </worldbody>
+    </mujoco>)";
+
+    static constexpr char xml2[] = R"(
+      <mujoco>
+        <asset>
+          <mesh name="box" file="cube_complete.obj"/>
+        </asset>
+        <worldbody>
+          <geom type="mesh" pos="0 0 2" mesh="box"/>
+          <geom type="mesh" pos="0 0 0" mesh="box"/>
+        </worldbody>
+      </mujoco>)";
+
+  mjVFS vfs;
+  mj_defaultVFS(&vfs);
+  mj_addFileVFS(&vfs, "", GetTestDataFilePath(kCubeCompletePath).c_str());
+
+  std::array<char, 1000> error;
+  mjModel* model = LoadModelFromString(xml1, error.data(), error.size(), &vfs);
+  ASSERT_THAT(model, NotNull()) << "Failed to load model: " << error.data();
+  EXPECT_THAT(model->mesh_graphadr[0], -1);
+
+  mj_deleteModel(model);
+
+  model = LoadModelFromString(xml2, error.data(), error.size(), &vfs);
+  ASSERT_THAT(model, NotNull()) << "Failed to load model: " << error.data();
+  EXPECT_GT(model->mesh_graphadr[0], -1);
+
+  mj_deleteModel(model);
+  mj_deleteVFS(&vfs);
+}
+
 TEST_F(MjCMeshTest, LoadSkin) {
   const std::string xml_path = GetTestDataFilePath(kCubeSkinPath);
   std::array<char, 1024> error;