Check if the requested game exists (Game change command)

[Diordany]

Context

Closely related to issue #84, which I closed based on input from @Shpoike and @ericwa, who suggested that my patch (proposed in #84) would cause problems due to inconsistency in naming for assets from various Quake releases, which I ended up agreeing with (apologies for not clarifying that before closing the issue).

Patch

I want to propose a different solution that does not use case sensitivity to compare the requested game name with the current one, but rather cancels the command if no matching game directory was found in the first place (briefly mentioned in #84).

I used <filetype> != FS_ENT_DIRECTORY to make sure that it cancels only if the path does not point to an existing directory (also works for symbolic links on Linux).

This patch aims to prevent situations where #84 would become a hindrance in Unix-based environments.

Testing

game <name>

Should load the game if the directory was found by Sys_FileType or print ~The game "<name>" couldn't be found.~ No such game directory "<name>" if not.

Edit: try this for both cases where user dirs are enabled and disabled (build with DO_USERDIRS=1 to enable).

Notes

• I tested this in a Linux environment, and it also works for symbolic links to game directories.
• I've not tested this in a Windows environment.
• I've not tested this in a Mac OS environment (which I believe also uses the syscall stat implementation of Sys_FileType, similar to the Linux build).
• Spike also mentioned a different approach in #84 where the procedure is willing to accept partial matches.

[sezero]

@ericwa , @andrei-drexler : What do you think?

[sezero]

Left a note above until others speak their minds.

Another note: If this is to be accepted, do we not error out during first launch time if a non-existent directory name is given with -game ?

[Diordany]

Another note: If this is to be accepted, do we not error out during first launch time if a non-existent directory name is given with -game ?

That seems to be handled elsewhere.

I tested this and the change in COM_Game_f does not impact the initialization stage. During init, the game is just set to the value of -game.

[sezero]

Another note: If this is to be accepted, do we not error out during first launch time if a non-existent directory name is given with -game ?

That seems to be handled elsewhere.

I tested this and the change in COM_Game_f does not impact the initialization stage. During init, the game is just set to the value of -game.

Yes, I know, but a non-existent gamedir is not created either, and who knows maybe there is a good reason for it: What I'm asking is should we error out in such a case of non-existing gamedir - would that be a good or a bad idea?

[sezero]

Another thing of concern is whether the build supports userdirs, e.g. the engine may be built with DO_USERDIRS=1 (distros do that for e.g.) In that case maybe we need something like the following ?? i.e.: also search gamedir under userdir ???

diff --git a/Quake/common.c b/Quake/common.c
index e53cdf6..d32ae31 100644
--- a/Quake/common.c
+++ b/Quake/common.c
@@ -2164,6 +2164,7 @@ static void COM_Game_f (void)
        const char *p = Cmd_Argv(1);
        const char *p2 = Cmd_Argv(2);
        searchpath_t *search;
+       int exists;

        if (!registered.value) //disable shareware quake
        {
@@ -2188,8 +2189,11 @@ static void COM_Game_f (void)
                return;
            }
        }
-       
-       if (Sys_FileType(va("%s/%s", com_basedir, p)) != FS_ENT_DIRECTORY)
+
+       exists = Sys_FileType(va("%s/%s", com_basedir, p)) == FS_ENT_DIRECTORY;
+       if (!exists && host_parms->userdir != host_parms->basedir)
+            exists = Sys_FileType(va("%s/%s", host_parms->userdir, p)) == FS_ENT_DIRECTORY;
+       if (!exists)
        {
            Con_Printf("The game \"%s\" couldn't be found.\n", p);
            return;

(This seems to have more complications than it sounds...)

[Diordany]

(This seems to have more complications than it sounds...)

Yeah that's right.. I guess this needs a lot more careful consideration than I initially thought.

[Diordany]

I tested your patch @sezero. It works on my Linux machine. And just like before, in this case the va's can also be used directly without requiring the exists var (unless there's another reason why you want to use that):

diff --git a/Quake/common.c b/Quake/common.c
index 5e32afa6..39b38650 100644
--- a/Quake/common.c
+++ b/Quake/common.c
@@ -2189,6 +2189,15 @@ static void COM_Game_f (void)
                        }
                }

+               if (Sys_FileType(va("%s/%s", com_basedir, p)) != FS_ENT_DIRECTORY)
+               {
+                       if (host_parms->userdir != host_parms->basedir && (Sys_FileType(va("%s/%s", host_parms->userdir, p)) != FS_ENT_DIRECTORY))
+                       {
+                               Con_Printf("The game \"%s\" couldn't be found.\n", p);
+                               return;
+                       }
+               }
+
                if (!q_strcasecmp(p, COM_SkipPath(com_gamedir))) //no change
                {
                        if (com_searchpaths->path_id > 1) { //current game not id1

Any comments or objections to that @sezero?

[sezero]

Any comments or objections to that @sezero?

Not yet :)

But I'd like to hear what others have to say to all this

[Diordany]

Not yet :)

But I'd like to hear what others have to say to all this

Alright, I'll keep those patches on separate branches until more is known.

[sezero]

Not yet :)

Well, I seem to have lied: The patch above is broken. It should read like the following instead: (I also tweaked the error message there)

diff --git a/Quake/common.c b/Quake/common.c
index 5e32afa..87f3e48 100644
--- a/Quake/common.c
+++ b/Quake/common.c
@@ -2189,6 +2189,15 @@ static void COM_Game_f (void)
            }
        }

+       if (Sys_FileType(va("%s/%s", com_basedir, p)) != FS_ENT_DIRECTORY)
+       {
+           if (host_parms->userdir == host_parms->basedir || (Sys_FileType(va("%s/%s", host_parms->userdir, p)) != FS_ENT_DIRECTORY))
+           {
+               Con_Printf ("No such game directory \"%s\"\n", p);
+               return;
+           }
+       }
+
        if (!q_strcasecmp(p, COM_SkipPath(com_gamedir))) //no change
        {
            if (com_searchpaths->path_id > 1) { //current game not id1

[Diordany]

Well, I seem to have lied: The patch above is broken. It should read like the following instead: (I also tweaked the error message there)

Right.. the other patch would fail if DO_USERDIRS was not defined. Good catch. Should I update my PR branch, so the others can see the current state of the patch more easily?

[sezero]

Should I update my PR branch, so the others can see the current state of the patch more easily?

Yes

[sezero]

But I'd like to hear what others have to say to all this

Well, no one else seems to be interested in qs dev anymore, so I just went ahead and applied this patch.

Thanks.