[DO NOT MERGE] Mount behavior v2 - testing branch, destined for upstream - #2
Open
icemansparks wants to merge 10 commits into
Open
[DO NOT MERGE] Mount behavior v2 - testing branch, destined for upstream#2icemansparks wants to merge 10 commits into
icemansparks wants to merge 10 commits into
Conversation
Bots hold a raw Player* master that dangles when the master logs out and the Player object is destroyed between AI ticks on the map-update threads. FollowAction::isUseful then crashes on fTarget->GetGUID() (reproduced twice, cores captured; same UAF family as mod-playerbots#2474; the crashing branch was introduced in mod-playerbots#2462). SetMaster() now records the master GUID and GetMaster() re-validates it through ObjectAccessor::FindPlayer, which never dereferences the stored pointer, returning nullptr once the master is gone. HasRealPlayerMaster, HasActivePlayerMaster and the GetGroupLeader fallback go through the validated accessor, UpdateAIGroupMaster clears a stale master at entry, and GossipHelloAction handles the now-possible null master.
Reattempt of PR mod-playerbots#1760 (reverted in mod-playerbots#1855). Bots now stay mounted when the master dismounts far away, and mount up to reach a distant master, dismounting at assist range when the master is in combat. Unlike the original PR, the shared target-based mount/dismount logic is untouched and the feature is scoped entirely to the master-following branch, which battleground bots never enter. This removes the BG mounted-loop regression that caused the revert. Both helpers also require the follow strategy, so bots told to stay are unaffected.
- Gate the whole master mount-mirror block on the follow strategy: a bot told to stay is parked and no longer mounts when the master mounts - Clamp the in-combat dismount range to at least 18yd: for non-warrior melee CalculateDismountDistance() is ~3yd, which made the bot ride to the master's exact position and hover at follow distance without engaging - Reduce the out-of-combat proximity range to 5yd so the bot follows until near the master and then mirrors their mount state
A mob fighting the master resolves as the bot's dps target well before the bot itself is in combat, but the master-follow branch decided mount state only against the master. The bot rode past the fight to the master's exact position and could oscillate between assisting and re-mounting without attacking. When an assist target exists, dismount at engage range of the target (dismount distance + combat reach) and stop touching the mount state while next to it, so the dps/tank assist trigger can take over. Also drop the 18yd assist-range clamp again: dismount happens at the class combat distance.
The mirror path had no distance condition, so a bot at 15yd mounted the moment the master did. Now: mirror within 5yd, walk between 5 and the mount threshold (21+ yd, where the cast time isn't worth it), and mount to close distance beyond that. Shared proximity constant for the mirror gate and the stay-mounted dismount range.
Replaces the hardcoded proximity constant with the existing (previously unused) AiPlayerbot.TooCloseDistance option, default 5.
GetMaster() validated the master GUID via ObjectAccessor::FindConnectedPlayer but then returned the cached raw pointer, which is exactly the stale pointer the validation was meant to guard against: after a same-GUID relog the lookup finds the new Player object yet the old (freed) pointer was handed back. Return the resolved object instead - a logged-out master yields nullptr, a relogged one yields the fresh Player.
…ting Execute() already computes distToMaster once per tick; StayMountedToCloseDistance and ShouldMountToCloseDistance each recomputed the same GetDistance2d, so a mounted bot near a dismounted master ran the distance calc three times per tick. Thread the value through - no behaviour change.
UpdateAIGroupMaster only cleared the raw master pointer when the master resolved to nullptr. A same-GUID relog yields a fresh Player object, so GetMaster() returns the new one while the cached raw pointer still points at the destroyed one, and every raw master dereference further down the function is a use-after-free until PlayerbotMgr happens to call SetMaster again. Compare against the re-resolved master instead.
…ot only when near The near-only mirror gate left a dead band between TooCloseDistance and CalculateMountDistance() where neither the mirror branch nor the mount-to-close branch fires: 5-21 yd for melee, and 5-38.5 yd for casters, since CalculateMountDistance() is max(21, SpellDistance + 10) and SpellDistance defaults to 28.5. CalculateMountDistance() is the break-even for closing a FIXED gap, so it is the wrong test against a master who is actively opening one. And a ground mount matches the master's speed rather than beating it, so a bot that waits for the gap to cross the threshold does not claw it back - it holds that distance for the rest of the ride. A caster trailing 38 yd behind is the visible symptom. Mirror when near or when the master is moving. ShouldFollowMasterMountState already requires the master mounted, the bot unmounted, no attackers and the bot out of combat, so the added clause only fires for a safe bot on foot behind a moving mounted master. A stationary master keeps the deliberate near-only rule.
icemansparks
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not for merge here. This branch carries the mount behavior v2 work, rebased onto a current base so it builds again, and is kept as a staging branch until it is proposed upstream.
What is in here
Master pointer validation. Bots hold a raw
Player* masterthat dangles when the master logs out and the Player object is destroyed between AI ticks on the map-update threads.SetMaster()now records the master GUID andGetMaster()re-resolves it throughObjectAccessor::FindConnectedPlayer, returning the freshly resolved object or nullptr. Callers that dereferenced the raw member go through the validated accessor, andUpdateAIGroupMasterre-syncs the cached pointer on any change rather than only on loss, so a same-GUID relog cannot leave a stale pointer behind.Mount behavior. Bots stay mounted while closing distance to a dismounted master, mount up when riding actually beats running, and dismount to engage an assist target instead of riding past the fight to the master's position. Mount handling only applies while the bot is following; a bot told to stay keeps its current mount state.
The mirror rule fires when the bot is within
TooCloseDistanceof the master or whenever the master is actually riding. Distance alone left a band betweenTooCloseDistanceandCalculateMountDistance()where neither the mirror nor the mount-to-close branch fired: 5 to 21 yd for melee and 5 to 38.5 yd for casters, sinceCalculateMountDistance()ismax(21, SpellDistance + 10)andSpellDistancedefaults to 28.5.CalculateMountDistance()is the break-even for closing a fixed gap, which is the wrong test against a master who is opening one, and a ground mount matches the master's speed rather than beating it, so a bot that waited for the gap to cross the threshold held that distance for the rest of the ride.Rebase notes
Upstream replaced
HasRealPlayerMaster,HasActivePlayerMasterandIsAltwithHasGameClientMasterandIsAltBot. Resolved by keeping the upstream shape and routingHasGameClientMasterthrough the validated accessor:Both helpers are null-safe, and the
master == botshort circuit inGetMaster()keeps selfbots out of a recursion throughIsSelfBot.feature/mount-behavior-v2-upstreamis the same commits rebased onto current upstream master, for the eventual upstream proposal. Its only conflicts were the east-const conversion touching the line above the two new declarations inCheckMountStateAction.h;apps/codestyle/codestyle-cpp.pypasses all checks on it.Status
Builds clean on both bases. Behaviour still needs review and hands-on testing before this is proposed upstream.