Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support game INIs that match all regions #2018

Merged
merged 7 commits into from Feb 21, 2015

Conversation

JosJuice
Copy link
Member

@JosJuice JosJuice commented Feb 6, 2015

There are a lot of game INIs. Too many, in my opinion. Because they're so tiny and many, the space they take up in total on my hard drive is 8.63 MiB, even though the files themselves only are 1.20 MiB in total. Also, when extracting or deleting a newly downloaded dev release, most of the time spent is because of all the game INI files. I thought about some ways to reduce the number of files, like placing all the game INIs in a single ZIP file, or de-duplicating INIs so that only one INI is needed per game instead of one per region. This PR is about the latter. It will reduce the problem, but not get rid of it completely. It should also save time when editing game INIs.

How does it work? Basically, instead of having RSBE01.ini, RSBJ01.ini, RSBK01.ini and RSBP01.ini, just a RSB-01.ini is enough. INIs without region wildcards will work like before, and they will still be needed for INIs that contain things like patches and AR codes. If both a file with a wildcard and a file without a wildcard match the current game, both will be used, and the content of the file without a wildcard will be prioritized if there is a conflict. Note that the code only supports wildcards for the fourth character, not any other characters.
EDIT: I have changed how this works.

This PR doesn't de-duplicate any of the INIs (at least not yet). I could write a little program that detects duplicates and turns them into INIs with wildcards, but I'm wondering how people think the comments on top of the INIs should be handled. Should they contain all game IDs for the game? Should they contain the game ID with a wildcard? Should they contain no game ID at all?

@Tilka
Copy link
Member

Tilka commented Feb 6, 2015

I like this idea. As for comments, I'd go for all game IDs because that way it's more user-friendly to search for them.

return game_ini;
}

void SCoreStartupParameter::LoadGameIni(IniFile* game_ini, std::string path) const

This comment has been minimized.

@JosJuice JosJuice force-pushed the gameini-region-wildcard branch from 00ff108 to b9d430b Compare Feb 6, 2015
@Stevoisiak
Copy link
Contributor

Stevoisiak commented Feb 6, 2015

My god, this seems incredibly useful. Can someone test if starting Dolphin with a large gamelist is any slower/faster with this PR?

@JosJuice
Copy link
Member Author

JosJuice commented Feb 6, 2015

The game list doesn't use this code as far as I know, and it has no reason to.

@Stevoisiak
Copy link
Contributor

Stevoisiak commented Feb 6, 2015

IIRC, the game list uses this to display the emulation state rating, as well as any emulation notes. I'm pretty sure this wouldn't cause any noticeable differences in the startup time, but better safe than sorry.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 6, 2015

Oh right, the emulation rating... I forgot about that. Sounds like something worth testing.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 6, 2015

@Stevoisiak Haven't tested but probably not since the chances of someone having the same games for different game regions is not that high. For example only if you had Twilight Princess for all game regions they would share the same gameini.
@JosJuice The info from the gameinis shows up in the gui, doesn't it? Is it saved in a separate file though in user cache?

@JosJuice
Copy link
Member Author

JosJuice commented Feb 6, 2015

I think I found the emulation state parsing code. It parses the INIs on its own without going through CoreParameter, so I'll need to fix it to support region wildcards.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 6, 2015

Ok, my opinion is that for maintaining seems a bit more complicated, right now i can immediately find the gameini in the folder i want to edit by it's filename. The difference in the filenames is hindering easy access. Personally i would prefer if every gameini was merged in a single txt file for dealing with the increasing number of gameinis.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 6, 2015

How are you accessing the game INIs? If they're sorted alphabetically, the INIs with region wildcards are in the same place as the ones without.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 6, 2015

Alphabetically, but often i just search by typing to find them.

@Stevoisiak
Copy link
Contributor

Stevoisiak commented Feb 6, 2015

The full INI names would be in the header comments, so you should still be able to search the full gameID without any issues.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 6, 2015

Hm and how will the differences be handled between game inis of different regions? Can you upload the result of automatically de - duplicate inis to test? Also what happens with locally saved gameinis, are they also affected by this change?

@Stevoisiak
Copy link
Contributor

Stevoisiak commented Feb 6, 2015

Linktothepast: The idea is that this will only go into effect if it finds a "Generalized" ini region file. All old INI files would continue working as normal.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 6, 2015

Oh so local gameinis would still be saved by the old naming and only if already renamed gameinis exist to the new format they would be read? Ok, now it doesn't sound that bad.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 6, 2015

I haven't written a de-duplicating program yet, so I can't get any results, but what I'm planning to do is to not de-duplicate if there is any difference other than the game ID.

User game INIs will continue working as before. It will be possible to use region wildcards with them, but I don't think people actually are going to do that.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 6, 2015

Ok final question, is what i proposed of using a single txt file for all the database gameinis impossible? Something similar to what happens with pcsx2 where they use only a single database file for all games.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 6, 2015

That's possible, but it would require bigger changes.

{
std::string id_without_region(GetUniqueID());
id_without_region.replace(3, 1, "-");
game_ini->Load(File::GetSysDirectory() + GAMESETTINGS_DIR DIR_SEP + id_without_region + ".ini", true);

This comment has been minimized.

@JosJuice JosJuice force-pushed the gameini-region-wildcard branch 2 times, most recently from 33a2591 to b7a2c73 Compare Feb 7, 2015
@JosJuice
Copy link
Member Author

JosJuice commented Feb 7, 2015

All code that accesses game INIs is now updated, except for some code in watch and breakpoint windows that loads and saves user game INIs, because region wildcards would be more of an obstacle rather than something helpful there.

@JosJuice JosJuice force-pushed the gameini-region-wildcard branch 2 times, most recently from 949177d to 891b9dc Compare Feb 7, 2015
@JosJuice
Copy link
Member Author

JosJuice commented Feb 7, 2015

561 INIs have been merged. That's less than I thought would be possible, but it's still a nice number. 284 INIs couldn't be automatically merged, because they had differences in lines other than the first one. I suspect that some (or many?) of them could be merged manually.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 7, 2015

Hmm, btw will the use of wildcards cover cases where a region was missing before? For example let's say a virtual console game was missing the japanese region gameini before, using a wildcard instead will cover the japanese region too that was missing before from the database, correct? This will make the database more complete with less trouble.

Oh and what happens with the few cases that are the same game but have different the two last numbers in the gameini between regions? Example:
SVVEG9 = The Croods: Prehistoric Party!
SVVPAF = The Croods: Prehistoric Party!

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 7, 2015

Anyway, if this is ready for merge, the sooner it is merged the better, to check manually for those not automatically merged cases and alter some ini changes i made accordingly to this pr. Otherwise there will be merge conflicts. Any gameini prs not merged by others should also be merged before this pr.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 7, 2015

Yes, wildcards will cover regions that were missing before. Adding the additional game IDs to the comment at the top would be nice for the sake of documentation, but the only step that is required technically is to replace the fourth character with a -. In case there is interest in it, I could add wildcards to all INIs that only have one region right now. It would cause some false positives, but it would also give us better coverage without having to do anything manually.

If the last two characters are different, it isn't possible to cover both games with a single INI. Should I change that? It would just be 3-5 extra lines of code or so.

JosJuice added 6 commits Feb 11, 2015
A side effect of this is that user INIs now can specify revisions.
I don't think anyone will use it, but there's no reason to not allow it.
It contains no useful information,
and it causes problems in my de-duplication program.
761 INIs were merged. 319 INIs couldn't be merged because of differences.
@JosJuice JosJuice force-pushed the gameini-region-wildcard branch from faee23c to b85a4ce Compare Feb 11, 2015
@CarlKenner
Copy link
Contributor

CarlKenner commented Feb 12, 2015

I like this idea, even though I use region-specific and revision-specific ini files for a few things. This is especially good for Japanese and Korean users because most Japanese and Korean games don't have INI files.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 12, 2015

If everything is ok please merge so that further work in ini files can be done.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 16, 2015

There are a lot of gameini changes that need to be done and i can't work with them if this isn't merged, it will cause a lot of unnecessary work for me plus i will have to redo a pr after this will be merged to fine-tune the ini files. Who do i need to annoy about it? @skidau , sorry for the annoyance but please merge if this pr is ok.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 20, 2015

Final appeal, @delroth there are a lot of gameinis that need to be updated due to the changes that are made recently, if this pr seems fine and it is going to be merged please do, otherwise if this change is dismissed it would still be good to know to plan the changes i will make accordingly.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

No answers, i take it this will not be merged then and i will change inis according to that.

@degasus
Copy link
Member

degasus commented Feb 21, 2015

@Linktothepast or more likely that nobody was motivated enough to look over this patch.... As our ini maintainer: Do you like this PR?

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

Yes, it covers cases of regions that were previously missing from the database with no extra work involved, and you can still use region specific gameinis if necessary. Which is a win - win situation. No extra work needed for missing gameinis and things will become more tidy in the database. http://www.gametdb.com/ that i use is not entirely complete database i think regarding gameregions, there were times in the past that new game specific regions were added afterwards, and those cases will also be covered by this pr.

degasus added a commit that referenced this issue Feb 21, 2015
Support game INIs that match all regions
@degasus degasus merged commit 95c48b2 into dolphin-emu:master Feb 21, 2015
10 checks passed
@JosJuice JosJuice deleted the gameini-region-wildcard branch Feb 21, 2015
@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

Nice, thanks!

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

Jocjuice, it seems you didn't delete the unnecessary inis though, can you upload the ones in your database to use as a base for the changes i will make, thanks. It currently has 2787 inis compared to 2323 that the previous build has.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 21, 2015

I'm still pretty sure I deleted them... Anyway, I'm away over the weekend with only my phone, so I can't send any files until Monday.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

Damn, i will try to do it manually then. Anyway i will still need to manually check them all to finetune them to the new system, in case some weren't merged for a minor difference, etc.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

Found another small issue, with the unnecessary ini files removed, while things work fine, you can't open the general ini in the dolphin gui. It works but it doesn't show up to check things, you have to search for the ini file manually in the folder for details of it's contents. For most people this doesn't matter but it is a bit of a pain to me.

@degasus
Copy link
Member

degasus commented Feb 21, 2015

@Linktothepast Which one should we open on this button? I mean it's easy to just open the other path, but it will be hard to decide what to do if there different files...

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

If a file with the complete gameini id exists it should always open, if one with just the 3 first letters exists without any complete gameid ini overriding it, it should open in it's place instead, which doesn't happen now.

@JosJuice
Copy link
Member Author

JosJuice commented Feb 21, 2015

@Linktothepast: How are you downloading the INIs? If it only is a problem with the buildbot, GitHub's "Download ZIP" option and regular git fetches/pulls should work.

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 21, 2015

Thanks JosJuice, it is definitely a buildbot issue. Did the above and the inis are correct.

@Stevoisiak
Copy link
Contributor

Stevoisiak commented Feb 22, 2015

@Linktothepast: I want to help work on converting our INI files to the new system, but I'd rather not cause any merge conflicts with your work.

@escape209
Copy link
Contributor

escape209 commented Feb 22, 2015

While this isn't *exactly *the same:

GB_0_E51 - Burnout (1)
GB_4_E51 - Burnout 2: Point of Impact

On Sun, Feb 22, 2015 at 7:44 PM, Steven V. notifications@github.com wrote:

@Linktothepast https://github.com/Linktothepast: I want to help work on
converting our INI files to the new system, but I'd rather not cause any
merge conflicts with your work.


Reply to this email directly or view it on GitHub
#2018 (comment).

@Linktothepast
Copy link
Contributor

Linktothepast commented Feb 22, 2015

Stevoisiak thanks for the offer, i plan to do it on steps and post a gameini pr when some important prs get merged, you can take it from there if you wish.
Adamd.. the fourth to sixth letter are common and not the third

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet