Alternate Steam Libraries #872

scfinniss · 2017-02-27T00:18:39Z

I noticed while working on the Watcom C stuff that Chocolate Doom was not locating my IWADs.

What I've done here is scrape the alternative library locations from Steam's libraryfolders.vdf file and use them to add more search paths. The file itself is mostly key value pairs with a header. Here is the file from my setup for reference.

"LibraryFolders"
{
	"TimeNextStatsReport"		"1488133041"
	"ContentStatsID"		"198096677914456296"
	"1"		"D:\\Steam"
	"2"		"E:\\Steam"
	"3"		"C:\\Steam"
}

I specifically ignore LibraryFolders, thereafter I assume every second string is a path, as long as the preceding key is numeric. I make no real effort to understand the structure of the file beyond this.

It's important to note that the unlisted C:\Program Files (x86)\Steam is also a library. Presumably Steam always considers its install path a library.

I believe this may resolve issue #482 (excluding Mac and Linux support).

fragglet · 2017-02-27T03:24:42Z

src/d_iwad.c

+
+static void CheckSteamLibrary(long library_file_size, char *library_file_buffer)
+{
+    long i, start;


Please don't use type long, since it means very different things in different compilers. Please remove all uses of long.

fragglet · 2017-02-27T03:24:53Z

src/d_iwad.c

+{
+    long i, start;
+    boolean key, key_valid, value;
+    char* buffer;


char *buffer;

fragglet · 2017-02-27T03:25:42Z

src/d_iwad.c

+        else if (start > -1 && key)
+        {
+            //Keys for libraries are always numeric
+            if (library_file_buffer[i] < '0' || library_file_buffer[i] > '9')


Please use isdigit().

fragglet · 2017-02-27T03:26:38Z

src/d_iwad.c

+
+    if (probe != NULL)
+    {
+        library_file = fopen(probe, "rb");


Please factor this whole block out into a separate function.

fragglet · 2017-02-27T03:31:18Z

src/d_iwad.c

+
+    for (i=0;i<library_file_size; ++i)
+    {
+        if (library_file_buffer[i] == '"')


The logic in this function is very complicated and hard to follow. It looks like you're trying to do tokenization and higher-level logic in a single function. I think you can probably simplify this greatly by splitting out these into separate concerns.

scfinniss · 2017-02-27T23:44:16Z

The main change here is restructuring based on the feedback.
Changes in CheckSteamEdition have been mostly removed, replaced with one call to CheckSteamLibraries.

Then CheckSteamLibrary was replaced with

GetSteamNextLibPath
IsValidSteamLibKey
GetNextSteamLibToken
This hopefully makes the logic easier to follow.

Instances of long replaced with size_t, hopefully I've not introduced any style issues this time.

scfinniss · 2017-02-28T15:17:30Z

I'm going to use the reference from Valve to make some changes to how I parse the libraryfolders.vdf.

Is it worth moving this logic to a new file? (eg m_steam.h, m_steam.c)

fragglet

Sorry that I haven't responded to this sooner.

fragglet · 2017-08-16T20:06:23Z

src/d_iwad.c

+
+    if (library_file_size != count)
+    {
+        return NULL;


free(library_file_buffer);

fragglet · 2017-08-16T20:10:02Z

src/d_iwad.c

+        if (buffer[i] == '"')
+        {
+            //first " is the start of the token
+            if (token == NULL)


It becomes simpler if you just do two loops: one to find the starting ", another to find the ending ".

Thanks, this has helped.

fragglet · 2017-08-16T20:12:00Z

src/d_iwad.c

+static char *steam_libraries[MAX_STEAM_LIBRARIES];
+static int num_steam_libraries = 0;
+
+static void AddSteamLibraryDir(char *dir)


Do you need the steam_libraries[] array? Can't you just make this a wrapper around AddIWADDir?

Yes, that array is not needed.

fragglet · 2017-08-16T20:14:02Z

src/d_iwad.c

+            return NULL;
+        }
+
+        //file always starts with this line before key/value pairs


So in that case, just put a call before the start of the loop in CheckSteamLibraries() that calls GetNextSteamLibToken() and checks it's equal to "LibraryFolders". Then this function should become much simpler.

I've changed how I parse the file. I treat the whole VDF file as key value pairs, and I don't bother checking that it starts with "LibraryFolders". The code is a lot simpler now.

scfinniss · 2017-09-16T15:48:22Z

I've addressed the reviews, and also made the tokenizer understand VDF more completely.

The tokenizer now handles tokens that are not always quoted, as well as ones with escaped characters.

To make the parsing simpler I've avoided the issue of subkeys completely. I ignore closing braces and treat the opening ones as part of a regular unquoted token.

So

"LibraryFolders"
{
	"TimeNextStatsReport"		"1505818818"
	"ContentStatsID"		"198096677914456296"
	"1"		"D:\\Steam"
	"2"		"E:\\Steam"
	"3"		"C:\\Steam"
}

Get tokenized to

"LibraryFolders"
{
"TimeNextStatsReport"
"1505818818"
"ContentStatsID"
"198096677914456296"
"1"
"D:\\Steam"
"2"
"E:\\Steam"
"3"
"C:\\Steam"

Every odd token is a key, every even token is a value.

fragglet

Hi,

I hope my feedback here isn't too disheartening, since I have a bunch of comments. I want you to understand that I hold the Chocolate Doom codebase to a very high standard, and I want to work with you to get this into a satisfactory state, because I do really want to get this feature merged.

Every odd token is a key, every even token is a value.

This seems like a strange approach to me, and not how one would usually structure a parser. Doing this essentially ignores the syntactic structure of the file, which is a recipe for fragile code that will potentially break in unexpected ways. For example, if the parser were to read something that begins "ArgleBargle" "ASDFASDF" instead of "LibraryFolders" { you'd expect parsing fail, but in the current approach it is silently accepted. Similarly the LibraryFolders block is supposed to end in a } but the current parser doesn't care if it ends in one, none, or a hundred of them.

If you'd like some philosophical background I'd suggest reading the chapter Programming by Coincidence from the book The Pragmatic Programmer, which is a great read in general.

fragglet · 2017-09-16T23:15:23Z

src/d_iwad.c

+//Read whole of libraryfolders.vdf as string
+//silently fail is something goes wrong reading the file
+
+static char *GetSteamLibText(char *library_path)


Can you just use M_ReadFile()?

Yes, I've got it working with M_ReadFile().

The first difference I see between M_ReadFile() and my code is that I'm adding the \0 to the end of the buffer. I assume I did that to ensure strlen() is guaranteed to work later. Not sure if that was ever necessary.

The other difference is that I'm using malloc(), where M_ReadFile() uses Z_Malloc().

There are a few places where malloc() is used over Z_Malloc(), are these intentional or should Z_Malloc() always be used?

fragglet · 2017-09-16T23:19:36Z

src/d_iwad.c

+        {
+            if (isspace(c) || c == '}')
+            {
+                success = true;


Just:

return NULL;

Then you can eliminate the success variable and the if (success) check below.

fragglet · 2017-09-16T23:22:00Z

src/d_iwad.c

+    {
+        c = buffer[i];
+
+        //{ and } are control characters in VDF used to indicate subkeys.


A tokenizer with baked-in knowledge of syntax is probably a bad design. The usual approach would be to define an enum representing the different token types, then let the higher level parsing take care of things.

fragglet · 2017-09-16T23:22:23Z

src/d_iwad.c

+            continue;
+        }
+
+        quote = c == '"';


This is essentially a loop to skip past leading whitespace. Why is this part of the loop body?

fragglet · 2017-09-17T00:04:06Z

src/d_iwad.c

+        break;
+    }
+
+    for (; i<buff_size; ++i)


This ends up being complicated because you're trying to process different types of things using the same loop. You'd be better off just treating them separately. For example:

c = ... whatever if (c == '"') { return ReadStringToken(.....); } else if (c == '{') { return TOKEN_OPEN_BRACE; } else if (c == '}') { return TOKEN_CLOSE_BRACE; } else if (isalpha(c)) { return ReadKeyword(......); } else { return TOKEN_UNKNOWN; }

scfinniss · 2017-09-17T00:53:42Z

Thank you

I'm not disheartened, I appreciate the feedback and the time you've spent on it. I'm quite happy to improve this code so it better fits the Chocolate Doom codebase.

mfrancis95 · 2020-06-03T22:56:57Z

@scfinniss any updates?

Otherwise I don't think it's good to keep a nearly 3 year-old PR up.

scfinniss · 2020-06-04T01:04:32Z

Real life got between me and finishing this, and I guess I forgot about it after awhile.

I'm not sure how much Chocolate Doom has changed since I worked on this, but I don't mind looking into it again if there's still interest in this feature?

mfrancis95 · 2020-06-04T13:39:10Z

There have always been efforts to improve WAD-locating, even in recent times. I would say it's still worth it, but it just happens to be a fairly sizable undertaking.

Alternate Steam Libraries

cc747bb

fragglet requested changes Feb 27, 2017

View reviewed changes

Alternate Steam Libraries

89daf9b

fragglet reviewed Aug 16, 2017

View reviewed changes

Alternate Steam Libraries

df3ae92

fragglet requested changes Sep 17, 2017

View reviewed changes

turol mentioned this pull request Feb 3, 2018

Read whole music substition config file at once #986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternate Steam Libraries #872

Alternate Steam Libraries #872

scfinniss commented Feb 27, 2017

fragglet Feb 27, 2017

fragglet Feb 27, 2017

fragglet Feb 27, 2017

fragglet Feb 27, 2017

fragglet Feb 27, 2017

scfinniss commented Feb 27, 2017

scfinniss commented Feb 28, 2017

fragglet left a comment

fragglet Aug 16, 2017

fragglet Aug 16, 2017

scfinniss Sep 16, 2017

fragglet Aug 16, 2017

scfinniss Sep 16, 2017 •

edited

fragglet Aug 16, 2017

scfinniss Sep 16, 2017

scfinniss commented Sep 16, 2017

fragglet left a comment

fragglet Sep 16, 2017

scfinniss Sep 17, 2017

fragglet Sep 16, 2017

fragglet Sep 16, 2017

fragglet Sep 16, 2017

fragglet Sep 17, 2017

scfinniss commented Sep 17, 2017

mfrancis95 commented Jun 3, 2020

scfinniss commented Jun 4, 2020

mfrancis95 commented Jun 4, 2020 •

edited

Alternate Steam Libraries #872

Are you sure you want to change the base?

Alternate Steam Libraries #872

Conversation

scfinniss commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scfinniss commented Feb 27, 2017

scfinniss commented Feb 28, 2017

fragglet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scfinniss Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scfinniss commented Sep 16, 2017

fragglet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scfinniss commented Sep 17, 2017

mfrancis95 commented Jun 3, 2020

scfinniss commented Jun 4, 2020

mfrancis95 commented Jun 4, 2020 • edited

scfinniss Sep 16, 2017 •

edited

mfrancis95 commented Jun 4, 2020 •

edited