Added patrols.lua library file #4513
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new library file patrols.lua that provides functionality for managing creature patrols. The library allows assigning heroes or parties to patrol between specified locations, with automatic succession when patrol members die.
Changes:
- Added patrol management system with circular route navigation
- Implemented death event handlers for leader and backup succession
- Added RegisterPatrol public API for creating patrol routes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else | ||
|
|
||
| RemoveTrigger(triggerData.trigger) | ||
| end |
There was a problem hiding this comment.
When the leader dies and there is no backup, the old trigger for the dead leader is removed but the patrol remains in the Game.patrols table. The UpdatePatrol function will continue to be called for this patrol, but will do nothing since patrol.leader is nil. However, the patrol entry remains in memory indefinitely. Consider removing the patrol from the Game.patrols table when there are no more creatures to continue it, to prevent accumulation of dead patrol entries.
| if patrol.leader and patrol.leader.party[2] then | ||
| patrol.partybackup = patrol.leader.party[2] | ||
| local trigger2 = RegisterCreatureDeathEvent(BackupDeath, patrol.partybackup) | ||
| trigger2.triggerData.patrol_idx = triggerData.patrol_idx |
There was a problem hiding this comment.
When the old backup creature dies and a new backup is assigned from the leader's party, the old trigger for the deceased backup is not removed. This will cause the BackupDeath trigger to remain in the Game.triggers table even though the creature is dead, leading to a memory leak of unused triggers. The trigger should be removed before registering a new one, similar to how it's handled in the LeaderDeath function at line 69.
| patrol.next_post = (patrol.next_post % #patrol.positions) + 1 | ||
| target = patrol.positions[patrol.next_post] | ||
| end | ||
| if (patrol.leader.moveto_pos.stl_x ~= target.stl_x or patrol.leader.moveto_pos.stl_y ~= target.stl_y) then |
There was a problem hiding this comment.
Potential nil access error. If the creature has not been given any movement orders yet, moveto_pos might be nil or uninitialized, causing this line to error when accessing moveto_pos.stl_x or moveto_pos.stl_y. Consider adding a nil check or initializing moveto_pos to a safe default value when first creating the patrol.
| if (patrol.leader.moveto_pos.stl_x ~= target.stl_x or patrol.leader.moveto_pos.stl_y ~= target.stl_y) then | |
| local moveto_pos = patrol.leader.moveto_pos | |
| if (moveto_pos == nil | |
| or moveto_pos.stl_x ~= target.stl_x | |
| or moveto_pos.stl_y ~= target.stl_y) then |
| if leader.party[2] ~= nil then | ||
| local partybackup = leader.party[2] |
There was a problem hiding this comment.
Potential nil access when checking leader.party. If leader doesn't have a party field (or party is nil), accessing party[2] will cause an error. Although Creature.lua defines party as a field, it may not always be initialized for all creatures. Consider adding a check to ensure party exists before accessing party[2].
| patrol.leader = patrol.partybackup.party[1] | ||
| if patrol.leader == nil then | ||
| patrol.leader = patrol.partybackup | ||
| end |
There was a problem hiding this comment.
The succession logic is confusing. According to Creature.lua line 9, party[1] is always the leader of the party. So patrol.partybackup.party[1] would be partybackup itself if partybackup is a party leader. The condition at line 53 will always be false if partybackup has a party (since party[1] == partybackup), making this check unnecessary. The intent appears to be to promote partybackup to the new leader, which should be done directly without the intermediate check.
| patrol.leader = patrol.partybackup.party[1] | |
| if patrol.leader == nil then | |
| patrol.leader = patrol.partybackup | |
| end | |
| -- Promote the backup unit directly to be the new leader | |
| patrol.leader = patrol.partybackup |
| function BackupDeath(eventData,triggerData) | ||
| local patrol = Game.patrols[triggerData.patrol_idx] | ||
| if patrol == nil then | ||
| return | ||
| end | ||
|
|
||
| if triggerData.unit == patrol.leader then | ||
| -- Leader died, ignore backup death | ||
| return | ||
| end | ||
|
|
||
| if patrol.leader and patrol.leader.party[2] then | ||
| patrol.partybackup = patrol.leader.party[2] | ||
| local trigger2 = RegisterCreatureDeathEvent(BackupDeath, patrol.partybackup) | ||
| trigger2.triggerData.patrol_idx = triggerData.patrol_idx | ||
| else | ||
| patrol.partybackup = nil | ||
| end | ||
|
|
||
| end |
There was a problem hiding this comment.
The BackupDeath trigger is never removed when the backup dies. Unlike LeaderDeath which removes the trigger at line 69 when there's no backup left, BackupDeath doesn't clean up its own trigger. This means when a backup creature dies, their death trigger remains in the Game.triggers table indefinitely, causing a memory leak. The trigger should be removed using RemoveTrigger(triggerData.trigger) before potentially registering a new one or when setting partybackup to nil.
| if patrol.leader.party[2] then | ||
| patrol.partybackup = patrol.leader.party[2] |
There was a problem hiding this comment.
Potential nil access when checking patrol.leader.party. If patrol.leader exists but doesn't have a party field (or party is nil), accessing party[2] will cause an error. Although Creature.lua defines party as a field, it may not always be initialized for all creatures. Consider adding a check to ensure party exists before accessing party[2].
| if patrol.leader and patrol.leader.party[2] then | ||
| patrol.partybackup = patrol.leader.party[2] |
There was a problem hiding this comment.
Potential nil access when checking patrol.leader.party. If patrol.leader exists but doesn't have a party field (or party is nil), accessing party[2] will cause an error. Although Creature.lua defines party as a field, it may not always be initialized for all creatures. Consider adding a check to ensure party exists before accessing party[2].
| end | ||
| else | ||
|
|
||
| RemoveTrigger(triggerData.trigger) |
There was a problem hiding this comment.
The code attempts to remove the trigger using triggerData.trigger, but triggerData.trigger is never set anywhere in this file. The trigger system doesn't automatically populate triggerData with a reference to the trigger object. This means RemoveTrigger is being called with nil, which will fail to remove the trigger and cause a memory leak. The trigger reference needs to be stored in triggerData when it's created, or the trigger should be removed using a different mechanism.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.