Standalone save & persistence improvements by MhaWay · Pull Request #874 · rwmt/Multiplayer

MhaWay · 2026-04-11T15:40:47Z

Summary

Consolidated PR containing all standalone save improvements (previously split across #874-#877):

Save trigger foundation: standalone protocol and session identification, typed autosave and join-point reason plumbing
Snapshot persistence: File.Replace atomic writes, safe struct defaults, single-snapshot autosave
Async reconnect fixes: reset server instance in TearDown, int.TryParse in SeedFromSaveZip
Prepublish maintenance: correct misleading log text in designator patches
Save path unification: unified standalone save path, Days autosave support

Changes

Split standalone save trigger foundation
Unify standalone save path and add Days autosave support
Add standalone snapshot persistence
Add safe defaults to snapshot state structs
Use File.Replace in SaveGameToFile_Overwrite and eliminate double snapshot
Enforce standalone async time and control fixes
Reset MultiplayerServer.instance in TearDown
Use int.TryParse in SeedFromSaveZip
Clean up standalone prepublish maintenance
Correct misleading blocked log text in designator patches

Validation

dotnet build Source/Multiplayer.sln -c Release
dotnet test Source/Tests/Tests.csproj -c Release --no-build

Note

PRs #875, #876, #877 have been closed and consolidated here as requested.

Copilot

Pull request overview

This PR lays the groundwork for a “standalone save trigger” flow by extending the handshake to identify standalone servers and by plumbing a typed autosave/join-point request reason from client to server, so standalone clients can request join point creation for save- and travel-related events.

Changes:

Extend protocol handshake (Server_ProtocolOk) to include whether the server is standalone; persist this on the client session.
Introduce ClientAutosavingPacket + JoinPointRequestReason and route autosave/save/world-travel triggers through it to server-side join point creation.
Adjust standalone-specific join/join-point behavior and add a synthetic autosave timer for standalone connections.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
Source/Common/Networking/Packet/ProtocolPacket.cs	Adds `isStandaloneServer` field to protocol OK packet for client session identification.
Source/Common/Networking/Packet/AutosavingPacket.cs	Introduces typed autosaving packet with a reason enum.
Source/Common/JoinPointRequestReason.cs	Adds enum describing why a join point is requested (save, world travel, etc.).
Source/Common/Networking/State/ServerPlayingState.cs	Updates autosaving handler to typed packet + reason; loosens world upload policy for standalone.
Source/Common/Networking/State/ServerJoiningState.cs	Adjusts join-point creation policy for standalone joins; sends standalone flag during handshake.
Source/Common/PlayerManager.cs	Treats first non-arbiter joiner as “primary” faction on standalone servers.
Source/Client/Networking/State/ClientJoiningState.cs	Stores `isStandaloneServer` from protocol OK on the client session.
Source/Client/Session/MultiplayerSession.cs	Adds session flag + helper property for “connected to standalone server”.
Source/Client/Session/Autosaving.cs	Sends typed autosaving packet after save; changes save routine to return `bool` and tweaks file replacement logic.
Source/Client/Windows/SaveGameWindow.cs	Sends autosaving packet on manual save; special-cases standalone to trigger server-side save flow.
Source/Client/Patches/VTRSyncPatch.cs	On standalone, triggers a join point when leaving a map (world travel).
Source/Client/ConstantTicker.cs	Adds standalone-only synthetic autosave timer.
Source/Client/AsyncTime/AsyncWorldTimeComp.cs	Adjusts join-point creation upload behavior during `CreateJoinPoint`.
Source/Tests/PacketTest.cs	Updates packet roundtrip expectations for the extended protocol OK packet.
Source/Tests/packet-serializations/ServerProtocolOkPacket.verified.txt	Updates verified serialization output for the extended protocol OK packet.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-11T15:48:28Z

    private static void CreateJoinPointAndSendIfHost()
    {
        Multiplayer.session.dataSnapshot = SaveLoad.CreateGameDataSnapshot(SaveLoad.SaveAndReload(), Multiplayer.GameComp.multifaction);

-        if (!TickPatch.Simulating && !Multiplayer.IsReplay &&
-            (Multiplayer.LocalServer != null || Multiplayer.arbiterInstance))
+        if (!TickPatch.Simulating && !Multiplayer.IsReplay)
            SaveLoad.SendGameData(Multiplayer.session.dataSnapshot, true);
    }


CreateJoinPointAndSendIfHost now uploads world data from every non-simulating, non-replay client. On non-standalone servers the server will still reject Client_WorldDataUpload from non-host/non-arbiter, but the upload cost (fragmenting + network/bandwidth + compression work) is still paid by all clients, which can be very expensive for large saves. Please gate SendGameData so only the designated uploader sends (e.g., arbiter/local host, plus standalone clients), or introduce a server-directed "uploader" selection instead of broadcasting uploads from everyone.

Related to my comment #874 (comment)

Copilot · 2026-04-11T15:48:28Z

        {
            Log.Error($"Exception saving multiplayer game as {fileNameNoExtension}: {e}");
            Messages.Message("MpGameSaveFailed".Translate(), MessageTypeDefOf.SilentInput, false);
+            return false;


SaveGameToFile_Overwrite now returns bool and swallows exceptions internally. Callers that relied on exceptions to detect failure (e.g., code wrapping this call in try/catch) will no longer observe failures unless they check the return value, and may continue follow-up steps assuming a save exists. Consider either re-throwing (and letting callers handle) or ensure all call sites are updated to handle the false return explicitly.

Suggested change

return false;

throw;

Copilot · 2026-04-11T15:48:29Z

+                if (!Autosaving.SaveGameToFile_Overwrite(curText, currentReplay))
+                    return;
+
+                Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save));


ClientAutosavingPacket is sent after saving even when currentReplay is true. In replay sessions the connection ignores non-command packets, so this send is a no-op and adds confusing coupling between replay saving and network join-point logic. Consider skipping the send when currentReplay is true (or when Multiplayer.Client is null) to keep replay saves purely local.

Suggested change

Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save));

if (!currentReplay && Multiplayer.Client != null)

Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save));

mibac138 · 2026-04-11T20:01:50Z


-        if (!TickPatch.Simulating && !Multiplayer.IsReplay &&
-            (Multiplayer.LocalServer != null || Multiplayer.arbiterInstance))
+        if (!TickPatch.Simulating && !Multiplayer.IsReplay)


Instead of having every player send the game data back to the server, could you have the server send info which player is expected to send the game data back? WorldData.cs Server.commands.Send(CommandType.CreateJoinPoint, ScheduledCommand.NoFaction, ScheduledCommand.Global, Array.Empty<byte>()); - the Array.Empty could be changed to include playerId or perhaps a value of true could be sent to the desired player and false to the rest?

What do you think about this approach: when a player wants to save, they request a joinpoint from the server, and the server asks a specific player to create it? I'm still leaning towards the idea that the player who triggered the save should be the one doing it for the maps they have loaded — so the server would still delegate it back to the requester. But if there are two players, maybe this server-mediated step would better coordinate who should do it?

Honestly, I'm not sure it adds much in practice — it feels like an extra round-trip for the same result. But I'd like your thoughts on it.

The philosophy I'm following is "each player uploads their own maps." The goal is to avoid a bottleneck where, say, 10 players need to save sequentially through a single designated uploader. On a standalone server there's no natural "authoritative" client like the host in a hosted game, so having each player responsible for their own maps seemed like the most resilient approach.

Currently, the IssuedBySelf gate (which was missing from PR #876+ due to a rebase oversight — now restored) ensures that when a CreateJoinPoint command is broadcast, only the client who originally requested the save actually executes CreateJoinPointAndSendIfHost. The other clients receive the command but skip execution. So the Array.Empty<byte>() payload doesn't need to carry a playerId — the client-side gate already handles it by checking currentExecutingCmdIssuedBySelf.

That said, if you'd prefer making this explicit via the command payload (e.g. embedding the requester's playerId so the server decides who saves), I'm open to it. It would make the intent clearer on the wire, even if the behavior is the same.

I'm still leaning towards the idea that the player who triggered the save should be the one doing it for the maps they have loaded

I'm fine with that, and that's not my issue with this change. The issue is that right now everyone saves every map and everyone sends the world data back to the server.

The philosophy I'm following is "each player uploads their own maps." The goal is to avoid a bottleneck where, say, 10 players need to save sequentially through a single designated uploader. On a standalone server there's no natural "authoritative" client like the host in a hosted game, so having each player responsible for their own maps seemed like the most resilient approach.

That probably is a better approach. Not sure how much of a bottleneck it actually would be. Also, right now there isn't such a functionality to have the player only save their own map. If it exists in another PR of yours, please include that info in this PR's description so I'm aware of the overarching goal and have more context to review on

Currently, the IssuedBySelf gate (which was missing from PR #876 due to a rebase oversight — now restored) ensures that when a CreateJoinPoint command is broadcast, only the client who originally requested the save actually executes CreateJoinPointAndSendIfHost. The other clients receive the command but skip execution. So the Array.Empty() payload doesn't need to carry a playerId — the client-side gate already handles it by checking currentExecutingCmdIssuedBySelf.

I only reviewed this PR and didn't look at the others. If that PR fixes my concern, please move the relevant part to this PR. Also, IssuedBySelf will only work if you send the CreateJoinPoint command with a player provided. Right now, StartJoinPointCreation never sends an associated player

That said, if you'd prefer making this explicit via the command payload (e.g. embedding the requester's playerId so the server decides who saves), I'm open to it. It would make the intent clearer on the wire, even if the behavior is the same.

As long as the IssuedBySelf code you are referencing keeps working in non-standalone mode, I'm fine with just doing that. It's probably even better than my suggestion, so I think I'd even prefer your approach

As soon as i get home ill send you more context, i was working on it.

This PR does not implement map streaming itself, but it keeps the standalone join-point flow compatible with the upcoming streaming architecture. In hosted MP, join-point creation is host/arbiter-owned; in standalone, snapshot creation/upload must already happen client-side, and future streaming work extends that further by allowing the upload work to be split across assigned clients. Some of the standalone branching here is meant to preserve that separation cleanly without changing hosted behavior.

Also i used ur idea about make the server handle who is the one who save for the streaming map one

mibac138 · 2026-04-11T20:02:00Z

+            if (dst.Exists)
+                dst.Delete();
+
+            tmp.MoveTo(dst.FullName);


I'm not particularly against this change, but I'm curious why is this better? Edit: it probably isn't #874 (comment)

Had errors on overwrite tmp, this fixed

Moved to replace

I think you might've missed something in the commit/push? It looks like it's still using move for me

you should see it now

mibac138 · 2026-04-11T20:03:39Z

                    }
+
+                    // On standalone, trigger a join point when leaving a map
+                    if (Multiplayer.session?.ConnectedToStandaloneServer == true)


Isn't it going to be pretty annoying (for the players) to save the game every time anyone changes the map? As far as I remember, the game pauses for everyone during join point creation

Tecnically just for the one saving and for the maps it is on. The idea is to let players save their maps, but im still trying to test

mibac138 · 2026-04-11T20:06:24Z


        private static void TickAutosave()
        {
+            // Only standalone connections use the synthetic autosave timer.


I don't understand this comment. autosaveCounter is also used by other code in this method?

You're right, the "synthetic" wording was confusing. I've rewritten the comment to:

// When connected to a remote standalone server, the client drives // the autosave timer using the interval received at connection time // (from the server's TOML settings via ServerProtocolOkPacket).

The point is: when the client is connected to a remote standalone server (Multiplayer.LocalServer == null), it can't read server.settings directly. So the autosave interval and unit are now sent in the ServerProtocolOkPacket at connection time — the client stores them in session.autosaveInterval / session.autosaveUnit and drives its own timer from those values.

The standalone path now also supports both Minutes and Days (in-game) units, just like the host-local path.

That makes sense that the client doesn't have access to server settings, but why are you sending the autosave settings to the client then? I think it'd be better to have the server run the timer and send a CreateJoinPoint command to the client

Also, I'm not sure what do you mean by "supports both Minute and Days"? This code explicitly only supports minutes (which I'm fine with, I don't think it's critical to support autosaving by Days in the standalone server)

if (session.autosaveUnit != AutosaveUnit.Minutes || session.autosaveInterval <= 0) return;

Umh no, i was using the autosave timing setting from the toml, isn't it using the game days?

mibac138 · 2026-04-11T20:16:54Z

+            var forceJoinPoint = packet.reason == JoinPointRequestReason.Save;
+
+            // On standalone, any playing client can trigger a join point (always, regardless of settings)
+            // On hosted, only the host can trigger and only if the Autosave flag is set
+            if (Server.IsStandaloneServer ||
+                (Player.IsHost && Server.settings.autoJoinPoint.HasFlag(AutoJoinPointFlags.Autosave)))
+                Server.worldData.TryStartJoinPointCreation(forceJoinPoint);


Unless I'm missing something, this change along with the changes to CreateJoinPointAndSendIfHost and HandleWorldDataUpload, mean that anyone can request a join point to be created, which makes everyone on the server upload a joinpoint to the server, and then the server just accepts the world data from every player. I don't think this is a good workflow. I haven't really thought deeply about this, but I think I'd prefer the server to choose one player to send the game data, or in some other way determine a single player to save.

I actually found a todo somewhere about the implementation to load map joinpoints independently and thought it might lighten the loading.. so the idea is that Each player can load their own active maps

The intent here was not to have every client produce and upload a joinpoint. The idea is that only the player who explicitly triggers a save, or another deliberate action tied to their current context, should send the joinpoint for that map.

The main goal is to avoid unnecessary interruptions or loading work for players who are active on separate maps. I tried to keep this as continuity-friendly as possible: if the server arbitrarily designates a client to provide the joinpoint, that creates awkward edge cases where the chosen player may be in the middle of leaving, changing state, or otherwise not be the best source for that save. Tying joinpoint creation to an explicit player action keeps the flow more predictable and reduces the impact on everyone else.

…pshot

…gate

…ignment - CreateJoinPointAndSendIfHost: restore LocalServer/arbiter guard for hosted mode so only host/arbiter uploads world data (was accidentally ungated) - Standalone path kept separate with ConnectedToStandaloneServer gate - Remove redundant ofPlayer assignment in ChangeRealPlayerFaction (FactionContext.Set already does the same thing on the next line)

…oin point - Add no-op HandleKeepAlive to TestJoiningState and TestLoadingKeepAliveState to prevent crash when server sends KeepAlive during async handshake - Disable autoJoinPoint in test server settings since the test server has no game simulation to process CreateJoinPoint commands

mibac138 · 2026-04-15T21:32:04Z

The changes in this file look unrelated to the rest of the PR. Unless I'm wrong, please extract these changes to another PR.

Agreed. This ended up being unrelated cleanup rather than part of the standalone persistence change. I removed it locally from this PR scope.

mibac138 · 2026-04-15T21:32:11Z


            // Not in a landing session, use vanilla logic for player control
-            __result = Current.Game.PlayerHasControl;
+            __result = true;


The changes in this file look unrelated to the rest of the PR. Unless I'm wrong, please extract these changes to another PR.

Agreed. This was not essential to the standalone save/persistence work. I dropped it locally from this PR as part of the scope cleanup.

mibac138 · 2026-04-15T21:33:45Z

+                    {
+                        Log.Warning(


This shouldn't ever happen, did you observe this behavior? Is this related to the rest of this PR? Unless it is, please move this change to another PR

You're right for this PR. I did not have a convincing standalone-persistence-specific justification for carrying this here, so I removed it locally from the PR scope.

I may keep a variant of this behavior only in the later streaming branch, where reconnect/stale-command timing is part of the actual problem being worked on, but it does not belong in this foundation PR.

mibac138 · 2026-04-15T21:36:31Z

    public override void Handle(IChatSource source, string[] args)
    {
-        if (!Server.worldData.TryStartJoinPointCreation(true))
+        if (!Server.worldData.TryStartJoinPointCreation(true, sourcePlayer: source as ServerPlayer))


Commands can not only be run by the players, but also by the server (by writing the command name in the console). Doesn't this change break the command when running from the server?

Uh i didnt know that, ill test it

Good catch. In standalone this was indeed wrong for the server-console path.

The intent of passing sourcePlayer here was to preserve the issuing player when the command comes from a real player, because that later becomes the origin for CreateJoinPoint. But when the command is issued from the server console, source as ServerPlayer is null, and in standalone that can leave us without any client actually executing the join point creation.

I fixed that locally in WorldData.TryStartJoinPointCreation: if standalone starts a join point without a source player, it now falls back to a real playing player (prefer host, otherwise the first playing player), and aborts with a log if there is none. So the player-issued case keeps the original attribution, while the console-issued case still works correctly.

mibac138 · 2026-04-15T21:37:27Z

-                Server.worldData.TryStartJoinPointCreation();
+            var forceJoinPoint = packet.reason == JoinPointRequestReason.Save;
+
+            ServerLog.Detail($"Received Client_Autosaving from {Player.Username}, standalone={Server.IsStandaloneServer}, isHost={Player.IsHost}, reason={packet.reason}, force={forceJoinPoint}");


Reformat - too long line. Also applies to some of the other logging calls in this file, please fix them too.

Agreed. I reformatted the long logging lines in this file locally while cleaning up the standalone snapshot handling changes, so the next push will include that.

mibac138 · 2026-04-15T21:57:17Z

        public bool pauseOnDesync = true;
        public TimeControl timeControl;

+        public void EnforceStandaloneRequirements(bool isStandaloneServer)


Isn't isStandaloneServer always going to be true? The parameter seems redundant in a method called Enforce*Standalone*Requirements

Yes, agreed. That parameter ended up redundant in this PR shape. I removed it locally and switched the call sites to a parameterless EnforceStandaloneRequirements().

mibac138 · 2026-04-15T22:03:31Z

I didn't review this file yet, but as a first thought, why does this use a custom save format?

The main reason is that in standalone the dedicated server needs a durable server-owned representation of the accepted game state, and that state is naturally split into world data, session data, command state, and per-map data.

A single monolithic save/replay archive works poorly for that use case because most updates are partial: after an autosave or a standalone snapshot upload, we often only need to replace one artifact, not rebuild the whole thing. Keeping those pieces separate lets the server update them incrementally and atomically (File.Replace/move per artifact), which is much safer for crash recovery than rewriting one large bundle every time.

So this is not meant as a new interchange format. It is the server's internal durable persistence layout for standalone. That also happens to be the shape that later map-streaming work can build on, but this PR is still only the standalone persistence/upload groundwork, not the streaming implementation itself.

mibac138 · 2026-04-15T22:10:54Z

+        return true;
+    }
+
+    private static byte[] ComputeHash(params byte[][] payloads)


Nit: Effectively the same code is used in SaveLoad. Maybe extract this to some utility method and use both here and there?

Yes, I agree with that direction. WorldData and SaveLoad should ideally share one helper for the standalone world/session hash computation so the upload side and the validation side cannot drift.

I kept this PR focused and answered the same point in the SaveLoad thread, but if I touch this area again I would do it as that shared-helper extraction rather than as two separate local cleanups.

mibac138 · 2026-04-15T22:11:30Z

+{
+    public StandaloneWorldSnapshotState() { }
+    public int tick;
+    public int leaseVersion;


What's the leaseVersion?

It was leftover scaffolding from the later streaming/job-based variant, where a versioned lease made sense while assignments were still evolving.

In this PR it does not carry its weight, because the standalone-only path does not actually need that extra discriminator. I removed it locally from the snapshot packets and the corresponding server-side state so the standalone groundwork here stays minimal.

Co-authored-by: Michael <5672750+mibac138@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 11, 2026 15:40

Copilot started reviewing on behalf of MhaWay April 11, 2026 15:41 View session

Copilot AI reviewed Apr 11, 2026

View reviewed changes

This was referenced Apr 11, 2026

Standalone snapshot persistence #875

Closed

Standalone async reconnect fixes #876

Closed

Standalone prepublish maintenance #877

Closed

notfood added enhancement New feature or request. 1.6 Fixes or bugs relating to 1.6 (Not Odyssey). standalone server Fix or bugs relating to the standalone server. labels Apr 11, 2026

notfood added this to 1.6 and Odyssey Apr 11, 2026

notfood moved this to In review in 1.6 and Odyssey Apr 11, 2026

mibac138 reviewed Apr 11, 2026

View reviewed changes

MhaWay force-pushed the pr/standalone-save-trigger-foundation branch from 94ad756 to dc76dbc Compare April 12, 2026 09:16

Split standalone save trigger foundation

67f32f9

MhaWay force-pushed the pr/standalone-save-trigger-foundation branch from dc76dbc to 67f32f9 Compare April 12, 2026 10:12

MhaWay added 9 commits April 12, 2026 14:58

Unify standalone save path and add Days autosave support

0390728

Add standalone snapshot persistence

e15d083

Add safe defaults to snapshot state structs

6981562

Use File.Replace in SaveGameToFile_Overwrite and eliminate double sna…

506617e

…pshot

Enforce standalone async time and control fixes

bb1c510

Reset MultiplayerServer.instance in TearDown

a3ff2ca

Use int.TryParse in SeedFromSaveZip

77c52d6

Clean up standalone prepublish maintenance

df7f99c

Correct misleading blocked log text in designator patches

ae87eda

MhaWay changed the title ~~Split standalone save trigger foundation~~ Standalone save & persistence improvements Apr 12, 2026

MhaWay added 3 commits April 12, 2026 21:06

Pass sourcePlayer to standalone join point creation for IssuedBySelf …

53e86cf

…gate

MhaWay requested a review from mibac138 April 14, 2026 19:42

mibac138 reviewed Apr 15, 2026

View reviewed changes

Update Source/Common/WorldData.cs

a459786

Co-authored-by: Michael <5672750+mibac138@users.noreply.github.com>

	Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save));
	if (!currentReplay && Multiplayer.Client != null)
	Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save));

Uh oh!

Conversation

MhaWay commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Validation

Note

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 11, 2026

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!

mibac138 Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

MhaWay commented Apr 11, 2026 •

edited

Loading

mibac138 Apr 11, 2026 •

edited

Loading