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

DiscIO: Improve RegionSwitch/CountrySwitch #7471

Merged
merged 7 commits into from Oct 12, 2018

Conversation

3 participants
@JosJuice
Contributor

JosJuice commented Oct 8, 2018

This is more of a code cleanup than a PR that's intended to change the behavior of Dolphin, although it does fix the minor problem of a warning being logged when playing a game with the 'V' country code. (There are 5 such games.)

JosJuice added some commits Oct 5, 2018

DiscIO: Fix the 'W', 'X', 'Y' and 'Z' country codes
These country codes have the unfortunate property that they are used
by Wii disc games in two different regions. We already correct for this
in VolumeGC::GetCountry and VolumeWii::GetCountry, so this commit
shouldn't really have any effect on how the game list behaves,
but it will be useful if we in the future would want to call
CountrySwitch directly without having extra code in the caller for
handling region weirdness.
@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Oct 8, 2018

Contributor

By the way, this opens up for the possibility of adding Sweden as a country. There are three Swedish GameCube releases that use M as the country code, but that country code is also used for PAL VC releases that aren't Swedish whatsoever, which would make it hard to add Sweden as a country without this PR's changes.

@MayImilae Could you create a Swedish flag to match the other flags? If so, I can make a separate PR to add that later.

Contributor

JosJuice commented Oct 8, 2018

By the way, this opens up for the possibility of adding Sweden as a country. There are three Swedish GameCube releases that use M as the country code, but that country code is also used for PAL VC releases that aren't Swedish whatsoever, which would make it hard to add Sweden as a country without this PR's changes.

@MayImilae Could you create a Swedish flag to match the other flags? If so, I can make a separate PR to add that later.

@BhaaLseN

LGTM in general, even though the individual commits probably don't compile as-is (at least the first one that adds a new parameter to RegionSwitch but leaves a bunch of callers until the next commit with one parameter; it just isn't worth going back and fixing those up just for the benefit of bisecting between them with the nature of changes there)

// E is normally used for America, but it's also used for English-language Korean GC releases.
// K is used by games that are in the Korean language.
// W means Taiwan for Wii games, but on the GC, it's used for English-language Korean releases.
// (There doesn't seem to be any pattern to which of E and W is used for Korean GC releases.)

This comment was marked as resolved.

@BhaaLseN

BhaaLseN Oct 8, 2018

Member

Should (parts of) this comment be moved to CountryCodeToRegion/CountryCodeToCountry? I could imagine that at least the expected_region ones would benefit from some distinction (since it isn't necessarily obvious from just the code).

@BhaaLseN

BhaaLseN Oct 8, 2018

Member

Should (parts of) this comment be moved to CountryCodeToRegion/CountryCodeToCountry? I could imagine that at least the expected_region ones would benefit from some distinction (since it isn't necessarily obvious from just the code).

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Oct 8, 2018

Contributor

I've added some more comments. And the first commit does compile – note that it only modifies RegionSwitch, not CountrySwitch.

Contributor

JosJuice commented Oct 8, 2018

I've added some more comments. And the first commit does compile – note that it only modifies RegionSwitch, not CountrySwitch.

JosJuice added some commits Oct 8, 2018

@lioncash lioncash merged commit b3cd615 into dolphin-emu:master Oct 12, 2018

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@JosJuice JosJuice deleted the JosJuice:country-region-switch branch Oct 12, 2018

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