Remove getdir from mjpResourceProvider.

All known implementations were effectively the same as the default fallback. It was just adding unneeded complexity. Removing this now will help with upcoming improvements to resource providers.

PiperOrigin-RevId: 855163273
Change-Id: I529fc18d0d03a8dc676c909ec9e4b1da1454d414

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
 ^^^^^^^
 

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)
 };

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

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)
 };

src/engine/engine_plugin.cc

@@ -252,7 +252,6 @@ bool GlobalTable<mjpResourceProvider>::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);
 }

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);
 }
-

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;
 }

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<MockFilesystem*>(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);
 }

test/user/user_resource_test.cc

@@ -19,6 +19,8 @@
 #include <cstring>
 #include <ctime>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -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<std::pair<std::string, std::string>> 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<char*>(name.c_str());
+    mju_getResourceDir(&resource, &dir, &ndir);
+    EXPECT_THAT(std::string(dir, ndir), expected);
+  }
+}
+
+TEST_F(ResourceTest, GetResourceDirProvider) {
+  const std::vector<std::pair<std::string, std::string>> 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<char*>(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;