Added support for saving and loading directly into savestate slots 0 through 9

[kimimaru4000]

Guidelines

1. Rebase before opening a pull request
2. If you are sending several unrelated fixes or features, use a branch and a separate pull request for each
3. If possible try squashing everything in a single commit. This is particularly beneficial in the case of feature merges since it allows easy bisecting when a problem arises
4. RetroArch codebase follows C89 coding rules for portability across many old platforms check using C89_BUILD=1

Description

This is a cleaner PR for #13024, as it's been many years since that was last opened. All the changes have been updated and are in one commit.

This PR adds new hotkeys for directly saving and loading into save states.

• The savestate_max_direct_slot setting determines the highest slot that can be used with the hotkeys. For example, if this value is 1, hotkeys for directly saving and loading slots 0 and 1 can be used. This defaults to -1, which disables the feature.
• Similarly, the options and bindings for the direct slots won't show up in the Hotkeys menu if the slot is greater than savestate_max_direct_slot.
• None of the new hotkeys are binded by default.

I've been maintaining and testing this feature for years, and it's been stable and performant.

Related Issues

#12990

Related Pull Requests

[Any other PRs from related repositories that might be needed for this pull request to work]

Reviewers

@LibretroAdmin

[kimimaru4000]

Hi @LibretroAdmin can you please have a look when you have time and let me know what you think? I will fix the new conflict that popped up.

[kimimaru4000]

I've fixed the conflicts. May I ask @Splaser @LibretroAdmin @fpscan or another contributor to have a look and inform me if this is good to merge?

[hizzlekizzle]

Hey, sorry for the delay, and thanks again for rebasing on the new codebase. We're looking at it right now.

[Splaser]

Thanks for the ping. I can take a look, mainly from the Android/runtime behavior side.

One thing I noticed during an initial read is that the direct slot number is passed through (void *)num and later converted back with (int)data. This will probably work on the common targets, but integer/pointer round-tripping is implementation-defined and may be worth avoiding given RetroArch's portability requirements.

I also want to verify whether using a direct-slot hotkey is intended to permanently change settings->ints.state_slot, or whether the previously selected slot should be restored after the save/load operation.

[kimimaru4000]

Thanks for the feedback! Nice catch about the permanent state_slot change. I had a look at other emulators with this feature, and they do preserve the current selected slot, so I will change it to do the same.

From your perspective on the Android side, what do you think may be a better way to pass in this data? I initially had 10 different methods to handle this that bypassed the casts, but I was able to condense it down to 1 by passing in the state number using the available data slot. Would conversion to (char *) then back to int be more portable?

[kimimaru4000]

I looked into preserving the selected slot, and while it functions perfectly, it's non-trivial to fix the OSD, which incorrectly mentions the preserved slot when saving. This is due to related parts of the code referencing the savestate slot setting rather than passing in the state number.

Therefore, I would prefer to keep the behaviour as-is and address that separately in another PR.

[Splaser]

Thanks for looking into it.

Converting through char * would not make the integer-to-pointer round trip more portable; it would still be encoding a small integer value as a pointer.

A conservative option would be to pass the address of an object with static lifetime, for example a static array containing the slot values, and then read it back as an int:

static const int direct_state_slots[10] = {
   0, 1, 2, 3, 4, 5, 6, 7, 8, 9
};

/* Pass: */
&direct_state_slots[num]

/* Receive: */
const int slot = *(const int *)data;

That avoids integer/pointer conversion entirely and should remain straightforward across platforms. intptr_t would also express the intent better than a direct cast, but the static-object approach may be safer for RetroArch's older portability targets.

Regarding preservation of the selected slot, I agree that fixing the OSD and related call paths would expand the scope of this PR. Keeping the current behavior for now and addressing preservation separately sounds reasonable. It may just be worth documenting that a direct-slot hotkey also changes the currently selected slot.

[kimimaru4000]

Thank you @Splaser. I just pushed up a commit with the changes. I tried to find the best place to define the array but could not, so I just added it to runloop.c. Please let me know if there's a more suitable location. I also documented the selected savestate slot behaviour.

I'm unsure why some of the CI is failing. All I see is "Error:". I can at least confirm that it compiles on my machine (Linux x86_64) and the feature works the same after the compatibility changes.

[sonninnos]

Duplicating this 10 times is bad since they will all need to be translated too, which means more pointless bloat and maintenance.. Rather use sprintf magic for changing the number dynamically.

Also I say that this direct slot hotkey should not change the selected slot.

[kimimaru4000]

Thanks for the feedback. I'm working on having it preserved the selected slot. C is not my primary language, so may I ask for more clarification on the method you have in mind regarding sprintf?

For example, which buffer the string would be stored in and whether it would change the string when hovering over the menu or at compile time. Different languages would have the number in a different location in the translated string - would we be looking for some token to replace with the number?

[sonninnos]

See MSG_LOADED_STATE_FROM_SLOT for example.

[Splaser]

Thank you @Splaser. I just pushed up a commit with the changes. I tried to find the best place to define the array but could not, so I just added it to runloop.c. Please let me know if there's a more suitable location. I also documented the selected savestate slot behaviour.

I'm unsure why some of the CI is failing. All I see is "Error:". I can at least confirm that it compiles on my machine (Linux x86_64) and the feature works the same after the compatibility changes.

Thanks for updating this. The static array approach looks fine to me, and since it is currently only used by the runloop hotkey dispatch, keeping it in runloop.c seems reasonable. A maintainer may have a preferred location, but I do not think this needs to block the change.

The CI failure may be caused by the new declarations appearing directly after the case labels. RetroArch's C89 checks may reject that form. Wrapping each case body in its own block should avoid it:

case CMD_EVENT_LOAD_STATE_SLOT:
{
   const int load_slot = *(const int *)data;
   ...
   break;
}

The same would apply to CMD_EVENT_SAVE_STATE_SLOT.

I also saw the broader feedback from @sonninnos regarding the translated labels and preserving the selected slot, so I will defer to you and the maintainers on those design decisions.