[GSoC 2021] RetroPlayer: Add Achievements

Shardul555 commented 2 years ago

Description

This Pull request adds support for Achievements in RetroPlayer, data fetched from RetroAchievements API have information about the achievements provided for a particular game, so we used that data for activating achievements, obtaining achievement state for every frame and then awarding it whenever it triggered. User will be notified about an unlocked achievement through a pop-up notification and also through their RetroAchievement.org profile. This PR will use https://github.com/kodi-game/game.libretro/pull/73 for calling rcheevos functions.

Motivation and Context

Last year we have added support for RCheevos in RetroPlayer, so adding support for Achievements is one another task that was needed to be accomplished.

How Has This Been Tested?

Screenshots (if appropriate):

Notifying user through pop-up notification: Information updated in RetroAchievements profile:

Types of change

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] Clean up (non-breaking change which removes non-working, unmaintained functionality)
[ ] Improvement (non-breaking change which improves existing functionality)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that will cause existing functionality to change)
[ ] Cosmetic change (non-breaking change that doesn't touch code)
[ ] None of the above (please explain below)

Checklist:

[x] My code follows the Code Guidelines of this project
[x] My change requires a change to the documentation, either Doxygen or wiki
[ ] I have updated the documentation accordingly
[x] I have read the Contributing document
[ ] I have added tests to cover my change
[ ] All new and existing tests passed

Shardul555 commented 2 years ago

I have squashed commits into one and rebase my work to feature_achievements_19.2, if any other fixes are required with it then please let me know.

garbear commented 2 years ago

Thanks for rebasing! Much easier to review.

Besides style issues, I only see one problem, and unfortunately there's no good fix. The problem is the static variable and callback function. Basically, the achievements library needs to accept a "context" parameter (void pointer), that it returns to us, so that we can recover the class that called the function. The C language doesn't have classes, so far too often this context parameter is forgotten. As an example of how it should be, see here: https://github.com/garbear/xbmc/blob/retroplayer-19.2/xbmc/games/addons/GameClient.cpp#L853 - the first parameter of the C callback is the class that we cast to, then immediately call the appropriate function of the class where the actual logic is.

I think we'll need to patch the library. I would hope that it's accepted upstream, because they cause the problem by not having a contextual API, but it's a breaking change, and a single-game frontend like RetroArch doesn't have a use for context like we do. It's a delicate issue due to the headache of breakage and lack of advantage for their team. Do you want to make the change quick and dirty, and I'll handle cleanup and PRing upstream?

As far as style goes, lets first do a clang-format run. I ran the following command in the project directory:

clang-format-9 -style=file -i $(find xbmc/cores/RetroPlayer xbmc/games xbmc/input xbmc/peripherals -name "*.cpp" -o -name "*.h")

It led to this commit, which you can cherry-pick and squash:

591e6a9363ef3cd21b0ae2ab9d4acf341459a351

Next, the hallmark of a good PR is one with no lines changed or removed - only added. When you view the diff, these show up as red. A fully green PR is safest, because every change to existing code can break something. Event if it's only a cosmetic, a red line is a warning sign. Also they are unrelated changes which shouldn't be included in a PR. Every red line should be scrutinized and justified. In this case, the PR should probably have exactly two red lines: the changes to the Game API version.

Once the achievements library is made contextual, we adopt the modified API, and the style changes are fixed, then this PR is mostly ready to go. I'll see if I find any other issues.

garbear commented 2 years ago

OK, only two other changes are to drop using and drop the unrelated changes.

garbear commented 1 year ago

@Shardul555 You did such great work that I've fully rebased this on master and now include it in my test builds. Would you be interested in PRing upstream to the xbmc repo?

garbear commented 1 year ago

The rebased work is here: https://github.com/garbear/xbmc/commit/6116b663f5fb4ea1737a6a59ac78d90fa4002988

You can cherry-pick that, push to GitHub, and open a PR.

Shardul555 commented 1 year ago

Hi Garbear,

I am really grateful for the opportunity provided by you and xbmc, I have opened a PR : https://github.com/xbmc/xbmc/pull/22652 Please review it whenever possible.

Thanks and regards, Shardul Semwal

On Mon, Jan 30, 2023 at 6:52 AM Garrett Brown @.***> wrote:

The rebased work is here: 6116b66 https://github.com/garbear/xbmc/commit/6116b663f5fb4ea1737a6a59ac78d90fa4002988

You can cherry-pick that, push to GitHub, and open a PR.

— Reply to this email directly, view it on GitHub https://github.com/garbear/xbmc/pull/127#issuecomment-1407849762, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANSVQFN7OXXD5IBW2MDKJ53WU4JVNANCNFSM5F3EVGSA . You are receiving this because you were mentioned.Message ID: @.***>

garbear / xbmc