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

Qt: Fix HiDPI icon scaling #5463

Merged
merged 1 commit into from May 30, 2017

Conversation

@spycrab
Copy link
Contributor

commented May 21, 2017

No description provided.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch from 15d5194 to ef17aff May 21, 2017

@@ -93,7 +93,10 @@ void ControllersWindow::CreateGamecubeLayout()
m_gc_box = new QGroupBox();
m_gc_layout = new QFormLayout();
m_gc_label = new QLabel();
m_gc_label->setPixmap(QPixmap(m_gamecube_icon));

auto pixmap = QPixmap(m_gamecube_icon).scaled(QSize(64, 64));

This comment has been minimized.

Copy link
@JosJuice

JosJuice May 21, 2017

Contributor

Just scaling the image is doing to look ugly. You need to load the @2x version (or higher) when the user is using a high DPI.

This comment has been minimized.

Copy link
@spycrab

spycrab May 21, 2017

Author Contributor

Now happens automatically.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch 12 times, most recently from e2c4490 to 21c9891 May 21, 2017

@spycrab spycrab changed the title [WIP] Qt: Fix HiDPI icon scaling Qt: Fix HiDPI icon scaling May 21, 2017

@spycrab spycrab changed the title Qt: Fix HiDPI icon scaling [WIP] Qt: Fix HiDPI icon scaling May 21, 2017

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch 3 times, most recently from 1919a22 to 7ea4b1c May 21, 2017

@spycrab spycrab changed the title [WIP] Qt: Fix HiDPI icon scaling Qt: Fix HiDPI icon scaling May 22, 2017

@spycrab

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2017

Ready to be reviewed & merged now.

@JosJuice
Copy link
Contributor

left a comment

Unless I've missed something, the game list is still using low-res images on high-DPI screens.

m_wii_icon(Settings().GetResourcesDir().append(QStringLiteral("Platform_Wii.png")))
m_gamecube_icon(
Settings().GetResourcesDir().append(QStringLiteral("Platform_Gamecube@2x.png"))),
m_wii_icon(Settings().GetResourcesDir().append(QStringLiteral("Platform_Wii@2x.png")))

This comment has been minimized.

Copy link
@JosJuice

JosJuice May 22, 2017

Contributor

Aren't we supposed to use the low-res versions of images when using low DPI?

This comment has been minimized.

Copy link
@spycrab

spycrab May 22, 2017

Author Contributor

The icon is scaled in such a way that at almost all scales it's appropriate to use a @2x version

This comment has been minimized.

Copy link
@JosJuice

JosJuice May 22, 2017

Contributor

Then you should use the @4x version on high-DPI screens.

std::string number = QString::number(num, 'f', 1).toStdString();

if (num == static_cast<int>(num))
number = number.substr(0, number.length() - 2);

This comment has been minimized.

Copy link
@JosJuice

JosJuice May 22, 2017

Contributor

How is this related to the PR?

This comment has been minimized.

Copy link
@spycrab

spycrab May 22, 2017

Author Contributor

If not checked, it'll break the size column on high DPI screens.

This comment has been minimized.

Copy link
@ligfx

ligfx May 26, 2017

Contributor

Can you explain this more?

@Starsam80

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

Toolbar icons are still not scaled, and the icons seem a bit fuzzy
snip

Here is WX for a comparison
snip

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch 6 times, most recently from 7a445e7 to 8b3216f May 22, 2017

@Starsam80

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

LGTM:
image

break;
case GameListModel::COL_BANNER:
DrawPixmap(painter, option.rect,
data.value<QPixmap>().scaled(NORMAL_BANNER_SIZE, Qt::KeepAspectRatio,
Qt::SmoothTransformation));
data.value<QPixmap>().scaled(NORMAL_BANNER_SIZE * scale, Qt::KeepAspectRatio,

This comment has been minimized.

Copy link
@ligfx

ligfx May 26, 2017

Contributor

When AA_EnableHighDpiScaling is set, Qt operates in device-independent pixels, so just NORMAL_BANNER_SIZE is correct.

So, this should be scaled to NORMAL_BANNER_SIZE, not NORMAL_BANNER_SIZE * scale (or not scaled at all, I would think), and should be drawn to NORMAL_BANNER_SIZE, not NORMAL_BANNER_SIZE * scale.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch 2 times, most recently from 65e643c to 3846e90 May 27, 2017

@Starsam80
Copy link
Contributor

left a comment

Other than a few comments, tested and LGTM.

for (QString country : countries)
m_countries.append(QPixmap(country.prepend(sys_dir)));
m_countries.append(QPixmap(country.arg(GetPreferredIconScale()).prepend(sys_dir)));

QStringList ratings{QStringLiteral("rating0.png"), QStringLiteral("rating1.png"),

This comment has been minimized.

Copy link
@Starsam80

Starsam80 May 28, 2017

Contributor

You should use the new rating stars that are in master

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

That also involves a change to load them from the current theme, it can probably be pushed off to another PR.

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Another PR, please.

@@ -40,7 +56,9 @@ void Resources::Init()
m_ratings.append(QPixmap(rating.prepend(sys_dir)));

m_misc.append(QPixmap(QStringLiteral("nobanner.png").prepend(sys_dir)));

This comment has been minimized.

Copy link
@Starsam80

Starsam80 May 28, 2017

Contributor

You should load the hidpi version here

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Done.

m_misc.append(QPixmap(QStringLiteral("dolphin_logo.png").prepend(sys_dir)));
m_misc.append(QPixmap(QStringLiteral("dolphin_logo%1.png")
.arg(Resources::GetPreferredIconScale())
.prepend(sys_dir)));
m_misc.append(QPixmap(QStringLiteral("Dolphin.png").prepend(sys_dir)));

This comment has been minimized.

Copy link
@Starsam80

Starsam80 May 28, 2017

Contributor

You should make a 2x and 4x image using the dolphin-emu.svg in Data/, and then load that here.

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Qt can load SVG, could we / do we want to use that directly?

@ligfx
Copy link
Contributor

left a comment

Works great on macOS. Couple code cleanup things.

for (QString country : countries)
m_countries.append(QPixmap(country.prepend(sys_dir)));
m_countries.append(QPixmap(country.arg(GetPreferredIconScale()).prepend(sys_dir)));

QStringList ratings{QStringLiteral("rating0.png"), QStringLiteral("rating1.png"),

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

That also involves a change to load them from the current theme, it can probably be pushed off to another PR.

m_misc.append(QPixmap(QStringLiteral("dolphin_logo.png").prepend(sys_dir)));
m_misc.append(QPixmap(QStringLiteral("dolphin_logo%1.png")
.arg(Resources::GetPreferredIconScale())
.prepend(sys_dir)));
m_misc.append(QPixmap(QStringLiteral("Dolphin.png").prepend(sys_dir)));

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Qt can load SVG, could we / do we want to use that directly?

m_screenshot_action->setIcon(QIcon(QStringLiteral("screenshot.png").prepend(dir)));
m_config_action->setIcon(QIcon(QStringLiteral("config.png").prepend(dir)));
m_controllers_action->setIcon(QIcon(QStringLiteral("classic.png").prepend(dir)));
m_open_action->setIcon(

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Same as above with the common logic. Maybe this logic could be moved into the Resources namespace as well?

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Moved.

@@ -13,25 +17,37 @@ QList<QPixmap> Resources::m_countries;
QList<QPixmap> Resources::m_ratings;
QList<QPixmap> Resources::m_misc;

QString Resources::GetPreferredIconScale(int max)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

This max argument is never used.

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Not true anymore.

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Ah, okay. I still don't understand this max argument. Why not always try to load the best pixel ratio image, and fallback if it doesn't exist, just like QIcon does internally?

for (QString country : countries)
m_countries.append(QPixmap(country.prepend(sys_dir)));
m_countries.append(QPixmap(country.arg(GetPreferredIconScale()).prepend(sys_dir)));

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

This copy-pasting is getting unwieldy, and its common logic should be extracted out. Depending on how its done, the QStringLists probably aren't even needed, it could potentially just be

m_countries.append(LoadDolphinPixmap("Flag_Europe"));
m_countries.append(LoadDolphinPixmap("Flag_Japan"));

etc...

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Less cluttered now.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch 2 times, most recently from dffdf50 to df36821 May 28, 2017

for (QString platform : platforms)
m_platforms.append(QPixmap(platform.prepend(sys_dir)));
m_platforms.append(GetScaledIcon(platform.prepend(QStringLiteral("Platform_"))));

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

I don't think that splitting e.g. "Platform_Gamecube" into "Platform_" and "Gamecube" is a good idea. It hurts greppability of the codebase, and there's no common logic here other than the filenames looking similar.

for (QString country : countries)
m_countries.append(QPixmap(country.prepend(sys_dir)));
m_countries.append(GetScaledIcon(country.prepend(QStringLiteral("Flag_"))));

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Ditto.

m_fullscreen_action->setIcon(QIcon(Resources::GetScaledThemeIcon(QStringLiteral("fullscreen"))));
m_screenshot_action->setIcon(QIcon(Resources::GetScaledThemeIcon(QStringLiteral("screenshot"))));
m_config_action->setIcon(QIcon(Resources::GetScaledThemeIcon(QStringLiteral("config"))));
m_controllers_action->setIcon(QIcon(Resources::GetScaledThemeIcon(QStringLiteral("classic"))));

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Why not have GetScaledThemeIcon return a QIcon directly? It has "Icon" in the name already.

@@ -13,25 +17,37 @@ QList<QPixmap> Resources::m_countries;
QList<QPixmap> Resources::m_ratings;
QList<QPixmap> Resources::m_misc;

QString Resources::GetPreferredIconScale(int max)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Ah, okay. I still don't understand this max argument. Why not always try to load the best pixel ratio image, and fallback if it doesn't exist, just like QIcon does internally?

return QStringLiteral("");
}

QPixmap Resources::GetScaledIcon(const QString& name, int max, const QString& dir)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

This returns a pixmap, its name should reflect that rather than sounding like it returns an icon.

return QStringLiteral("");
}

QPixmap Resources::GetScaledIcon(const QString& name, int max, const QString& dir)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

The dir parameter shouldn't be exposed to callers outside of this namespace. "Get image from Resources" and "Get image from theme" should be adequate, and anything else that's needed can be added instead of encouraging spreading filesystem details around the code.


QList<QPixmap> Resources::m_platforms;
QList<QPixmap> Resources::m_countries;
QList<QPixmap> Resources::m_ratings;
QList<QPixmap> Resources::m_misc;

QString Resources::GetPreferredIconScale(int max)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Since this is only used internally, it could be merged into GetScaledIcon

@@ -26,6 +26,11 @@ class Resources final
LOGO_SMALL
};

static QString GetPreferredIconScale(int max = 4);

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

This isn't used externally, so shouldn't be public.

static QString GetPreferredIconScale(int max = 4);
static QPixmap GetScaledIcon(const QString& name, int max = 4,
const QString& dir = QStringLiteral(""));
static QPixmap GetScaledThemeIcon(const QString& name, int max = 4);

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Since we control all inputs to these functions, would it be easier to have them take std::strings, or at least const char*s, and convert internally?

This comment has been minimized.

Copy link
@ligfx

ligfx May 29, 2017

Contributor

Awesome, that looks much better.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch from df36821 to 15d8e6d May 28, 2017

@spycrab

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2017

I think this can be merged now as all points have been addressed

@ligfx

This comment has been minimized.

Copy link
Contributor

commented May 28, 2017

@spycrab That's not how it works :) Make changes, get more reviews, then merge.

return QStringLiteral("");
}

QPixmap Resources::GetScaledPixmap(const std::string& name, int max, Resources::Directory dir)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

This isn't what I meant at all. The two differently-named functions before were good, just not the exposure of the dir argument to public callers. This way makes things more complicated with a new enum.

Theme
};

static QString GetPreferredIconScale(int max = 4);

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

Undefined function, please remove.

m_screenshot_action->setIcon(QIcon(QStringLiteral("screenshot.png").prepend(dir)));
m_config_action->setIcon(QIcon(QStringLiteral("config.png").prepend(dir)));
m_controllers_action->setIcon(QIcon(QStringLiteral("classic.png").prepend(dir)));
m_open_action->setIcon(QIcon(Resources::GetScaledPixmap("open", 4, Resources::Directory::Theme)));

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

This isn't specific to the latest round of changes, but I didn't think of it earlier: on all desktop platforms Qt supports, this is strictly worse than how it is currently, since it won't do size mediation between the @1x and @2x versions (no current desktop is xdpi or xxdpi). Depending on the icon size needed, QIcon will fall back to the @1x versions and treat them as if they're smaller icons at the screen pixel ratio.

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Can't see the issue tbh honest, tried at different scales and couldn't notice any negative side effects, definitely not worse than how it currently is. Maybe if you could elaborate and suggest a way to fix that, until then I'm unable to address this.

This comment has been minimized.

Copy link
@ligfx

ligfx May 29, 2017

Contributor

Here's the issue: if you ever want to display these icons on a @2x screen at half their native size, e.g. 16x16, the appropriate strategy is to treat the @1x icons as actually 16x16@2x icons rather than downscale the 32x32@2x icons. This is the default behavior of QIcon, which is designed for easy handling of different sizes/densities/states, but it's prevented by this PR.

If using @1x pixmaps is really no different than downscaling @2x pixmaps, then we should get rid of the @1x ones altogether and always use @2x. Otherwise, you can fix this by adding each scale pixmap to the QIcon manually with something like:

QIcon icon;
icon.addFile(icon_name + QStringLiteral(".png")); // also automatically picks up @2x

QPixmap pixmap4x(icon_name + QStringLiteral("@4x.png"));
if (!pixmap4x.isNull()) {
  pixmap4x.setDevicePixelRatio(4.0);
  icon.addPixmap(pixmap4x);
}

QList<QPixmap> Resources::m_platforms;
QList<QPixmap> Resources::m_countries;
QList<QPixmap> Resources::m_ratings;
QList<QPixmap> Resources::m_misc;

QString Resources::GetPreferredPixmapScale(int max)

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

You probably missed it in the shuffle, I asked on an earlier commit: Instead of max, why not always try to load the best pixel ratio image, and fallback if it doesn't exist, just like QIcon does internally?

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

I think that logic would be quite a bit worse. Also if we know that something is definitely going to fail why try at all?

This comment has been minimized.

Copy link
@ligfx

ligfx May 28, 2017

Contributor

What do you mean "worse"? And because it separates the logic of "get this icon" from "knowing which scale versions exist on the filesystem", which doesn't belong in places like the Toolbar class.

This comment has been minimized.

Copy link
@spycrab

spycrab May 28, 2017

Author Contributor

Hm, you got a point there. Will change.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch 2 times, most recently from 21721f8 to 37db2f8 May 28, 2017

QPixmap pixmap;
for (int scale : {4, 2, 1})
{
pixmap = QPixmap(QStringLiteral("%1%2%3.png")

This comment has been minimized.

Copy link
@ligfx

ligfx May 29, 2017

Contributor

Pixmaps at @4x do not have their devicePixelRatio automatically set by Qt, and need to be set manually.

return QStringLiteral("");
}

QPixmap Resources::GetScaledPixmap(const std::string& name, bool is_themed)

This comment has been minimized.

Copy link
@ligfx

ligfx May 29, 2017

Contributor

Boolean parameters that aren't immediately obvious aren't a very good pattern, since it reduces readability and leads to mistakes. One way to deal with them is to introduce a typed enum, but that's overkill here. I'd like to see something like this (which you basically had before, just with the additional dir parameter that shouldn't be exposed):

QPixmap GetResourcePixmap(const std::string& name);
QIcon GetThemeIcon(const std::string& name);

with any functions that take a file/directory path or boolean parameter being static to this file.

QPixmap Resources::GetScaledPixmap(const std::string& name, bool is_themed)
{
QPixmap pixmap;
for (int scale : {4, 2, 1})

This comment has been minimized.

Copy link
@ligfx

ligfx May 29, 2017

Contributor

If devicePixelRatio is less than 4 (which it will be everywhere right now) or less than 2, then this is needlessly repeating the same code.

Something unrolled like this seems simpler to me:

const float screen_ratio = QGuiApplication::primaryScreen()->devicePixelRatio();
std::string dir = is_themed ? Settings().GetThemeDir() : Settings().GetResourcesDir();

if (screen_ratio > 2) {
  QPixmap pixmap(dir + name + "@4x.png");
  pixmap.setDevicePixelRatio(4.0);
  if (!pixmap.isNull()) return pixmap;
}
if (screen_ratio > 1) {
  QPixmap pixmap(dir + name + "@2x.png");
  if (!pixmap.isNull()) return pixmap;
}
QPixmap pixmap(dir + name + "png");
return pixmap;

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch from 37db2f8 to 75af209 May 29, 2017

@spycrab

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2017

@ligfx Can you take a look again?

@ligfx
Copy link
Contributor

left a comment

Looks great @spycrab! One small thing left.

const auto sizes = icon.availableSizes();
const auto dpr = QGuiApplication::primaryScreen()->devicePixelRatio();

return icon.pixmap(sizes[std::min((dpr <= 1) ? 1 : (dpr <= 2) ? 2 : 4, sizes.size() - 1)]);

This comment has been minimized.

Copy link
@ligfx

ligfx May 30, 2017

Contributor

This breaks on macOS (icons are too large). I think the right thing to do here is icon.pixmap(sizes[0])?

This comment has been minimized.

Copy link
@spycrab

spycrab May 30, 2017

Author Contributor

Hold on, I think I got this.

This comment has been minimized.

Copy link
@spycrab

spycrab May 30, 2017

Author Contributor

Try it now.

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch from 75af209 to fe662f9 May 30, 2017

@spycrab spycrab force-pushed the spycrab:qt_hidpi branch from fe662f9 to 1737e81 May 30, 2017

@ligfx

ligfx approved these changes May 30, 2017

Copy link
Contributor

left a comment

Cool, that fixed it. Great job. This makes DolphinQt2 look so much better!

@Helios747 Helios747 merged commit 25f24d3 into dolphin-emu:master May 30, 2017

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

@spycrab spycrab deleted the spycrab:qt_hidpi branch May 31, 2017

@leoetlino leoetlino modified the milestone: Qt Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.