rwmt / Multiplayer

Zetrith's Multiplayer mod for RimWorld
MIT License
481 stars 96 forks source link

Fix/sync CompObelisk_Abductor (Warped Obelisk/Labyrinth) #476

Open SokyranTheDragon opened 3 months ago

SokyranTheDragon commented 3 months ago

The obelisk currently causes desyncs due to the map being generated in a separate thread.

We currently make change asynchronous long events queued when executing synced commands into synchronous ones. To err on the side of caution, I've allowed this to be applied to any long event but only when a new field (forceSynchronously) is set to true.

I could make all long events (if MP is active) synchronous, rather than specific ones, if this is preferred - @Zetrith let me know and I'll change it.

Additional changes to LongEventAlwaysSync were also needed:

The final change is to add prefix/finalizer to CompObelisk_Abductor.GenerateLabyrinth to set forceSynchronously to true/false. If we decide to make all long events in MP synchronous then this patch will be safe to remove.

As a side note - #474 is required to allow for the generated maps to be random. Without it, all the generated maps will have the same layout, and the pawns will spawn in the same room.

SokyranTheDragon commented 2 months ago

I've fixed conflicts withe the base branch, so it should be safe to merge (unless we decide on a different approach to fixing this issue). However, I assume merging either this or #489 will cause another conflict as both edit the same file by adding new code at basically the same location.

Zetrith commented 2 weeks ago

Why does asynchrony cause desyncs here?

We definitely don't want to make all long events synchronous. For example, generating a map for settling with a caravan is async because it's better UX (async long events show the waiting screen with tips and mod list).

SokyranTheDragon commented 2 weeks ago

After some deeper consideration, I've realized I've over-complicated the solution and haven't investigated enough.

I've found out that asynchronous long events with a callback cause desyncs, but I did not consider seeding the callback as I misunderstood the async long events. So the simple solution here is to just seed the callback and the issue should be fixed.

On top of that, we'll also need (part of) the changed LongEventAlwaysSync from here for compatibility. Since we force long events while executing commands to be synchronous, we'll need to make sure the callback (if it exists) is also seeded. It does not matter for Vanilla, but may be important to prevent desyncs with mods.

Anyway, I'll make the necessary changes soon to properly fix the issue (without using workarounds, like here).

For example, generating a map for settling with a caravan is async because it's better UX (async long events show the waiting screen with tips and mod list).

I believe this is not really the case in MP (unless we're talking mods). Long events are made synchronous when executing synced commands (settling in an empty tile, creating a new faction in multifaction), so they can't display that info. The obelisk specifically doesn't include extra info, despite being asynchronous. And no other async long events seem relevant to MP.

SokyranTheDragon commented 2 weeks ago

I've reworked the patch for the Warped Obelisk: