Fix AddToAggroList() LOS Checks #489
Conversation
There were two AddToAggroList() methods, and the wrong was was being overriden. I've replaced it with a single method that takes an optional parameter.
Added always_check_los_keep_guard server property, which defaults to false, to replace keep guards being hardcoded to not check LOS.
Moved checks in LordBrain.BringFriends() to KeepGuardBrain.AssistLord(). Also reduced lord aggro range to the room/roof they'll be found on.
Keep guards were hardcoded in two different ways to not check LOS in AddtoAggroList(). That is now instead chosen by a new server property.
AddToAggroList() was sending a LOS request to the client, and expecting an instantaneous response stored in m_AggroLOS. Not only does that not get the actual output until the next Think() cycle, but it also risks applying the result to the wrong target. I've split AddtoAggroList() into two parts to allow for a proper callback response akin to StartAttack() and ContinueStartAttack(). Aggroamounts are stored in a list by ObjectID while the LOS check is pending, and I've tried to streamline that as much as possible. I've also added a server property to determine whether or not to check LOS when adding aggro to a keep guard, to replace them being hardcoded to not check. The property defaults to false, so keep guard behaviour shouldn't change. I'm seeing some odd behaviour by keep guards when set to true, but I want to make sure the LOS code in general is good before I get into the weeds puzzling that out. Mobs checking for LOS will be impacted by this, but my testing hasn't revealed any unusual behaviour. It should, if anything, be more efficient by adding aggro before the next Think() cycle and saving redundant CheckNPC() and CheckPlayer() calls and their associated LOS checks.
|
This conflicts with #488 😅 This PR is going to need a bit of testing as LOS checking and aggro is finicky in general. How did you test correct behavior? 🙂 |
|
I use a LOT of log entries to track behaviour in real time, and filter which NPCs I'm tracking on via OID. Keeps and DF are particularly good in terms of LOS checks, but I suspect parts of Tuscaren Glacier would also work pretty well. One of the things I noticed was GameKeepGuard.Think() runs CheckPlayerAggro() and CheckNPCAggro(), and then keep guard brains then run it a second time. I think I'm going to break this into smaller pieces, so it's easier to test each step independently. I've been working on something else recently, without this change added, and now that I know to look for it, I'm seeing those weird intermittent aggro issues every now and then when using a pet. If only one of us is in LOS, the mob will sometimes fail to aggro for a few cycles, and at other times will get aggro to the one out of LOS, turn to face them, but then stall out because the attack LOS fails. |
Would it be possible to add something to the admin/GM tooling via a command or such to debug this kind of issue?
Are the problems you describe with or without this PR?
Yeah, the AI code is really tangled. It's certainly in need of cleaning/refactoring. 😅 |
I noticed PvE keep lords calling BringFriends() and bringing all the keep guards to them before a hostile player was anywhere near them, because keep guards are hardcoded twice over to never check LOS in AddToAggroList(). That appears to cause other problems down the line with keep guard specific code needed to stop them from spamming LOS checks in StartAttack() when they try to attack players who shouldn't be in their aggro tables in the first place. I've tried to remedy that by adding a server property to enable keep guard LOS in AddToAggroList, which is disabled by default to maintain existing functionality.
In exploring that, I discovered that StandardMobBrain.AddAggroToList()'s LOS check is flawed. The LOS request is sent to the client, a callback is set up, but the result for all LOS checks are stored in a single local property so only the most recent LOS result is stored, and that property is checked before the callbacks have had time to run. Each Think() cycle calls CheckNPCAggro() and CheckPlayerAggro(), which make decisions based on the previous cycle's most recent LOS check result, which is then applied to all NPCs and players within aggrorange whether they're in LOS or not. It's possible the previous LOS check is from a target that is no longer in LOS or even in aggro range. I expect there are a lot of weird intermittent initial aggro issues stemming from this.
So I've split up AddAggroToList() in a manner akin to StartAttack() and ContinueStartAttack(). If no LOS check is done, AddAggroToList() calls ContinueAddAggroToList(), and behaviour shouldn't change. If a LOS check is done and there is LOS, CheckAggroLOS() will call ContinueAddAggroToList() and AttackMostWanted() to get the mob to attack immediately instead of waiting for the next Think() cycle. CheckAggroLOS() doesn't get the aggro amount passed to it, so a list is used to store the pending aggro amounts based on ObjectID. I've tried to streamline that as much as possible to prevent a race condition in which AddToAggroList() is adding items to the list faster than CheckAggroLOS() is removing them.
There are more issues with keep guards that I need to look into, but I want to make sure the StandardMobBrain LOS checks are correct before I get too far into that.
I expect we're going to want to change a few things before merging this. I don't have a lot of experience with callbacks and thread safety, so there may be potential issues I've overlooked, or a better way to get aggro amounts into CheckAggroLOS().