Add -additionaldir command-line option
#471
Conversation
|
Hi. I'm working on cfile / ddio refactoring and some of your changes already done in my branch. I'd suggest to wait it since after that path handling and general filesystem operations will be much simpler. Still, there some comments on general idea (as I see it, regarding to #373 and similar issues). We have read-only and writable paths. Read-only are some preconfigured on build-time paths (like |
So you’re saying that I should rebase this PR on main once your PR gets merged?
I agree. This PR implements all of what you said, except for these things:
|
Jayman2000
force-pushed
the
additionaldir-arg
branch
5 times, most recently
from
089244c to
89ec30e
Compare
Jayman2000
force-pushed
the
additionaldir-arg
branch
from
89ec30e to
fba08d3
Compare
|
I just pushed a new version of this PR. Here are the changes:
Here are some things that I could use some help with:
|
Jayman2000
force-pushed
the
additionaldir-arg
branch
from
fba08d3 to
6894792
Compare
|
I just pushed another new version of this PR. Here are the changes:
I could still use some help with the whole |
Jayman2000
force-pushed
the
additionaldir-arg
branch
4 times, most recently
from
7dc8ad8 to
66eb9ce
Compare
|
Tell us if/when you need feedback or help with anything :) |
Like I mentioned earlier, I could use some help with the |
|
I would like to correct some points of PR. It's rather big feature request. Let's try to implement feature in one of already exist submodule. As it's more file access and discovery thing, I suggest to use I thinking about using some cache path resolver that adds already resolved paths into LRU cache or similar by lowercased relative path filename. This cache can be used to keep tracking already opened resources and avoiding excessive case-insensitive look-ups, like cfile's Regarding |
Finally, someone is reading my code! Before I start implementing your suggestions, I want to make sure that I fully understand them.
When I move all of the stuff from the
I think that you’re saying two things here:
Am I understanding correctly?
So you’re saying that you might open another PR that optimizes Descent 3’s case insensitive path code further?
Yeah, but how am I going to make those minimal changes? Like I said previously:
|
I'm planning heavily rewrite cfile / ddio modules (related to file operations), I still see some optimization points here to improve overall performance. Path cache is one of them. Currently we are about on halfway of this refactoring :). Main issue of case insensitive discovering is that can't be optimized much. On each level of directories directory iterator loses so much time, making whole traversal incredibly slow. Optimization of |
OK. In that case, I’ll move everything to the Those are just some examples. There’s a lot of other stuff that I’ve had to deal with in order to finish this PR. The thought of having to hunt down and change every absolute-path
It sounds like you’re saying that you agree with the changes in this PR. Specifically, it sounds like you agree with the parts of this PR that replace
I don’t think that you understood this question that I asked:
Here’s my motivation for asking that question. Sometimes, you mention things that could be done but haven’t been done yet. Here are some examples:
When someone comments on my PR, I expect the comments to fall into one of four categories: approvals, disapprovals, questions about the PR or suggestions for how the PR can be improved. Some of your comments don’t fall into any of those categories so I don’t know what to do with them. In the past, I tried asking you how one of those comments affects my PR, but you never responded.
This is really helpful. Part of the reason why I didn’t understand
If that’s the case, then I think that you’ll like the changes in this PR’s “[WIP] Consolidate case-sensitive filesystem functions” commit. |
Jayman2000
force-pushed
the
additionaldir-arg
branch
2 times, most recently
from
cece3f5 to
c08e82a
Compare
|
I pushed a new version of this PR that resolves merge conflicts. I haven’t gotten rid of the |
|
Jayman2000
force-pushed
the
additionaldir-arg
branch
from
c08e82a to
8a63b5a
Compare
OK. I just pushed a new version of this PR that restores the code that was in
I feel like this PR already did that by the time that you left that comment. When you left that comment, this PR added functions named What do you think? Do you think that the way that this PR uses those functions counts as avoiding direct access to the |
| #endif | ||
|
|
||
| std::vector<std::filesystem::path> cf_LocatePathMultiplePathsHelper(std::filesystem::path relative_path, bool stop_after_first_result) { | ||
| ASSERT(("realative_path should be a relative path.", relative_path.is_relative())); |
There was a problem hiding this comment.
You don't have to use ASSERTs here since you just can resolve relative paths with std::filesystem::absolute().
There was a problem hiding this comment.
I don’t understand how I could replace the ASSERTs with code that uses std::filesystem::absolute(). Could you post a diff showing what you would do here?
Before this change, API.vp[5] was cast to a pilot pointer and then cast to a char pointer immediately afterwards. Nothing gets done to API.vp[5] while it’s a pilot pointer, so we can remove the pilot pointer cast without changing any behavior.
The main motivation behind this commit is to make it easier to create a future commit. That future commit will modify code in CheckHogFile(). If I don’t deduplicate the code now, then I’ll have to edit the same lines of code in multiple different places.
The main motivation behind this commit is to make it easier to create a future commit. That future commit will change how CheckHogFile() finds the path for d3.mn3 and d3_2.mn3. Simplifying CheckHogFile() now decreases the amount of code that needs to be changed in that future commit.
Before this change, mve_Init() took two arguments. Neither of the arguments were ever used. This commit simplifies some code by removing those unused arguments. The main motivation behind this commit is to make it easier to make a future commit. One of the arguments that this commit removes was named dir. dir was a path to the movies/ directory. The future commit will make it so that there can be more than one movies/ directory. Removing dir means that I won’t have to mess with dir when I create that future commit.
Before this change, the Base_directory variable would get accessed before we set its value. This seems to be OK. I’m not sure about the details, but it seems like C++ guarantees that Base_directory will be filled with zeros when it’s created. That being said, the code that was modified by this commit used to be misleading. The code would copy the contents of the Base_directory variable into another variable named path. If you read the code, you would think that path would be set to something along the lines of "C:\\Games\\Descent3" or "/home/username/D3-open-source", but in reality, path would be set to "". This change makes it clear that path is guaranteed to be set to "". The main motivation behind this commit is to make it easier to create a future commit. That future commit will replace Base_directory with a new variable named Base_directories. Base_directories will have a different data type that does not get filled with zeros by default. In other words, we need to set the new Base_directories variable before we use it. Unfortunately, neither the current Base_directory variable nor the future Base_directories variable will get set in time. Both of them will be set after path variable gets set. This commit allows me to not worry about that detail when I create the future Base_directories commit.
Before this change, Base_directory was a char array. In general, it’s better to use an std::filesystem::path to represent paths. Here’s why: • char arrays have a limited size. If a user creates a file or directory that has a very long path, then it might not be possible to store that path in a given char array. You can try to make the char array long enough to store the longest possible path, but future operating systems may increase that limit. With std::filesystem::paths, you don’t have to worry about path length limits. • char arrays cannot necessarily represent all paths. We try our best to use UTF-8 for all char arrays [1], but unfortunately, it’s possible to create a path that cannot be represent using UTF-8 [2]. With an std::filesystem::paths, stuff like that is less likely to happen because std::filesystem::path::value_type is platform-specific. • char arrays don’t have any built-in mechanisms for manipulating paths. Before this commit, the code would work around this problem in two different ways: 1. It would use Descent 3–specific functions that implement path operations on char arrays/pointers (for example, ddio_MakePath()). Once all paths use std::filesystem::path, we’ll be able to simplify the code by removing those functions. 2. It would temporarily convert Base_directory into an std::filesystem::path. This commit simplifies the code by removing those temporary conversions because they are no longer necessary. [1]: 7cd7902 (Merge pull request DescentDevelopers#494 from Jayman2000/encoding-improvements, 2024-07-20) [2]: <rust-lang/rust#12056>
The main motivation behind this commit is to make it easier to create a future commit. That future commit will will take multiple different functions from throughout the codebase and replace them with a new function named cf_LocatePath(). One of the functions that will get replaced is cf_FindRealFileNameCaseInsensitive(). There are tests for cf_FindRealFileNameCaseInsensitive() in cfile/tests/cfile_tests.cpp. When I make that future commit, I will have to change the tests in cfile/test/cfile_tests.cpp so that they test the cf_LocatePath() function instead of the cf_FindRealFileNameCaseInsensitive() function. There is an important difference between cf_LocatePath() and cf_FindRealFileNameCaseInsensitive(). cf_LocatePath() depends on the Base_directory variable. cf_FindRealFileNameCaseInsensitive() does not. In order to update the tests so that they use cf_LocatePath(), I need to make sure that the tests have access to the Base_directory variable. Before this change, the Base_directory variable was declared in Descent3/init.cpp. That meant that if a program wanted to access Base_directory, then it would have to link to Descent3/init.cpp. In other words, we would have to link to Descent3/init.cpp when compiling the program that currently tests cf_FindRealFileNameCaseInsensitive() but will test cf_LocatePath() in the future. I tried making that program link to Descent3/init.cpp, but I gave up after a wile. Descent3/init.cpp depends on a lot of other things in the codebase. In order to make it easier to create that future commit, this commit moves the Base_directory variable into the cfile module. When I create that future commit, I won’t have to mess with anything linking related because the cfile tests already link to the cfile module. Additionally, this change will make compiling that test program more efficient. There’s not need for the compiler to look at the entirety of Descent3/init.cpp just because we need a single variable from it.
The main motivation behind this commit is to make it easier to create a future commit. That futures commit will rename the cf_FindRealFileNameCaseInsensitive() function and change its code slightly. I was struggling to create that future commit because I found the code in cf_FindRealFileNameCaseInsensitive() difficult to understand. This change will make it easier to create that future commit by making the code in cf_FindRealFileNameCaseInsensitive() easier to understand.
Descent 3 is case-insensitive. It doesn’t matter if a file is named “ppics.hog” or “PPPICS.HOG”. Descent 3 will load the file regardless. In order to accomplish this, Descent 3 has to have special code for case-sensitive filesystems. That code must take a fake case-insensitive path and turn it into a real case-sensitive path. Before this change, there was multiple chunks of code that helped turn fake case-insensitive paths into real case-sensitive paths. There was cf_FindRealFileNameCaseInsenstive(), mve_FindMovieFileRealName() and a chunk of code in open_file_in_directory() that only exists if __LINUX__ is defined. This removes each of those pieces of code and replaces them with a new cf_LocatePath() function. Using the new cf_LocatePath() function has two main advantages over the old way of doing things. First, having a single function is simpler than having three different pieces of code. Second, the new cf_LocatePath() function will make it easier to create a future commit. That future commit will make Descent 3 look for files in more than just the -setdir directory. Having a single function that’s responsible for determining the true path of a file will make it much easier to create that future commit.
Before this change, there were two functions in Descent3/menu.cpp that had parameters named base_directory. base_directory wasn’t a very good name for those variables. We have another variable declared in cfile/cfile.cpp named Base_directory. Confusingly, the Base_directory variable and the base_directory parameters both referred to different things. For example, let’s say that a user installed Descent 3 into /home/username/D3-open-source. In that case, the Base_directory variable would be set to /home/username/D3-open-source and the base_directory parameters would be set to /home/username/D3-open-source/missions. To make the code easier to understand, this commit renames both of the base_directory parameters to “missions_directory”.
Before this change, the FindArg() function was well suited for finding
command-line options that only appear once. It’s very resonable for
FindArg() to only support arguments that appear one time because Descent
3 doesn’t have any command-line options that should be specified
multiple times. For example, it would be pretty pointless to do
something like this:
Descent3 -useexedir -useexedir
or something like this:
Descent3 -aspect 1.0 -aspect 1.6
It does, however, sometimes makes sense to repeat command-line options for
other applications. For example, you can specify -e multiple times when
running grep [1]:
grep -e pattern1 -e pattern2 file.txt
The main motivation behind this change is to make it easier to create a
future commit. That future commit will add a command-line option named
“-additionaldir”. -additionaldir will be similar to grep’s -e flag. In
other words, it will make sense to do this:
Descent3 -additionaldir /home/user/dir1 -additionaldir /home/user/dir2
Adding a start parameter to FindArg() now will make it easier for that
future commit parse the multiple occurrences of -additionaldir.
Unfortunately, there is one drawback to this change. In Descent3/args.h,
I gave the start parameter a default value of 1. Giving the start
parameter a default value allowed me to add the start parameter without
having to change most of the calls to the FindArg() function. There was
one situation where I had to change how FindArg was called, though.
DLLFindArg is a pointer to the FindArg() function. You cannot give
function pointers default arguments [2]. As a result,
FindArg("-someargument") works, but DLLFindArg("-someargument") does
not. Instead, you have to write DLLFindArg("-someargument", 1) which is
a little bit annoying.
[1]: <https://www.gnu.org/software/grep/manual/html_node/Matching-Control.html>
[2]: <https://stackoverflow.com/a/9760710/7593853>
Before this change, Descent 3 would look for all of its game data files in a single directory. This change allows users to spread out Descent 3’s game data over multiple directories. Building Descent 3 produces multiple files that can be freely redistributed (Descent3, d3-linux.hog, online/Direct TCP~IP.d3c, etc.). Running Descent 3 requires those files and several additional files that cannot be freely redistributed. Before this change, the files that were redistributable had to be in the same directory as the files that were not redistributable. This change makes it so that they can be in separate directories. The main motivation behind this change is to allow people to package Descent 3 for Linux in a reasonable manner. For the most part, binary packages for Descent 3 will contain all of the freely redistributable components. Package managers will copy those components into system directories that are owned by root and that users probably shouldn’t edit manually. Users will then create a new directory and copy the game data from their copy of Descent 3 into that new directory. Users will then be able to run: Descent3 -setdir <path-to-proprietary-files> -additionaldir <path-to-open-source-files> The -additionaldir option can also be used to support more complicated scenarios. For example, if the user is using Debian’s game-data-packager [1], then they would do something like this: Descent3 -setdir <path-to-writable-directory> -additionaldir <path-to-gdp-directory> -additionaldir <path-to-open-source-files> The -additionaldir option can also be used to load a mod that replaces .hog files: Descent3 -setdir <path-to-base-game-data> -additionaldir <path-to-mod-files> [1]: <DescentDevelopers#373 (comment)>
Jayman2000
force-pushed
the
additionaldir-arg
branch
from
8a63b5a to
f5d2a43
Compare
|
Tested on Linux and Windows, and generally changes works as intended. Thanks for patience and contribution. |
|
Thanks for this improvement, big step forward for file management! |
By default, Descent 3 will only look for game files in the current working directory. In order to function properly, Descent 3 needs to look for some of its files in the Nix store. This commit creates a wrapper around the main Descent3 executable that automatically looks in the correct path in the Nix store. Hopefully, this wrapper will only exist temporarily. I have an unfinished Descent 3 pull request that adds a DEFAULT_ADDITIONAL_DIRS CMake option [1]. Once I can finish that pull request (DescentDevelopers/Descent3#623 needs to get merged first), we’ll be able to get rid of this wrapper and use DEFAULT_ADDITIONAL_DIRS instead. Additionally, if DescentDevelopers/Descent3#628 gets merged first, then that pull request will also probably make this wrapper unnecessary. [1]: <DescentDevelopers/Descent3#471>
By default, Descent 3 will only look for game files in the current working directory. In order to function properly, Descent 3 needs to look for some of its files in the Nix store. This commit creates a wrapper around the main Descent3 executable that automatically looks in the correct path in the Nix store. Hopefully, this wrapper will only exist temporarily. I have an unfinished Descent 3 pull request that adds a DEFAULT_ADDITIONAL_DIRS CMake option [1]. Once I can finish that pull request (DescentDevelopers/Descent3#623 needs to get merged first), we’ll be able to get rid of this wrapper and use DEFAULT_ADDITIONAL_DIRS instead. Additionally, if DescentDevelopers/Descent3#628 gets merged first, then that pull request will also probably make this wrapper unnecessary. [1]: <DescentDevelopers/Descent3#471>
By default, Descent 3 will only look for game files in the current working directory. In order to function properly, Descent 3 needs to look for some of its files in the Nix store. This commit creates a wrapper around the main Descent3 executable that automatically looks in the correct path in the Nix store. Hopefully, this wrapper will only exist temporarily. I have an unfinished Descent 3 pull request that adds a DEFAULT_ADDITIONAL_DIRS CMake option [1]. Once I can finish that pull request (DescentDevelopers/Descent3#623 needs to get merged first), we’ll be able to get rid of this wrapper and use DEFAULT_ADDITIONAL_DIRS instead. Additionally, if DescentDevelopers/Descent3#628 gets merged first, then that pull request will also probably make this wrapper unnecessary. [1]: <DescentDevelopers/Descent3#471>
By default, Descent 3 will only look for game files in the current working directory. In order to function properly, Descent 3 needs to look for some of its files in the Nix store. This commit creates a wrapper around the main Descent3 executable that automatically looks in the correct path in the Nix store. Hopefully, this wrapper will only exist temporarily. I have an unfinished Descent 3 pull request that adds a DEFAULT_ADDITIONAL_DIRS CMake option [1]. Once I can finish that pull request (DescentDevelopers/Descent3#623 needs to get merged first), we’ll be able to get rid of this wrapper and use DEFAULT_ADDITIONAL_DIRS instead. Additionally, if DescentDevelopers/Descent3#628 gets merged first, then that pull request will also probably make this wrapper unnecessary. [1]: <DescentDevelopers/Descent3#471>
By default, Descent 3 will only look for game files in the current working directory. In order to function properly, Descent 3 needs to look for some of its files in the Nix store. This commit creates a wrapper around the main Descent3 executable that automatically looks in the correct path in the Nix store. Hopefully, this wrapper will only exist temporarily. I have an unfinished Descent 3 pull request that adds a DEFAULT_ADDITIONAL_DIRS CMake option [1]. Once I can finish that pull request (DescentDevelopers/Descent3#623 needs to get merged first), we’ll be able to get rid of this wrapper and use DEFAULT_ADDITIONAL_DIRS instead. Additionally, if DescentDevelopers/Descent3#628 gets merged first, then that pull request will also probably make this wrapper unnecessary. [1]: <DescentDevelopers/Descent3#471>
Pull Request Type
Description
This PR adds a command-line argument named
-additionaldir.-additionaldirallows users to store game assets in multiple different directories. For example, if a user installs Descent 3 using a package manager, then they’ll be able to do this:Related Issues
This is related to #373 and #534, but it doesn’t fully implement either of those feature requests.
Checklist
Additional Comments
This PR is meant to be a first step. If it gets accpeted, then I’ll open PRs to do the following:
DEFAULT_ADDITIONAL_DIRS. This way, package maintainers will be able to createDescent3executables that automatically find the other files that get installed by their Descent 3 package.-additionaldirand how it compares to-setdir..pldfiles from-additionaldirdirectories.