From da01bd37a2666630629abf14806d2dd177ea9164 Mon Sep 17 00:00:00 2001 From: Sam Haves Date: Tue, 21 Apr 2026 06:59:17 -0700 Subject: [PATCH] Remove modelfiledir_ from compiled assets, use owning spec modelfiledir instead. Previously specs that were attached to some parent spec would resolve its asset paths relative to the modelfiledir of the parent spec. This means path resolution would change depending on the source of the parent spec. Instead, this change makes asset file path resolution relative to the "owning spec" i.e. the spec where the asset was created. This enables workflows such as loading a parent spec via resource provider, then loading a child spec via `from_zip` or in memory providing `spec.assets` and resolution will work as intended. PiperOrigin-RevId: 903207910 Change-Id: Ia58020ab372a3ceadf31e804d145e2ae53d8e5f9 --- doc/changelog.rst | 7 +++ src/user/user_mesh.cc | 20 ++------ src/user/user_objects.cc | 20 ++------ src/user/user_objects.h | 12 ----- test/user/user_api_test.cc | 99 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+), 44 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index 03b768f7..ff3091c8 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -23,6 +23,13 @@ General **Migration:** The flag :ref:`multiccd` must be explicitly disabled. +Bug fixes +^^^^^^^^^ + +- Asset paths in attached child specs are now resolved relative to the model file directory of the child spec, rather + than the parent spec. This prevents the origin of the parent spec to affect the resolution of asset paths in the child + spec. + Version 3.7.0 (April 14, 2026) ------------------------------ diff --git a/src/user/user_mesh.cc b/src/user/user_mesh.cc index ae54b634..a7b7dc04 100644 --- a/src/user/user_mesh.cc +++ b/src/user/user_mesh.cc @@ -300,9 +300,6 @@ void mjCMesh::NameSpace(const mjCModel* m) { name = mjuu_stripext(stripped); } mjCBase::NameSpace(m); - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(m->spec_modelfiledir_); - } if (!plugin_instance_name.empty()) { plugin_instance_name = m->prefix + plugin_instance_name + m->suffix; } @@ -712,17 +709,14 @@ void mjCMesh::TryCompile(const mjVFS* vfs) { mujoco::user::FilePath meshdir_; meshdir_ = FilePath(mjs_getString(compiler->meshdir)); - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(model->modelfiledir_); - } - // remove path from file if necessary if (model->strippath) { file_ = mjuu_strippath(file_); } + mjSpec* owning_spec = model->FindSpec(compiler); FilePath filename = meshdir_ + FilePath(file_); - resource_ = LoadResource(modelfiledir_.Str(), filename.Str(), vfs); + resource_ = LoadResource(owning_spec->modelfiledir->c_str(), filename.Str(), vfs); // try loading from cache if (cache != nullptr && LoadCachedMesh(cache, resource_)) { @@ -2957,9 +2951,6 @@ void mjCSkin::NameSpace(const mjCModel* m) { for (auto& name : spec_bodyname_) { name = m->prefix + name + m->suffix; } - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(m->spec_modelfiledir_); - } } @@ -3046,15 +3037,12 @@ void mjCSkin::Compile(const mjVFS* vfs) { throw mjCError(this, "Unknown skin file type: %s", file_.c_str()); } - // copy paths from model if not already defined - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(model->modelfiledir_); - } mujoco::user::FilePath meshdir_; meshdir_ = FilePath(mjs_getString(compiler->meshdir)); FilePath filename = meshdir_ + FilePath(file_); - mjResource* resource = LoadResource(modelfiledir_.Str(), filename.Str(), vfs); + mjSpec* owning_spec = model->FindSpec(compiler); + mjResource* resource = LoadResource(owning_spec->modelfiledir->c_str(), filename.Str(), vfs); try { LoadSKN(resource); diff --git a/src/user/user_objects.cc b/src/user/user_objects.cc index f83bd387..75a7e679 100644 --- a/src/user/user_objects.cc +++ b/src/user/user_objects.cc @@ -4668,9 +4668,6 @@ void mjCHField::NameSpace(const mjCModel* m) { name = mjuu_stripext(stripped); } mjCBase::NameSpace(m); - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(m->spec_modelfiledir_); - } } @@ -4798,15 +4795,12 @@ void mjCHField::Compile(const mjVFS* vfs) { throw mjCError(this, "unsupported content type: '%s'", asset_type.c_str()); } - // copy paths from model if not already defined - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(model->modelfiledir_); - } mujoco::user::FilePath meshdir_; meshdir_ = FilePath(mjs_getString(compiler->meshdir)); FilePath filename = meshdir_ + FilePath(file_); - mjResource* resource = LoadResource(modelfiledir_.Str(), filename.Str(), vfs); + mjSpec* owning_spec = model->FindSpec(compiler); + mjResource* resource = LoadResource(owning_spec->modelfiledir->c_str(), filename.Str(), vfs); struct CachedHField { int nrow, ncol; @@ -4965,9 +4959,6 @@ void mjCTexture::NameSpace(const mjCModel* m) { name = mjuu_stripext(stripped); } mjCBase::NameSpace(m); - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(m->spec_modelfiledir_); - } } @@ -5388,7 +5379,8 @@ void mjCTexture::LoadFlip(std::string filename, const mjVFS* vfs, } // try loading from cache - mjResource* resource = LoadResource(modelfiledir_.Str(), filename, vfs); + mjSpec* owning_spec = model->FindSpec(compiler); + mjResource* resource = LoadResource(owning_spec->modelfiledir->c_str(), filename, vfs); if (cache && cache->PopulateData(GetCacheId(resource, asset_type), resource, callback)) { mju_closeResource(resource); return; @@ -5640,10 +5632,6 @@ void mjCTexture::LoadCubeSeparate(const mjVFS* vfs) { void mjCTexture::Compile(const mjVFS* vfs) { CopyFromSpec(); - // copy paths from model if not already defined - if (modelfiledir_.empty()) { - modelfiledir_ = FilePath(model->modelfiledir_); - } mujoco::user::FilePath texturedir_; texturedir_ = FilePath(mjs_getString(compiler->texturedir)); diff --git a/src/user/user_objects.h b/src/user/user_objects.h index 5c81e40c..4f4da58d 100644 --- a/src/user/user_objects.h +++ b/src/user/user_objects.h @@ -1122,9 +1122,6 @@ class mjCMesh_ : public mjCBase { // octree mjCOctree octree_; // octree of the mesh - - // paths stored during model attachment - mujoco::user::FilePath modelfiledir_; }; class mjCMesh: public mjCMesh_, private mjsMesh { @@ -1336,9 +1333,6 @@ class mjCSkin_ : public mjCBase { int matid; // material id std::vector bodyid; // body ids - - // paths stored during model attachment - mujoco::user::FilePath modelfiledir_; }; class mjCSkin: public mjCSkin_, private mjsSkin { @@ -1391,9 +1385,6 @@ class mjCHField_ : public mjCBase { std::string spec_file_; std::string spec_content_type_; std::vector spec_userdata_; - - // paths stored during model attachment - mujoco::user::FilePath modelfiledir_; }; class mjCHField : public mjCHField_, private mjsHField { @@ -1442,9 +1433,6 @@ class mjCTexture_ : public mjCBase { std::string spec_file_; std::string spec_content_type_; std::vector spec_cubefiles_; - - // paths stored during model attachment - mujoco::user::FilePath modelfiledir_; }; class mjCTexture : public mjCTexture_, private mjsTexture { diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 3c7af426..57e889bb 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include // NOLINT #include #include @@ -32,6 +33,7 @@ #include "src/cc/array_safety.h" #include #include +#include #include "src/xml/xml_api.h" #include "src/xml/xml_numeric_format.h" #include "test/fixture.h" @@ -203,6 +205,103 @@ TEST_F(MujocoTest, AttachAndChildDeletion) { mj_deleteSpec(parent_spec); } +int open_mock(mjResource* resource) { + static const char parent_xml[] = R"( + + + + + + )"; + resource->data = mju_malloc(sizeof(parent_xml)); + std::strcpy((char*)resource->data, parent_xml); + return 1; +} + +int read_mock(mjResource* resource, const void** buffer) { + *buffer = resource->data; + return std::strlen((const char*)resource->data); +} + +void close_mock(mjResource* resource) { + mju_free(resource->data); + resource->data = nullptr; +} + +TEST_F(MujocoTest, AttachedSpecDoesNotInheritURI) { + // This test checks that when we attach a child spec to a parent spec that was + // loaded from a resource provider, the child spec does not inherit the + // resource URI from the parent. This allows the child spec to specify assets + // relative to its model file or in the VFS. + mjpResourceProvider provider = { + .prefix = "fakeprovider", + .open = open_mock, + .read = read_mock, + .close = close_mock, + }; + + mjp_registerResourceProvider(&provider); + + std::array err; + mjSpec* parent_spec = + mj_parseXML("fakeprovider:parent.xml", nullptr, err.data(), err.size()); + mjs_setString(parent_spec->modelname, "parent"); + ASSERT_THAT(parent_spec, NotNull()) << err.data(); + + // Create child spec + static constexpr char child_xml[] = R"( + + + + + + + + + + + )"; + + // Setup VFS with asset + mjVFS vfs; + mj_defaultVFS(&vfs); + static constexpr char asset_data[] = R"( + v 0 0 0 + v 1 0 0 + v 0 1 0 + v 0 0 1 + f 1 2 3 + f 1 2 4 + f 2 3 4 + f 3 1 4 + )"; + mj_addBufferVFS(&vfs, "asset.obj", asset_data, sizeof(asset_data)); + + mjSpec* child_spec = + mj_parseXMLString(child_xml, &vfs, err.data(), err.size()); + mjs_setString(child_spec->modelname, "child"); + ASSERT_THAT(child_spec, NotNull()) << err.data(); + + // Attach child spec to parent spec's world body + mjsBody* world = mjs_findBody(parent_spec, "world"); + ASSERT_THAT(world, NotNull()); + + mjsElement* attached = + mjs_attach(world->element, child_spec->element, "", ""); + ASSERT_THAT(attached, NotNull()); + + mjModel* model = mj_compile(parent_spec, &vfs); + mj_deleteVFS(&vfs); + + EXPECT_THAT(model, NotNull()) << mjs_getError(parent_spec); + + if (model) { + mj_deleteModel(model); + } + mj_deleteSpec(parent_spec); + mj_deleteSpec(child_spec); +} + TEST_F(MujocoTest, ActivatePlugin) { mjSpec* spec = mj_makeSpec(); mjs_activatePlugin(spec, "mujoco.elasticity.cable");