garbear / xbmc

XBMC Main Repository
http://xbmc.org
Other
132 stars 53 forks source link

RetroPlayer: Savestate manager #120

Closed NikosSiak closed 2 years ago

NikosSiak commented 4 years ago

Description

This PR adds a savestate manager for the RetroPlayer. When the user selects a game instead of prompting him/her to select the game client the savestate manager will prompt him/her to select a savestate. If the user wants to create a new savestate or there are no savestates for the selected game then the game client prompt will be displayed. From the dialog the user can rename a savestate or delete . This PR also adds an in-game savestate manager to quickly change between savestates without exiting the game (this dialog shows only savestates created from the loaded game client). This PR includes the auto captioning feature for savestates using RetroAchievements rich presence evaluation from https://github.com/garbear/xbmc/pull/121. To get the necessary data from the RetroAchievements API the user must login to their account from the game settings. For the auto captioning feature this https://github.com/kodi-game/game.libretro/pull/67 PR must be merged. You can find test builds here

Motivation and Context

Now RetroPlayer saves only one savestate for each game and it saves it next to the rom, this changes store the savestates in a special folder and give the ability to save multiple savestate for one game

How Has This Been Tested?

Screenshots (if appropriate):

Savestate manager image

Context menu image

In game savestate manager image

Types of change

Checklist:

garbear commented 4 years ago

@NikosSiak can you cherry-pick 0a5b7b7b66ec7aad99044d692cec3817b3bf52cb? the order of class members is very specific to manage dependencies between them. Necessary is that all members are destructed in the opposite order as they were constructed. Constructor parameters are first in, lasting longest, so we want m_renderManager to appear in the header right under m_gameClient in the // Construction parameter section. Notice how the order is reflected everywhere these are used together.

garbear commented 4 years ago

Please cherry-pick 36d7b3033d4145b72e95bacaeba6cf10e758c24b and c2216ecb7116432688d3d9869882fdd2bc6331e1. The former not only deduplicates code, but it fulfills the contract of the savestate database to provide fully filled-in CGUIListItems.

EDIT: The former needs some work

NikosSiak commented 4 years ago

Please cherry-pick 36d7b30 and c2216ec. The former not only deduplicates code, but it fulfills the contract of the savestate database to provide fully filled-in CGUIListItems.

EDIT: The former needs some work

I changed this and this to

items[i]->SetLabel(items[i]->m_dateTime.GetAsLocalizedDateTime());

because the time was wrong (probably because of the timezone)

garbear commented 4 years ago

The time returned from that function comes from GetDirectory(), which sets the time to the file creation time (or maybe modification time). Because files can be copied/modified, we probably want the savestate creation time, which is stored in the .sav flatbuffer file.

EDIT: Yeah, see how I set created and then forgot to use the new variable :)

garbear commented 3 years ago

I left some comments on some technical debt I saw. Doesn't need to be paid down before this is merged, but we should make a list of code scalability improvements we could make.

garbear commented 3 years ago

I've created Kodi and LibreELEC test builds based on 18.8 with your PR: https://github.com/garbear/xbmc/releases and https://github.com/kodi-game/LibreELEC.tv/releases/

garbear commented 3 years ago

few minor comments about char arrays, otherwise the new rich presence stuff looks good

garbear commented 3 years ago

Can you add descriptions to new strings using the #. comment style?

garbear commented 3 years ago

This PR introduces a warning on non-windows platforms. See https://jenkins.kodi.tv/job/OSX-64/19334/clang/new/. Just need to make the order of the fields in the header match the initialization order in the .cpp file.

GUIDialogSelect.cpp:30:5: warning: field 'm_buttonLabel' will be initialized after field 'm_selectedItem' [-Wreorder]
     m_buttonLabel(-1),
     ^
garbear commented 2 years ago

Small favor to ask of the comments, can you wrap lines at 80 chars?

garbear commented 2 years ago

Test builds for the final PR: https://github.com/garbear/xbmc/releases/tag/retroplayer-19.3-20220126

garbear commented 2 years ago

PR has been upstreamed: https://github.com/xbmc/xbmc/pull/20913