Bump up delay to end game back to 3 seconds#7050
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a global Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/sim/VictoryCondition/AbstractVictoryCondition.lua (1)
46-49:DelayBeforeGameEnds = 3lacks safety margin against the documented minimum.The comment requires "at least three seconds," but the value is set to exactly three with no buffer.
DelayBeforeVictory = 5demonstrates the codebase already uses buffers for timing-sensitive operations;DelayBeforeGameEndsshould follow the same pattern. Bump to4or5to provide headroom against timing drift (e.g., tick-rate variance or future changes toScenarioFramework.EndOperation). Document why the chosen value provides adequate margin.Additionally, since this is a public field, a future concrete subclass could override it to a value below 3 and reintroduce the bug. Document this constraint in the field's comment or add a runtime assertion in subclasses to enforce it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/sim/VictoryCondition/AbstractVictoryCondition.lua` around lines 46 - 49, Increase DelayBeforeGameEnds from 3 to 5 (matching DelayBeforeVictory's buffer) and update its comment to state it must be >= 3 and why the extra margin is used (tick drift / ScenarioFramework.EndOperation timing); also add a runtime check in AbstractVictoryCondition (e.g., in the initializer or validation method) that asserts self.DelayBeforeGameEnds >= 3 to prevent subclasses from accidentally lowering it below the documented minimum, referencing DelayBeforeGameEnds, DelayBeforeVictory and ScenarioFramework.EndOperation so reviewers can find the related symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lua/sim/VictoryCondition/AbstractVictoryCondition.lua`:
- Around line 46-49: Increase DelayBeforeGameEnds from 3 to 5 (matching
DelayBeforeVictory's buffer) and update its comment to state it must be >= 3 and
why the extra margin is used (tick drift / ScenarioFramework.EndOperation
timing); also add a runtime check in AbstractVictoryCondition (e.g., in the
initializer or validation method) that asserts self.DelayBeforeGameEnds >= 3 to
prevent subclasses from accidentally lowering it below the documented minimum,
referencing DelayBeforeGameEnds, DelayBeforeVictory and
ScenarioFramework.EndOperation so reviewers can find the related symbols.
|
I think the delay was to ensure the final score is fully synced, since syncing doesnt happen every tick |
|
iirc this wasnt the case just for the coop, but generally for all games. There was a bug when the final stats didnt match the game, since they were not fully synced. That was a long time ago, maybe things has changed a lot since then in how the score or game ending works, but thats what I remember. |
|
Maybe we should hardcode the delay then, to prevent regression in the future? |
|
All the defeat/end-of-game stuff is overly complicated timing-wise because the victory thread and ai brain callback threads are too separated. The reason the delay was decreased was because the unit transfer thread could finishing waiting while the game is over and then it would kill all units because the game was over and no valid allies were left over. #6965 (review) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/sim/VictoryCondition/AbstractVictoryCondition.lua (1)
46-49: Fix looks correct; consider documenting the timing coupling more explicitly.The restoration of the 3-second delay fixes the campaign sync issue. The comment helpfully references
ScenarioFramework.EndOperation, but the dependency is implicit—EndOperationThreadalso usesWaitSeconds(3)and both need to complete beforeSync.GameEndedis set.To prevent future regressions (as discussed in the PR comments), consider either:
- Referencing the shared constant from a single source, or
- Adding a cross-reference comment in
ScenarioFramework.luapointing back here.This would make the coupling explicit and reduce the risk of one value being changed without the other.
,
💡 Optional: Add a cross-reference note
--- Once the game is guaranteed to end, it will take this many seconds to end the game. This needs --- to be three or more seconds for campaign/coop to end gracefully. It takes three seconds for - --- an operation (campaign/coop) to end via `ScenarioFramework.EndOperation`. + --- an operation (campaign/coop) to end via `ScenarioFramework.EndOperation`. Note: The + --- `EndOperationThread` in ScenarioFramework.lua also waits 3 seconds—keep these in sync. DelayBeforeGameEnds = 3,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/sim/VictoryCondition/AbstractVictoryCondition.lua` around lines 46 - 49, DelayBeforeGameEnds is coupled with ScenarioFramework.EndOperation/EndOperationThread which also uses WaitSeconds(3) before setting Sync.GameEnded; make that coupling explicit by either exposing and referencing a single shared constant (e.g., move DelayBeforeGameEnds to a shared constant module and update ScenarioFramework.EndOperation/EndOperationThread to use it) or by adding a clear cross-reference comment in ScenarioFramework.lua that points back to DelayBeforeGameEnds in AbstractVictoryCondition.lua and explains both must match; update all references (DelayBeforeGameEnds, ScenarioFramework.EndOperation, EndOperationThread, WaitSeconds, and Sync.GameEnded) accordingly so future changes remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lua/sim/VictoryCondition/AbstractVictoryCondition.lua`:
- Around line 46-49: DelayBeforeGameEnds is coupled with
ScenarioFramework.EndOperation/EndOperationThread which also uses WaitSeconds(3)
before setting Sync.GameEnded; make that coupling explicit by either exposing
and referencing a single shared constant (e.g., move DelayBeforeGameEnds to a
shared constant module and update
ScenarioFramework.EndOperation/EndOperationThread to use it) or by adding a
clear cross-reference comment in ScenarioFramework.lua that points back to
DelayBeforeGameEnds in AbstractVictoryCondition.lua and explains both must
match; update all references (DelayBeforeGameEnds,
ScenarioFramework.EndOperation, EndOperationThread, WaitSeconds, and
Sync.GameEnded) accordingly so future changes remain synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3aa8863b-55c6-463f-83d0-cedc153992a7
📒 Files selected for processing (2)
changelog/snippets/fix.7050.mdlua/sim/VictoryCondition/AbstractVictoryCondition.lua
✅ Files skipped from review due to trivial changes (1)
- changelog/snippets/fix.7050.md
3 participants
Description of the proposed changes
As posted on Discord, this fixes the bug where a campaign mission no longer shows the 'Operation Complete/Failed' dialog. It takes three seconds for the dialog to show, this is likely for dramatic effect. In #6965 the delay before the game ends was reduced from 3 seconds to 1 second, as a result the operation data was not synced and therefore the dialog would never show.
Testing done on the proposed changes
Run using the init of coop in dev-mode by pointing to the repositories and confirm that the dialog shows with the changes made.
Additional context
Checklist
Summary by CodeRabbit
Bug Fixes
Documentation