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.

Sign up for GitHub

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

Jump to bottom

RetroAchievements - Leaderboards Tab #12027

Merged
merged 3 commits into from Oct 15, 2023
Merged

RetroAchievements - Leaderboards Tab #12027

merged 3 commits into from Oct 15, 2023

Conversation

LillyJadeKatrin
Copy link
Contributor

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 a leaderboards tab to the Achievements dialog, and add the necessary calls to the RetroAchievements server to collect the relevant information. Each leaderboard is shown with the player's current score, if one is submitted, and a selection of other players' submissions they may find useful.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch from 3ee9286 to 1d44488 Compare July 3, 2023 04:03
@iwubcode
Copy link
Contributor

iwubcode commented Jul 3, 2023

The rulings is a bit weird but it is what it is. I'd suggest at least making some constants for the '4', '5', and '3' values. At least 4...

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch from 1d44488 to 74c8457 Compare July 3, 2023 05:12
@LillyJadeKatrin LillyJadeKatrin changed the title Retroachievements leaderboards tab RetroAchievements - Leaderboards Tab Jul 3, 2023
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch from 74c8457 to d15ae7f Compare July 4, 2023 10:58
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch from d15ae7f to c970a41 Compare July 4, 2023 14:53
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch 2 times, most recently from 59821e2 to aad4827 Compare September 9, 2023 23:11
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch 2 times, most recently from fe162dc to 6747957 Compare September 26, 2023 11:45
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch 2 times, most recently from e3cc974 to 6420ad6 Compare October 1, 2023 17:04
AdmiralCurtiss
AdmiralCurtiss reviewed Oct 10, 2023
View reviewed changes
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch 4 times, most recently from e9ad2f8 to 457c1b9 Compare October 12, 2023 03:23
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch from 457c1b9 to b32ab7f Compare October 12, 2023 12:10
@AdmiralCurtiss
Copy link
Contributor

Okay, the backend code seems fine now, and I know we're not gonna win a prize in UI design either way but this looks just kinda terrible:

ui

Can you at least show separators between the individual leaderboards?

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-leaderboards-tab branch 2 times, most recently from 9f1e474 to a055480 Compare October 15, 2023 03:39
@AdmiralCurtiss
Copy link
Contributor

Okay, UI-wise this is good enough for a prototype now, but clearly something is still wrong here somewhere (did you actually test this?), I'm not seeing the scores I would expect.

12312

Also something is causing a stack overflow in the Qt code in some instances, not sure what's going on there, I think I need to build Qt with symbols to understand that one...

It's also very unresponsive while loading (probably due to holding the lock for the entire duration of the UI construction) but please don't try to solve that one as part of this PR, we should do that once everything actually works properly.

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Oct 15, 2023 •
edited

Okay this is fixed by just not reusing the variables in FetchBoardInfo() so I guess rcheevos just doesn't like that for some reason. You really shouldn't do that anyway for memory safety reasons. Spoke too soon, that looked okay in the debugger but still produces a wrong GUI...

@AdmiralCurtiss
Copy link
Contributor

Ah, I found it! You're using rank where you should be using index! The former can be non-unique if multiple players have the exact same score.

@AdmiralCurtiss AdmiralCurtiss force-pushed the retroachievements-leaderboards-tab branch from a055480 to 756c773 Compare October 15, 2023 15:53
@AdmiralCurtiss
Copy link
Contributor

Okay, now for that stack overflow, how does that happen...

@AdmiralCurtiss AdmiralCurtiss mentioned this pull request Oct 15, 2023
Merged
@AdmiralCurtiss AdmiralCurtiss force-pushed the retroachievements-leaderboards-tab branch from 756c773 to 1ad6829 Compare October 15, 2023 16:44
@AdmiralCurtiss
Copy link
Contributor

Now it works, Qt did not like that you reused the layout without clearing it first.

@LillyJadeKatrin
Copy link
Contributor Author

Ah, I found it! You're using rank where you should be using index! The former can be non-unique if multiple players have the exact same score.

Genuinely didn't realize this, that is an excellent find, thank you.

@LillyJadeKatrin @AdmiralCurtiss
Added Leaderboard info map to AchievementManager
61dded7 
The leaderboard map created here contains information useful to displaying leaderboard stats in the Achievement dialog, including each leaderboard's name and description and a partial list of entries for display. The entire map is exposed to the UI in a single call for simplicity.
@LillyJadeKatrin @AdmiralCurtiss
Added FetchBoardInfo to AchievementManager
04df930 
FetchBoardInfo is called (via the work queue asynchronously) on a leaderboard every time it is activated or submitted to. It makes two calls to the RetroAchievements API for fetching leaderboard info, one that requests the top four entries in the leaderboard and another that requests the player's entry, the two entries above the player and the two entries below. All of these are inserted into a single map (resolving any overlaps) so the result can be exposed to the UI.
@LillyJadeKatrin @AdmiralCurtiss
Add Leaderboards tab to Achievement dialog
b824d55 
A new tab is added to the Achievements dialog to chart out the leaderboards in a table. Each row of the table contains the leaderboard information and up to four relevant entries, varying based on how many entries are in the leaderboard, whether or not the player has a submitted score, and where in the leaderboard the player's score is.
@AdmiralCurtiss AdmiralCurtiss force-pushed the retroachievements-leaderboards-tab branch from 1ad6829 to b824d55 Compare October 15, 2023 19:29
@AdmiralCurtiss AdmiralCurtiss merged commit bbf3fed into dolphin-emu:master Oct 15, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lioncash lioncash lioncash left review comments

@AdmiralCurtiss AdmiralCurtiss AdmiralCurtiss left review comments

@iwubcode iwubcode iwubcode left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@LillyJadeKatrin @iwubcode @AdmiralCurtiss @lioncash