Retroachievements dialog

[LillyJadeKatrin]

This is a portion of an integration with the RetroAchievements tools and libraries for connecting to the website, downloading data, unlocking achievements and submitting to leaderboards for games running in Dolphin. In this PR, I add an AchievementsWindow, a dialog accessible through the main window's menu bar, that here contains settings and login for RetroAchievements, and in the future, visual data about the player's unlocks and rankings on the site.

[AdmiralCurtiss]

The first commit does not build, it needs a #include <QVBoxLayout> in AchievementsWindow.cpp.

[LillyJadeKatrin]

That's strange, when I goto source in Visual Studio on QVBoxLayout it takes me to QBoxLayout.h. Is this a qt version thing I wonder, or just my environment being stupid.

EDIT: Never mind, I see what you mean, got it.

[AdmiralCurtiss]

Pinging @MayImilae because this is a user-facing GUI change. It currently looks like this:

I'm not sure if this is really the right place for this, I'd expect it to be somewhere in the regular Config window instead.

[AdmiralCurtiss]

I'm also not sure whether it's a good idea to even show these options to the user before any actual achievement data exists on the RetroAchievements end. I think that'll just lead to confusion.

[mbc07]

I'm not sure if this is really the right place for this, I'd expect it to be somewhere in the regular Config window instead.

I agree. Keeping it on Tools makes it a bit too buried IMO. The Tools menu is also getting bigger and bigger, but that's not related to this PR.

I'm also not sure whether it's a good idea to even show these options to the user before any actual achievement data exists on the RetroAchievements end. I think that'll just lead to confusion.

Perhaps we could keep it hidden from the GUI unless a specific setting is added to Dolphin.ini, or unless Dolphin is launched with a specific command line parameter? Then, when everything is ready (on both Dolphin's and RA's ends), we do another PR removing the setting and making the RA GUI always available...

[LillyJadeKatrin]

Note that one of the popup messages that has been merged in will directly notify the user that achievements don't (yet) exist for the game they're playing, would that be sufficient?

[AdmiralCurtiss]

I mean... yes, but since that happens for every single game right now that's still not great. I imagine we want to do a big, "hey this feature exists now, and it works on a number of games already!" kinda announcement, but maybe I'm overthinking this.

[LillyJadeKatrin]

An announcement, sure, but the question is what do players see before that.

[MayImilae]

I like the current placement under Tools. Like, Retroachievements does not belong in general config - it's not really a "configuration" for the emulator. Tools may be getting bigger but it is good for misc standalone things like, and it's not that bad yet. It'll do. Also I do think we need a way to access it and turn it on within the GUI itself. We could move it elsewhere, or split tools into two menu bar items, but I think that should be left to another PR.

As for the window itself, I think it looks pretty good. I like that it is a single column of checkmarks here (as opposed to two column), and it's all laid out quite nicely, with enable at the top, then username, enable achievements, etc etc cascading down. Good work!

One small question though:

Enable Rich Presence

It is unclear what that Rich Presence is for. I assumed that it was Discord when I first saw it, but looking into the code it appears to be retroachievements specific. Would someone who is using this know what that means?

[LillyJadeKatrin]

Certainly! RetroAchievements has its own Rich Presence system. Devs create a script based on a game's memory to provide a detailed description of what exactly they're doing in game. For example, this is a screencap from the site front page:

Note the level of detail, such as current level/location, stats, inventory/collectables, current party, etc. In the long term I hope to integrate this with the Discord RP that Dolphin provides, though that's a future detail and for now this is merely to support the RP on retroachievements.org.

EDIT: To clarify, basically if you turn it off it'll just report "player is playing Super Mario Sunshine" instead of anything more detailed.

[mbc07]

Had time to properly test this, and I have some remarks:

Related to Design

• The current tab design wraps all UI controls into an unnamed QGroupBox, and that results in an unnecessary square being added around the tab contents. The default padding is also removed, and the result is... weird. That's not consistent with other similar layouts we already have (e.g. alternate input sources), so consider removing the QGroupBox and adding the widgets directly to the QVBoxLayout component. After removing the QGroupBox, you'll also need to remove the padding override to make it look right (remove the call to setContentsMargins() during the QVBoxLayout creation):

  

• When login fails, a "login failed" message appears on the GUI, but it appears above the username input box. I think this message should appear below the login button, as it's the login button that might trigger it. Having it above the username input feels out of place and disconnected.

• Related to the previous point, the tab height seems to be hardcoded. This results in empty space on the RA window when you open it, and also causes a scrollbar to appear when the "login failed" message is triggered. Resizing the containing window has no effect either and I think this should be fixed (removing the QGroupBox might fix that without requiring further changes):

  

• This one is more of a nitpick, so feel free to ignore: the username input box is populated with "Username" by default, consider making it blank, or switching to a placeholder (see the "add server" dialog from Alternate Input Sources for a reference of what I mean)

Related to Behavior
Interacting with the RA window is prone to crashing Dolphin entirely. It seems related to the username/password validation, I hadn't dig too much. Some scenarios I found that crashes the emulator:

• Input a username and a wrong password, "login failed" message will appear. Correct the password, then press login again, Dolphin will crash.
• Input a valid username and password, login will succeed and the login button will be replaced by a logout button. Click the logout button, then try logging in again, Dolphin will crash.
• Leave both username and password empty, press login. Nothing will happen. After that, input an username and password, press login, Dolphin will crash.

In other words, if for any reason you need to click the login button a second time, the entire emulator crashes...

[LillyJadeKatrin]

Ahh, the behavior stuff has already been fixed but I forgot to rebase it in, my mistake.

[Mhmmm]

Hey, so I just wanted to add that the dialog window should also include a toggle for hardcore/softcore achievements, as most players want to play in hardcore mode, while softcore mode allows cheats and loading save states.

[AdmiralCurtiss]

Hardcore mode requires additional work in the emulator itself to actually disable the relevant features. It will come later.

[LillyJadeKatrin]

Yeah, hardcore is its own entire PR, there's a lot to it - flipping that switch means disabling quite a few emulator features, from cheats to state loads to emulator speed. There's also one or two other buttons that will be added to this as their functionality gets added elsewhere.

[mbc07]

The most recent build from this PR addresses all remarks I made previously. This LGTM now...

[AdmiralCurtiss]

Okay, I'll ask once more explicitly: Are we fine with exposing this config window to users, even though there are no games with which they can actually use it yet?

[mbc07]

I don't have a strong opinion about that. If we decide this should be kept buried until things are fully ready for an official announcement (e.g. integration done on both sides, at least a few games with achievement sets, etc.), it should be trivial to keep the "achievements" entry hidden from the Tools menu unless a specific INI key is added to Dolphin.ini in the mean time...

[LillyJadeKatrin]

I did include a bit in the first tooltip that this is not currently working and an announcement will be made when it's ready.

[AdmiralCurtiss]

do you really expect most of our users to read

[LillyJadeKatrin]

It was at this point she knew she lost the argument.

[iwubcode]

Just curious, what are "Unofficial" and "Encore" achievements?

I'm also not sure whether it's a good idea to even show these options to the user before any actual achievement data exists on the RetroAchievements end. I think that'll just lead to confusion.

Agree. My obvious suggestion is to not display this in the Tools menu until the feature is ready to go live, it can be left in while testing for this PR though (removed before merge).

[LillyJadeKatrin]

Just curious, what are "Unofficial" and "Encore" achievements?

Unofficial: each achievement is enumerated Official or Unofficial (there's support for other values but currently none are used) for whether it is deemed officially submitted on the site. When a dev makes a set of achievements or changes to an existing set, they're all Unofficial until the set is ready to go live, so these are largely for testing, but in some cases I've seen devs leave a handful of unofficial achievements just for fun.

Encore: when active, you'll get achievement unlock popups both for achievements you're unlocking for the first time and for achievements you've already unlocked and have met the requirements for again. Primarily a just-for-fun thing but can also be used for speedrunning categories, like say 100% deathless or the like.

Both are explained in the tooltip descriptions.

Agree. My obvious suggestion is to not display this in the Tools menu until the feature is ready to go live, it can be left in while testing for this PR though (removed before merge).

Fixed this in the newest change, now it only shows up if Enable is active.

[AdmiralCurtiss]

If we're going this way (hiding the menu until it's ready) then you can also remove the blurb about the feature not being ready in the tooltip.

[LillyJadeKatrin]

Honestly I'd argue to keep it because it's still theoretically possible someone can mess with their own config and get there, albeit now much less likely.