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 - Challenge Icons #12247

Merged

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 handle achievement events for when a "challenge" is "primed" or unprimed, and display the results on screen.
A "challenge" (not an official term) in this context is the completion of a portion of a game while meeting certain optional conditions. Examples would be completing a level in under a certain amount of time or a boss fight without taking damage. While a challenge is currently active, emulators that support RetroAchievements will display the achievement's badge in the corner of the screen to denote that the challenge is still active and valid. If the player completes the achievement's requirements while the badge is on screen, it will be rewarded; if the player fails the challenge (eg from above, once too much time elapses or once the player takes damage) the icon will be removed, denoting that the challenge is no longer active.
In the official RetroAchievements terminology, the runtime will emit an AchievementPrimedEvent when a challenge becomes active and an AchievementUnprimedEvent when it deactivates, whether the challenge was failed or completed. The achievement logic uses the denotation "Trigger" on the portion of the achievement logic that is not the challenge; when every portion of the logic is true except the trigger the achievement is primed, and then when the trigger is also true the achievement is rewarded. This terminology is a bit unwieldy so I use the term "challenge" to denote when the achievement is primed.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-challenges branch 2 times, most recently from 4a5fb77 to 47e054e Compare October 20, 2023 13:10
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-challenges branch 2 times, most recently from f96cfbf to 442bafc Compare October 21, 2023 04:49
Source/Core/Core/AchievementManager.h Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.h Outdated Show resolved Hide resolved
Source/Core/Core/AchievementManager.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/OnScreenUI.cpp Outdated Show resolved Hide resolved
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-challenges branch 5 times, most recently from c4b4361 to 89b5be3 Compare October 24, 2023 01:36
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not entirely happy with the fact that this puts achievement-specific code into OnScreenUI, and I suspect one can make optimizations on the lock hold time as well as memory allocation behavior, this seems basically fine. Needs to be tested though.

@@ -61,6 +61,9 @@ class OnScreenUI

private:
void DrawDebugText();
#ifdef USE_RETRO_ACHIEVEMENTS
void DrawChallenges();
Copy link
Contributor

@iwubcode iwubcode Nov 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned this in the chat but this shouldn't be owned by the OnScreenUI, it should be owned by AchievementManager.

Should do something like:

const auto achievement_manager = AchievementManager::GetInstance();
achievement_manager->Display();

instead and have achievement manager know how to display the challenge icons. This keeps the logic out of OnScreenUI which should stay somewhat generic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should, but that makes the texture destruction logic a lot more complicated. I'd recommend we come together and refactor this at some point in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can see how I do this for the editor:

graphics_mod_editor.Shutdown();
. I assumed you wanted to punt on this but wanted to call this out in a more public place. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a first-pass attempt at migrating this into AchievementManager. Now if only I could get it to work on all builders...

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-challenges branch 2 times, most recently from ca40c46 to 275d62a Compare November 24, 2023 16:05
@AdmiralCurtiss
Copy link
Contributor

Not gonna lie, while I appreciate the attempt, I think adding an imgui dependency to core is even worse than just having the challenge icon handling in OnScreenUI. You're also making the AchievementsManager an even bigger class than it already is by just stuffing this in the middle of it. I meant what I said, we should refactor this later once Achievements actually work and we don't have to blindly guess on every change because there's no test data -- in fact that's the only reason I didn't merge it when I made this comment, I couldn't figure out how to test that this actually works.

Sorry if this comes across as rude but I'm very tired of this entire Achievements thing...

@LillyJadeKatrin
Copy link
Contributor Author

Well, that's why it's a separate commit, I'll just drop that one then.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-challenges branch 2 times, most recently from 2fd03bb to 309ea3c Compare December 2, 2023 00:10
@@ -74,6 +77,10 @@ class OnScreenUI
u32 m_backbuffer_height = 1;
float m_backbuffer_scale = 1.0;

#ifdef USE_RETRO_ACHIEVEMENTS
std::map<std::string, std::unique_ptr<AbstractTexture>> m_challenge_texture_map;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::map<std::string, std::unique_ptr<AbstractTexture>> m_challenge_texture_map;
std::map<std::string, std::unique_ptr<AbstractTexture>, std::less<>> m_challenge_texture_map;

const u32 width = icon->width;
const u32 height = icon->height;
TextureConfig tex_config(width, height, 1, 1, 1, AbstractTextureFormat::RGBA8, 0);
m_challenge_texture_map[name] = g_gfx->CreateTexture(tex_config);
Copy link
Member

@lioncash lioncash Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
m_challenge_texture_map[name] = g_gfx->CreateTexture(tex_config);
auto res = m_challenge_texture_map.insert_or_assign(name, g_gfx->CreateTexture(tex_config));

then use res for the if statement and below, to avoid doing two redundant map lookups

When an achievement is "primed", a challenge is active, for example completing a portion of the game in under a time limit or without taking damage or using certain items. This is stored in a map in the Achievement Manager (and removed when the achievement is unprimed) so a later commit can display it on screen.
The active challenges, aka the primed achievements, are displayed on screen as a series of icons in the bottom right corner of the screen via OnScreenUI.
@lioncash lioncash merged commit 3a4cf57 into dolphin-emu:master Dec 7, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants