Simplify defeat backend #6965
Simplify defeat backend #6965
Conversation
|
Warning Rate limit exceeded@lL1l1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughCentralizes defeat/recall/abandon handling via AIBrain:SetDefeatStatus and threaded Kill* handlers; adds many SimUtils helpers for transfers, upgrades, waits, and kill flows; tightens scope/type annotations; adds lobby share type aliases; and reduces end-game delay default to 1 second. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant AIBrain
participant SimUtils
participant Engine
Note over Player,AIBrain: Player or game triggers defeat/recall/abandon
Player->>AIBrain: trigger defeat/recall/abandon
AIBrain->>AIBrain: SetDefeatStatus(status)
AIBrain->>SimUtils: ForkThread(KillAbandonedArmy / KillRecalledArmy / KillArmyOnDelayedRecall, args)
alt Transfer & upgrade phase
SimUtils->>Engine: TransferUnitsToBrain / TransferUnitsToHighestBrain
SimUtils->>Engine: GiveResourcesToPlayer / UpdateUnitCap
SimUtils->>SimUtils: PauseTransferredKennels / UpgradeTransferredKennels / UpgradeUnits
end
alt Kill/cleanup phase
SimUtils->>Engine: KillUnits / QueryUnitState
SimUtils->>SimUtils: WaitUntilUnitsDeadOrTimeout
end
SimUtils-->>AIBrain: final state/update (defeat/recall processed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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
left a comment
lL1l1
left a comment
There was a problem hiding this comment.
Looks mostly good to me. Needs a snippet and after you unset the draft status I can approve it.
| ---@param shareOption 'FullShare' | 'ShareUntilDeath' | 'PartialShare' | 'TransferToKiller' | 'Defectors' | 'CivilianDeserter' | ||
| ---@param shareTime number Game time in ticks | ||
| ---@param shareOption ShareOption | ||
| ---@param shareTime integer Game time in ticks |
There was a problem hiding this comment.
| ---@param shareTime integer Game time in ticks | |
| ---@param shareTime integer # Game time in ticks |
There was a problem hiding this comment.
As I understand, the majority of the repo doesn't use this optional syntax
There was a problem hiding this comment.
It would be nice to clean up the comment style for annotations (using a # or not) in a separate pr then, though that would be low priority.
lua/SimUtils.lua
Outdated
| -- KillArmy waits 10 seconds before acting, while FakeTeleport waits 3 seconds, so the ACU shouldn't explode. | ||
| FakeTeleportUnits(safeCommanders, true) |
There was a problem hiding this comment.
No longer forking the teleport as a thread means it always recalls the ACU before possibly killing it, so the comment isn't needed.
It also means that the grace period for an abandoned army is 13 seconds instead of 10. In my opinion that is reasonable.
There was a problem hiding this comment.
Ah, thanks, I didn't notice, but that is reasonable indeed - it mirrors the recall behavior where the defeat doesn't happen until the recall finishes (which uses the same function), except that the defeat state is set first.
![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)
1106-1142: Return type annotation mismatch.The function is annotated as returning
integer|falsebut never returnsfalse. It returns:
0when units are emptytimeoutwhen timeout is reached (including negative values)elapsedwhen all units are deadConsider updating the annotation to just
integer.---@param units Entity[] ---@param timeout? integer in ticks ----@return integer|false elapsed +---@return integer elapsed function ThreadSuspendUntilUnitsDead(units, timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changelog/snippets/fix.6965.md(1 hunks)engine/Sim.lua(1 hunks)lua/SimSync.lua(1 hunks)lua/SimUtils.lua(26 hunks)lua/aibrain.lua(8 hunks)lua/ui/lobby/lobbyOptions.lua(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lua/SimUtils.lua (4)
lua/system/utils.lua (3)
table.empty(68-71)table.getsize(82-89)table.destructiveCat(277-281)engine/Sim.lua (5)
OkayToMessWithArmy(1057-1058)SetArmyUnitCap(1170-1171)GetArmyBrain(588-589)GetGameTick(624-625)IssueStop(966-967)lua/SimSync.lua (1)
StartCountdown(168-179)lua/ScenarioFramework.lua (1)
FakeTeleportUnits(944-982)
🔇 Additional comments (30)
lua/SimSync.lua (1)
169-169: LGTM!Good change to explicitly scope
cdDurationas a local variable, improving code clarity without changing behavior.engine/Sim.lua (1)
623-624: LGTM!Correct type refinement — game ticks are discrete integer values, so
integeris more accurate thannumber.lua/ui/lobby/lobbyOptions.lua (2)
13-15: LGTM!Well-organized type aliases that centralize share option definitions and improve type safety across the codebase.
37-39: LGTM!Using the new type aliases for GameOptions fields provides consistency with the type definitions above.
changelog/snippets/fix.6965.md (1)
1-1: LGTM!Clear and concise changelog entry accurately summarizing the refactor.
lua/aibrain.lua (8)
11-13: LGTM!Clean imports for the new centralized army kill utilities.
68-68: LGTM!Type annotation change from
Armytointegercorrectly reflects that this cachesGetArmyIndex()which returns an integer.
168-176: LGTM!Using
_for unused loop variables is idiomatic and improves code clarity.
434-448: LGTM!
SetDefeatStatuseffectively centralizes the common defeat/recall state transition logic: updating unit cap, triggering ping callbacks, recall notifications, and disabling AI. This is the core of the refactoring.
451-458: LGTM!
OnDrawandOnVictorycorrectly only set the status without callingSetDefeatStatus, as they don't need unit cap redistribution or AI disabling.
461-473: LGTM!
OnDefeatnow properly delegates state management toSetDefeatStatusbefore forking the kill thread.
507-525: LGTM!
OnRecallednow properly usesSetDefeatStatusand delegates toKillRecalledArmy. The guard against duplicate recalls is appropriate.
1169-1174: LGTM!Backwards compatibility imports ensure existing code depending on these functions from this module continues to work.
lua/SimUtils.lua (17)
30-31: LGTM!Using
_for unused loop indices is appropriate.Also applies to: 49-51
113-114: LGTM!The pattern
a.Blueprint or a:GetBlueprint()handles both direct blueprint access and method-based access, improving compatibility.
579-592: LGTM!Good addition of the
@type RebuildTrackerannotation for clarity.
709-730: LGTM!The
GiveUnitsToPlayerfunction now has cleaner nil-safety and proper type annotations.
743-766: LGTM!
KillSharedUnitsnow properly iterates backwards to safely remove elements while iterating, and the logic is cleaner.
768-806: LGTM!
UpdateUnitCaphas improved validation and clearer logic for distributing unit cap.
815-854: LGTM!
TransferUnitsToBrainis cleaner with better early returns and usestable.destructiveCatfor efficiently merging unit lists.
856-905: LGTM!
GetAllegianceCategoriesandTransferUnitsToHighestBrainare well-structured with proper sorting by rating.
907-936: LGTM!Good extraction of
EndGameGracePeriod,KillUnits,KillWalls, andKillRemainingas helper functions. These improve code organization and reusability.
996-1036: LGTM!
TransferUnitsToKillercorrectly handles different victory conditions and determines the appropriate killer index.
1038-1081: LGTM!
KillArmyis well-organized with clear handling of each share condition.
1083-1104: LGTM!
KillRecalledArmycorrectly handles the recall scenario by excluding subcommanders from transfer (since they also recall) and simplifying the share logic.
1155-1172: LGTM!
KillUnsafeCommanderscorrectly filters commanders based on theCommanderSafeTimethreshold and kills those that were recently damaged.
1174-1224: LGTM!
KillArmyOnDelayedRecallproperly handles the delayed recall scenario with countdown display and timeout logic. The abuse detection (all commanders dying early in assassination mode) is a good safeguard.
1226-1246: LGTM!
KillArmyOnACUDeathcorrectly handles the permanent share scenario, waiting for shared commanders to die before applying the share condition.
1248-1299: LGTM!
KillAbandonedArmyis the main entry point for handling disconnected players. It correctly:
- Resolves
SameAsShareto the actual share option- Handles
ExplodeandRecallACU options with safety checks- Handles
RecallDelayedandPermanentoptions by delegating to appropriate functions- Falls back to
KillArmywith warning on unknown options
1303-1372: LGTM!
DisableAIhas improved nil-safety with explicitnilassignments when destroying managers, ensuring cleaner cleanup.
lL1l1
left a comment
lL1l1
left a comment
There was a problem hiding this comment.
Some testing using the launch script and 4 players (fullshare, assassination):
- Defeat through recalling seems fine, although I know that in the live game that it kills units before the sim stops.
- Defeat through ctrl k seems fine
- Defeat through abandonment kills units before the sim stops.
There is a ~109 tick difference between AbandonedByPlayer being called and the game ending, which varies due to the ACU kill and victory condition running on different threads. The difference in recall between here and live game is probably due to the additional 3 seconds to the grace period.
The ideal solution would be to make recall and abandon work sequentially with the victory condition thread, but for now the grace period can be increased.
| ---@param shareOption 'FullShare' | 'ShareUntilDeath' | 'PartialShare' | 'TransferToKiller' | 'Defectors' | 'CivilianDeserter' | ||
| ---@param shareTime number Game time in ticks | ||
| ---@param shareOption ShareOption | ||
| ---@param shareTime integer Game time in ticks |
There was a problem hiding this comment.
It would be nice to clean up the comment style for annotations (using a # or not) in a separate pr then, though that would be low priority.
This reverts commit b457ca9ef6c73e08d524469eba7387d5cee034d1.
This reverts commit 7e19835.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lua/SimUtils.lua (1)
114-132: Missing dead-check afterWaitTicks.After
WaitTicks(1), a kennel may have been destroyed. UnlikeUpgradeTransferredKennels(line 140) andUpgradeUnits(line 190), this function doesn't guard against destroyed units before callingSetPaused.for _, unit in kennels do + if unit:BeenDestroyed() then + continue + end unit:SetPaused(true)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lua/SimUtils.lua(30 hunks)
🔇 Additional comments (15)
lua/SimUtils.lua (15)
5-6: LGTM! CachingArmyBrainsas a local upvalue is a standard Lua optimization for frequently accessed globals.
134-172: LGTM! Good use ofBeenDestroyed()checks after eachWaitTickscall, and theor {}pattern safely handles nilPodData.
174-208: LGTM! The upgrade logic correctly guards against destroyed units and properly manages build rates and consumption.
210-215: LGTM! The fallback toGetBlueprint()method call handles units where theBlueprintfield may not be directly accessible.
672-709: LGTM! Well-structured extraction ofRebuildUnitsand proper filtering inTransferUnfinishedUnitsAfterDeathto exclude upgrades and factory-built units.
746-808: LGTM! Backward iteration inKillSharedUnitscorrectly handles removal during iteration.UpdateUnitCaphas proper defensive checks and clear redistribution logic.
810-907: LGTM! Clean separation betweenTransferUnitsToBrain(handles the actual transfer) andTransferUnitsToHighestBrain(handles rating-based sorting). TheWaitSeconds(1)between transfers correctly prevents double-transfers.
1108-1145: LGTM! Well-designed polling function with proper edge case handling for empty tables and non-positive timeouts. The return value semantics (returningtimeoutvselapsed) enable callers to detect timeout vs natural completion.
1151-1175: LGTM! The commander safety check logic correctly identifies commanders that were recently damaged (potential disconnect abuse) and kills them while preserving genuinely safe commanders.
1177-1228: LGTM! The delayed recall flow correctly orchestrates unit sharing, countdown display, timeout waiting, and conditional handling for disconnect abuse detection.
1252-1303: LGTM! Comprehensive handling of all disconnect share options with appropriate fallbacks and warnings for unknown options.
1307-1376: LGTM! Thorough AI cleanup that properly disables and destroys all manager components, with nil assignments to break reference cycles and allow garbage collection.
1432-1454: LGTM! Improved defensive programming with early returns for invalid states (observers, self-transfers, enemies) and proper validation of defeat status and resource values before transfer.
476-487: LGTM! Proper type annotation for the external factory component access pattern.
496-508: LGTM! Proper guards before forking threads ensure no unnecessary thread creation for empty work sets.
|
Your changes look good to me, here's my recap so we're on the same page:
At a glance, increasing the grace period to 12 seconds seems curious to me because even 10 seconds already seems too long to me for players to have to wait for. If these changes really started blowing the units too soon, I think we should decrease the victory monitor's wait interval instead. |
I think it's nice to have in the name because as you said, most functions would have it. It also makes more clear what the options are for using the function.
There's a 5 second delay that checks for draws, and the wait interval is already 4 ticks (5 in code but WaitTicks is weird). There's |
|
Another way to get all units to blow up is to have a player die before the game ends, and then have their last teammate die in the middle of the transfer. That could be another common scenario in games since one player leaving or dying can result in the rest of their team leaving shortly after. I think it's better to close up the PR though, this is "simplify defeat backend", not adding some mechanism that integrates army transfer on defeat and the overall match state. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/sim/VictoryCondition/AbstractVictoryCondition.lua (1)
274-274: Update the outdated comment.The comment still references "three seconds later" but the delay is now 1 second. Consider updating it to either specify the current delay or reference the configurable field.
🔎 Suggested fix
- --- Ends the game. The monitoring thread is stopped. The game ends three seconds later to give all players a window of opportunity to share the game results with the server. + --- Ends the game. The monitoring thread is stopped. The game ends after DelayBeforeGameEnds seconds to give all players a window of opportunity to share the game results with the server.
♻️ Duplicate comments (1)
lua/SimUtils.lua (1)
735-741: Critical: Potential nil dereference on empty table.Line 737 checks
not units(nil), but an empty table{}passes this check. Line 741 then accessesunits[1].Army, which will cause a nil dereference ifunitsis an empty table.🔎 Proposed fix
function GiveUnitsToPlayer(data, units) local manualShare = ScenarioInfo.Options.ManualUnitShare - if manualShare == 'none' or not units then + if manualShare == 'none' or not units or not units[1] then return end
🧹 Nitpick comments (1)
lua/SimUtils.lua (1)
1465-1466: Optional: Inconsistent brain access pattern.Lines 1465-1466 use
GetArmyBrain()instead of theArmyBrains[]table used elsewhere in the file (e.g., lines 246, 800, 1053). While this function is infrequently called (UI-triggered), using the upvalued table would be more consistent with the performance optimization applied throughout this file.🔎 Proposed change for consistency
- local fromBrain = GetArmyBrain(data.From) - local toBrain = GetArmyBrain(data.To) + local fromBrain = ArmyBrains[data.From] + local toBrain = ArmyBrains[data.To]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lua/SimSync.lua(1 hunks)lua/SimUtils.lua(28 hunks)lua/sim/VictoryCondition/AbstractVictoryCondition.lua(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lua/SimSync.lua
🧰 Additional context used
🧬 Code graph analysis (2)
lua/sim/VictoryCondition/AbstractVictoryCondition.lua (1)
lua/simInit.lua (1)
WaitSeconds(33-39)
lua/SimUtils.lua (6)
engine/Sim.lua (3)
GetGameTick(624-625)IssueStop(966-967)GetArmyBrain(588-589)lua/system/utils.lua (3)
table.empty(68-71)table.getsize(82-89)table.destructiveCat(277-281)engine/Core.lua (3)
ForkThread(181-182)WARN(560-561)IsDestroyed(243-244)lua/sim/score.lua (1)
CalculateBrainScore(84-103)lua/SimSync.lua (1)
StartCountdown(169-180)lua/ScenarioFramework.lua (1)
FakeTeleportUnits(944-982)
🔇 Additional comments (30)
lua/sim/VictoryCondition/AbstractVictoryCondition.lua (2)
47-47: LGTM: Delay reduction aligns with PR objectives.The reduction from 3 to 1 second matches the planned 2-second decrease discussed in the PR comments.
289-289: Excellent refactor: Makes delay configurable.Replacing the hardcoded value with
self.DelayBeforeGameEndsimproves maintainability and allows subclasses to customize the delay if needed.lua/SimUtils.lua (28)
5-7: LGTM: Performance optimization.Upvaluing
ArmyBrainsis a standard Lua optimization that improves access speed by avoiding repeated global lookups.
91-91: LGTM: Typo correction.Fixed "hiearchy" → "hierarchy" in the TODO comment.
200-203: Verify re-entrancy intention forDefaultBuildRate.Line 200 only sets
DefaultBuildRateif it doesn't already exist. IfUpgradeUnitsis called multiple times on the same units, this preserves the original build rate, which seems intentional. However, if this function is meant to be called only once per unit, the check may hide a logic error.Confirm whether
UpgradeUnitscan be called multiple times on the same units and whether preserving the originalDefaultBuildRateacross calls is the intended behavior.
231-233: LGTM: Defensive blueprint access.Added fallback to
GetBlueprint()method for units that don't have theBlueprintfield cached, making the comparator more robust.
246-246: LGTM: Consistent with performance optimization.Uses the upvalued
ArmyBrainstable for direct access, consistent with the optimization at line 6.
496-496: LGTM: Enhanced type annotation.Added type annotation for better IDE support and code clarity.
519-530: LGTM: More idiomatic empty check.Replaced
table.getn() > 0withnot table.empty(), which is clearer and more robust (handles both nil and empty tables correctly).
565-565: LGTM: Type documentation.Added explicit type annotation for better code documentation.
616-618: LGTM: Explicit indexing.Using explicit index variable
imakes the relationship betweenunitsandtrackersclearer.
641-645: LGTM: Conventional unused variable.Using
_instead ofkwhen the key is unused follows Lua conventions.
694-731: LGTM: Well-structured rebuild logic.The new functions properly:
- Encapsulate the rebuild workflow (start, try for each army, finalize)
- Filter unbuilt units appropriately (excluding upgrades and factory-parented units)
- Use defensive checks before processing
The nil-safety check on Line 727 correctly handles empty tables by checking the first element.
768-791: LGTM: Safe shared unit cleanup.Properly handles empty tables with early return and correctly iterates backwards when removing elements from the array.
796-830: LGTM: Robust unit cap redistribution.Properly validates share options, checks army states, and correctly distributes cap to eligible brains (allies or all, depending on settings).
840-878: LGTM: Improved transfer orchestration.Enhanced function now:
- Returns transferred units for caller tracking
- Properly sequences transfers with 1-second delays to prevent duplicate transfers
- Uses consistent nil-safety patterns
882-899: LGTM: Clear allegiance categorization.The refactored function clearly categorizes brains into Civilians, Enemies, and Allies with consistent patterns.
908-929: LGTM: Rating-based transfer prioritization.The function correctly prioritizes transfer to highest-rated players, with a sensible fallback to score-based ratings for AI players.
933-950: LGTM: Grace period and unit cleanup.The 10-second grace period allows observers to see the final game state before units are destroyed. The
KillUnitshelper safely checks if units are destroyed before killing them.
952-960: LGTM: Clear helper functions.Simple, focused helpers that improve code readability in the kill flow.
966-1018: LGTM: Improved borrowed unit handling.All three functions properly:
- Check for empty collections before processing
- Validate army states before transfers
- Use consistent nil-safety patterns
1022-1060: LGTM: Enhanced killer transfer logic.The function now accepts an optional category parameter for flexibility. The killer determination correctly handles team-based scenarios in decapitation mode by finding the killer of the last allied commander to die.
1064-1105: LGTM: Well-structured army defeat handling.The centralized
KillArmyfunction clearly orchestrates the defeat flow:
- Applies the grace period consistently
- Handles each share option with appropriate logic
- Properly sequences operations (share, kill walls, kill remaining)
1107-1128: LGTM: Recall-specific defeat handling.
KillRecalledArmycorrectly simplifies the logic for recall scenarios by:
- Excluding SUBCOMMANDERs (since they recall too)
- Only handling the relevant share options (CivilianDeserter, Defectors)
- Using the same grace period
1130-1167: LGTM: Robust timeout utility.
WaitUntilUnitsDeadOrTimeoutproperly handles all edge cases:
- Empty unit lists (returns 0)
- Negative/zero timeouts (returns timeout for comparison purposes)
- Distinguishes between natural completion and timeout
The return value semantics are well-documented and consistent.
1171-1196: LGTM: Commander safety handling.The new constants and
KillUnsafeCommandersfunction properly implement disconnect abuse prevention by killing commanders that took damage within the safety window (2 minutes).
1199-1250: LGTM: Complex but correct delayed recall logic.
KillArmyOnDelayedRecallproperly handles the delayed recall disconnect option:
- Shares all units temporarily
- Creates countdown UI for players
- Detects abuse (early commander death) and falls back to standard share
- Filters unsafe commanders before recall
- Sequences operations correctly
The complexity is justified by the feature requirements.
1252-1272: LGTM: Permanent ACU share logic.
KillArmyOnACUDeathcorrectly implements the permanent ACU share option with abuse detection for early deaths.
1274-1325: LGTM: Comprehensive disconnect handling.
KillAbandonedArmyproperly orchestrates all disconnect scenarios:
- Routes to appropriate kill functions based on ACU option
- Applies abuse detection only in assassination mode
- Forces FullShare in non-assassination modes for delayed/permanent options
- Handles all edge cases with appropriate warnings
1335-1398: LGTM: Improved AI cleanup consistency.The refactored
DisableAIfunction uses consistent patterns for:
- Empty table checks
- Platoon handle validation
- Explicit nil assignments for cleanup
|
Yeah, that will have to wait for another time. |
This PR refactors the AIBrain's defeat code. The main change is moving the unit destruction logic to be all in one spot, in
SimUtils.lua, no matter how the brain was defeated.In the process, in was discovered that the armies are handled differently in a recall (it transferred only built units before the 10 seconds wait - it now also transfers unbuilt units, and after the 10 grace period like in a regular defeat), so that has now been standardized.
TransferUnitsToKillerwas also using the incorrect callback in one particular case.Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.