From f4774a544938383343fe911cde8b94e1dae02336 Mon Sep 17 00:00:00 2001 From: Kyle Bayes Date: Thu, 8 May 2025 02:41:06 -0700 Subject: [PATCH] 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 | 6 ++-- src/user/user_cache.h | 8 ++--- src/user/user_mesh.cc | 65 +++++++++++++----------------------- src/user/user_objects.cc | 19 +++++++---- test/user/user_cache_test.cc | 1 + test/user/user_mesh_test.cc | 46 +++++++++++++++++++++++-- 6 files changed, 88 insertions(+), 57 deletions(-) diff --git a/src/user/user_cache.cc b/src/user/user_cache.cc index 715e8e6a..ee22d7eb 100644 --- a/src/user/user_cache.cc +++ b/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 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 true; + return asset->PopulateData(fn); } diff --git a/src/user/user_cache.h b/src/user/user_cache.h index a2d5577f..4b0e9288 100644 --- a/src/user/user_cache.h +++ b/src/user/user_cache.h @@ -28,7 +28,7 @@ #include -typedef std::function mjCDataFunc; +typedef std::function 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 - void PopulateData(mjCDataFunc fn) const { - fn(data_.get()); + // pass data in the cache to the given function, return true if data was copied + bool PopulateData(mjCDataFunc fn) const { + return fn(data_.get()); } private: diff --git a/src/user/user_mesh.cc b/src/user/user_mesh.cc index f999fc9a..e03896ed 100644 --- a/src/user/user_mesh.cc +++ b/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(data); // check if maxhullvert is different - maxhullvert_ = mesh->maxhullvert_; - if (maxhullvert != mesh->maxhullvert_) { - return; + if (maxhullvert_ != mesh->maxhullvert_) { + return false; } // check if inertia is different - inertia = mesh->inertia; - if (old_inertia != mesh->inertia) { - return; + if (inertia != mesh->inertia) { + return false; } // check if scale is different - memcpy(scale, mesh->scale, 3*sizeof(double)); - if (old_scale[0] != mesh->scale[0] || old_scale[1] != mesh->scale[1] || - old_scale[2] != mesh->scale[2]) { - return; + if (scale[0] != mesh->scale[0] || + scale[1] != mesh->scale[1] || + scale[2] != mesh->scale[2]) { + 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_; - graph_ = nullptr; - if (szgraph_) { - graph_ = (int*)mju_malloc(szgraph_*sizeof(int)); - std::copy(mesh->graph_, mesh->graph_ + szgraph_, graph_); + // only copy graph if needed + if (needhull_ || mesh->face_.empty()) { + szgraph_ = mesh->szgraph_; + graph_ = nullptr; + 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 - if (!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; + // check that cached asset has all data + return cache->PopulateData(resource, process_mesh); } // load STL binary mesh diff --git a/src/user/user_objects.cc b/src/user/user_objects.cc index cff97b72..82853307 100644 --- a/src/user/user_objects.cc +++ b/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(mj_globalCache()); + // cache callback + auto callback = [&image](const void* data) { + const PNGImage *cached_image = static_cast(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) { - const PNGImage *cached_image = static_cast(data); - if (cached_image->color_type_ == image.color_type_) { - image = *cached_image; - } - })) { - if (!image.data_.empty()) return image; + if (cache && cache->PopulateData(resource, callback)) { + return image; } // open PNG resource diff --git a/test/user/user_cache_test.cc b/test/user/user_cache_test.cc index f226827b..82d0f57b 100644 --- a/test/user/user_cache_test.cc +++ b/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(data)); + return true; }); mju_closeResource(resource); mj_deleteVFS(&vfs); diff --git a/test/user/user_mesh_test.cc b/test/user/user_mesh_test.cc index b9c1f3af..759561b2 100644 --- a/test/user/user_mesh_test.cc +++ b/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"( + + + + + + + + + )"; + + static constexpr char xml2[] = R"( + + + + + + + + + )"; + + mjVFS vfs; + mj_defaultVFS(&vfs); + mj_addFileVFS(&vfs, "", GetTestDataFilePath(kCubeCompletePath).c_str()); + + std::array 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 error;