Working Game IDs for Elf/Dol files

[cbartondock]

I've added game IDs for ELF and DOL files. This allows the user to set custom game configurations for ELF files and DOL files, as well as to load custom textures at a per elf/dol level. This is extremely useful if you have, for example, multiple mods of Super Smash Bros Brawl. At a technical level, all I'm doing is hashing the file name of the elf and using that as the game ID (thus collisions are possible but exceedingly unlikely). The user may need to do View->Purge Game List Cache before the the new title IDs appear for their old ELFs and DOLS.

For a discussion of reasoning for this pull request see here.

[cbartondock]

Whoopsie - many of the non windows builds seem to be failing because of "error: no type named 'string' in namespace 'std'". Someone with more experience willing to tell me what I am doing wrong?

Edit: A quick google search revealed I am probably just missing #include <string> in Hash.cpp. Will add that and try building for linux myself.

[cbartondock]

Still not passing. I am unable to get my linux development environment in good order at the moment because Qt's servers are down. As soon as I can get qt5.9 installed I should be able to fix this.

[JosJuice]

I think you just need to #include <string> in Hash.h as well.

Please also try to fix the lint errors. The changed indentation makes the changeset rather hard to review.

[cbartondock]

Thanks! And sorry about that - my vim configuration fixes indentation to it's liking upon saving. I will try to fix. Also, if anyone needs qt while their servers are down you can also find it here https://www.mirrorservice.org/sites/download.qt-project.org/official_releases/qt/5.9/5.9.0/

[cbartondock]

Fixed the formatting differences with clang-format -i */*.cpp */*.h. Checked the file differences and none of them are due to indentation changes now.

[cbartondock]

Interestingly this doesn't seem to work when games are launched via "Path\To\Dolphin.exe" --exec "Path\to\gamelauncher.elf", only when they are launched directly from the games list in Dolphin. This is insufficient for my purposes so I will try to fix it.

Edit: Could it possibly be that I have to set m_debugger_game_id in Core\ConfigManager.cpp?

Edit: I found the problem! The issue is that in the case of dolphin launching games from the game list, the executable.path uses forward slashes and hence PathToFileName works correctly, whereas in the case of launching games from the command line the executable.path simply trusts what it is fed and hence has backwards slashes, which break PathToFileName. A possible solution is to just replace all of the backward slashes in executable.path in Core\ConfigManager.cpp, but that would leave PathToFileName still broken when it is handed a windows style path. A better solution is probably to do so within PathToFileName, but this function is used elsewhere hence I am wary of doing so. If someone were willing to vouch that that should be fine I'll do it that way, but until then I'll just fix executable.path.

[JosJuice]

I've mentioned this before in the related forum thread, but I'll mention it again here so that everyone else can see: Personally I think it would make sense to just set the game ID to the file name (including the extension), not a hash of it. It makes it easier for users to know what to name game INIs, it means you can see a meaningful name for the executable in the title bar of the emulation window instead of some random numbers, and it makes it clearer to people that this isn't a real game ID. I'm assuming the reason why you decided to go with a game ID that always is 16 characters is to match how Dolphin currently works for real games, but there's really nothing in Dolphin's code that requires game IDs to be a specific length. Collisions with real game IDs won't be a problem as long as the extension is included.

While I would prefer not using hashes, if everyone else wants to use hashes then I'm fine with going with that.

[lioncash]

Agreed that it would be nicer to have meaningful names for users and also be more straightforward for user INI naming.

[JosJuice]

Also, I would suggest updating the version number here so that users don't have to purge the cache manually:

dolphin/Source/Core/UICommon/GameFileCache.cpp

Line 30 in caff472

static constexpr u32 CACHE_REVISION = 19; // Last changed in PR 9135

[cbartondock]

I guess my issue with the direct approach is that then you get files with two extensions ".elf.ini" and ".elf.txt" which I find ugly/misleading (quite a different situation from .rar.zip where it actually makes sense). This is the kind of thing which to me seems likely to cause problems for someone in the future.

But you guys are the experts - if that's what you'd prefer I can definitely do it.

As for the other suggested changes I'll get right on those.

[JosJuice]

Well, if it wasn't for the problem of conflicts between ELF/DOL "game IDs" and real game IDs, we could skip the extension... Maybe it would be fine to assume that if someone intentionally names their DOL file RSBE01 or something, they know what they're doing? I do agree that extensions like .elf.txt look a little weird (but they don't cause any problems).

[cbartondock]

Another possibility is to add a small hash at the end, to make it readable but also unique. Or even just something like "gameid(filename)". Alternatively, I could just change the logic that displays the game_id in the emulation window and make it display the file name instead for elf's and dol's, and keep the hash. I don't think it's really that much more trouble to go into properties and copy the game id than it is to go to the file and copy its name.

I actually think a really nice feature would be to give the right click menu a "copy game id to clipboard" option, maybe I'll look into that when this is done.

Edit: Actually the window header is a non issue, at least for "launcher" elf files, it just shows the information for the default iso.

[larsenv]

Years ago, for GameTDB we came up with our own game IDs for homebrew, and made covers for them. Here is a full list of them.

The thing is that not all the homebrew are in there, it's not perfect by any means and they would mostly have to be hard-coded to use them.

I just thought I'd mention this, I wouldn't expect these to actually be used by Dolphin because they're a mess.

[JosJuice]

If we exclude the extension, then I suppose anyone who would want to use those game IDs could do so by naming the file after the game ID. Though I don't expect these game IDs to be popular.

[cbartondock]

I went with file_name.elf --> ID(file_name)

[cbartondock]

So that a game ini for SSB Project M.elf would be ID(SSB Project M).ini

[cbartondock]

Let me know if there is anything else I should do.

[cbartondock]

Last one I hope - sorry about that I missed a format change last time.

[cbartondock]

Okay everything is passing again.

[cbartondock]

1. I don't think I should make it a static method of SConfig as if you'll note it is also called from GameFile.cpp.

2. I no longer need string_view as I'm not accessing the length of the string / relying on the string being null terminated. Native C++ strings are used ubiquitously in dolphin elsewhere and I see no reason they shouldn't be used here.

3. The argument for ID(filename) is contained in the previous posts. To sum up, if we just use the file name then it would be possible to have a conflict by naming something RSBE01.elf, for example. If you can think of a prettier way to avoid this, by all means I will change it.

[leoetlino]

1. I don't think I should make it a static method of SConfig as if you'll note it is also called from GameFile.cpp.

That's not a problem, you can call Core code from UICommon (but not vice versa) so you can just call SConfig::MakeGameID.

2. I no longer need string_view as I'm not accessing the length of the string / relying on the string being null terminated. Native C++ strings are used ubiquitously in dolphin elsewhere and I see no reason they shouldn't be used here.

Changing it to std::string_view avoids an unnecessary allocation + copy in most cases. Furthermore, not requiring the string to be null terminated is an argument in favour of using std::string_view rather than std::string :)

You may see std::string used in situations where std::string_view would make more sense; that's because the latter is a pretty new addition to the C++ standard library (C++17) and we haven't fully updated those legacy usages yet.

3. The argument for ID(filename) is contained in the previous posts. To sum up, if we just use the file name then it would be possible to have a conflict by naming something RSBE01.elf, for example. If you can think of a prettier way to avoid this, by all means I will change it.

Oh right, because the extension is removed... Maybe ID- instead? Or FILE-, just to make it clear it's a file name... Just avoid parentheses and spaces because they're annoying to deal with.

[cbartondock]

Okay fair enough! The only issue with string_view is because string is used elsewhere I will probably have to do some conversions at some point.

[cbartondock]

But I will make the changes

[leoetlino]

You can pass a std::string to a function that takes a std::string_view without explicit conversions. Only the opposite operation (getting a std::string out of a string view) requires a manual conversion.

[cbartondock]

So the output of MakeGameID is passed to SetGameRunningMetadata, which I am not going to mess with and which expects a std::string. By using string_view in the make_game_id method all you're doing is an implicit conversion for like two steps and then a manual conversion back immediately after (which if I recall is an expensive type cast). I really don't think it's the right move to use string_view for this one thing when everything else in the ConfigManager.cpp uses string. I'm not up to the job of migrating the whole file (likely all of dolphin) over to string_view, but I think it should probably happen all at once and not piece meal with a bunch of hacked in conversions.

[JosJuice]

We're not saying you should return std::string_view, only that the parameter should be std::string_view. The return type would likely remain std::string even if we migrated the whole file over to using std::string_view where appropriate.

By using string_view in the make_game_id method all you're doing is an implicit conversion for like two steps and then a manual conversion back immediately after (which if I recall is an expensive type cast).

It's no more expensive than what's happening in the current code.

[cbartondock]

Won't I then need to do the conversion inside the function? Or is specifying that it returns a string enough to tell it to do the conversion (which, while no more expensive, I believe is also no less expensive, the double copying is just happening in a different place)

[cbartondock]

Sorry to bug about this. I'm learning as I go here.

[JosJuice]

Won't I then need to do the conversion inside the function?

I believe you have to manually convert to std::string in order to do concatenation, but I'm not entirely sure. You'll find out if you try.

which, while no more expensive, I believe is also no less expensive, the double copying is just happening in a different place

Yes, the gain of using std::string_view here isn't that the function becomes more efficient, it's that callers may be able to avoid creating a copy. (If PathToFileName returned std::string_view instead of std::string, for instance.)

[cbartondock]

I just did find out lol, the plus operator indeed does not work for string view. so I pass in as string view and convert to string literally immediately? I hope you see what I mean that this sounds a little funky, but I'll take your word(s) for it.

[JosJuice]

You should do it after the substr call but before the concatenation. (Getting a substring happens to be one thing std::string_view is really fast at.)

[cbartondock]

Okay! Thanks, that makes more sense.

[leoetlino]

Is this still needed?

[cbartondock]

Pretty sure I use it in line 854 when I do std::replace(executable_path.begin(), executable_path.end(), BACKSLASH, FORWARDSLASH);

See: https://www.cplusplus.com/reference/algorithm/replace/

The replace line is necessary so that the game ids we are using work both when launching directly from dolphin and when using the command line (assuming the naive windows user passes in a path to their elf file which uses backslashes).

[leoetlino]

Then the answer is no, this should be removed from the header since std::replace is only used in the .cpp.

[leoetlino]

Changes LGTM after the extraneous include is removed and after squashing all the commits (with an interactive rebase for example).

[cbartondock]

Done.

[cbartondock]

All checks passing.

[cbartondock]

Any reason not to merge? Changes were approved 10 days ago.

[leoetlino]

I'll merge this later today if there are no more objections.