Allow combining saves in content dir with save sorting #16369
Conversation
| @@ -7956,114 +7958,112 @@ void runloop_path_set_redirect(settings_t *settings, | |||
| runloop_st->runtime_content_path_basename, | |||
| sizeof(content_dir_name)); | |||
|
|
|||
| if (sysinfo && !string_is_empty(sysinfo->library_name)) | |||
There was a problem hiding this comment.
Moved pretty much unmodified below.
|
Any reason for that new string when it will get overwritten at the end anyway? Can't you just make do with the string variable that already exists? I think it would be prudent to not require too many additional extra strings for local stack if we can help it. |
Yes, the codepath on line 8022/8055 where we fall back on failing to create the desired directory. If we don't have the intermediate string, we would fall back to putting the saves in the libretro config directory, when really we should fall back to putting them in the content directory IMO. It's possible to avoid this with less stack memory and more code by trying to create the directory, then if that fails chopping it back down (or redoing the setup). I thought 8k of momentary stack memory usage during setup (not emulator execution) wouldn't be noticed even on extremely constrained targets, but it's your call. |
|
Alright, I guess I shouldn't make a big deal about it. I guess we can merge this then. |
|
One last thing before merge that would be nice - we try to do 3 space indents, without tabs. I think another contributor noticed some mixing of tabs for indents and 4 space indents. If you could change all of that to 3 space indents, then we could merge it. |
|
Okay, I'll update the whitespace in here. Sorry about that. This is a Breaking Change (TM), but I think that it's okay because the old behavior can be restored by turning off the "saves sorted by blah" setting. |
|
I did an autoformat pass, this function is now consistently three-space indents. The code now matches the (very loose) .editorconfig file in the project. Feel free to test+merge if you're okay with this. |
|
Hmm, not that much of a fan of the autoformat pass to be honest, I'd prefer if you just rolled that back that and instead just manuallymade sure that when you do indents, you do it with 3 spaces instead of with tabs or with 4 spaces. LIke the way the variables were formatted previously was deliberate for readability purposes. You can read about these guidelines here |
Pushed a change manually re-aligning the variable assignments. I've had a read through the guidelines and, after I corrected one incorrectly indented following argument from before my change, it looks like this function now complies with them. Personally, I find it easier to be consistent if the computer does it for me via an in-repository clangformat or similar than if I have to remember what each of twenty different software projects chooses, you know? Let me know if there are other problems, formatting or otherwise. |
|
Alright, it's good to go now. Sorry for putting you through this ordeal, it might seem nitpicky but I'd figure it was best to just ask you directly to make these changes rather than me making manual edits afterwards. |
Absolutely no problem, glad to help. I hope it ends up working well for users! |
|
This has been released in retroarch 1.18.0 (currently available as a flatpak on flathub) and appears to work just fine. Save states go into per-core subdirectories of the content root when both options are set. |
* Allow combining saves in content dir with save sorting * Formatting * Realign var assignments
Description
Background: save files are usually compatible between cores, but save states basically never are. Retroarch currently has three settings for each of (savestate, savefile):
Currently, turning on the first option overrides the later two, and save(files|states) will be placed directly into the content directory.
This isn't a great experience because it means that if you put save states into the content directory, and then change the core you're using for the content, you'll get a save state that's invalid for the current core.
I thought it might be better if the options instead stacked. The logic would be:
So if you have content in
/roms, and you use core X and core Y, with both the content-dir and sort-by-core-name options enabled you'll end up with saves in/roms/Xand/roms/Y.To me this is more useful because it allows doing a cross-device sync of the content directory and carrying everything - save files, save states, whatever - while still not risking trying to load an incompatible save state on the wrong core. It feels like this better respects what the user is asking for when they toggle both options.
What do you think? PR attached as an example of one potential way to do this.