Add delay before disconnected army's ACUs are killed#7153
Conversation
Fixes online replays being cut off at the point where someone disconnected due to the disconnected player reporting a game over state.
to make KillArmy's timing consistent across different scenarios
📝 WalkthroughWalkthroughIntroduces ChangesDelayed ACU explosion on disconnect
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lL1l1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lua/SimUtils.lua (1)
1275-1285: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocument that unsafe commanders are killed asynchronously.
KillUnsafeCommandersnow schedules destruction afterAbandonedArmyGracePeriod, so unsafe commanders may still be alive after Line 1284. A short comment makes that contract clear for callers.Proposed clarification
- DelayedKillUnits(unsafeCommanders, AbandonedArmyGracePeriod) + -- Unsafe commanders remain alive until the grace period elapses; callers only receive the immediately safe set. + DelayedKillUnits(unsafeCommanders, AbandonedArmyGracePeriod)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/SimUtils.lua` around lines 1275 - 1285, Add a comment in the KillUnsafeCommanders function to document that the DelayedKillUnits call schedules destruction asynchronously. Place the comment near the DelayedKillUnits invocation with unsafeCommanders to clarify that unsafe commanders are killed after the AbandonedArmyGracePeriod delay, not immediately, so they may still exist as valid units after the function returns. This documents the asynchronous contract for callers relying on this function's behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog/snippets/fix.7153.md`:
- Line 1: Fix the typo in the changelog entry in the fix.7153.md file by
changing the word "cuttting" to "cutting" in the sentence that reads "This
should fix replays cuttting off at the first disconnected player."
---
Nitpick comments:
In `@lua/SimUtils.lua`:
- Around line 1275-1285: Add a comment in the KillUnsafeCommanders function to
document that the DelayedKillUnits call schedules destruction asynchronously.
Place the comment near the DelayedKillUnits invocation with unsafeCommanders to
clarify that unsafe commanders are killed after the AbandonedArmyGracePeriod
delay, not immediately, so they may still exist as valid units after the
function returns. This documents the asynchronous contract for callers relying
on this function's behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb3a1c0d-5254-4ce8-80af-afc1f5d9bab3
📒 Files selected for processing (2)
changelog/snippets/fix.7153.mdlua/SimUtils.lua
| @@ -0,0 +1 @@ | |||
| - (#7153) Add a delay before ACUs explode when their player disconnects. This should fix replays cuttting off at the first disconnected player. No newline at end of file | |||
There was a problem hiding this comment.
Fix the changelog typo.
cuttting should be cutting.
Proposed fix
-- (`#7153`) Add a delay before ACUs explode when their player disconnects. This should fix replays cuttting off at the first disconnected player.
+- (`#7153`) Add a delay before ACUs explode when their player disconnects. This should fix replays cutting off at the first disconnected player.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - (#7153) Add a delay before ACUs explode when their player disconnects. This should fix replays cuttting off at the first disconnected player. | |
| - (`#7153`) Add a delay before ACUs explode when their player disconnects. This should fix replays cutting off at the first disconnected player. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog/snippets/fix.7153.md` at line 1, Fix the typo in the changelog
entry in the fix.7153.md file by changing the word "cuttting" to "cutting" in
the sentence that reads "This should fix replays cuttting off at the first
disconnected player."
1 participant
Description of the proposed changes
Adds a delay when an army is abandoned before the ACU is killed.
Should fix online replays being cut off at the point where someone disconnected due to the disconnected player reporting a game over state.
The way a game over state is reported is that when a player disconnects from the game, their instance of the game thinks everyone else disconnected, so it runs the abandonment code for all other players. Without a delay, this causes all ACUs to explode and the sim to tell the UI to report a game over state. It's a bit unintuitive that the sim runs and gets time to report an end state while exiting the game, but that is how SessionEndGame() works (it pauses the game and pauses are a request to the sim that happen on a small delay of sim beats).
Move all extra delays to threads so that EndGameGracePeriod in KillArmy is a consistent delay players can rely on.
Testing done on the proposed changes
ACUs no longer explode before exiting when testing multiplayer. I don't have a good way to test the replays because I don't understand how they're saved on the server (I looked a little at the parser and server but didn't dig into it and figure stuff out) and my idea above is just a hypothesis.
Checklist
Summary by CodeRabbit