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

Fallback Region Option #9254

Merged
merged 1 commit into from Nov 28, 2020

Conversation

flagrama
Copy link
Contributor

A user-selected fallback to use instead of the default PAL

This can be used for unknown region or region free titles to give them the ability to force a different region than the current fallback region of PAL. This can be useful if a user is trying to play a region free title that is originally NTSC and expects to be run at NTSC speeds. This may be done when a user attempts to dump a WAD of their own without understanding the settings they have chosen, or could be an intentional decision by a developer of a ROM hack that can be injected into a Virtual Console WAD.

This is primarily a proof-of-concept in its current stage.

@JosJuice
Copy link
Member

If we're going to have a setting for this, the setting should be checked before checking the region of the system menu, as the region of the system menu is rather annoying to change.

It would be simpler if we used the Region enum as the type of the setting instead of a string.

I personally don't think it makes sense to have both "override" and "fallback" in the name. It's either an override or a fallback, not both. The way it's implemented, I would not call it an override, as it has no effect for games with a known region.

@flagrama
Copy link
Contributor Author

If we're going to have a setting for this, the setting should be checked before checking the region of the system menu, as the region of the system menu is rather annoying to change.

I can certainly do that.

It would be simpler if we used the Region enum as the type of the setting instead of a string.

I tried that at first, didn't work immediately, went with strings for the POC. I definitely plan to look into this more.

I personally don't think it makes sense to have both "override" and "fallback" in the name. It's either an override or a fallback, not both. The way it's implemented, I would not call it an override, as it has no effect for games with a known region.

That's fair I suppose. I named it as such because it overrides the default fallback of forcing PAL if no other method exists to find one. (not identifiable from the running title, no System Menu installed)

I'll try to get some of this done soon.

@JosJuice
Copy link
Member

Hm, maybe "fallback override" does actually make sense if it overrides the system menu check... I would appreciate if someone else could weigh in on this.

@flagrama
Copy link
Contributor Author

flagrama commented Nov 17, 2020

The fallback override should now take precedence over the current installed Wii System Menu. It also uses the DiscIO::Region enum instead of strings. Converting between the Combo Box selection and the region still uses separate index values because I personally find Disabled at the top of the list much more user-friendly than having it fourth in the list, not even as the last one, that using the Region enum directly would cause.

@flagrama flagrama marked this pull request as ready for review November 24, 2020 02:24
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe "fallback override" does actually make sense if it overrides the system menu check... I would appreciate if someone else could weigh in on this.

I agree with this reasoning. However, I'm not sure most users know what fallback it's overriding, or even that the system menu region is used as a fallback... Removing the "override" wouldn't be a good solution because it is technically an override and you can disable the override, so I think what we should do is add an explanation in the UI.

@@ -246,6 +246,7 @@ void SConfig::SaveCoreSettings(IniFile& ini)
core->Set("PerfMapDir", m_perfDir);
core->Set("EnableCustomRTC", bEnableCustomRTC);
core->Set("CustomRTCValue", m_customRTCValue);
core->Set("FallbackRegionOverride", m_fallback_region_override);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
core->Set("FallbackRegionOverride", m_fallback_region_override);

@@ -514,6 +515,7 @@ void SConfig::LoadCoreSettings(IniFile& ini)
core->Get("EnableCustomRTC", &bEnableCustomRTC, false);
// Default to seconds between 1.1.1970 and 1.1.2000
core->Get("CustomRTCValue", &m_customRTCValue, 946684800);
core->Get("FallbackRegionOverride", &m_fallback_region_override, DiscIO::Region::Unknown);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
core->Get("FallbackRegionOverride", &m_fallback_region_override, DiscIO::Region::Unknown);

@@ -308,6 +308,9 @@ struct SConfig
std::string m_auto_update_track;
std::string m_auto_update_hash_override;

// Fallback Region Override setting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three lines should be removed: instead, you should add it to the list in Core/ConfigLoaders/IsSettingSaveable.cpp (and then use Config::SetBase / Config::Get to interact with the setting)

if (region == GetFallbackRegionOverride())
return;

SConfig::GetInstance().m_fallback_region_override = region;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use Config::SetBase (as mentioned above)


DiscIO::Region Settings::GetFallbackRegionOverride() const
{
return SConfig::GetInstance().m_fallback_region_override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config::Get

@@ -929,6 +931,13 @@ bool SConfig::SetPathsAndGameMetadata(const BootParameters& boot)

DiscIO::Region SConfig::GetFallbackRegion()
{
// Fall back to the override option if set
const DiscIO::Region fallback = SConfig::GetInstance().m_fallback_region_override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this one, but this should use Config::Get as well, of course.

@flagrama
Copy link
Contributor Author

[...] so I think what we should do is add an explanation in the UI.

How does this work for an explanation as a QLabel under the form row?

Overrides the fallback region for titles that do not have one.

Titles without a region that can be determined automatically will fallback to this option. If this option is disabled, instead the region will fallback to the installed Wii System Menu region, or if one cannot be found, the PAL region.

I'm certainly not a wordsmith, so am up for changing it any way it is needed.

@leoetlino
Copy link
Member

I'd remove the first sentence as vertical space is at a premium in the setting windows:

Dolphin will use this for titles whose region cannot be determined automatically. If this option is disabled, the region of the installed Wii System Menu will be used, or if one cannot be found, the PAL region.

@flagrama
Copy link
Contributor Author

I've added that explanation text and it appears like so:
image

@flagrama
Copy link
Contributor Author

Appearance after latest commit:
image

@flagrama flagrama changed the title Fallback Region Override Option Fallback Region Option Nov 28, 2020
Comment on lines 7 to 9
#include <DiscIO/Enums.h>
#include <string>

#include "Common/Config/Config.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <DiscIO/Enums.h>
#include <string>
#include "Common/Config/Config.h"
#include <string>
#include "Common/Config/Config.h"
#include "DiscIO/Enums.h"

@@ -11,6 +11,7 @@
#include <sstream>
#include <variant>

#include <Core/Config/MainSettings.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. (Place the include below #include "Common/Config/Config.h".)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, #include "name" for Dolphin headers, not #include <name>.

Fallback Region
A user-selected fallback to use instead of the default PAL

This is used for unknown region or region free titles to give them
the ability to force region to use. This replaces the current fallback region
of PAL. This can be useful if a user is trying to play a region free
tilte that is originally NTSC and expects to be run at NTSC speeds. This
may be done when a user attempts to dump a WAD of their own without
understanding the settings they have chosen, or could be an intentional
decision by a developer of a ROM hack that can be injected into a
Virtual Console WAD.

Remove using System Menu region being checked in GetFallbackRegion

Use DiscIO::Region instead of std::String for fallback

Add explanation text for Fallback Region
@leoetlino leoetlino merged commit 361bf25 into dolphin-emu:master Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants