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

Retroachievements rcheevos integration #11669

Conversation

LillyJadeKatrin
Copy link
Contributor

This is the first 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, a connection is created to RetroAchievements and the player is logged in, though nothing else, for the sake of keeping this PR manageable. The new functionality is turned on via the USE_RETRO_ACHIEVEMENTS compile-time parameter (-DUSE_RETRO_ACHIEVEMENTS).

Logins are performed with a username and an API token as stored in the new Achievements.ini in the Dolphin config folder. RetroAchievements advises against storing passwords in plaintext config files so an API token is generated upon first login; this token is safe to store in configurations for later use. In its current state, this API token must be generated elsewhere, as there is not yet any user interface for providing a password, though the token should remain constant for a player no matter which emulator is used until the player resets it on the RetroAchievements website.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch 6 times, most recently from 874bba7 to 6cb3ef6 Compare March 18, 2023 16:43
@shuffle2
Copy link
Contributor

Personally, I'd rather if this did not add another compile-time feature macro (what is the reason?).

@AdmiralCurtiss
Copy link
Contributor

Personally, I'd rather if this did not add another compile-time feature macro (what is the reason?).

Why, for the record? I get that having mode configuration combinations makes it harder to ensure all of them work as expected, but this kinda feature seems like a prime thing you want to have the core emulator buildable without; similar to say the Discord integration, where you're relying on an external service that may stop working at any time. At the very least, having a compile-time flag enforces that the feature doesn't end up closely coupled with the emulator itself.

@LillyJadeKatrin
Copy link
Contributor Author

Personally, I'd rather if this did not add another compile-time feature macro (what is the reason?).

At bare minimum I need this to exist for the first several PRs until I have enough of RetroAchievements' integration merged in to be properly functional - as long as it's incomplete, it should be able to be turned off. Past that, I'm inclined to agree with Curtiss on this one, but that's up to Dolphin's staff.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch 3 times, most recently from 212bd8f to 74b657b Compare March 19, 2023 13:48
@shuffle2
Copy link
Contributor

Personally, I'd rather if this did not add another compile-time feature macro (what is the reason?).

Why, for the record? I get that having mode configuration combinations makes it harder to ensure all of them work as expected, but this kinda feature seems like a prime thing you want to have the core emulator buildable without; similar to say the Discord integration, where you're relying on an external service that may stop working at any time. At the very least, having a compile-time flag enforces that the feature doesn't end up closely coupled with the emulator itself.

I think discord also shouldn't be a compile-time option. Both should be optional at runtime. I don't really see why you'd want to deal with having different flavors of builds which may or may not support the feature.

@Pokechu22
Copy link
Contributor

Both discord and rcheevos have external libraries that you might want to exclude from the build. IMO it's reasonable to have a compile-time flag to exclude them. If it were entirely within dolphin code then forgoing a compile-time flag for only a runtime flag would make sense, but with an external library I think it makes more sense to have both.

@shuffle2
Copy link
Contributor

In practice, discord is always compile-time enabled. In fact, disabling discord at compile time was broken for a while until recently and no one had noticed.

IMO having compile-time AND runtime enablement does not make sense for a feature. The only reason something should be compile-time optional is if certain platforms cannot support the feature at all (i.e. can not even link against some dynamic library or whatever).

@JosJuice
Copy link
Member

The only reason something should be compile-time optional is if certain platforms cannot support the feature at all (i.e. can not even link against some dynamic library or whatever).

For what it's worth, this is the case for Discord on Android.

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
Source/Core/DolphinQt/MainWindow.cpp Outdated Show resolved Hide resolved
@shuffle2
Copy link
Contributor

shuffle2 commented Mar 21, 2023

The only reason something should be compile-time optional is if certain platforms cannot support the feature at all (i.e. can not even link against some dynamic library or whatever).

For what it's worth, this is the case for Discord on Android.

OK, I guess that's a decent enough reason ( :( ). It would be nice if #ifdefs could be limited if possible, though.

fwiw if Android was compiling OK all that time...it means Android build was silently accepting this undefined behavior?
Although in practice it wouldn't have been called...just an example of over-use of #ifdefs...

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch from 74b657b to c6f3839 Compare March 21, 2023 04:26
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch 2 times, most recently from b693b98 to 9747cff Compare March 22, 2023 10:12
Externals/rcheevos/CMakeLists.txt Outdated Show resolved Hide resolved
Externals/rcheevos/CMakeLists.txt Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Source/Core/DolphinQt/CMakeLists.txt Outdated Show resolved Hide resolved
Source/Core/DolphinQt/MainWindow.cpp Outdated Show resolved Hide resolved
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch from 6326b68 to 0d4b5ac Compare March 30, 2023 04:28
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.h Show resolved Hide resolved
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
Source/Core/Core/Config/AchievementSettings.cpp Outdated Show resolved Hide resolved
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch from 0d4b5ac to 7df8105 Compare March 31, 2023 00:19
@LillyJadeKatrin
Copy link
Contributor Author

Memo: Make sure the rcheevos submodule is pointing at the most recent version in master.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch 4 times, most recently from 47c5f49 to a211b9a Compare March 31, 2023 04:38
Added the RetroAchievements rcheevos library as a submodule from GitHub.
Adds the rcheevos library from RetroAchievements to the Dolphin Externals as a submodule. Change was verified to import correctly and build both via Visual Studio and via cmake ninja.
Added a flag to VS and CMake for enabling RetroAchievements integration.
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch from a211b9a to 8dbce43 Compare April 2, 2023 12:28
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch 3 times, most recently from ec527cc to 2e5383a Compare April 3, 2023 23:49
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch 3 times, most recently from f024dc8 to 3ecc1ab Compare April 4, 2023 01:08
LillyJadeKatrin and others added 2 commits April 3, 2023 21:17
Added AchievementSettings in Config with RA_INTEGRATION_ENABLED, RA_USERNAME, and RA_API_TOKEN. Includes code to load and store from Achievements.ini file in config folder.
Added AchievementManager class. Upon startup (currently only in DolphinQt), logs into RetroAchievements with the login credentials stored in achievements.ini.

Co-authored-by: AdmiralCurtiss <AdmiralCurtiss@users.noreply.github.com>
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-rcheevos-integration branch from 3ecc1ab to 84b3df0 Compare April 4, 2023 01:17
@delroth
Copy link
Member

delroth commented Apr 4, 2023

I'm still not particularly convinced about the internals design of the AchievementsManager but we can iterate on that if problems arise. LGTM.

@delroth delroth merged commit b63b574 into dolphin-emu:master Apr 4, 2023
14 checks passed
@Danyelalejandro
Copy link

You don't know how much we love you in the RA community @LillyJadeKatrin. Infinite thanks!!!

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