Set Game metadata #21286

NikosSiak · 2022-04-19T16:34:19Z

Description

This PR extends the Python API to support game metadata, like title, developer, publisher and captions (from #20913)

Motivation and context

How has this been tested?

I have wrote a small addon that uses the API to update the discord rich presence status displaying what game you are playing and what you are doing in the game at this moment https://github.com/NikosSiak/service.games.discord.richpresence

What is the effect on users?

Screenshots (if appropriate):

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

lrusak · 2022-04-19T20:56:40Z

xbmc/cores/RetroPlayer/RetroPlayer.cpp

-  : IPlayer(callback), m_gameServices(CServiceBroker::GetGameServices())
+  : IPlayer(callback),
+    m_gameServices(CServiceBroker::GetGameServices()),
+    m_fileItem(new CFileItem())


Suggested change

m_fileItem(new CFileItem())

m_fileItem(std::make_shared<CFileItem>())

lrusak · 2022-04-19T20:59:06Z

xbmc/cores/RetroPlayer/RetroPlayer.h

@@ -136,6 +144,9 @@ class CRetroPlayer : public IPlayer,

  // Synchronization parameters
  CCriticalSection m_mutex;
+
+  // File metadata
+  std::shared_ptr<CFileItem> m_fileItem;


I'm not sure this actually needs to be a shared_ptr when looking at its usage.

garbear · 2022-04-20T06:32:49Z

xbmc/interfaces/legacy/Player.cpp

+    {
+      XBMC_TRACE;
+      if (!g_application.GetAppPlayer().IsPlayingGame())
+        throw PlayerException("XBMC is not playing any game file");


Should probably change XBMC -> KODI

If you want it 100% correct, you want it to be CCompileInfo::GetAppName().
The question though: Is it really needed to specifiy our application name in such an exception.

garbear · 2022-04-20T06:34:31Z

xbmc/interfaces/legacy/InfoTagGame.cpp

@@ -128,6 +133,12 @@ void InfoTagGame::setGameClient(const String& gameClient)
  setGameClientRaw(infoTag, gameClient);
 }

+void InfoTagGame::setCaption(const String& gameClient)


Should this parameter be caption instead of gameClient?

enen92 · 2022-04-20T08:39:11Z

xbmc/Application.cpp

@@ -2252,6 +2252,19 @@ void CApplication::OnApplicationMessage(ThreadMessage* pMsg)
  }
  break;

+  case TMSG_SET_PLAYER_ITEM:


it seems really odd that some component (in this case retroplayer) sends some thread message to the application so that it resets its internal state. Can't this be done in some other way, for example by extending the IPlayerCallback interface? This has been the cause for a lot of bugs regarding state inconsistency between the app and the player (in json-rpc for example).

I took a look into the python addon and you seem to follow some polling approach in the service. If callbacks are used instead you could just react to the OnSomething event on the xbmc.Player() object

I wonder if this is needed at all.
Retroplayer calls OnPlaybackStarted just as VideoPlayer or any other player.
CApplications OnPlayBackStarted handler sets m_itemCurrentFile already.

Also CServiceBroker::GetGUI()->GetInfoManager().SetCurrentItem(*m_itemCurrentFile); is done in in
GUI_MSG_PLAYBACK_STARTED of CApplication (and this message is sent at the end of the OnPlaybackStarted callback handler, so this code ultimately runs already as well).

My take: If it does not work without this new message, you should probably find out the root cause and find a better solution to your problem.

My guess is that maybe during playback the item changes without restarting the player (some video game level or something similar? Probably just the game infotag with some level caption judging by the discord screenshot) and you get to a point where there is state inconsistency between the item the app holds and the item the player holds. That's why my suggestion was to extend the player callback intertface with some other event that represents what happens here if this is really needed. Basically following the same approach that already takes place OnPlaybackStarted

Not sure how the item would change without a call to OpenFile.

The item tags change without opening an other file, every time the game saves a caption is generated and it might change depending your progress

It's worth thinking that maybe we took the wrong approach. The need is to pass the caption to Python. We use the player API and pass game info tags of the currently-playing item. The advantage of going through the player is that we don't need to worry about modifying the Python API. But maybe, to avoid the loop mentioned in the comment added to CRetroPlayer, we extend the player API?

It's worth noting that an item not changing without OpenFile isn't necessarily a current assumption - recall all the dyn stuff. This change just solidifies that items can change, and exposes a mechanism to keep changes in sync throughout the system.

enen92 · 2022-04-20T08:41:13Z

xbmc/interfaces/legacy/Player.h

+      /// @brief \python_func{ isPlayingGame() }
+      /// Check for playing game.
+      ///
+      /// @return                    True if kodi is playing a game


missing doxygen python revision tags

enen92 · 2022-04-20T08:41:26Z

xbmc/interfaces/legacy/Player.h

+      /// @return                    Game info tag
+      /// @throws Exception          If player is not playing a file or current
+      ///                            file is not a game file.
+      ///


same on this one

enen92 · 2022-04-20T09:16:01Z

xbmc/cores/RetroPlayer/cheevos/Cheevos.cpp

 constexpr auto RICH_PRESENCE = "RichPresencePatch";
+constexpr auto GAME_TITLE = "Title";


would be nice to avoid the auto keyword here. What is the type of those vars? I assume they are cstrings / char*
There seems to be a recent "preference" for std::string_view instead

jenkins4kodi · 2022-10-02T17:21:08Z

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

For more information please see our current code style guidelines.

jenkins4kodi · 2022-10-15T20:32:19Z

@NikosSiak this needs a rebase

garbear · 2023-03-25T23:07:12Z

I've split this PR in two and submitted the first half upstream: #23054

garbear · 2023-04-03T05:35:21Z

@NikosSiak Thanks to the work here, I was able to extract and upstream most of the code: #23054

On top of that, I rebased on that had another go at this PR. Check out: https://github.com/garbear/xbmc/commits/update-discord

As you can see, we're down to the bare minimum needed to get caption in the game metadata. At least this should let the discord plugin work in my test builds.

However, I don't think game infotags are the right way to get the caption to Python. It should probably be in a player-related API. I'll leave it up to you if you want to pursue the Discord feature (I'd certainly like to use it).

NikosSiak requested a review from garbear as a code owner April 19, 2022 16:34

NikosSiak added API change: Python v20 Nexus labels Apr 19, 2022

NikosSiak added this to the Nexus 20.0 Alpha 1 milestone Apr 19, 2022

NikosSiak force-pushed the game-api branch from b57c382 to b1529dc Compare April 19, 2022 18:36

lrusak reviewed Apr 19, 2022

View reviewed changes

garbear reviewed Apr 20, 2022

View reviewed changes

enen92 reviewed Apr 20, 2022

View reviewed changes

NikosSiak mentioned this pull request May 1, 2022

[service.games.discord.richpresence] 1.0.0 xbmc/repo-scripts#2267

Open

3 tasks

fuzzard modified the milestones: Nexus 20.0 Alpha 1, Nexus 20.0 Alpha 2 May 15, 2022

fuzzard modified the milestones: Nexus 20.0 Alpha 2, Nexus 20.0 Alpha 3 Jul 8, 2022

fuzzard modified the milestones: Nexus 20.0 Alpha 3, O* 21.0 Alpha 1 Sep 8, 2022

fuzzard added v21 Omega and removed v20 Nexus labels Sep 8, 2022

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Sep 22, 2022

NikosSiak added 2 commits October 2, 2022 19:27

Set Game metadata

7434d64

fixup! Set Game metadata

42436ee

NikosSiak force-pushed the game-api branch from b1529dc to 42436ee Compare October 2, 2022 16:29

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 2, 2022

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 15, 2022

garbear mentioned this pull request Mar 25, 2023

Python API: Add missing gameinfo to Player class #23054

Merged

7 tasks

garbear self-assigned this Apr 3, 2023

fuzzard modified the milestones: Omega 21.0 Alpha 1, Omega 21.0 Alpha 2 Apr 14, 2023

fuzzard modified the milestones: Omega 21.0 Alpha 2, Omega 21.0 Alpha 3 Jun 7, 2023

fuzzard removed the v21 Omega label Sep 1, 2023

fuzzard removed this from the Omega 21.0 Alpha 3 milestone Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set Game metadata #21286

Set Game metadata #21286

NikosSiak commented Apr 19, 2022

lrusak Apr 19, 2022

lrusak Apr 19, 2022

garbear Apr 20, 2022

a1rwulf Apr 20, 2022 •

edited

garbear Apr 20, 2022

enen92 Apr 20, 2022 •

edited

a1rwulf Apr 20, 2022

a1rwulf Apr 20, 2022

enen92 Apr 20, 2022 •

edited

a1rwulf Apr 20, 2022

NikosSiak May 1, 2022

garbear May 1, 2022 •

edited

enen92 Apr 20, 2022

enen92 Apr 20, 2022

enen92 Apr 20, 2022

jenkins4kodi commented Oct 2, 2022

jenkins4kodi commented Oct 15, 2022

garbear commented Mar 25, 2023

garbear commented Apr 3, 2023

	m_fileItem(new CFileItem())
	m_fileItem(std::make_shared<CFileItem>())

		constexpr auto RICH_PRESENCE = "RichPresencePatch";
		constexpr auto GAME_TITLE = "Title";

Set Game metadata #21286

Are you sure you want to change the base?

Set Game metadata #21286

Conversation

NikosSiak commented Apr 19, 2022

Description

Motivation and context

How has this been tested?

What is the effect on users?

Screenshots (if appropriate):

Types of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a1rwulf Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enen92 Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enen92 Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garbear May 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenkins4kodi commented Oct 2, 2022

jenkins4kodi commented Oct 15, 2022

garbear commented Mar 25, 2023

garbear commented Apr 3, 2023

a1rwulf Apr 20, 2022 •

edited

enen92 Apr 20, 2022 •

edited

enen92 Apr 20, 2022 •

edited

garbear May 1, 2022 •

edited