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

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

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

Conversation

SokyranTheDragon
Copy link
Member

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:

  • Adding the callback action to action to be executed after, as callback is only executed on the main thread for asynchronous long events
  • Adding HarmonyPriority(Priority.HigherThanNormal) to the patch, as this method needs to run before SeedLongEvents patch (to seed both action and callback)

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.

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.

Additional changes to `LongEventAlwaysSync` were also needed:
- Adding the `callback` action to `action` to be executed after, as `callback` is only executed on the main thread for asynchronous long events
- Adding `HarmonyPriority(Priority.HigherThanNormal)` to the patch, as this method needs to run before `SeedLongEvents` patch (to seed both action and callback)

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.
@SokyranTheDragon SokyranTheDragon added fix Fixes for a bug or desync. anomaly Fix or bugs relating to Anomaly (Not 1.5) labels Jun 18, 2024
…ductor

# Conflicts:
#	Source/Client/Patches/Determinism.cs
@SokyranTheDragon
Copy link
Member Author

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
Copy link
Member

Zetrith commented Sep 15, 2024

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
Copy link
Member Author

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.

- Removed forcibly making Warped Obelisk long event synchronous
- Seeded long event callback
  - This will fix Warped Obelisk long event desync, along with other asynchronous long events using callbacks
- When a long event is made synchronous (executing commands), the callback (if present) will be added to the action itself
  - The callback is normally skipped for synchronous long events
  - This is technically unnecessary with Vanilla and MP, but may become relevant with mods (or in a future update)
- Removed harmony priority attribute, as it's not needed anymore
…abductor

# Conflicts:
#	Source/Client/Patches/Seeds.cs
@SokyranTheDragon
Copy link
Member Author

I've reworked the patch for the Warped Obelisk:

  • Removed forcibly making Warped Obelisk long event synchronous
  • Seeded long event callback
    • This will fix Warped Obelisk long event desync, along with other asynchronous long events using callbacks
  • When a long event is made synchronous (executing commands), the callback (if present) will be added to the action itself
    • The callback is normally skipped for synchronous long events
    • This is technically unnecessary with Vanilla and MP, but may become relevant with mods (or in a future update)
  • Removed harmony priority attribute, as it's not needed anymore

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

Successfully merging this pull request may close these issues.

2 participants