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

[GSoC 2021] RetroPlayer: Add Achievements #127

Open
wants to merge 1 commit into
base: feature_achievements_19.2
Choose a base branch
from

Conversation

Shardul555
Copy link

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 kodi-game/game.libretro#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:
image
Information updated in RetroAchievements profile:
image

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)
  • 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:

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

@Shardul555 Shardul555 requested a review from garbear as a code owner October 12, 2021 17:44
@Shardul555
Copy link
Author

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
Copy link
Owner

garbear commented Oct 12, 2021

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:

591e6a9

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.

@@ -12,6 +12,9 @@

#include <map>
#include <string>
#include <vector>
#include <unordered_map>
using std::vector;
Copy link
Owner

Choose a reason for hiding this comment

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

using must not appear in headers, because it pollutes the global namespace of all files that include the header. using can appear in the cpp file, but usually it's best to not pollute the global namespace and just type out the entire type every time.

@@ -98,7 +98,7 @@ IF "%store%" NEQ "" (
)

rem execute cmake to generate makefiles processable by nmake
cmake "%ADDONS_PATH%" -G "NMake Makefiles" ^
cmake "%ADDONS_PATH%" -G "Visual Studio 16 2019" -A x64 ^
Copy link
Owner

Choose a reason for hiding this comment

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

Drop this change from the main commit because it's another red line, which reveals that it's unrelated to the feature. If you wanted, you can could a second commit that makes this change and add the commit to the PR, but honestly it wouldn't be accepted because it breaks CI.

add_custom_command(OUTPUT ${CPP_FILE}
COMMAND ${SWIG_EXECUTABLE}
ARGS -w401 -c++ -o ${file}.xml -xml -I${CMAKE_SOURCE_DIR}/xbmc -xmllang python ${CMAKE_CURRENT_SOURCE_DIR}/../swig/${file}
COMMAND ${Java_JAVA_EXECUTABLE}
ARGS ${JAVA_OPEN_OPTS} -cp "${classpath}" groovy.ui.GroovyMain ${CMAKE_SOURCE_DIR}/tools/codegenerator/Generator.groovy ${file}.xml ${CMAKE_CURRENT_SOURCE_DIR}/../python/PythonSwig.cpp.template ${file}.cpp > ${devnull}
ARGS ${JAVA_OPEN_OPTS} -cp "${classpath}" groovy.ui.GroovyMain ${CMAKE_SOURCE_DIR}/tools/codegenerator/Generator.groovy ${file}.xml ${CMAKE_CURRENT_SOURCE_DIR}/../python/PythonSwig.cpp.template ${file}.cpp > ${devnull}
Copy link
Owner

Choose a reason for hiding this comment

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

Same as the unrelated change above, I can't tell if this is only a whitespace change, but either way it needs to not be in the feature commit. If the change is indeed justified, it can be included as a second commit.

#define ADDON_INSTANCE_VERSION_GAME "2.1.0"
#define ADDON_INSTANCE_VERSION_GAME_MIN "2.1.0"
#define ADDON_INSTANCE_VERSION_GAME "4.0.0"
#define ADDON_INSTANCE_VERSION_GAME_MIN "4.0.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Just a note, we'll play version games when we upstream the savestate manager and achievements, but keep it like this for now.

@garbear
Copy link
Owner

garbear commented Oct 12, 2021

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

@garbear
Copy link
Owner

garbear commented Jan 30, 2023

@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
Copy link
Owner

garbear commented Jan 30, 2023

The rebased work is here: 6116b66

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

@Shardul555
Copy link
Author

Shardul555 commented Jan 31, 2023 via email

@garbear garbear changed the title RetroPlayer: Add Achievements [GSoC 2021] RetroPlayer: Add Achievements May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants