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

1280x720 resolution is no-longer "recommended" in settings window #9164

Closed
Asinin3 opened this issue Oct 29, 2020 · 3 comments · Fixed by #9176
Closed

1280x720 resolution is no-longer "recommended" in settings window #9164

Asinin3 opened this issue Oct 29, 2020 · 3 comments · Fixed by #9176
Assignees

Comments

@Asinin3
Copy link
Contributor

Asinin3 commented Oct 29, 2020

After #8037 merged, the default 1280x720 resolution no-longer says (recommended).
image
This is one of the reasons we keep seeing more people set it to 1920x1080 lately, as they don't realize you're not really meant to change the setting for 95% of games.

@Ds886
Copy link

Ds886 commented Oct 30, 2020

The main issue seems to come from rpcs3/rpcs3qt/settings_dialog.cpp:345

if (game && game->resolution > 0)
	{
		const std::map<u32, std::string> resolutions
		{
			{ 1 << 0, fmt::format("%s", video_resolution::_480) },
			{ 1 << 1, fmt::format("%s", video_resolution::_576) },
			{ 1 << 2, fmt::format("%s", video_resolution::_720) },
			{ 1 << 3, fmt::format("%s", video_resolution::_1080) },
			// { 1 << 4, fmt::format("%s", video_resolution::_480p_16:9) },
			// { 1 << 5, fmt::format("%s", video_resolution::_576p_16:9) },
		};

		for (int i = ui->resBox->count() - 1; i >= 0; i--)
		{
			bool has_resolution = false;
			for (const auto& res : resolutions)
			{
				if ((game->resolution & res.first) && res.second == sstr(ui->resBox->itemText(i)))
				{
					has_resolution = true;
					break;
				}
			}
			if (!has_resolution)
			{
				ui->resBox->removeItem(i);
				if (i == saved_index)
				{
					saved_index_removed = true;
				}
			}
		}
	}
	const int res_index = ui->resBox->findData("1280x720");
	if (res_index >= 0)
	{
		// Rename the default resolution for users
		ui->resBox->setItemText(res_index, tr("1280x720 (Recommended)", "Resolution"));

		// Set the current selection to the default if the original setting wasn't valid
		if (saved_index_removed)
		{
			ui->resBox->setCurrentIndex(res_index);
		}
	}

I've checked and this

const int res_index = ui->resBox->findData("1280x720");
	if (res_index >= 0)
	{
		// Rename the default resolution for users
		ui->resBox->setItemText(res_index, tr("1280x720 (Recommended)", "Resolution"));

		// Set the current selection to the default if the original setting wasn't valid
		if (saved_index_removed)
		{
			ui->resBox->setCurrentIndex(res_index);
		}
	}

seems to be implemented as the settings file are synced based on the value "1280x720" changing the values in /rpcs3/Emu/system_config_types.cpp:48 making it that the config is unreadable but the value change in the settings menu.

so after loading the settings a function change the display of the value.

I am having a bit of hard time to debug but checking.

edit: it skip over the if statement.

@Ds886
Copy link

Ds886 commented Oct 30, 2020

could be more general solution by tying it to the fmt::format("%s", video_resolution::_720) in case the text would be changed in the future
https://github.com/Ds886/rpcs3/commit/699d0db98e83390eb5f6e99b96340ab2f4d77174

@Megamouse
Copy link
Contributor

that's highly unlikely, because these are config strings and should ideally never change.
I was also thinking about using the fmt, but then I got distracted and it shouldn't be a problem anyway.

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

Successfully merging a pull request may close this issue.

3 participants