Adding support for Achievements in RetroPlayer by Shardul555 · Pull Request #1 · NikosSiak/game.libretro

Shardul555 · 2021-07-17T18:03:07Z

I have changed client.cpp, client.h, cheevos.cpp, cheevo.h for the Achievement feature till now, recent commit have a log debug command for testing whether activate achievement function will be called successfully from xbmc code or not.

NikosSiak · 2021-08-08T09:22:29Z

                                   const char* richPresence)
 {
+  m_username = username;
+  m_token = token;


same here, you are keeping a reference to a kodi pointer, also I believe it's better to add a new method SetRetroAchievementsCredentials(const char* username, const char* token) and call it once and then use the instance variables instead of passing username and token each time a post is happening and setting the value again

How can we limit the function call to take place only once, I am not sure of it.
I can check if m_username and m_token is NULL, if yes then I will call this function, will it be fine?

you will call the new function once when you activate achievements for example here. you wont limit the calls to this function, you will remove 2 arguments an 2 assignments

Okay so I have done it now through a little different way which I will show you in a day or two

NikosSiak · 2021-08-08T09:24:39Z


+void CCheevos::ActivateAchievement(unsigned cheevo_id, const char* memaddr) //1
+{
+  int res = rc_runtime_activate_achievement(&m_runtime, cheevo_id, memaddr, NULL, 0);


you could make this method to return bool and do something like the other methods

Suggested change

int res = rc_runtime_activate_achievement(&m_runtime, cheevo_id, memaddr, NULL, 0);

return rc_runtime_activate_achievement(&m_runtime, cheevo_id, memaddr, NULL, 0) == 0;

NikosSiak · 2021-08-08T09:27:06Z

+                                const char* username,
+                                const char* token,


again here you can use the instance variables instead of getting them as parameters

NikosSiak · 2021-08-08T09:30:39Z

+bool CCheevos::GetCheevo_ID(void (*Callback)(char* achievement_url, unsigned cheevo_id))
+{
+  //kodi::Log(ADDON_LOG_ERROR, "Fine till nowwwww in Callback");
+  Callback(url, m_cheevo_id);
+  return true;
+}


this method isn't used anywhere and the callback here isn't needed you could just return the m_cheevo_id the point of the callback was to be called when the cheevo id is set and not call a function each frame from kodi. Here you just pass a callback as an argument and you immediately call it which doesn't help anywhere you could just return the url

NikosSiak · 2021-08-08T10:08:03Z


  m_client.retro_run();
-
+  CCheevos::Get().TestAchievementPerFrame();


minor: add an empty line before and after

NikosSiak · 2021-08-08T10:10:03Z

+  CCheevos::Get().ActivateAchievement(cheevo_id, memaddr);
+  return GAME_ERROR_NO_ERROR;


you should change ActivateAchievement to return a boolean value and return GAME_ERROR_FAILED if it returns false like the other methods

NikosSiak · 2021-08-08T10:11:00Z

+  {
+    return GAME_ERROR_FAILED;
+  }
+  return GAME_ERROR_NO_ERROR();


GAME_ERROR_NO_ERROR is not a function, you should remove the ()

NikosSiak · 2021-08-08T10:29:26Z

+  CCheevos::Get().DeactivateTriggeredAchievement(cheevo_id);
+  return GAME_ERROR_NO_ERROR;


same here, return GAME_ERROR_FAILED if DeactivateTriggeredAchievement returns false

I think there is no need of this function in client.cpp so I will remove it, as it is called from cheevos.cpp and used there only

NikosSiak · 2021-08-08T10:29:56Z

+  return true;
+}
+
+void CCheevos::DeactivateTriggeredAchievement(unsigned cheevo_id)


you could make this method to return bool and do something like the other methods

gusandrianos

lgtm 👍

Btw, please rewrite the commit history in a meaningful way.
You can use interactive rebase. You can also do git reset 693d4aa22386e594bf39404573d155af6498392f && git reset HEAD~ which will delete all the commits (it will not delete the changes you made) and then you can add all the files and do git commit -m "RetroPlayer achievements support", for example

gusandrianos · 2021-08-20T07:45:28Z

+void CCheevos::ActivateAchievement(unsigned cheevo_id, const char* memaddr)
+{
+  rc_runtime_activate_achievement(&m_runtime, cheevo_id, memaddr, NULL, 0);
+  // it will return integer value 0 in case achivement is activated successfully.


rc_runtime_activate_achievement will return 0 if the achievement is activated successfully but the method ActivateAchievement returns void?

Yes, I will make these changes after rebasing and adding new commits

gusandrianos · 2021-08-20T07:50:03Z

+{
+  CCheevos::Get().ActivateAchievement(cheevo_id, memaddr);
+
+  return GAME_ERROR_NO_ERROR;


Is this supposed to return GAME_ERROR_NO_ERROR at all times? What if something happens and CCheevos::Get().ActivateAchievement (which returns void) fails? If we don't want to handle the errors at all for now, that's fine.

gusandrianos · 2021-08-20T07:52:33Z

+GAME_ERROR CGameLibRetro::GetCheevo_URL_ID(void (*Callback)(const char* achievement_url,
+                                                            unsigned cheevo_id))
+{
+  CCheevos::Get().GetCheevo_URL_ID(Callback);


Same here, if GetCheevo_URL_ID fails, we never know it because it returns void. Again, if it's not essential to handle errors around this for now, it's fine.

NikosSiak and others added 11 commits October 18, 2020 12:34

set memory maps

5be93fb

Added rcheevos dependency

8216c36

Rcheevos evaluate rich presence

54005eb

Rcheevos expose hash and url functions

9344d27

RCheevos expose post rc_url_ping function

1be43d1

fix crash when cheevos is deleted

f0255c7

Added comment to the base file of RetroArch

c2caf68

Reset rc_runtime

af8185d

Fix compiler error and refactor Peek()

4d041e9

Guard against uninitialized variable

26d65db

Cheevos: Implement hrash VFS API

940016b

Shardul555 force-pushed the gsoc2021_cheevo branch from 9a4a4a8 to 693d4aa Compare August 4, 2021 18:13

NikosSiak reviewed Aug 8, 2021

View reviewed changes

Shardul555 changed the title ~~PR for debugging the changes done for Achievements portion~~ Adding support for Achievements in RetroPlayer Aug 17, 2021

gusandrianos approved these changes Aug 20, 2021

View reviewed changes

Achievement support for RetroPlayer

5e2f992

Shardul555 force-pushed the gsoc2021_cheevo branch from 45fb977 to 5e2f992 Compare August 20, 2021 17:39

Support for function call from Kodi for Implementing Achievements

214cf1b

NikosSiak force-pushed the rcheevos branch 2 times, most recently from 2554122 to 40ada13 Compare November 8, 2021 16:41

NikosSiak force-pushed the rcheevos branch 3 times, most recently from d54d5a6 to 19efb51 Compare January 27, 2022 18:20

NikosSiak force-pushed the rcheevos branch 4 times, most recently from 0a3df37 to 5b8d4c9 Compare February 20, 2022 14:15

NikosSiak force-pushed the rcheevos branch from 5b8d4c9 to 2f97ff9 Compare March 5, 2022 09:53

NikosSiak force-pushed the rcheevos branch 2 times, most recently from 2fd03e8 to 262c7b3 Compare March 20, 2022 11:55

NikosSiak force-pushed the rcheevos branch from 262c7b3 to 2f7bb2b Compare March 24, 2022 09:45

	int res = rc_runtime_activate_achievement(&m_runtime, cheevo_id, memaddr, NULL, 0);
	return rc_runtime_activate_achievement(&m_runtime, cheevo_id, memaddr, NULL, 0) == 0;


		m_client.retro_run();

		CCheevos::Get().TestAchievementPerFrame();

		CCheevos::Get().ActivateAchievement(cheevo_id, memaddr);
		return GAME_ERROR_NO_ERROR;

		CCheevos::Get().DeactivateTriggeredAchievement(cheevo_id);
		return GAME_ERROR_NO_ERROR;

Conversation

Shardul555 commented Jul 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shardul555 Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikosSiak Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gusandrianos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Shardul555 Aug 8, 2021 •

edited

Loading

NikosSiak Aug 8, 2021 •

edited

Loading

gusandrianos left a comment •

edited

Loading