Enhance multiple base directory usage #628

winterheart · 2024-10-08T19:09:30Z

Pull Request Type

Description

This is further enhancement of #471 feature.

Priority of base directory changed to following list (top is higher priority):

Writable preference path
User defined paths (cmd-line and configuration)
Platform defined paths (such as /usr/share/Descent3 on Linux or Steam installation paths on all platforms (TBD))
Directory of executable

User defined path can be set by -additionaldir or GAME_base_directory in config file.

Added ddio_GetPrefPath() and ddio_GetBasePath() helper functions for building base directory entries.

Make cf_LocatePathCaseInsensitiveHelper() to iterate over all path items (previously it's only iterated only on filename of relative path).

Update functions related to writable base directory functionality: save / load games, demo writing / playback, logo and audiotaunts, screenshots.

Screenshots now are saved into <writable base path> / screenshots with timestamp in name.

If cmake project configured with -DFORCE_PORTABLE_INSTALL=OFF, Descent3 will search data files in platform defined path (${CMAKE_INSTALL_DATADIR}/Descent3 or /usr/share/Descent3).

Related Issues

Fixes #326
Fixes #373
Fixes #534

Screenshots (if applicable)

Checklist

I have tested my changes locally and verified that they work as intended.
I have documented any new or modified functionality.
I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

Make case-insensitive path work on all path components instead of only filename.

Fix error message when real_name is empty.

First added entries to base directories has higher priority on search path.

ddio_GetPrefPath() implements retrieving writable location path. ddio_GetBasePath() implements retrieving parent path of executable.

After initialization, we get following base directories list (in priority order): * Writable preference path * User defined paths (cmd-line and configuration) * Platform defined paths (such as /usr/share/Descent3 on Linux or Steam installation paths on all platforms (TBD)) * Directory of executable Removing `-setdir` and `-useexedir` as redundant (both can be replaced with `-additionaldir` option).

Convert functions to use std::fs::path, minor cleanups.

…s::path Write custom graphics and sounds to writable base directory.

Now screenshots are saved in "screenshots" directory of writable base path. Fix minor issues.

Lgt2x · 2024-10-08T20:18:49Z

Really nice, I'm not sure yet how this fixes #373 though

winterheart · 2024-10-08T20:31:04Z

Changes implements platform specific file locations for read (/usr/share/Descent3), it's critical piece for Linux distribution packagers.

CMakeLists.txt

Lgt2x · 2024-10-08T20:48:17Z

Changes implements platform specific file locations for read (/usr/share/Descent3), it's critical piece for Linux distribution packagers.

it is! I hope we can still get the XDG runtime variables in there to make the packagers happier

Jayman2000 · 2024-10-08T21:48:39Z

At the moment, files in -additionaldir directories override files in the writable base directory. If I’m understanding correctly, this PR flips that order. This PR makes it so that files in the writable base directory override files in the -additionaldir directories.

Why did you make that change?

winterheart · 2024-10-12T23:01:41Z

At the moment, files in -additionaldir directories override files in the writable base directory. If I’m understanding correctly, this PR flips that order. This PR makes it so that files in the writable base directory override files in the -additionaldir directories.

Why did you make that change?

Writable base directory should have highest priority as user can override in that way underlying files. Additional dirs is just another way to provide additional paths to search locations.

Jayman2000 · 2024-10-13T11:10:02Z

Writable base directory should have highest priority as user can override in that way underlying files.

OK, so it sounds like you’re saying that users would not be able to override the underlying files if you changed the order to this:

-additionaldir paths
Writable preference path
User defined paths (configuration)
Platform defined paths (such as /usr/share/Descent3 on Linux or Steam installation paths on all platforms (TBD))
Directory of executable

Am I understanding you correctly?

winterheart · 2024-10-18T11:45:35Z

Priority of paths is described as follows:

Writable preference path
User defined paths (cmd-line and configuration, -additionaldir included)
Platform defined paths (such as /usr/share/Descent3 on Linux or Steam installation paths on all platforms (TBD))
Directory of executable

-additionaldir paths are one of user defined paths and they should be considered as read-only paths. -additinaldir can be used as override of low-priority paths that predefined by executable.

Jayman2000 · 2024-10-18T12:18:49Z

I don’t think that you understood my question. My question was about a hypothetical change to your code. Your answer was about the actual code that you currently have written.

Priority of paths is described as follows:

Writable preference path

User defined paths (cmd-line and configuration, -additionaldir included)

Platform defined paths (such as /usr/share/Descent3 on Linux or Steam installation paths on all platforms (TBD))

Directory of executable

-additionaldir paths are one of user defined paths and they should be considered as read-only paths. -additinaldir can be used as override of low-priority paths that predefined by executable.

I agree that that is how this pull request is currently implemented. The problem is: I don’t know why you implemented it that way. I don’t know why you changed the order from “-additionaldir directories override the writable base directory” to “the writable base directory overrides -additionaldir directories”. I want to figure out if that’s a good or a bad change, but I can’t because I don’t know why you made that change.

Let’s say that you changed your code so that it searched through directories in this order:

-additionaldir paths
Writable preference path
User defined paths (configuration only, -additionaldir not included)
Platform defined paths (such as /usr/share/Descent3 on Linux or Steam installation paths on all platforms (TBD))
Directory of executable

Based on your previous comment, it sounds like the user would not be able to override the underlying files if that change was made. Am I understanding correctly?

winterheart · 2024-10-18T12:58:22Z

First of all, current logic of additionaldir implements same way of search path, they get added after of writable path. My PR changes executable and current path priorities, not additionaldir. I stated before that current and executable directory is not always can be writable directory for user, and this PR fixes that issue.

Writable path is highest priority, because user controls it and can do anything in that path. -additionadir may not be fully controlled by user, it's just another (read-only) path where game can search additional resources. If you as user want override something in game's underlying filesystem, you have two options:

Add override file into additionaldir and pass argument to executable.
Add override file into writable path and just run the game.

In case where override placed in both places priority goes to writable path, because user controls it. In case if such clash undesirable, user always can remove file from writable path. Opposite case (where additional dir is highest priority) may not allows to user remove file (it's read-only for him), so only solution here is remove additionaldir, which is also undesirable.

Jayman2000 · 2024-10-18T17:43:38Z

First of all, current logic of additionaldir implements same way of search path, they get added after of writable path. My PR changes executable and current path priorities, not additionaldir. I stated before that current and executable directory is not always can be writable directory for user, and this PR fixes that issue.

I partially agree with you here, but I also partially disagree. I agree that the current working directory and the executable directory might not by writable by the current user. I also agree that this PR prevents Descent 3 from trying to write to the current working directory, and I agree that that is a good thing. Finally, I agree that the Base_directories variable is stored in this order:

The writable base directory,
the first -additionaldir directory,
the second -additionaldir directory,
the third -additionaldir directory,
…
the last -additionaldir directory.

The code that you linked to clearly shows that the Base_directories variable is stored in that order. That being said, I disagree with this part of what you said: “current logic of additionaldir implements same way of search path”. At the moment, files in -additionaldir directories override files in the writable base directory. That’s because items at the end of the Base_directories list override items at the beginning of the Base_directories list. Take a look at this comment:

/* The "root" directories of the D3 file tree
 *
 * Directories that come later in the list override directories that come
 * earlier in the list. For example, if Base_directories[0] / "d3.hog" exists
 * and Base_directories[1] / "d3.hog" also exists, then the one in
 * Base_directories[1] will get used. The one in Base_directories[0] will be
 * ignored.
 */
std::vector<std::filesystem::path> Base_directories = {};

When I worked on the -additionaldir pull request, I tried to make sure that my code did what that comment said that it did. Specifically, I tried to make sure that the code always iterates over the Base_directories list backwards. Here is a list of places where the code iterates over the Base_directories list:

If you know of any places in the code where I accidentally iterate forwards instead of iterating backwards, then please let me know because that’s a bug with my code and I would like to take responsibility for it.

Also, I wrote this script to test out my theory. When I run that script, it seems to prove that files in -additionaldir directories override files in the writable base directory. Here’s how you can use that script:

Build Descent3’s main branch from source. (At the time of writing, the main branch is at 652e31f).
Copy all of the files required to run Descent 3 (including the executable) into a single directory. I’m going to call this directory the “base directory that will be copied”.
Make sure that you copied all of the required files into the base directory that will be copied. You can do so by running this command:
```
<path-to-base-directory-that-will-be-copied>/Descent3 -useexedir
```

Run the script:

./additionaldir-override-test.sh <path-to-base-directory-that-will-be-copied>

When I run that script, it seems to prove that (in the main branch) files in -additionaldir directories override files in the writable base directory. What happens when you run that script?

Writable path is highest priority, because user controls it and can do anything in that path.

I disagree. I don’t think that the writable base directory should have the highest priority. You are correct that the user controls it and can do anything with it. However, the user also controls what command-line arguments are passed to Descent 3. The user can run Descent3 -additionaldir <path-that-they-dont-control>, or they can run Descent3 -additionaldir <path-that-they-do-control>, or they can just run Descent3 and not use the -additionaldir option at all.

-additionadir may not be fully controlled by user, it's just another (read-only) path where game can search additional resources. If you as user want override something in game's underlying filesystem, you have two options:

Add override file into additionaldir and pass argument to executable.

Add override file into writable path and just run the game.

I completely agree with what you wrote here. It looks like you correctly understand this part of the code that I wrote.

In case where override placed in both places priority goes to writable path, because user controls it. In case if such clash undesirable, user always can remove file from writable path. Opposite case (where additional dir is highest priority) may not allows to user remove file (it's read-only for him), so only solution here is remove additionaldir, which is also undesirable.

That last part sounds vague to me. You said “so only solution here is remove additionaldir, which is also undesirable.” Why is that undesirable?

Personally, I think that having the files in the writable base directory override files in the -additionaldir is counterintuitive. It violates the principle of least astonishment. Let’s say that I’m loading an ECWolf mod. I would load the mod by doing this:

ecwolf --data wl6 --file mod.pk3

If both ~/.config/ecwolf/ and mod.pk3 contain a file named GAMEMAPS.WL6, then the one in mod.pk3 will get used. As another example, let’s say that I’m loading a Doom mod. I would load the mod by doing this:

DOOM2.EXE -file MOD.WAD

If both DOOM2.WAD and MOD.WAD contain a MAP01, then the one in MOD.WAD will get used. I believe that Quake’s -game option works the same way, but I’m not as familiar with Quake. My main concern is that users will be confused and frustrated if we make Descent 3 work opposite to how most other software works.

Additionally, switching the override order makes the -additionaldir argument less useful. f5d2a43 was the commit that added the -additionaldir option. Here’s a quote from its commit message:

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>

Let’s say that a user wants to install the PPICS 2005 mod. At the moment, they can keep things organized by doing this:

Descent3 -additionaldir <path-to-mods-folder>

If this PR was merged, then the user would have to either delete PPICS.HOG from their writable base directory and either replace it with the 2005 one or put the 2005 one in a -additionaldir directory. Now, let’s say that the user changes their mind and wants to uninstall the PPICS 2005 mod. At the moment, uninstalling the mod is easy: just stop using the -additionaldir command-line option. If this PR was merged, then uninstalling the mod would be inconvenient. The user would have to restore a backup of their old PPICS.HOG file. If they don’t have a backup, then they would potentially have to reinstall the game.

winterheart · 2024-10-21T18:01:06Z

Current logic not working as expected. When I launch local build with custom location of d3.hog, I lose default pilot file (which intended to be in writable path, #534):

./Descent3 -nointro -additionaldir "/mnt/windows/SteamLibrary/steamapps/common/Descent 3/"

[mng_InitLocalTables@682] Local dir: /home/winterheart/playground/Descent3/cmake-build-debug/Descent3
[PltGetPilots@1714] Found 0 pilots

Here log from this branch:

./Descent3 -nointro -additionaldir "/mnt/windows/SteamLibrary/steamapps/common/Descent 3/"

[InitIOSystems@1420] Setting writable preference path /home/winterheart/.local/share/Outrage Entertainment/Descent 3
[InitIOSystems@1454] Base directories: [/home/winterheart/.local/share/Outrage Entertainment/Descent 3/, /mnt/windows/SteamLibrary/steamapps/common/Descent 3/, /home/winterheart/playground/Descent3/cmake-build-debug/Descent3/]
[PltGetPilots@1714] Found 1 pilots

Jayman2000 · 2024-10-21T21:31:43Z

Current logic not working as expected. When I launch local build with custom location of d3.hog, I lose default pilot file (which intended to be in writable path, #534):

./Descent3 -nointro -additionaldir "/mnt/windows/SteamLibrary/steamapps/common/Descent 3/"

[mng_InitLocalTables@682] Local dir: /home/winterheart/playground/Descent3/cmake-build-debug/Descent3
[PltGetPilots@1714] Found 0 pilots

Here log from this branch:

./Descent3 -nointro -additionaldir "/mnt/windows/SteamLibrary/steamapps/common/Descent 3/"

[InitIOSystems@1420] Setting writable preference path /home/winterheart/.local/share/Outrage Entertainment/Descent 3
[InitIOSystems@1454] Base directories: [/home/winterheart/.local/share/Outrage Entertainment/Descent 3/, /mnt/windows/SteamLibrary/steamapps/common/Descent 3/, /home/winterheart/playground/Descent3/cmake-build-debug/Descent3/]
[PltGetPilots@1714] Found 1 pilots

Hmm… we should probably open an issue for that because that’s definitely a bug. I would open the issue right now, but I’m not sure how to reproduce the bug. I tried to reproduce the bug by following these steps, but the game was able to find the pilot profile successfully:

If it exists, delete <path-to-Decent3-repo>/builds/linux/Descent3/Debug/

Rebuild the project:

cmake --preset linux
cmake --build --preset linux --config Debug

Change directory into the directory that contains the Descent3 executable:
```
cd builds/linux/Descent3/Debug
```

Start the game for the first time:

./Descent3 -nointro -additionaldir <path-to-proprietary-game-data>

After the game finishes loading, create a new pilot.
Quit the game.

Start the game for the second time:

./Descent3 -nointro -additionaldir <path-to-proprietary-game-data>

After the game finishes loading, make sure that the pilot that you created is on the pilots list.
Quit the game.
Make sure that there’s a .plt file in your current working directory.

Is there anything that I need to change about those steps to reproduce?

winterheart · 2024-10-21T22:35:36Z

Created pilot flie should be in pref path, not in current directory. Again, all writable files (savefiles, pilot files, demos, user content, configuration) should be in writable directory, current path may not be writable.

Jayman2000 · 2024-10-21T23:37:16Z

Created pilot flie should be in pref path, not in current directory. Again, all writable files (savefiles, pilot files, demos, user content, configuration) should be in writable directory, current path may not be writable.

As far as I know, the version of the game that’s in the main branch (496b2ed at the moment) puts all of the files that it creates in either the writable base directory or a temporary directory. If you know of any exceptions to that rule, then please mention them so that we can file a bug report.

The version of the game that’s in the main branch sets the writable base directory to the current working directory. I agree that Descent 3 should not do that. It’s a bad design decision. One of the good things about this PR is that it changes that bad design decision. I agree that we should use the SDL pref path as the writable base directory.

That being said, I still think that my steps to reproduce are correct for the main branch. The steps to reproduce take into account the fact that the main branch contains a bad design decision at the moment.

Lgt2x · 2024-10-27T22:17:27Z

I tested again the changes and logic of this branch, and it does look good enough compared to what we had before.

@Jayman2000 are you ok with moving forward with this MR?

Jayman2000 · 2024-10-27T23:29:07Z

@Jayman2000 are you ok with moving forward with this MR?

Not really. I put a lot of work into getting the -additionaldir PR merged, and I really don’t like the fact that this PR breaks one of the main intended use cases of the -additionaldir option.

If you guys really think that files in the writable base directory should override files in the -additionaldir directories, then we need to come up with a robust reason for why we’re making that change, and we need to put that reason into a commit message. The closest thing that I’ve seen to a robust reason is this comment from @winterheart:

Writable path is highest priority, because user controls it and can do anything in that path. -additionadir may not be fully controlled by user, it's just another (read-only) path where game can search additional resources. If you as user want override something in game's underlying filesystem, you have two options:

Add override file into additionaldir and pass argument to executable.

Add override file into writable path and just run the game.

In case where override placed in both places priority goes to writable path, because user controls it. In case if such clash undesirable, user always can remove file from writable path. Opposite case (where additional dir is highest priority) may not allows to user remove file (it's read-only for him), so only solution here is remove additionaldir, which is also undesirable.

I don’t think that reason is robust at the moment (see this previous comment), but I can help you guys make it more robust if you guys really think that files in the writable base directory should override files in the -additionaldir directories.

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>

halprin · 2024-11-25T23:22:06Z

@Lgt2x, and others, I checked this on macOS, and it works like a charm!

SDL correctly uses ~/Library/Application Support/Outrage Entertainment/Descent 3 for the folder. In fact, I can just double click on Descent3.app and everything runs correctly if I moved over all the data that was in the main folder! E.g. ~/Library/Application Support/Outrage Entertainment/Descent 3/d3.hog or ~/Library/Application Support/Outrage Entertainment/Descent 3/missions, etc.

You may recall in the current 1.5.0 patch, double clicking on the Descent3.app doesn't work because the the current working directory is not set to the main folder where all the data resides normally. One needs to manually run cd /path/to/the/main/D3/folder; ./Descent3.app/Contents/MacOS/Descent3, so this PR is great since one wouldn't need to do the manual process.

winterheart added 9 commits October 6, 2024 17:39

Update cf_LocatePathCaseInsensitiveHelper()

a9c4ef1

Make case-insensitive path work on all path components instead of only filename.

Fix error message in mve_PlayMovie()

a237a53

Fix error message when real_name is empty.

Fix ordering of base directories

8e4dede

First added entries to base directories has higher priority on search path.

Implementing ddio_GetPrefPath() and ddio_GetBasePath()

f2118ae

ddio_GetPrefPath() implements retrieving writable location path. ddio_GetBasePath() implements retrieving parent path of executable.

Cleanup loadgame / savegame functions

c567a84

Convert functions to use std::fs::path, minor cleanups.

Ensure demo directory exists before writing a demo file

36d89ea

Convert LocalCustomGraphicsDir and LocalCustomSoundsDir to use std::f…

1fd8789

…s::path Write custom graphics and sounds to writable base directory.

Ensure that screenshot are saved in writable path

9f6fb05

Now screenshots are saved in "screenshots" directory of writable base path. Fix minor issues.

Lgt2x reviewed Oct 8, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

Lgt2x mentioned this pull request Oct 8, 2024

honor the $XDG_CACHE_HOME env variable #625

Open

13 tasks

Lgt2x mentioned this pull request Nov 25, 2024

Update macOS Usage Documentation #649

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance multiple base directory usage #628

Enhance multiple base directory usage #628

winterheart commented Oct 8, 2024

Lgt2x commented Oct 8, 2024

winterheart commented Oct 8, 2024

Lgt2x commented Oct 8, 2024

Jayman2000 commented Oct 8, 2024

winterheart commented Oct 12, 2024

Jayman2000 commented Oct 13, 2024

winterheart commented Oct 18, 2024

Jayman2000 commented Oct 18, 2024

winterheart commented Oct 18, 2024 •

edited

Loading

Jayman2000 commented Oct 18, 2024

winterheart commented Oct 21, 2024

Jayman2000 commented Oct 21, 2024

winterheart commented Oct 21, 2024

Jayman2000 commented Oct 21, 2024

Lgt2x commented Oct 27, 2024

Jayman2000 commented Oct 27, 2024

halprin commented Nov 25, 2024

Enhance multiple base directory usage #628

Are you sure you want to change the base?

Enhance multiple base directory usage #628

Conversation

winterheart commented Oct 8, 2024

Pull Request Type

Description

Related Issues

Screenshots (if applicable)

Checklist

Additional Comments

Lgt2x commented Oct 8, 2024

winterheart commented Oct 8, 2024

Lgt2x commented Oct 8, 2024

Jayman2000 commented Oct 8, 2024

winterheart commented Oct 12, 2024

Jayman2000 commented Oct 13, 2024

winterheart commented Oct 18, 2024

Jayman2000 commented Oct 18, 2024

winterheart commented Oct 18, 2024 • edited Loading

Jayman2000 commented Oct 18, 2024

winterheart commented Oct 21, 2024

Jayman2000 commented Oct 21, 2024

winterheart commented Oct 21, 2024

Jayman2000 commented Oct 21, 2024

Lgt2x commented Oct 27, 2024

Jayman2000 commented Oct 27, 2024

halprin commented Nov 25, 2024

winterheart commented Oct 18, 2024 •

edited

Loading