Add support for async saving by ac-branislav · Pull Request #146 · sinbad/SPUD

ac-branislav · 2026-03-13T03:44:13Z

I added support for async saving and also implemented passing the user index correctly through all the functions since that is needed to correctly support the unreal async save flow.

One thing I wasnt too sure is that instead of adding another function to do SaveGameAsync, I ended up passing a flag through and than adding a new state that we are doing SavingAsync so we can not duplicate code but i'm open to either solution whatever you prefer.

This fixes issue #142

I tested all of this inside the spud sample project, and a warning that you will need to fix the widget blueprint since there is a new parameter passed to the save game complete function which is the user index

@krojew since you wanted this.

I was on vacation last few days, so finally got around to pushing this through)
I have been using this on PS5 for the last maybe 9 months, so it definitely works on consoles as well

krojew · 2026-03-13T06:52:39Z

Looks good in general, although it's a breaking change (code, not the save games). One problem that I see is that it requires the save game system, which is explicitly disabled on desktop machines. Thus, the async save will only work on consoles, while the intent is to have it available, when possible. Should we use the save game system everywhere? Or maybe, make an alternative async path when not used? @sinbad thoughts?

ac-branislav · 2026-03-13T15:50:47Z

Which part of it is a breaking change? Async does not only work on consoles, we use it on both pc and ps5 and it works perfectly fine.

I'm actually not sure why the non-save game system version exists when the unreal engine expects you to use their save game system and it works perfectly fine. The last commit which disabled this for PC in my mind was going backwards, and I would argue that it should be using the save game system by default and only people who want to use the non-save game system can do so but I will leave that up to @sinbad .

In our game we use save game system for all platforms and it works great (been doing it like this for a few years)

sinbad · 2026-03-13T16:02:16Z

I'm travelling for a funeral right now so won't be able to review this until mid-late next week.

krojew · 2026-03-13T16:09:00Z

Which part of it is a breaking change? Async does not only work on consoles, we use it on both pc and ps5 and it works perfectly fine.

You made changes to public API. Since the save games themselves are unchanged, this might be ok, but it's @sinbad decision.

I'm actually not sure why the non-save game system version exists when the unreal engine expects you to use their save game system and it works perfectly fine. The last commit which disabled this for PC in my mind was going backwards, and I would argue that it should be using the save game system by default and only people who want to use the non-save game system can do so but I will leave that up to @sinbad .

To be honest - I'm wondering that too. I don't know why the save system is disabled for PC by default.

ac-branislav · 2026-03-13T17:54:54Z

I wasn't aware that we can't change the headers, I was under the impression that we can change them since its not touching any of the internal api (which we have made a bunch of changes on our side to extend it to support saving a few more things that dont work out of the box, but I removed that from my commit).

Either way is good for me, i'm not picky about it, once sinbad is back we can proceed

sinbad

Right I'm back. I'm afraid that @krojew is correct, changing the public API like that (inserting a parameter before existing arguments) is not something we should do, it'll break everyone else's code. Your options are:

Add new public functions instead, and re-route the internal implementations so they use a shared expanded internal API, or
Always add new arguments to existing public APIs after any existing arguments, and give them defaults so they're optional

You did 2 with the bAsync argument but you added UserIndex in front of existing arguments, which will break people's code. Moving UserIndex to the end of the arguments list and giving it a default like INDEX_NONE would probably resolve this.

sinbad · 2026-03-19T11:02:48Z

Source/SPUD/Public/SpudSubsystem.h


 DECLARE_DYNAMIC_MULTICAST_DELEGATE_OneParam(FSpudPreLoadGame, const FString&, SlotName);
-DECLARE_DYNAMIC_MULTICAST_DELEGATE_TwoParams(FSpudPostLoadGame, const FString&, SlotName, bool, bSuccess);
+DECLARE_DYNAMIC_MULTICAST_DELEGATE_ThreeParams(FSpudPostLoadGame, const FString&, SlotName, const int32, UserIndex, bool, bSuccess);


This will break people's code. Please add a new delegate instead

Is it ok to just add it to the end?

In this case no, because it's a delegate. Any changes to the signature will break everyone's code, it has to be a new delegate if you want to add a parameter to it.

sinbad · 2026-03-19T11:03:05Z

Source/SPUD/Public/SpudSubsystem.h

+DECLARE_DYNAMIC_MULTICAST_DELEGATE_ThreeParams(FSpudPostLoadGame, const FString&, SlotName, const int32, UserIndex, bool, bSuccess);
 DECLARE_DYNAMIC_MULTICAST_DELEGATE_OneParam(FSpudPreSaveGame, const FString&, SlotName);
-DECLARE_DYNAMIC_MULTICAST_DELEGATE_TwoParams(FSpudPostSaveGame, const FString&, SlotName, bool, bSuccess);
+DECLARE_DYNAMIC_MULTICAST_DELEGATE_ThreeParams(FSpudPostSaveGame, const FString&, SlotName, const int32, UserIndex, bool, bSuccess);


Ditto, this is a breaking change. A new delegate is needed to avoid this.

Also i can add it to the end?

Unfortunately not, see previous

What do I name this new delegate? FSpudPostSaveGameAsync? Slightly confused, since it iwll make it a bit more complicated

That would probably be fine. But now I'm wondering why you need this index at all? Surely you're not going to have more than one load or save request outstanding at any one time? Even when they're async you're surely not going to want to perform any other load or save until the last one is complete?

The index is mostly because the async loading system requires the user index and we need to pass it through correctly in all of the instances so at least its consistent but even so it feels a bit weird to add a new delegate versus just not passing the index at all if its a breaking change

Yeah I really don't think the index is needed. You could simply reject additional requests to save/load while an async save/load is in progress.

There is an edge case when split screen can have per-player save, but I've never seen something that convoluted.

Split screen setups usually share the same world so I don't think that's a likely case.

sinbad · 2026-03-19T11:03:39Z

Source/SPUD/Public/SpudSubsystem.h

 	 **/
 	UFUNCTION(BlueprintCallable, BlueprintAuthorityOnly)
-    void QuickSaveGame(FText Title = FText(), bool bTakeScreenshot = true, const USpudCustomSaveInfo* ExtraInfo = nullptr);
+    void QuickSaveGame(FText Title = FText(), const int32 UserIndex = 0, bool bTakeScreenshot = true, const USpudCustomSaveInfo* ExtraInfo = nullptr);


Please move UserIndex to the end of the argument list so as not to break existing code

sinbad · 2026-03-19T11:03:57Z

Source/SPUD/Public/SpudSubsystem.h

 	**/
 	UFUNCTION(BlueprintCallable, BlueprintAuthorityOnly)
-    void AutoSaveGame(FText Title = FText(), bool bTakeScreenshot = true, const USpudCustomSaveInfo* ExtraInfo = nullptr);
+    void AutoSaveGame(FText Title = FText(), const int32 UserIndex = 0, bool bTakeScreenshot = true, const USpudCustomSaveInfo* ExtraInfo = nullptr);


Please move UserIndex to the end of the argument list so as not to break existing code. Or alternatively introduce a new function instead.

sinbad · 2026-03-19T11:04:39Z

Source/SPUD/Public/SpudSubsystem.h

 	 */
 	UFUNCTION(BlueprintCallable, BlueprintAuthorityOnly)
-    void QuickLoadGame(const FString& TravelOptions = FString(TEXT("")));
+    void QuickLoadGame(const int32 UserIndex = 0, const FString& TravelOptions = FString(TEXT("")));


Same, UserIndex should not be inserted before existing params, it's API breaking.

sinbad · 2026-03-19T11:17:40Z

Source/SPUD/Public/SpudSubsystem.h

 	/// Get info about the quick save game, may return null if none
 	UFUNCTION(BlueprintCallable, BlueprintAuthorityOnly)
-    USpudSaveGameInfo* GetQuickSaveGame();
+    USpudSaveGameInfo* GetQuickSaveGame(const int32 UserIndex = 0);


I recommend INDEX_NONE instead of 0 as a default.

sinbad · 2026-03-19T11:17:44Z

Source/SPUD/Public/SpudSubsystem.h

 	/// Get info about the auto save game, may return null if none
 	UFUNCTION(BlueprintCallable, BlueprintAuthorityOnly)
-    USpudSaveGameInfo* GetAutoSaveGame();
+    USpudSaveGameInfo* GetAutoSaveGame(const int32 UserIndex = 0);


I recommend INDEX_NONE instead of 0 as a default.

sinbad · 2026-03-19T11:17:49Z

Source/SPUD/Public/SpudSubsystem.h

 	/// Get information about a specific save game slot
 	UFUNCTION(BlueprintCallable, BlueprintAuthorityOnly)
-	USpudSaveGameInfo* GetSaveGameInfo(const FString& SlotName);
+	USpudSaveGameInfo* GetSaveGameInfo(const FString& SlotName, const int32 UserIndex = 0);


I recommend INDEX_NONE instead of 0 as a default.

sinbad · 2026-03-19T11:18:12Z

Source/SPUD/Public/SpudSubsystem.h

 	 */
 	UFUNCTION(BlueprintCallable, meta=(Latent, LatentInfo = "LatentInfo"), Category="SPUD")
-	void UpgradeAllSaveGames(bool bUpgradeEvenIfNoUserDataModelVersionDifferences, FSpudUpgradeSaveDelegate SaveNeedsUpgradingCallback, FLatentActionInfo LatentInfo);
+	void UpgradeAllSaveGames(bool bUpgradeEvenIfNoUserDataModelVersionDifferences, FSpudUpgradeSaveDelegate SaveNeedsUpgradingCallback, FLatentActionInfo LatentInfo, const int32 UserIndex);


This will break other people's code. Please default UserIndex to INDEX_NONE.

sinbad · 2026-03-19T11:18:21Z

Source/SPUD/Public/SpudSubsystem.h

 	static FString GetSaveGameFilePath(const FString& SlotName);
 	// Lists saves: note that this is only the filenames, not the directory
-	static void ListSaveGameFiles(TArray<FString>& OutSaveFileList);
+	static void ListSaveGameFiles(TArray<FString>& OutSaveFileList, const int32 UserIndex);


This will break other people's code. Please default UserIndex to INDEX_NONE.

sinbad · 2026-03-19T11:22:18Z

As for why the SaveGameSystem is not used all the time, it's because it requires that the entire save is built in memory before being saved. When not using the SaveGameSystem, the data can be streamed directly to disk instead. An argument could be made that to simplify things we could use the SaveGameSystem always at the expense of some additional memory overhead. I'm probably OK with that if you want to do it.

krojew · 2026-03-19T11:43:27Z

I suggest moving to the save game system completely. Having proper async support is far more important.

ac-branislav · 2026-03-19T15:19:06Z

Yah I wasn't too sure if I should add all of the new parameters at the end of the function so I can fix that up.

I can make the async path the default since we all seem to agree with it.

sinbad · 2026-03-19T15:27:54Z

I can make the async path the default since we all seem to agree with it.

I'm fine with making the SaveGameSystem path the default, but still default to synchronous, otherwise that's a behaviour change that will break people's previous usage. Async behaviour should be opt-in.

Add support for async saving

31ebd25

sinbad requested changes Mar 19, 2026

View reviewed changes

Conversation

ac-branislav commented Mar 13, 2026

Uh oh!

krojew commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ac-branislav commented Mar 13, 2026

Uh oh!

sinbad commented Mar 13, 2026

Uh oh!

krojew commented Mar 13, 2026

Uh oh!

ac-branislav commented Mar 13, 2026

Uh oh!

sinbad left a comment

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!

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!

sinbad commented Mar 19, 2026

Uh oh!

krojew commented Mar 19, 2026

Uh oh!

ac-branislav commented Mar 19, 2026

Uh oh!

sinbad commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krojew commented Mar 13, 2026 •

edited

Loading