Skip to content
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

Rework some PawnColumnWorker syncing #502

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SokyranTheDragon
Copy link
Member

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.

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 rwmt#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.
@SokyranTheDragon SokyranTheDragon added the 1.5 Fixes or bugs relating to 1.5 (Not Anomaly). label Aug 25, 2024
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).
@SokyranTheDragon
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.5 Fixes or bugs relating to 1.5 (Not Anomaly).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant