search

DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.88k stars 251 forks source link

Enhance multiple base directory usage #628

Open winterheart opened 1 month ago

winterheart commented 1 month ago

Pull Request Type

Description

This is further enhancement of #471 feature.

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

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

Additional Comments

Lgt2x commented 1 month ago

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

winterheart commented 1 month ago

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

Lgt2x commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

Am I understanding you correctly?

winterheart commented 1 month ago

Priority of paths is described as follows:

-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 commented 1 month ago

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:

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 commented 1 month ago

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:

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 commented 1 month ago

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

  1. Build Descent3’s main branch from source. (At the time of writing, the main branch is at 652e31f44275195a65241b2469295e079eb3ea07).

  2. 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”.

  3. 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

  4. 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. f5d2a438638c1245d3c88684aaa30e4ac9416e3a 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 commented 1 month ago

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 commented 1 month ago

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:

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

  2. Rebuild the project:

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

  3. Change directory into the directory that contains the Descent3 executable:

    cd builds/linux/Descent3/Debug

  4. Start the game for the first time:

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

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

  6. Quit the game.

  7. Start the game for the second time:

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

  8. After the game finishes loading, make sure that the pilot that you created is on the pilots list.

  9. Quit the game.

  10. 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 commented 1 month ago

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 commented 1 month ago

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 (496b2ed7c94a1fc08c6ed2831fc64abf486c7ab5 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 commented 3 weeks ago

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 commented 3 weeks ago

@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.