ResourceManager only loads archives in current working directory

[DanRStevens]

The following test demonstrates the problem:

#include "../src/Archive/VolFile.h"

TEST(ResourceManager, ArchiveIsLoaded)
{
    Archive::VolFile::CreateArchive("./data/Test.vol", {});

    ResourceManager resourceManager("./data");
    EXPECT_EQ(std::vector<std::string>{"Test.vol"}, resourceManager.GetArchiveFilenames());
}

---

In the ResourceManager constructor, the .vol files are found with:

const auto volFilenames = GetFilesFromDirectory(".vol");

The GetFilesFromDirectory method is implemented as:

std::vector<std::string> ResourceManager::GetFilesFromDirectory(const std::string& fileExtension)
{
    auto directoryContents = XFile::GetFilenamesFromDirectory(resourceRootDir, fileExtension);
    XFile::EraseNonFilenames(directoryContents);

    return directoryContents;
}

The XFile::GetFilenamesFromDirectory method returns only the filename, with no directory component. The XFile::EraseNonFilenames(directoryContents); call implicitly uses the current working directory to test the files using XFile::IsFile(path). Presumably this is to remove directories, however, due to the difference in the path portion, it also filters out regular files which fail to exist in the current working directory.

The end result is, archives files will not be seen and loaded when the ResourceManager directory is not the current working directory (unless the current working directory contains an identically named file).

[Brett208]

I suspect I added this logic to make calls to this function in OP2Archive (and to a lesser extend OP2MapImager) work.

I'm not too picky if a better solution is picked, as long as it is tested against those two applications.

Maybe some of the logic to determine if the item is a directory needs to be moved out of XFile and instead tested against either in ResourceManager itself or the calling application?

-Brett

[DanRStevens]

The new Ranges addition to C++ with filter iterators would be really nice for this. Sadly that feature isn't here yet.

I've looked into manually creating a filter iterator with current C++ features, though that seems rather messy and verbose.

For now, it may be easiest to provide a method that filters out directories when returning the vector of files. I guess that's what I expected in the original plan, though we never really figured out what to call such a method so there'd be a clear distinction from the filtering version and the non-filtering version.

Or maybe we should extract out the filter/select criteria into a lambda, and use a template method to retrieve a vector of files. Filtering by extension or regex would then just be a wrapper with a fixed lambda.

Using the existing method, that could become:

template <typename FilterFunction>
std::vector<std::string> GetFilenamesFromDirectory(const std::string& directory, FilterFunction filterFunction) {
    auto filenames = GetFilenamesFromDirectory(directory);

    filenames.erase(
        std::remove_if(filenames.begin(), filenames.end(), filterFunction),
        filenames.end()
    );

    return filenames;
}

Or, for higher efficiency, we could filter/select before creating the vector:

template <typename SelectFunction>
std::vector<std::string> XFile::GetFilenamesFromDirectory(const std::string& directory, SelectFunction selectFunction)
{
    // Creating a path with an empty string will prevent the directory_iterator from finding files in the current relative path.
    auto pathStr = directory.length() > 0 ? directory : "./";

    std::vector<std::string> filenames;
    for (const auto& entry : fs::directory_iterator(pathStr)) {
        if (selectFunction(entry.path().generic_string())) {
            filenames.push_back(GetFilename(entry.path().generic_string()));
        }
    }

    return filenames;
}

[Brett208]

Exposing the lambda would encourage including the filesystem in other portions of the code. XFile was originally designed to limit the use of filesystem to a single file within the code base.

@DanRStevens, do you think the major compilers are all mature enough in their implementation of std::filesystem to render this requirement no longer necessary?

If we are okay with filesystem usage elsewhere, perhaps it is time to start deleting chunks of XFile and just directly calling filesystem in other portions of the code. Maybe XFile would be reduced to a couple of helper functions to extend filesystem if appropriate for this codebase?

Otherwise, I think we could replace the regex overload to instead expose a select function as mentioned. It would maybe be more flexible.

I would still want to provide simple ways to complete common tasks without dealing with lambdas. I think that list is:

1. Get contents of a directory
2. Get all files from a directory (not subdirectories)
3. Get all files with a specific extension from a directory
4. Some flexible way to be more creative in selection (regex, or lambda)

If we think filesystem is not mature enough yet, I guess this should be done in XFile. If filesystem is mature enough, maybe it should just be directly called from the resource manager?

[DanRStevens]

I think support for <filesystem> is still lagging on Mingw. I would prefer to maintain isolation and preserve XFile for now.

I agree with support for those use cases without explicit use of lambdas. It should be easy enough to provide wrappers to support those use cases.

---

Actually, if we want to allow for generic use of lambdas, while still preserving isolation, and are willing to sacrifice a bit of speed, we could offer a std::function overload. A lambda can convert to a std::function, and the method itself would not be a template, so it could still be implemented in the .cpp file.

Not particularly relevant in this case, but but so we're aware, there can be significant overhead when calling through std::function. I don't think we'd ever be dealing with a sufficient number of files for that to matter. We would however be placing such a call in an inner loop, so maybe not a great example for future code to be based off of.

---

Edit: To add some clarity about specific action items, I propose we create a few wrapper methods around an internal template method. Optionally, if we so choose, we might also add a std::function based overload, though I don't think that is required at this time.