Multiplayer icon indicating copy to clipboard operation
Multiplayer copied to clipboard

Rework some PawnColumnWorker syncing

Open SokyranTheDragon opened this issue 1 year ago • 1 comments

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 with adding DesignationManager.RemoveDesignation (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 for syncing
  • PawnColumnWorker_Sterilize.AddSterilizeOperation
  • PawnColumnWorker_Sterilize.CancelSterilizeOperations

This required adding extra sync worker delegates for Designation and DesignationManager. Those were taken from MP Compat (rwmt/Multiplayer-Compatibility#394).

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.

SokyranTheDragon avatar Aug 25 '24 23:08 SokyranTheDragon

I've fixed syncing null Designation/DesignationManager. This should fix issues with Vanilla Vehicles Expanded - Tier 3.

I originally 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).

SokyranTheDragon avatar Oct 09 '24 14:10 SokyranTheDragon