-
-
Notifications
You must be signed in to change notification settings - Fork 158
Ping categories, persistent markers, selection UI, and per-player/faction visibility filters #927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using Multiplayer.Client.Desyncs; | ||
| using Multiplayer.Client.Util; | ||
|
|
@@ -128,13 +129,60 @@ private void HandleDesync(ClientSyncOpinion oldOpinion, ClientSyncOpinion newOpi | |
| var diffAt = FindTraceHashesDiffTick(local, remote, out var found); | ||
| Multiplayer.Client.Send(new ClientDesyncedPacket(local.startTick, diffAt)); | ||
|
|
||
| var snapshotInfo = TryRefreshSnapshotForDesync(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any more details about this feature?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure no problem. I primarily used this on my side to get quicker debug info on desyncs I was hitting during dev. Essentially it gives you a quick snap point to when the desync got detected which is usually a few ticks after the actual divergence, but there's a Snapshot Lag Ticks field in the desync_info that tells you relatively how far behind it ended up. So you can load up the replay and look at what the world state was around the bad tick, instead of from the last cached snapshot. Regarding what info it actually gives you, you can extract the replay.rwmts from the desync zip and use dev tools to inspect what was happening at that moment. For the marker work that was mostly visually checking which markers each client actually had on the map at a bad tick, combined with the stack traces in local_traces.txt that gave me a quick way to spot something like "client 1 has marker X but client 2 doesn't" without having to manually step to get there. Implementation wise it's basically wrapping SaveLoad.SaveGameData + CreateGameDataSnapshot to grab the current state locally, without sending anything to the server or triggering a join point for other players. Runs on the main thread since I believe Scribe and Find.Maps aren't thread-safe. Probably should've taken this out of the PR though, it was actually just a quick thing I added to get more debug info on my side and isn't really related to the category-pings PR. The other thing is, the way it's done currently means the embedded replay in the desync zip starts at the desync tick instead of the earlier cached snapshot, so you lose the ability to forward-sim from that earlier point and see the points leading up to the bad ticks, which is most probably more useful than the snapshot of information I was using. I was actually thinking of doing this as a separate PR where we maybe expand the desync zip to include both the snap-to-the-point snapshot for quick debugging of the divergence itself and then the full replay version for seeing what led up to it. Would you like me to remove this from the current PR and possibly create a new PR for it or just scrap it fully?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a nice idea, but let's break this out into a separate PR, it's out of scope here and could use some refinement.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted, will be pushing a commit soon with updates for all of your comments 😁 |
||
|
|
||
| MpUI.ClearWindowStack(); | ||
| Find.WindowStack.Add(new DesyncedWindow( | ||
| desyncMessage, | ||
| new SaveableDesyncInfo(this, local, remote, diffAt, found) | ||
| new SaveableDesyncInfo(this, local, remote, diffAt, found, snapshotInfo) | ||
| )); | ||
| } | ||
|
|
||
| // Refresh dataSnapshot synchronously on the main thread so the embedded replay.rwmts | ||
| // captures the divergent tick instead of the (default 5 min stale) autosave-aligned join | ||
| // point. SaveGameData isn't thread-safe (Scribe, Find.Maps). No SendGameData - peers must | ||
| // not see a join-point broadcast spawned by a desync. If the capture overruns the budget | ||
| // below we keep the stale snapshot, since we can't predict the cost ahead of time. | ||
| private const int SnapshotRefreshBudgetMs = 2000; | ||
|
|
||
| private static SaveableDesyncInfo.SnapshotFreshness TryRefreshSnapshotForDesync() | ||
| { | ||
| var staleAt = Multiplayer.session.dataSnapshot?.CachedAtTime ?? -1; | ||
| var nowTick = TickPatch.Timer; | ||
| var watch = Stopwatch.StartNew(); | ||
|
|
||
| try | ||
| { | ||
| var fresh = Replay.CaptureLocalSnapshot(); | ||
| watch.Stop(); | ||
|
|
||
| if (watch.ElapsedMilliseconds > SnapshotRefreshBudgetMs) | ||
| { | ||
| Log.Warning( | ||
| $"[MP] Desync snapshot capture took {watch.ElapsedMilliseconds} ms " + | ||
| $"(budget {SnapshotRefreshBudgetMs} ms); keeping stale snapshot from tick {staleAt}."); | ||
| return new SaveableDesyncInfo.SnapshotFreshness( | ||
| IsFresh: false, ElapsedMs: watch.ElapsedMilliseconds, SnapshotTick: staleAt, | ||
| DesyncTick: nowTick, FallbackReason: $"capture exceeded {SnapshotRefreshBudgetMs} ms budget"); | ||
| } | ||
|
|
||
| Multiplayer.session.dataSnapshot = fresh; | ||
| return new SaveableDesyncInfo.SnapshotFreshness( | ||
| IsFresh: true, ElapsedMs: watch.ElapsedMilliseconds, SnapshotTick: fresh.CachedAtTime, | ||
| DesyncTick: nowTick, FallbackReason: null); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| watch.Stop(); | ||
| Log.Warning( | ||
| $"[MP] Desync snapshot capture threw after {watch.ElapsedMilliseconds} ms; " + | ||
| $"keeping stale snapshot from tick {staleAt}. Exception: {e.Message}"); | ||
| return new SaveableDesyncInfo.SnapshotFreshness( | ||
| IsFresh: false, ElapsedMs: watch.ElapsedMilliseconds, SnapshotTick: staleAt, | ||
| DesyncTick: nowTick, FallbackReason: $"exception: {e.GetType().Name}: {e.Message}"); | ||
| } | ||
| } | ||
|
|
||
| private static int FindTraceHashesDiffTick(ClientSyncOpinion local, ClientSyncOpinion remote, out bool found) | ||
| { | ||
| found = true; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even want to add this info to the desync report? Not sure how this is useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally added these for debugging marker specific desyncs during dev. The idea was that if you have two players with different
Marker CountorNext Marker Idin the desync report, you immediately know the divergence happened somewhere in the marker path. The "markerCap" was mainly just there for me to make sure that the marker cap set by the host is properly syncing to all clients. The info doesn't really provide any other info regarding desyncs so I fully understand if you'd prefer it removed.