diff --git a/doc/changelog.rst b/doc/changelog.rst index 60d51dc1..d09503b0 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -16,6 +16,9 @@ Upcoming version (not yet released) **Migration:** Replace ``orthographic = "false/true"`` with ``projection="perspective/orthographic"``, respectively. + - Removed ``getdir`` from the ``mjpResourceProvider`` struct. All Resource Providers now use the same shared + implementation. + General ^^^^^^^ diff --git a/doc/includes/references.h b/doc/includes/references.h index 10a40eb1..bbc75adb 100644 --- a/doc/includes/references.h +++ b/doc/includes/references.h @@ -1628,7 +1628,6 @@ struct mjpResourceProvider { mjfOpenResource open; // opening callback mjfReadResource read; // reading callback mjfCloseResource close; // closing callback - mjfGetResourceDir getdir; // get directory callback (optional) mjfResourceModified modified; // resource modified callback (optional) void* data; // opaque data pointer (resource invariant) }; diff --git a/doc/programming/extension.rst b/doc/programming/extension.rst index 763c43c3..2b25d493 100644 --- a/doc/programming/extension.rst +++ b/doc/programming/extension.rst @@ -449,7 +449,6 @@ Next we create the resource provider and register it with MuJoCo: .open = str_open_callback, .read = str_read_callback, .close = str_close_callback, - .getdir = NULL }; // return positive number on success diff --git a/include/mujoco/mjplugin.h b/include/mujoco/mjplugin.h index 55b143df..880da990 100644 --- a/include/mujoco/mjplugin.h +++ b/include/mujoco/mjplugin.h @@ -42,10 +42,6 @@ typedef int (*mjfReadResource)(mjResource* resource, const void** buffer); // callback for closing a resource (responsible for freeing any allocated memory) typedef void (*mjfCloseResource)(mjResource* resource); -// callback for returning the directory of a resource -// sets dir to directory string with ndir being size of directory string -typedef void (*mjfGetResourceDir)(mjResource* resource, const char** dir, int* ndir); - // callback for checking if the current resource was modified from the time // specified by the timestamp // returns 0 if the resource's timestamp matches the provided timestamp @@ -59,7 +55,6 @@ struct mjpResourceProvider { mjfOpenResource open; // opening callback mjfReadResource read; // reading callback mjfCloseResource close; // closing callback - mjfGetResourceDir getdir; // get directory callback (optional) mjfResourceModified modified; // resource modified callback (optional) void* data; // opaque data pointer (resource invariant) }; diff --git a/src/engine/engine_plugin.cc b/src/engine/engine_plugin.cc index 361cf6ae..e9255d8b 100644 --- a/src/engine/engine_plugin.cc +++ b/src/engine/engine_plugin.cc @@ -252,7 +252,6 @@ bool GlobalTable::ObjectEqual(const mjpResourceProvider& p1 p1.open == p2.open && p1.read == p2.read && p1.close == p2.close && - p1.getdir == p2.getdir && p1.modified == p2.modified && p1.data == p2.data); } diff --git a/src/user/user_resource.cc b/src/user/user_resource.cc index 5f2b41d3..a1482b44 100644 --- a/src/user/user_resource.cc +++ b/src/user/user_resource.cc @@ -94,12 +94,6 @@ void FileClose(mjResource* resource) { if (spec) delete spec; } -// OS filesystem getdir callback -void FileGetDir(mjResource* resource, const char** dir, int* ndir) { - *dir = resource->name; - *ndir = mjuu_dirnamelen(resource->name); -} - // OS filesystem modified callback int FileModified(const mjResource* resource, const char*timestamp) { if (mju_isValidBase64(timestamp) != sizeof(time_t)) { @@ -234,16 +228,22 @@ void mju_getResourceDir(mjResource* resource, const char** dir, int* ndir) { *dir = nullptr; *ndir = 0; - if (resource == nullptr) { - return; - } + if (resource && resource->name) { + // ensure prefix is included even if there is no separator in the + // resource name + int prefix_len = 0; + const mjpResourceProvider* provider = resource->provider; + if (provider && provider->prefix) { + prefix_len = strlen(provider->prefix) + 1; + } - const mjpResourceProvider* provider = resource->provider; - if (provider) { - if (provider->getdir) provider->getdir(resource, dir, ndir); - } else { - // fallback to OS filesystem - FileGetDir(resource, dir, ndir); + *dir = resource->name; + *ndir = prefix_len; + for (int i = prefix_len; resource->name[i]; ++i) { + if (resource->name[i] == '/' || resource->name[i] == '\\') { + *ndir = i + 1; + } + } } } @@ -278,4 +278,3 @@ mjSpec* mju_decodeResource(mjResource* resource, const char* content_type, const return decoder->decode(resource, vfs); } - diff --git a/src/user/user_vfs.cc b/src/user/user_vfs.cc index 4b4a736a..5e8cfedb 100644 --- a/src/user/user_vfs.cc +++ b/src/user/user_vfs.cc @@ -178,12 +178,6 @@ int Read(mjResource* resource, const void** buffer) { void Close(mjResource* resource) { } -// getdir callback for the VFS resource provider -void GetDir(mjResource* resource, const char** dir, int* ndir) { - *dir = (resource) ? resource->name : nullptr; - *ndir = (resource) ? mjuu_dirnamelen(resource->name) : 0; -} - // modified callback for the VFS resource provider // return > 0 if modified and 0 if unmodified int Modified(const mjResource* resource, const char* timestamp) { @@ -279,6 +273,6 @@ void mj_deleteVFS(mjVFS* vfs) { const mjpResourceProvider* GetVfsResourceProvider() { static mjpResourceProvider provider - = { nullptr, &Open, &Read, &Close, &GetDir, &Modified, nullptr }; + = { nullptr, &Open, &Read, &Close, &Modified, nullptr }; return &provider; } diff --git a/test/fixture.cc b/test/fixture.cc index 55c1739f..4229578e 100644 --- a/test/fixture.cc +++ b/test/fixture.cc @@ -313,21 +313,6 @@ MockFilesystem::MockFilesystem(std::string unit_test_name) { return (int) fs->GetFile(filename, (const unsigned char**) buffer); }; - resourceProvider.getdir = +[](mjResource* resource, const char** dir, - int* ndir) { - MockFilesystem *fs = static_cast(resource->provider->data); - *dir = resource->name; - - // find last directory path separator - int length = fs->Prefix().size() + 1; - for (int i = length; resource->name[i]; ++i) { - if (resource->name[i] == '/' || resource->name[i] == '\\') { - length = i + 1; - } - } - *ndir = length; - }; - resourceProvider.close = +[](mjResource* resource) {}; mjp_registerResourceProvider(&resourceProvider); } diff --git a/test/user/user_resource_test.cc b/test/user/user_resource_test.cc index 357e75e4..b783761f 100644 --- a/test/user/user_resource_test.cc +++ b/test/user/user_resource_test.cc @@ -19,6 +19,8 @@ #include #include #include +#include +#include #include #include @@ -68,11 +70,15 @@ void close_nop(mjResource* resource) { void close_str(mjResource* resource) { mju_free(resource->data); + resource->data = nullptr; } TEST_F(ResourceTest, RegisterProviderSuccess) { mjpResourceProvider provider = { - "my-prefix.123+45", open_nop, read_nop, close_nop + .prefix = "my-prefix.123+45", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; int count1 = mjp_resourceProviderCount(); @@ -85,16 +91,24 @@ TEST_F(ResourceTest, RegisterProviderSuccess) { TEST_F(ResourceTest, RegisterProviderMultipleSuccess) { mjpResourceProvider provider = { - "my-prefix.123+44", open_nop, read_nop, close_nop + .prefix = "my-prefix.123+44", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; mjpResourceProvider provider2 = { - "my-prefix.123+46", open_nop, read_nop, close_nop + .prefix = "my-prefix.123+46", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; - mjpResourceProvider provider3 = { - "my-prefix.123+41", open_nop, read_nop, close_nop + .prefix = "my-prefix.123+41", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; int count1 = mjp_resourceProviderCount(); @@ -111,7 +125,7 @@ TEST_F(ResourceTest, RegisterProviderMultipleSuccess) { TEST_F(ResourceTest, RegisterProviderMissingCallbacks) { mjpResourceProvider provider = { - "myprefix" + .prefix = "myprefix", }; // install warning handler @@ -130,7 +144,10 @@ TEST_F(ResourceTest, RegisterProviderMissingCallbacks) { TEST_F(ResourceTest, RegisterProviderMissingPrefix) { mjpResourceProvider provider = { - "", open_nop, read_nop, close_nop + .prefix = "", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; // install warning handler @@ -149,7 +166,10 @@ TEST_F(ResourceTest, RegisterProviderMissingPrefix) { TEST_F(ResourceTest, RegisterProviderInvalidPrefix1) { mjpResourceProvider provider = { - "1invalid", open_nop, read_nop, close_nop + .prefix = "1invalid", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; // install warning handler @@ -168,7 +188,10 @@ TEST_F(ResourceTest, RegisterProviderInvalidPrefix1) { TEST_F(ResourceTest, RegisterProviderInvalidPrefix2) { mjpResourceProvider provider = { - "invalid:", open_nop, read_nop, close_nop + .prefix = "invalid:", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; // install warning handler @@ -187,11 +210,17 @@ TEST_F(ResourceTest, RegisterProviderInvalidPrefix2) { TEST_F(ResourceTest, RegisterProviderSame) { mjpResourceProvider provider = { - "prefix", open_nop, read_nop, close_nop + .prefix = "prefix", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; mjpResourceProvider provider2 = { - "prefix", open_nop, read_nop, close_nop + .prefix = "prefix", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; int i1 = mjp_registerResourceProvider(&provider); @@ -206,11 +235,17 @@ TEST_F(ResourceTest, RegisterProviderSame) { TEST_F(ResourceTest, RegisterProviderSameCase) { mjpResourceProvider provider = { - "prefix", open_nop, read_nop, close_nop + .prefix = "prefix", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; mjpResourceProvider provider2 = { - "PREFIX", open_nop, read_nop, close_nop + .prefix = "PREFIX", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; int i1 = mjp_registerResourceProvider(&provider); @@ -225,7 +260,10 @@ TEST_F(ResourceTest, RegisterProviderSameCase) { TEST_F(ResourceTest, GeneralTest) { mjpResourceProvider provider = { - "str", open_str, read_str, close_str + .prefix = "str", + .open = open_str, + .read = read_str, + .close = close_str, }; // register resource provider @@ -246,7 +284,10 @@ TEST_F(ResourceTest, GeneralTest) { TEST_F(ResourceTest, GeneralFailureTest) { mjpResourceProvider provider = { - "str", open_str, read_str, close_str + .prefix = "str", + .open = open_str, + .read = read_str, + .close = close_str, }; // register resource provider @@ -265,7 +306,10 @@ TEST_F(ResourceTest, GeneralFailureTest) { TEST_F(ResourceTest, NameWithValidPrefix) { mjpResourceProvider provider = { - "nop", open_nop, read_nop, close_nop + .prefix = "nop", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; // register resource provider @@ -288,7 +332,10 @@ TEST_F(ResourceTest, NameWithValidPrefix) { TEST_F(ResourceTest, NameWithUpperCasePrefix) { mjpResourceProvider provider = { - "nop", open_nop, read_nop, close_nop, + .prefix = "nop", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; // register resource provider @@ -311,7 +358,10 @@ TEST_F(ResourceTest, NameWithUpperCasePrefix) { TEST_F(ResourceTest, NameWithInvalidPrefix) { mjpResourceProvider provider = { - "nop", open_nop, read_nop, close_nop + .prefix = "nop", + .open = open_nop, + .read = read_nop, + .close = close_nop, }; // register resource provider @@ -331,6 +381,78 @@ TEST_F(ResourceTest, NameWithInvalidPrefix) { ASSERT_THAT(resource, IsNull()); } +TEST_F(ResourceTest, GetResourceDir) { + const std::vector> cases = { + { "foo/bar/baz", "foo/bar/" }, + { "foo/bar/", "foo/bar/" }, + { "foo/bar", "foo/" }, + { "/foo/bar", "/foo/" }, + { "/foo", "/" }, + { "/", "/" }, + { "", "" }, + }; + + const char* dir = nullptr; + int ndir = 0; + + mjpResourceProvider provider; + provider.prefix = "provider"; + + for (const auto& [name, expected] : cases) { + mjResource resource; + resource.provider = nullptr; + resource.name = const_cast(name.c_str()); + mju_getResourceDir(&resource, &dir, &ndir); + EXPECT_THAT(std::string(dir, ndir), expected); + } +} + +TEST_F(ResourceTest, GetResourceDirProvider) { + const std::vector> cases = { + { "provider:/foo/bar", "provider:/foo/" }, + { "provider:/foo/", "provider:/foo/" }, + { "provider:foo/", "provider:foo/" }, + { "provider:foo", "provider:" }, + { "provider:/", "provider:/" }, + }; + + const char* dir = nullptr; + int ndir = 0; + + mjpResourceProvider provider; + provider.prefix = "provider"; + + for (const auto& [name, expected] : cases) { + mjResource resource; + resource.provider = &provider; + resource.name = const_cast(name.c_str()); + mju_getResourceDir(&resource, &dir, &ndir); + EXPECT_THAT(std::string(dir, ndir), expected); + } +} + +TEST_F(ResourceTest, GetResourceDirNullResource) { + const char* dir = nullptr; + int ndir = 0; + + mju_getResourceDir(nullptr, &dir, &ndir); + EXPECT_THAT(dir, IsNull()); + EXPECT_EQ(ndir, 0); +} + +TEST_F(ResourceTest, GetResourceDirNullName) { + const char* dir = nullptr; + int ndir = 0; + + mjResource resource; + resource.name = nullptr; + resource.provider = nullptr; + + mju_getResourceDir(nullptr, &dir, &ndir); + EXPECT_THAT(dir, IsNull()); + EXPECT_EQ(ndir, 0); +} + TEST_F(ResourceTest, OSFilesystemTimestamps) { time_t t;