From 9c04a59c528318f23fb7b437c3b94712873c50ce Mon Sep 17 00:00:00 2001 From: SokyranTheDragon Date: Mon, 26 Aug 2024 00:58:12 +0200 Subject: [PATCH 1/3] Rework some PawnColumnWorker syncing This change is primarily to fix `PawnColumnWorker_Designator` and `PawnColumnWorker_Sterilize` displaying confirmation dialogs, which caused issues as pressing the checkbox would open the dialog for all players and the dialogs themselves weren't synced. I've mentioned this issue in #429. The first change is to stop syncing `PawnColumnWorker_Designator.SetValue` and `PawnColumnWorker_Sterilize.SetValue`, as those could display confirmation dialogs. Instead of those 2 methods, we instead sync: - `DesignationManager.AddDesignation` - This one is not needed, but was included for consistency (and may come in handy for mod compat, it can be safely removed if desired) - `DesignationManager.RemoveDesignation` - Called from `PawnColumnWorker_Designator.SetValue` when value is false - `PawnColumnWorker_Designator.DesignationConfirmed` - This method calls `DesignationManager.AddDesignation` (along with another method), which is why that specific method is not needed - `PawnColumnWorker_Sterilize.AddSterilizeOperation` - `PawnColumnWorker_Sterilize.CancelSterilizeOperations` This required adding extra sync worker delegates for `Designation` and `DesignationManager`. By not syncing the `SetValue` method, it allows for a potential multiple calls to the other synced methods which generally don't have checks if the state already matches. This requires additional patches that cancel execution if it would cause issues (`PreventPawnTableDesignationErrors`, `PreventPawnTableMultipleSterilizeOperations`). Finally, by not syncing the `SetValue` methods we don't call `SetDirty` on the pawn tables. To fix this I've added a method (`TryDirtyCurrentPawnTable`) which is called in post invoke for the synced methods, as well as after syncing designators, to cause the tables to re-sort their values. This will cause the tables to be re-sorted in a few extra situations (like when a different player modifies designators outside of pawn tables). It may be expanded to include more methods to cause the tables to be re-sorted when they normally wouldn't be in vanilla (if we so desire). Alternatively, this could be reduced or removed if we don't want it. --- Source/Client/AsyncTime/AsyncTimeComp.cs | 2 + .../Client/Syncing/Dict/SyncDictRimWorld.cs | 42 ++++++++++++++ Source/Client/Syncing/Game/SyncMethods.cs | 55 +++++++++++++++++-- 3 files changed, 95 insertions(+), 4 deletions(-) diff --git a/Source/Client/AsyncTime/AsyncTimeComp.cs b/Source/Client/AsyncTime/AsyncTimeComp.cs index 837d28f1..add68fdf 100644 --- a/Source/Client/AsyncTime/AsyncTimeComp.cs +++ b/Source/Client/AsyncTime/AsyncTimeComp.cs @@ -396,6 +396,8 @@ void RestoreState() designator.DesignateThing(thing); designator.Finalize(true); } + + SyncMethods.TryDirtyCurrentPawnTable(designator); } finally { diff --git a/Source/Client/Syncing/Dict/SyncDictRimWorld.cs b/Source/Client/Syncing/Dict/SyncDictRimWorld.cs index 0ceefbe2..43ea2a81 100644 --- a/Source/Client/Syncing/Dict/SyncDictRimWorld.cs +++ b/Source/Client/Syncing/Dict/SyncDictRimWorld.cs @@ -666,6 +666,48 @@ public static class SyncDictRimWorld } } }, + { + (SyncWorker sync, ref DesignationManager manager) => + { + if (sync.isWriting) + sync.Write(manager?.map); + else + manager = sync.Read()?.designationManager; + } + }, + { + (SyncWorker sync, ref Designation designation) => + { + if (sync.isWriting) + { + sync.Write(designation?.designationManager); + + if (designation?.designationManager != null) + { + sync.Write(designation.target); + sync.Write(designation.def); + } + } + else + { + var manager = sync.Read(); + + if (manager != null) + { + var target = sync.Read(); + var def = sync.Read(); + + // If the target has Thing, read designation by def for it. + if (target.HasThing) + designation = manager.DesignationOn(target.Thing, def); + // If the target doesn't have a Thing then it must have a cell, + // get the designation by def for that specific cell. + else + designation = manager.DesignationAt(target.Cell, def); + } + } + }, true // implicit + }, #endregion #region ThingComps diff --git a/Source/Client/Syncing/Game/SyncMethods.cs b/Source/Client/Syncing/Game/SyncMethods.cs index dc63361b..10662f50 100644 --- a/Source/Client/Syncing/Game/SyncMethods.cs +++ b/Source/Client/Syncing/Game/SyncMethods.cs @@ -43,7 +43,7 @@ public static void Init() SyncMethod.Register(typeof(Pawn_GuestTracker), nameof(Pawn_GuestTracker.ToggleNonExclusiveInteraction)).CancelIfAnyArgNull(); SyncMethod.Register(typeof(Zone), nameof(Zone.Delete)); SyncMethod.Register(typeof(BillStack), nameof(BillStack.AddBill)).ExposeParameter(0); // Only used for pasting - SyncMethod.Register(typeof(BillStack), nameof(BillStack.Delete)).CancelIfAnyArgNull(); + SyncMethod.Register(typeof(BillStack), nameof(BillStack.Delete)).CancelIfAnyArgNull().SetPostInvoke(TryDirtyCurrentPawnTable); SyncMethod.Register(typeof(BillStack), nameof(BillStack.Reorder)).CancelIfAnyArgNull(); SyncMethod.Register(typeof(Bill_Production), nameof(Bill_Production.SetStoreMode)); SyncMethod.Register(typeof(Bill_Production), nameof(Bill_Production.SetIncludeGroup)); @@ -89,10 +89,13 @@ public static void Init() } } - SyncMethod.Register(typeof(PawnColumnWorker_Designator), nameof(PawnColumnWorker_Designator.SetValue)).CancelIfAnyArgNull(); // Virtual but currently not overriden by any subclasses + SyncMethod.Register(typeof(DesignationManager), nameof(DesignationManager.AddDesignation)).ExposeParameter(0).SetPostInvoke(TryDirtyCurrentPawnTable); // Added designation will be freshly constructed, so we need to expose it rather than sync it + SyncMethod.Register(typeof(DesignationManager), nameof(DesignationManager.RemoveDesignation)).CancelIfAnyArgNull().SetPostInvoke(TryDirtyCurrentPawnTable); + SyncMethod.Register(typeof(PawnColumnWorker_Designator), nameof(PawnColumnWorker_Designator.DesignationConfirmed)).CancelIfAnyArgNull().SetPostInvoke(TryDirtyCurrentPawnTable); // Called from SetValue and confirmation dialog SyncMethod.Register(typeof(PawnColumnWorker_FollowDrafted), nameof(PawnColumnWorker_FollowDrafted.SetValue)).CancelIfAnyArgNull(); SyncMethod.Register(typeof(PawnColumnWorker_FollowFieldwork), nameof(PawnColumnWorker_FollowFieldwork.SetValue)).CancelIfAnyArgNull(); - SyncMethod.Register(typeof(PawnColumnWorker_Sterilize), nameof(PawnColumnWorker_Sterilize.SetValue)).CancelIfAnyArgNull(); // Will sync even without this, but this will set the column to dirty + SyncMethod.Register(typeof(PawnColumnWorker_Sterilize), nameof(PawnColumnWorker_Sterilize.AddSterilizeOperation)).CancelIfAnyArgNull().SetPostInvoke(TryDirtyCurrentPawnTable); + SyncMethod.Register(typeof(PawnColumnWorker_Sterilize), nameof(PawnColumnWorker_Sterilize.CancelSterilizeOperations)).CancelIfAnyArgNull().SetPostInvoke(TryDirtyCurrentPawnTable); SyncMethod.Register(typeof(CompGatherSpot), nameof(CompGatherSpot.Active)); SyncMethod.Register(typeof(Building_Grave), nameof(Building_Grave.EjectContents)); @@ -367,7 +370,7 @@ public static void Init() // Previously we synced the delegate which created the bill, but it has side effects to it. // It can display confirmation like royal implant (no longer used?) or implanting IUD (if it would terminate pregnancy). // On top of that, in case of implanting the Xenogerm recipe, it will open a dialog with list of available options. - SyncMethod.Register(typeof(HealthCardUtility), nameof(HealthCardUtility.CreateSurgeryBill)); + SyncMethod.Register(typeof(HealthCardUtility), nameof(HealthCardUtility.CreateSurgeryBill)).SetPostInvoke(TryDirtyCurrentPawnTable); // Comp explosive SyncMethod.Register(typeof(CompExplosive), nameof(CompExplosive.StartWick)); // Called from Building_BlastingCharge (and some modded) gizmos @@ -706,6 +709,50 @@ static void SyncTargeterInterruptingSelfCast(Verb verb, Pawn casterPawn) job.verbToUse = verb; casterPawn.jobs.StartJob(job, JobCondition.InterruptForced); } + + // By no longer syncing "SetValue" method for pawn column workers, the + // tables aren't made dirty when a value changes. This will ensure that + // the table is made dirty when its state changes, so it can be properly + // sorted. It'll also allow us to dirty the table when it normally wouldn't + // be in vanilla, causing the table to be re-sorted when it wouldn't be in + // vanilla. For example, we can cause the table to be re-sorted when something + // is designated outside the pawn table. + public static void TryDirtyCurrentPawnTable(object instance = null, object[] args = null) + { + // Make sure there's a currently open tab whose window has pawn table, + // and ensure the table is not null and one of the columns is sorted. + // Otherwise, there would be no point in making the table dirty, + // as it wouldn't change anything about the table. + if (Find.MainTabsRoot.OpenTab?.TabWindow is MainTabWindow_PawnTable { table: { SortingBy: not null } table }) + { + // If the method was called on Designator or DesignationManager, + // set the DesignationDef from them as the instance. Instance + // is never Designation, so no point in getting def from it. + if (instance is Designator designator) + instance = designator.Designation; + else if (instance is DesignationManager && args is { Length: > 0 } && args[0] is Designation designation) + instance = designation.def; + + // If the synced object is PawnColumnWorker, then we can dirty the current + // table based on if it's being sorted by the specific PawnColumnDef. + if (instance is PawnColumnWorker worker) + { + if (table.SortingBy == worker.def) + table.SetDirty(); + } + // If we can get DesignationDef, we can check if the sorted column worker is + // designator worker and is the specific designation that + else if (instance is DesignationDef designation) + { + if (table.SortingBy.Worker is PawnColumnWorker_Designator des && des.DesignationType == designation) + table.SetDirty(); + } + // If it's a different type, then we always dirty as we don't + // know if making the table dirty is required or not. + else + table.SetDirty(); + } + } } } From 93fd1dec7d070818578487a9a64699826450d49e Mon Sep 17 00:00:00 2001 From: SokyranTheDragon Date: Mon, 26 Aug 2024 01:04:26 +0200 Subject: [PATCH 2/3] Include harmony patches, as I accidentally left them out. --- Source/Client/Patches/PawnTable.cs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Source/Client/Patches/PawnTable.cs diff --git a/Source/Client/Patches/PawnTable.cs b/Source/Client/Patches/PawnTable.cs new file mode 100644 index 00000000..b257ca2d --- /dev/null +++ b/Source/Client/Patches/PawnTable.cs @@ -0,0 +1,28 @@ +using HarmonyLib; +using RimWorld; +using Verse; + +namespace Multiplayer.Client.Patches; + +[HarmonyPatch(typeof(PawnColumnWorker_Designator))] +[HarmonyPatch(nameof(PawnColumnWorker_Designator.DesignationConfirmed))] +public static class PreventPawnTableDesignationErrors +{ + static bool Prefix(PawnColumnWorker_Designator __instance, Pawn pawn) + { + // Cancel execution if designation already exists to prevent + // errors about trying to double-add designations. + return !__instance.GetValue(pawn); + } +} + +[HarmonyPatch(typeof(PawnColumnWorker_Sterilize), nameof(PawnColumnWorker_Sterilize.AddSterilizeOperation))] +static class PreventPawnTableMultipleSterilizeOperations +{ + static bool Prefix(PawnColumnWorker_Sterilize __instance, Pawn animal) + { + // Cancel execution if any operations exist to prevent + // queueing up multiple sterilize operations. + return !__instance.SterilizeOperations(animal).Any(); + } +} From 2b75a74fa256402fcfdd199ac19ef49349969e28 Mon Sep 17 00:00:00 2001 From: SokyranTheDragon Date: Wed, 9 Oct 2024 16:50:17 +0200 Subject: [PATCH 3/3] Fixed syncing null Designation/DesignationManager I haven't considered how map syncing works when writing those 2 sync worker delegates, and made an error in the way they handle maps. Specifically, the issue here is that writing a null map may result in a non-null map when reading if another sync worker is syncing the map. Likewise, I believe attempting to sync a null map may have set the map to null for a different sync worker delegate, causing issues there. The change here is to, rather than sync the map itself (which we may have potentially synced as null and caused issues for other sync workers) we instead sync a bool that determines if the manager is null or has null map (and we then set the MpContext to use its map). --- Source/Client/Syncing/Dict/SyncDictRimWorld.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/Source/Client/Syncing/Dict/SyncDictRimWorld.cs b/Source/Client/Syncing/Dict/SyncDictRimWorld.cs index 43ea2a81..263ab48f 100644 --- a/Source/Client/Syncing/Dict/SyncDictRimWorld.cs +++ b/Source/Client/Syncing/Dict/SyncDictRimWorld.cs @@ -667,12 +667,18 @@ public static class SyncDictRimWorld } }, { - (SyncWorker sync, ref DesignationManager manager) => + (ByteWriter data, DesignationManager manager) => { - if (sync.isWriting) - sync.Write(manager?.map); - else - manager = sync.Read()?.designationManager; + var isNull = manager?.map == null; + data.WriteBool(isNull); + if (!isNull) + data.MpContext().map = manager.map; + }, + (ByteReader data) => + { + if (data.ReadBool()) + return null; + return data.MpContext().map.designationManager; } }, {