Playerbot correctness campaign for Zero: movement, visibility, class …
…combat, lifecycle (#476)
* [Playerbots] Two per-tick costs that were never meant to be per-tick
Both of these take a default their sibling declines, and both sit on the
path a bot walks when it has nothing to do.
HuntersPetDeadTrigger was declared with BEGIN_TRIGGER, which forwards only
`ai` and leaves checkInterval at 1. When the pet is not in world the trigger
falls through to Pet::GetStatusFromDB, so a petless hunter issued a
character_pet SELECT every tick -- roughly ten a second, on a map worker
thread, holding up every other player on that map. HuntersPetUnhappyTrigger
nine lines below already passes 300; this one just never did. Spelling the
class out gets it the same treatment, and CastRevivePetAction's two further
queries only run once the trigger fires, so they come down with it.
NearestGameObjects took the ObjectGuidListCalculatedValue default of 1 while
NearestUnitsValue explicitly passes 5. Gather strategy pairs "no possible
targets" with "add gathering loot", so the bot standing in an empty field --
precisely the idle one -- ran a full sightDistance GameObject grid search
twenty times a second. Nothing it looks for appears that fast.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Stop random bots being evicted into zones that evict them again
PR #219 added an untimed eviction branch to ProcessBot: a bot standing where
IsZoneSafeForBot says no is teleported away, and ProcessBot reaches every bot
about once a minute. That was self-consistent at the time, because
RandomTeleportForLevel built its candidate list from creature spawns and put
every candidate through the same predicate, so an evicted bot always landed
somewhere the next pass would accept.
ef7f5a42 rewrote that search to pick a random game_tele, vet only the anchor,
and hand off to RandomTeleport, which samples spawns within
randomBotTeleportDistance/2 of it with no vetting at all and then jitters by
grindDistance/2. The trap stayed; the guarantee did not. A bot could be
dropped up to 550 yards from the only point anyone checked, and be evicted
from there sixty seconds later, indefinitely.
The asymmetry made it certain rather than merely possible. The anchor filter
bounded the zone level from above only, and passed the zone's own level to
IsZoneSafeForBot -- measuring the zone against its own creature stats, which
nearly always passes. The eviction check passes no level, so it uses the
bot's. A level 60 bot was therefore free to be sent to a level 10 zone and
guaranteed to be thrown out of it on the next pass.
So: vet the landing point, after the jitter and the floor snap, with exactly
the predicate ProcessBot will apply to it. Bound the anchor's zone level from
both sides instead of vetting the anchor against the bot -- almost no
game_tele sits in an area whose stats bracket a given bot, and doing it there
rejected all hundred attempts. Offer the sampler pre-filtered candidates, as
it had before the rewrite. Put a cooldown on the eviction branch so a bot
with nowhere to go stops re-running the search every pass.
GetZoneLevel is in here because it is what made the search unaffordable: an
AVG() over a creature x creature_template join whose ABS() predicates no
index can serve, so it scanned every spawn on the continent, up to a hundred
times per teleport, inside a 50ms budget. CalculateAreaCreatureStats already
holds the same levels per area in memory and touches no database. It is also
the map IsZoneSafeForBot judges by, so reading it here makes the anchor
filter and the safety check agree rather than measure two different things.
Measured on 38 bots: "Cannot teleport bot" 39 -> 1, successful landings
17 -> 62, and the processing queue drains instead of reporting "more
pending" on every pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Only bank a teleport's bookkeeping when the teleport happened
Review follow-up on 465476a9. The eviction branch wrote both its events
unconditionally, so the two ways a relocation can fail were both recorded as
though it had worked.
RandomTeleportForLevel and both RandomTeleport overloads now report whether
the bot actually moved. ProcessBot's unsafe-zone branch marks "teleport" spent
only on success, and takes the evictcheck backoff only on failure -- which is
the right way round. A successful eviction leaves the bot somewhere this same
predicate accepts, so a cooldown over it would do nothing but delay the next
legitimate move; a failure is the case that must not re-run a hundred
game_tele draws every pass.
RandomizeFirst broke out of its search regardless of the result, which left a
bot freshly levelled but standing where it was created. That is how the fleet
came to be sitting in starting zones at every level: CleanRandomize had given
them a level to match a zone they were never carried to. It now stops only
once the bot has been placed, and the anchor is already vetted with the same
predicate the landing uses, so a retry should be rare rather than routine.
RandomTeleportForLevel likewise kept its 100 attempts instead of returning
after the first candidate, so one unlucky landing no longer ends the search.
Refresh still runs unconditionally in the mapId overload: the dead-bot path
calls it to revive in place, and a bot that could not be relocated still has
to come back alive where it stands.
Also two diagnostics, because the fleet is stuck at 38 bots against a
configured 50-200 with 502 free characters and no explanation visible in the
log: report the roster against the target each pass, and say which faction
came up empty when AddRandomBot finds nothing to add.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Remember the misses too, or the event cache never stops querying
GetEventValue only wrote m_eventValueCache when the SELECT returned a row.
Two consequences, and between them they were most of what the 50ms update
budget was being spent on.
An event with no row never reached the cache write at all, so every later
call issued a fresh synchronous SELECT on the world thread. Most events are
absent for a healthy bot, and ProcessBot asks after several per bot: "dead"
and "revive" for anything alive, and -- my own doing in 465476a9 --
"evictcheck", whose row exists only for a bot whose eviction failed. That
added one guaranteed query per bot per pass to a loop that walks the whole
roster.
An expired row was worse than useless: it cached {0, storedTime, validIn},
and since storedTime never moves, the freshness test could not pass again.
So it queried on every call from then on, forever.
Cache the zero either way, stamped now and good for one update interval. Any
SetEventValue overwrites the entry, so a real value is never masked by this.
Also report bots examined alongside bots processed. Only bots that did
something were counted, but every bot the pass walks past pays for its event
lookups, so the old number hid the work: at 200 bots the fleet was reporting
a mean of 0.95 processed per pass with the budget exhausted every single
time, which reads as idle rather than as saturated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Inherit an area's faction from its parent zone
A level 5 Horde bot was standing in Shadowglen, and 22 bots were on the wrong
side of the world at once.
IsZoneSafeForBot resolves a position with GetAreaId, which answers with the
most specific area. In AreaTable.dbc it is the parent zone that carries
FactionGroupMask, not the sub-area anyone actually stands in: Teldrassil is
AREATEAM_ALLY, but Shadowglen, Dolanaar and Aldrassil inside it are all
AREATEAM_NONE. Same for Northshire Valley under Elwynn, Coldridge Valley
under Dun Morogh, Valley of Trials under Durotar, Camp Narache under Mulgore
and Deathknell under Tirisfal. Reading only the leaf meant the faction test
was skipped in precisely the places a new player spends their first hour,
and the AREATEAM_NONE branch only rejects on guard presence -- which a
starting area has none of. So the check returned "safe", and once a bot was
there nothing would ever move it on.
Walk up ParentAreaID to the first ancestor that declares an owner. Checked
against the 1.12 DBC: 255 of 1081 areas were carrying no faction of their own
and now resolve to one. Contested ground is unaffected -- Stranglethorn Vale
and Ashenvale have no faction at any level of the chain, so they still fall
through to the guard-presence test, which is what should decide them.
The guard lookup deliberately keeps using the specific area id: guards are
recorded per area, and inheriting those would be wrong.
Expect a wave of relocations on the first run after this, as bots already
sitting in the 255 newly-owned areas are finally judged unsafe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Drop stale eviction cooldowns at startup
The evictcheck backoff added in ee37c57e is a runtime throttle, but
SetEventValue persists everything to ai_playerbot_random_bots, so it survives
a restart. A bot that had nowhere to go before a shutdown is still serving
its ten minutes afterwards -- including across the very restart that
installed the fix for whatever stranded it, which is exactly what happened
while this branch was being tested.
Clear the rows on the first pass. Nothing else reads them, and a fresh start
should be a fresh chance.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Let a hub both sides use keep its neutrality
Inheriting a faction from the parent zone handed Ratchet to the Horde along
with the rest of the Barrens. It is a goblin port Alliance players use for the
Booty Bay run, so excluding them from it is wrong in the one direction the
previous commit could be wrong in.
The world data already marks these places, so no list is needed:
FACTION_TEMPLATE_FLAG_CONTESTED_GUARD is carried by 8 of 314 faction
templates, and every one of them has an enemyGroup that is hostile to neither
player faction. The Steamwheedle bruisers in Ratchet, Booty Bay, Gadgetzan and
Everlook hold it; no faction guard does. An area containing such a creature is
recorded and keeps its own AREATEAM_NONE instead of inheriting, so it falls
through to the guard-presence test -- where those same bruisers, hostile to
nobody, exclude nobody. Which is the answer we want.
Not gated on CREATURE_FLAG_EXTRA_GUARD: Ratchet's and Gadgetzan's bruisers
carry that flag, Booty Bay's and Everlook's do not.
Checked what this actually marks. Spawned contested-guard creatures sit in
Ratchet, Booty Bay, Gadgetzan, Everlook, Moonglade, Cenarion Hold and Light's
Hope Chapel. Every one of those except Ratchet is already inside a contested
zone and resolved to NONE anyway, so Ratchet is the only place the behaviour
actually changes. The Lunar Festival spawns that share the faction are in
Moonglade rather than in a capital, so no faction city is affected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Review follow-ups: area resolution, the randomize retry, and a missed branch
Four fixes from the five-reviewer round on this branch.
Resolve areas per position instead of caching them per grid cell. This was the round's
one BLOCK. A cell is about 33 yards and area borders do not follow the grid, so caching
by cell let whichever position was asked first answer for every later position in it.
Harmless for a rough level band, actively wrong for the faction and neutral-hub decisions
added earlier on this branch: it can hand a border cell of the Barrens to Ratchet's
neutrality, or hide Ratchet behind the Barrens' owner -- the same cross-faction placement
the faction work set out to stop. The classification pass in CalculateAreaCreatureStats
had its own copy of the cache and the same flaw, so that goes too. GetAreaId is a terrain
lookup and this path no longer issues SQL, so the cache is not worth its correctness cost.
Hoist CleanRandomize out of RandomizeFirst's retry loop. All three reviewers flagged it
independently. Making the loop break only on a successful teleport put a full re-level and
re-gear -- talents, spells, inventory, equipment, four SaveToDB -- inside a loop that can
run a hundred times for one bot, and the 50ms budget is only checked between bots, so the
tail was a multi-second stall of the world thread. The adversarial pass found the trigger
that makes it reachable rather than theoretical: RandomTeleport returns false when the bot
is already IsBeingTeleported, so a bot with an in-flight teleport that catches the randomize
event walks all hundred attempts. Now the search runs as often as it likes, since every
check in it is cheap, and CleanRandomize runs exactly once after a destination has passed
them all. If the placement then fails the bot keeps its level and stays put, and the
eviction branch relocates it on a later pass like any other badly-placed bot.
Apply ee37c57e's own rule to the branch it missed. That commit exists to stop recording a
teleport that did not happen, and fixed the eviction branch while leaving the regular
teleport branch banking the event either way -- so a bot that could not be placed had its
next move suppressed for maxRandomBotInWorldTime regardless.
Cap the parent walk's depth. The shipped AreaTable has no self-references and a longest
chain of two, so this is a guard against a modded or corrupt DBC spinning the world thread
forever, not a real limit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Random bots could never be sent to Eastern Kingdoms
LoadList drops every token whose atoi is zero. That is right for the spell and item ids it
was written for and wrong for a map id, because map 0 is Eastern Kingdoms. The shipped
default "0,1,530,571" therefore parsed to {1, 530, 571}: Kalimdor, plus two maps that do
not exist on a 1.12 core. Half the world has never been reachable by a random bot, and
roughly half of every teleport search was spent drawing maps that resolve to nothing.
Confirmed before touching it. Across three 200-bot runs, every one of the thirty most
common landing areas was on Kalimdor -- Feralas, Desolace, Silithus, Felwood, Dustwallow,
Winterspring, the Barrens. Not one Eastern Kingdoms location appeared in any of them.
Parse the map list separately, accepting 0 and rejecting anything non-numeric, and fall
back to map 0 when the result is empty: both callers index with urand(0, size() - 1), so
an empty vector underflows and reads out of bounds rather than complaining.
Also floor RandomBotUpdateInterval at 1. Three throttles are derived from it -- the pass
reschedule, the negative event-cache TTL, and the eviction backoff at ten times it. At
zero every one of them silently becomes a no-op: a zero TTL can never satisfy its own
freshness test, so event lookups fall back to a synchronous query per bot per pass, and
the eviction search re-runs every tick. The adversarial reviewer's point stands that one
config value should not be able to turn off the fixes that now depend on it.
Expect a large redistribution on the first run after this. Eastern Kingdoms becomes
available for the first time, so a substantial share of the fleet will relocate there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Give AiObject a virtual destructor, and stop counting to ten
Two lifetime defects from the engine audit.
NamedObjectContext<T>::Clear deletes everything it created through a base pointer --
`delete *i` over a vector<T*> -- and T is Action, Trigger, Strategy or UntypedValue. The
first three each declare a virtual destructor of their own. UntypedValue does not, so
deleting a value through ValueContext ran ~UntypedValue and stopped: no derived destructor,
and every non-trivial member left behind. The list nodes in the ObjectGuidList values, the
Item* list in InventoryItemValue and the string buffer in RtiValue all leaked, on every bot
logout and every random-bot removal, which at a fleet of 200 on long-running timers is not
a rare event. Declaring the destructor on AiObject fixes the whole hierarchy at once
instead of patching each root as it is noticed.
Separately, NextAction::size stopped counting at ten. The arrays are NULL-terminated --
array(), clone() and merge() all place the terminator -- so ten was not a safety bound,
just an arbitrary stop. clone() and merge() therefore truncated any longer list, destroy()
freed the array while leaking every entry past the tenth, and Engine::MultiplyAndPush
pushed only the first ten alternatives. That last one carried a `// TODO: remove 10`
already. Nothing in the shipped strategies builds an array that long, so this is a latent
trap rather than a live leak, but it fails silently in both directions at once: actions
disappear and memory is kept.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Stop crashing on every shutdown, and actually save the bots
mangosd faulted on every single stop -- nine crash dumps, eight of them in one day -- and
the reason it also never persisted bot state turns out to be the same one.
RandomPlayerbotMgr is a MaNGOS::Singleton, so ~PlayerbotHolder runs during CRT static
destruction, after Master::ShutdownWorld has already called sMapMgr.UnloadAll(). It then
walked 200 bots through LogoutPlayerBot -> WorldSession::LogoutPlayer, which for a live bot
reaches Player::SaveToDB -> Map::GetEluna and for a dead one reaches BuildPlayerRepop ->
Spell::CheckCast -> WorldObject::GetTerrain. Both dereference a map that no longer exists.
Both dumps were present, which is why the crash wore two sizes depending on whether the bot
happened to be alive.
Nothing caught it because bot WorldSessions are created by the playerbot module and never
registered in World::m_sessions, so KickAll -- the thing that saves players before the maps
go -- could not see them. The save was not merely lost, it was interrupted mid-write.
So log them out from KickAll, while the maps are still there, and guard the destructor with
World::IsStopped(). The guard matters because two very different objects share that
destructor: a per-player PlayerbotMgr is destroyed while the world is running, where
logging out is exactly right, and WorldSession::LogoutPlayer already calls LogoutAllBots
explicitly on that path ungated -- so the guard only ever skips work that is already done.
Verified: the shutdown at 11:24:22 was the first today to leave the Crashes directory
unchanged.
Reviewed independently; it confirmed the ordering, that bot sessions are absent from
m_sessions, that a real player's bots are unaffected by the guard, and that there is no
re-entrancy between LogoutAllBots and the m_sessions loop. Two follow-ups it raised are not
addressed here: ShutdownServ's own LogoutAllBots call is now redundant on one path, and
Windows CTRL_CLOSE_EVENT is still unhandled so closing the console saves neither bots nor
real players.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Stop handing every bot a 499% flying mount
InitMounts grouped every SPELL_AURA_MOUNTED spell by speed and then learned one from every
group, twice over, with no check of level, class, race or riding skill. Two results, both
confirmed against the live character_spell table before touching the code: all 201 bots
knew 3363 Summon Riding Gryphon -- a 499% flyer, and the only member of its speed group --
including 40 characters below level 10; and class mounts leaked freely, with the Paladin
Warhorse (13819) on priests, rogues and druids, and the Warlock Felsteed (5784) likewise.
Riding skill is the real gate and InitSkills already derives it from level, so use it: 75
buys the 60% tier, 150 buys the 100%. Effect base points carry speed-minus-one, so the
ceiling is 59 or 99 and anything above is a flying mount 1.12 does not have.
For class restriction the shape of the data decided the rule. Checking SkillLineAbility
race and class masks was the obvious approach and would have been wrong on its own: in
1.12 the racial and vendor mounts have no SkillLineAbility row at all -- Brown Horse 458,
470, 6648, 468 all return nothing -- so treating an absent row as "forbidden" would have
left every bot on foot. Class mounts do carry one, and it names the class exactly
(Warhorse classMask 2, Felsteed classMask 256). So honour the masks where a row exists and
treat their absence as unrestricted.
Also learn one mount at the best tier the bot qualifies for, rather than one from every
tier and then the whole set again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Stop re-answering line of sight and re-walking bags twice a tick
Two per-tick costs the optimisation audit ranked first and second, both cheap to fix.
CurrentTargetValue::Get ran a vmap raycast on every read, and "current target" is consulted
several times per tick by triggers and actions alike -- an estimated ten to thirty thousand
raycasts a second across two hundred bots. Cache the answer for 250ms against the guid it
was decided for, and clear it in Set() so a new target is judged on its own. A quarter
second of staleness costs nothing: the core re-checks line of sight itself when the spell
is actually cast, so the worst case is one wasted cast attempt rather than a wrong outcome.
ItemCountValue, InventoryItemValue and ItemForSpellValue all took the CalculatedValue
default of checkInterval 1, so every food, drink, soul-shard and reagent trigger walked the
bot's whole bag and equipment set on every tick. Five ticks is a quarter of a second, and
inventory does not change inside that in any way a bot needs to react to.
The name argument passed to those constructors is display-only -- NamedObjectContext::create
keys on the string handed to it, not on the object's own name -- so giving them their
context keys changes nothing but the debug output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Give paladins a healing spec, and stop warlocks tanking
Paladin was the only healing class in the module with no heal strategy. Priest, druid and
shaman each register one and ship an implementation; paladin's directory held nothing but
Dps and Tank. So AiFactory had nowhere to send a Holy talent build and sent it to dps --
about a fifth of paladin bots by the shipped spec probabilities, running a damage rotation
on a healing build, which is a standing shortfall of healers in every bot group.
It reads like a missing branch in AiFactory and is not one. Adding that branch alone would
have bound "heal" to nothing, and Engine::CreateActionNode fills an unresolved name with a
null placeholder rather than complaining -- so the bot would have quietly done less than it
does today, with nothing in the log to say why.
The actions all existed already: holy light, flash of light, lay on hands, the three
cleanses and redemption, in both self and party form, all registered. Only the strategy
binding them to triggers was absent.
HealPaladinStrategy inherits GenericPaladinStrategy so the medium/low/critical bindings
keep one definition rather than a second copy that drifts. What it adds is the part that
makes a healer rather than a class that owns heal spells: flash of light top-ups at almost
full health, before anyone is in trouble; divine protection and divine shield when the
paladin itself is attacked, because a healer being hit is a healer not casting; mana
potions at medium mana; and redemption on a dead party member. Its default action is melee
-- a priest falls back to a wand and a 1.12 paladin has no ranged attack at all.
Every trigger and action name was checked against the registries before building. Two I
first reached for, "party member to heal out of spell range" and "reach party member to
heal", do not exist; the idiom the other three healers use is "enemy out of spell" ->
"reach spell", with heal-target range resolved inside HealPartyMemberAction.
Warlock Demonology mapped to the tank strategy. In 1.12 that tree makes the pet durable,
not the warlock, so by the configured probabilities a third of warlocks were running a tank
rotation they cannot perform. All three specs are damage builds here.
Note this does not take effect for random bots yet: randomBotCombatStrategies is applied
after the spec choice and its "+dps" evicts whatever was selected. That is a deliberate
behaviour change to every random bot and belongs in its own commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Let a random bot keep the spec its talents chose
AddDefaultCombatStrategies picks a combat strategy from the bot's talent tab and then, for
random bots, applies randomBotCombatStrategies over the top. That config defaults to
"+dps,+attack weak", and dps is a sibling of tank and heal in the same supportsSiblings
context, so addStrategy evicted whatever had just been selected.
The effect was that RandomClassSpecProbability did nothing for random bots. A Protection
warrior or a Restoration shaman was given its spec strategy and had it taken away a few
lines later, leaving a damage build whatever the talents said -- and the talents themselves
were still spent in the chosen tree, so the bot ended up with a tank's talents driving a
dps rotation. Mages were the accidental exception, having no dps sibling to be replaced by,
which is why the behaviour looked class-dependent rather than simply broken.
Apply the config only where it agrees with the build already selected. That is the same
rule the player-owned branch a few lines above already follows, via the same
ContainsStrategy check.
This is also what makes the paladin healer in 10dee47a reachable: without it, "heal" was
selected and then immediately replaced by "dps" for every random paladin.
A tank or heal build now also forgoes the "+attack weak" half of that config. That is a
deliberate consequence rather than an oversight -- the config is a single string and
splitting it per role would be a config change, not a code fix -- but it is worth knowing
if the setting has been customised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Index the equippable items once instead of per slot, per tier, per bot
InitEquipment walked every id from 0 to sItemStorage.GetMaxEntry() and called
GetItemPrototype on each, for every equipment slot, and again for each quality tier it fell
back through. The bound is 24,283 but only 14,422 of those ids exist, so two in five
lookups resolved to nothing at all, and only 8,923 belong to a class that can ever be
equipped. Multiplied across sixteen slots and up to four tiers that is on the order of 1.5
million prototype lookups to gear one bot, and Randomize is the largest single stall on the
world thread.
Neither of the first two filters depends on the bot and item prototypes are fixed once the
world has loaded, so they are answered once for the process and shared by every bot. What
stays in the per-bot loop is the part that genuinely varies: level, quality, armour type,
weapon type and the final CanEquipUnseenItem check.
This is the safe half of the change. The larger win is bucketing that index by
InventoryType so each slot only inspects its own candidates -- roughly six hundred rather
than nine thousand -- and it is deliberately not done here. Reading Player::FindEquipSlot
turns up two things that make it less mechanical than it appears: INVTYPE_RELIC resolves by
SubClass as well as InventoryType, so one representative per InventoryType would not be
exact, and FindEquipSlot only returns a slot when that slot is empty unless swap is true.
Both are answerable -- bucket by (InventoryType, SubClass) and pass swap -- but the payoff
does not justify guessing at them without a run behind it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Spend leftover talent points in the tree that was actually chosen
InitTalents rolls a spec, spends what it can, and then poured whatever was left into a
DIFFERENT tree -- literally 2 - specNo. One inner pass cannot spend a full budget, because
it caps itself at five points and three attempts per talent row, so a level 60 with 51
points always came back with most of them unspent. Since GetPlayerSpecTab later reads
whichever tree holds the most points, the bot became whatever the leftovers landed in
rather than what was rolled.
That is why no Holy paladin has ever existed on this server despite a 20% roll. Twenty
paladins were online when this was found: seven Protection, five Retribution, no Holy at
all. Two of them, Lyneat and Mikkileay, both level 52, were holding exactly 2 points in
Holy and 11 in Retribution -- rolled Holy, spent what one pass allowed, had the rest tipped
into Retribution, and were then given the damage strategy the AI correctly derives from the
talents it finds.
Keep filling the chosen tree until a pass places nothing more, and only spill elsewhere
once it genuinely cannot take another point. The no-progress break is what stops a
saturated tree spinning.
This is the layer under two earlier fixes of the same symptom: AiFactory had no branch for
Holy (10dee47a) and the random-bot override then evicted whatever survived (465b2f6b).
Each looked like the whole story until the next one surfaced.
Existing bots keep their talents until their next Randomize, which is on a 2h-14d timer,
so Holy paladins will appear gradually rather than on restart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Review follow-ups: revert the inventory interval, finish the spec guard
Three items from the batch review. No blockers were found; two of these are corrections to
my own work.
Revert ItemCountValue, InventoryItemValue and ItemForSpellValue to checkInterval 1. Raising
it to 5 was a real CPU saving on slow-changing inventory and an unacceptable trade on the
rest: two of those three hand out Item* and list<Item*>, so a half-second-stale entry can
name an item the bot has since used or destroyed, and GiveConjuredFoodAction dereferences
what it is handed. There is a behavioural half too -- CastConjureFoodAction::isUseful
consults the same cached count, so after conjuring, the count reads zero for five more
ticks and the bot conjures again. Caching the counts while re-querying the pointers at
execute time would get the saving safely; the interval on its own cannot. The comment now
says so, so the shortcut is not retried.
The LOS cache in the same commit stands: it caches a bool against the guid it was decided
for, not a pointer.
Extend the random-bot spec guard to ranged builds. It skipped TANK and HEAL, and missed
that "dps" is resolved per class -- for a druid it creates CatDpsDruidStrategy and for a
shaman MeleeShamanStrategy, both melee. So a Balance druid or an Elemental shaman was given
its caster strategy correctly and then had a feral or melee one bolted on beside it. A
melee dps build still takes the config, where "dps" resolves to what it already has.
DpsWarlockStrategy never declared STRATEGY_TYPE_DPS, inheriting COMBAT|RANGED from
RangedCombatStrategy and stopping there, unlike CasterDruidStrategy and
CasterShamanStrategy. Nothing depends on it today, but ContainsStrategy(STRATEGY_TYPE_DPS)
is exactly what the strategy selector asks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] A player-owned bot answers to its master, not to anyone who can whisper it
PlayerbotSecurity::LevelFor put every real check -- faction, group, level gap, distance,
guild, dead state -- inside `if (IsInRandomAccountList(account))`. A bot on a normal player
account skipped that block entirely and fell out to the function's closing
`return PLAYERBOT_SECURITY_ALLOW_ALL`.
ALLOW_ALL is full control. Any same-faction, non-GM, non-bot player who could reach the bot
at all -- a whisper, a party, a raid -- could sell its items, destroy them, move them in and
out of its bank, spend its gold repairing or training, equip and unequip it, put its money
into a trade window, teleport it, and rewrite its AI strategies. The audit's own list of
reachable commands runs to eighteen entries.
Now only the bot's master gets ALLOW_ALL. Everyone else is capped at TALK, which is what
that level exists for, and receives the "not yours" refusal the DenyReason enum already
carried a message for.
Two guards above it had a second bug of their own:
if (from->GetPlayerbotAI())
{
if (reason) { *reason = PLAYERBOT_DENY_IS_BOT; return PLAYERBOT_SECURITY_DENY_ALL; }
}
The return sat inside the `if (reason)`. CheckLevelFor passes reason == NULL for every
silent check, so on that path the condition was evaluated and then fallen straight past:
"is a bot" and "is the opposing faction" denied nothing at all. Both now return whether or
not the caller wanted the reason string.
And "who" no longer bypasses the security model. It answers with the bot's class, level,
spec and gear, and it was the one command that skipped CheckLevelFor completely, so anyone
could interrogate any random bot. It now requires TALK; everything else still requires full
control.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* [Playerbots] Stop fanning public channel chat out to every bot, and judge a zone by what a bot may fight
Two unrelated fixes in the same file.
RandomPlayerbotMgr::HandleCommand took one message and handed it to every random bot in the
world. For a public channel that is a fan-out of one line to the whole fleet, so anything
that answers answers once per bot: a single "~who" typed in trade chat returned two hundred
whispers to whoever typed it -- information disclosure and a whisper flood from one message.
Per-bot security still applies underneath, and "who" now requires TALK, but a broadcast is
the wrong shape for a public channel however well each bot behaves on its own. Channel
messages are dropped here; whisper, party and raid are unaffected.
CalculateAreaCreatureStats counted elites toward an area's level band. GrindTargetValue
refuses to attack anything above CREATURE_ELITE_NORMAL, so the map that decides where a bot
may be placed and the one that decides what it may then fight disagreed. An area could be
judged suitable on the strength of creatures the bot is forbidden to touch, and a solo bot
would be dropped into a camp it cannot pull and which is perfectly willing to pull it.
Observed in play: Layaeatema, a level 53 priest, working an area of Feralas holding 88
normal spawns and 19 elites between levels 44 and 60. The elites carry the average up, the
area reads as right for a 53, and the bot is placed among them.
Same principle as the teleport churn fix: the predicate that places a bot and the predicate
that judges it afterwards have to agree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Default to four database connections rather than one
A single SELECT connection per database serialises everything that is not a
transaction. It is enough for a near-idle realm and visibly not enough for one
carrying a large playerbot roster, where startup and the bot manager's own
queries queue behind each other. Measured startup on a populated realm is
substantially faster at four.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Keep a bot young enough for a newbie zone in its own race's newbie zone
A level 6 gnome was standing in Teldrassil. The game permits it and no player
does it: reaching another race's starting zone at that level means crossing a
continent, which in practice meant being dragged there by somebody higher. The
random manager has no such story to tell, so below RandomBotHomeZoneMaxLevel a
bot is confined to the zone its own race actually starts in -- Elwynn, Dun
Morogh, Teldrassil, Durotar, Mulgore or Tirisfal. Default 10, which is where the
starting quests run out and the second zone takes over; 0 disables the rule.
The comparison is against the zone rather than the leaf area, so Coldridge
Valley, Northshire, Shadowglen, Valley of Trials, Camp Narache and Deathknell
all resolve to the parent that the racial start position names. GetZoneAndAreaId
returns both in one terrain lookup, so the check on the way in costs nothing
over the GetAreaId call it replaces.
Landing sites come from creature spawns inside those six zones, collected by the
pass that already walks every spawn to build the area level bands. Drawing from
a pool that is confined to the right zone matters: the generic search picks
blind from a whole map's anchors, so once a bot may only land in one zone the
odds of hitting it are poor enough that a hundred attempts can still come up
empty -- which is exactly how the previous over-strict filter produced "Cannot
teleport bot" for the entire roster. The generic path is still the fallback, and
it cannot place a bot outside its zone either, since the same rule is applied by
IsZoneSafeForBot on the way out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Hold the first few levels in the starting sub-area, not just the zone
Confining a young bot to its racial zone still let a level 1 night elf walk out
of Shadowglen into the whole of Teldrassil, which is not where its content is.
A real character spends its first handful of levels inside the one sub-area it
woke up in and only then works outward, so the confinement now narrows the same
way: RandomBotHomeAreaMaxLevel (default 5) holds a bot in Shadowglen,
Northshire, Coldridge Valley, Valley of Trials, Camp Narache or Deathknell, and
RandomBotHomeZoneMaxLevel (10) then opens the surrounding zone.
The sub-area is not recorded anywhere. playercreateinfo's zone column gives
Teldrassil, not Shadowglen, so it is resolved from the create position itself --
the only record of where a race really begins. When the map is not loaded the
lookup yields 0 and the caller falls back to zone granularity, deliberately: an
area nobody resolved must not become an area a bot is confined to.
Landing sites are now recorded against both granularities. A spawn in Shadowglen
belongs to the Shadowglen pool a level 1 draws from and to the Teldrassil pool a
level 8 draws from, since Shadowglen is part of Teldrassil; a spawn out in
Teldrassil proper belongs only to the latter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Ship the starting-zone rules on, and stop offering maps and a spell that do not exist
The home-zone and home-area limits are the behaviour we want by default, and a
commented line reads as "off" to anyone skimming the file, so both ship active
at 10 and 5.
Two shipped defaults alongside them were for a different expansion. RandomBotMaps
offered 0,1,530,571 -- Outland and Northrend, neither of which exists on a 1.12
core, so half of every map draw picked an id that could never resolve and threw
the teleport attempt away. RandomBotSpellIds taught 54197, Cold Weather Flying,
which is a Wrath spell. Both now say what this core can actually do.
ConfVersion is a date stamp nothing in the code reads; it exists so an admin can
see their conf predates the dist. This revision changes shipped defaults, which
is exactly the case it is there to flag.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Let the group command the bot again, without reopening it to strangers
Restricting a player-owned bot to its master alone closed the hole but also
stopped a party member directing bots standing in the group with them, which is
the ordinary use of the feature and not what was being abused: party and raid
chat is how a group plays, and "follow", "attack my target" and "stay" all went
through it.
Group membership is the consent signal -- somebody is only in it because a
player accepted them -- and it is the same test the random-bot branch a few
lines above already applies, so this is the module's own rule rather than a new
one.
Being precise about the exposure, because it is easy to overstate: Group here is
party AND raid, so joining a raid containing somebody's bot does confer control
of it to up to 39 other people. That is the same exposure the random-bot branch
has always had and it is bounded by someone having accepted an invite. What
stays shut is the case that mattered and was unbounded -- any same-faction
stranger who could reach the bot with a whisper could sell, destroy, equip and
teleport it with no acceptance step at all. If raid-wide turns out too broad the
answer is party-subgroup membership, not a return to master-only.
Honours ignoreGroup, so the callers that deliberately ask to be judged without
the group -- petition signing and guild invites -- are unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Give a dead random bot a way back on its feet
A random bot that died had no working route to resurrect itself, which is why
they accumulate at the starting-zone graveyard doing nothing.
"revive from corpse" refuses unless the corpse is within SpellDistance, and once
the core auto-releases the spirit the ghost is at the graveyard while the corpse
is wherever it fell. Nothing closes that gap: no action walks a ghost to its
body, and MoveTo would refuse the distance even if one did. Every other route to
a spirit healer is master-driven -- "spirit healer with master" waits on the
master's CMSG_SPIRIT_HEALER_ACTIVATE, and the bare "spirit healer" action was
reachable only as a typed chat command. An ungrouped random bot has neither, so
it stood there until RandomPlayerbotMgr's timer resurrected it minutes later.
DeadStrategy now falls back to the spirit healer, at a lower relevance so
reclaiming the body still wins whenever that is possible.
Second defect in the same path, found while reading it: the revive timer is
scheduled as randomTime - 60 on a uint32, so any RandomBotReviveTime under 60
wrapped to roughly 136 years. The revive event then never expired, the branch
that resurrects never ran, and the bot stayed dead until the dead flag lapsed
and reset the pair -- forever. The shipped minimum is exactly 60 so nobody has
hit it, but the first thing anyone does to get bots up faster is lower that
number, which would have produced the exact opposite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Stop the sub-area rule stranding undead and tauren bots
The first run using the sub-area confinement left 30 of 127 bots unable to be
placed anywhere, each re-running the full hundred-attempt search every pass and
logging "Cannot teleport bot" around 55 times. Every one was undead or tauren,
all level 1 or 2.
Three things combined, and each is fixed here.
The confinement was enforced as a rejection criterion in IsZoneSafeForBot, so an
area that could not accept a bot left it with no legal position at all rather
than a worse one. The sub-area is now expressed only where a bot is placed, in
RandomTeleportHome. A rule that can leave a bot nowhere to stand must be a
preference, not an invariant.
The undead sub-area was never detected. They start at 1676,1678 inside the
Deathknell crypt, and that point resolves to Tirisfal Glades rather than to
Deathknell, so the sub-area came back equal to the zone and no Deathknell pool
was ever built -- which is why the boot log lists five sub-areas for six races.
A level 1 undead could then be placed neither in Deathknell, which had no pool,
nor anywhere else in Tirisfal, whose open-zone band starts around level 5 against
a tolerance of 3. FindStartSubArea now asks which sub-area the nearest ordinary
creatures stand in whenever the create point answers with the zone, which is the
better question anyway since every landing site is a creature spawn.
And the sub-area pool can simply run dry: Camp Narache holds 17 landing sites
against Mulgore's 1779, so ten draws against the outdoor, water and level-band
filters exhausting it is ordinary rather than exotic. RandomTeleportHome now
falls back to the zone pool, because a bot placed slightly further out than
intended is enormously better than a bot that cannot be placed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* A ghost with no corpse can still use the spirit healer
The spirit healer fallback could never fire, because SpiritHealerAction opened by
demanding a corpse and returning false without one. That is backwards: the spirit
healer is precisely what you use when there is no corpse to go back to. Observed
on a live server with a night elf standing at the Shadowglen graveyard as a wisp
and the corpse table holding zero rows for the entire realm -- so the test
rejected exactly the bots that had no other way up. Being dead is the whole
precondition, and that is what it checks now.
Also finishes the Deathknell fix, which was only half done. GetRacialStart learned
to fall back to the nearest ordinary creatures when the create position answers
with the zone, but the landing pools are built from a second, independent
resolution that still asked the create position alone. So bots were sent to a
sub-area whose pool had never been built: the boot log still listed five
sub-areas for six races. Both sides now resolve it the same way, because a pool
built for one area and a placement aimed at another is the mismatch itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Judge the home-zone rule at the level being assigned, not the level being replaced
The home-zone confinement asked bot->getLevel(), which is right when deciding
whether a bot standing somewhere should be moved, and wrong in the one place that
matters most.
RandomizeFirst picks a destination, derives from it the level the bot is ABOUT to
be given, and only then runs CleanRandomize to grant that level with its talents,
trainer spells, equipment and bags. Asking the bot's current level there asks
about a freshly created level 1, so every destination outside its racial starting
zone was refused. All hundred attempts failed, the loop fell out, and
CleanRandomize never ran -- no level, no gear, no talents, no trainer spells, and
the bot left standing at level 1 wearing its create-info shirt.
Measured on the roster this produced: 840 of 900 bots at level 1, 37 at level 2,
18 at 3, 5 at 4, and not one above. The handful that climbed did it by killing
things. Before the rule the same roster spread all the way to 60.
The fix is to evaluate it at useLevel when the caller supplies one, exactly as
the creature band immediately below already does. RandomizeFirst then asks the
question it means -- would a bot OF THE LEVEL I AM ABOUT TO ASSIGN belong here --
while ProcessBot's eviction check passes no useLevel and still asks about the bot
in front of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Let a roster that never got randomized recover without a database wipe
Fixing the cause left every install that already hit it broken, because the
damage is durable: a bot whose randomization failed still had its "randomize"
event banked, and nothing looks at that again until it lapses -- up to fourteen
days on the shipped MaxRandomRandomizeTime. Those bots stay at level 1 with no
talents, no trainer spells and no gear, and no amount of restarting helps.
Clearing the table by hand fixes one server, not anybody else's.
Three changes, so the state can neither be reached nor persist.
RandomizeFirst now reports whether it randomized, and ProcessBot banks the event
only when it did. This is the rule the teleport branches already follow: only
spend the event if the thing it records really happened. A failed pass now
retries next time instead of costing the bot a fortnight.
RandomizeFirst also stops being able to do nothing. If no destination passes the
search, the bot is randomized where it stands rather than left untouched, and
ProcessBot's eviction branch relocates it later like any other badly-placed bot.
The defensive index guard mid-loop breaks out rather than returning, so it
reaches that fallback too.
And a start-up sweep drops banked randomize events for bots still sitting at
level 1, since reaching level 1 with that event banked means RandomizeFirst never
granted a level -- it is the only path that does. Verified against the live
roster: 65 rows, exactly the stuck bots. Safe on every start, because a healthy
roster has almost nothing at level 1 holding one, and the worst case for a bot
that does is being randomized earlier than scheduled. It sits beside the
evictcheck sweep that already clears runtime state a restart should not preserve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Keep a share of the roster living in its starting zone instead of passing through
Confining low-level bots to their home zone was only half of it, and watching a
fresh roster made the other half obvious: Teldrassil emptied within minutes as
its bots were levelled and teleported away one after another. The rules hold a
bot at home only while it IS low level, and nothing keeps any bot low level --
RandomizeFirst rolls a level from a randomly chosen destination, so every bot
promptly levels out of the band and leaves.
RandomBotStarterZonePct, default 15, makes residency a property of the bot
instead. A resident is randomized inside the home band and placed in its own
racial starting zone, and IncreaseLevel sends it back to the start when it
outgrows that band rather than letting it graduate out -- otherwise the same
drain happens one level at a time instead of all at once.
Residency is derived from the bot's guid rather than rolled, so the answer is
identical across a randomize, a relog and a restart. A rolled one would move a
bot in and out of its starting zone every time it levelled, which is the opposite
of the intent, and this needs no storage and no extra event to persist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Catch a resident that levels out of its starting band by playing
The reset in IncreaseLevel was claimed as the guard that keeps a resident at
home, and it is not one. It runs only when the randomize event lapses, which is
between two hours and a fortnight away, so it governs the manager's own slow
increment and nothing else.
Bots earn ordinary kill, quest and exploration experience, and Player::GiveXP
calls GiveLevel straight out on the map worker without consulting this manager.
A resident rolled to level 10 therefore reaches 11 by simply playing. At 11 both
home-confinement tests stop applying -- each asks for <= RandomBotHomeZoneMaxLevel
-- so the next eviction or teleport event sends it to a level-appropriate zone
somewhere else. That is the drain the starting zones actually suffer: not the
manager levelling residents out, but residents levelling themselves out from
underneath it, with the reset that was supposed to stop it still hours or days
from running.
ProcessBot now checks it every pass, before anything that could move the bot, and
returns the resident to the start. On the world thread where the roster is owned,
rather than by hooking the level-up on the map worker.
The comment on the IncreaseLevel branch has been corrected to say what that
branch really covers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Three defects an outside review found in the bot roster and config
A meeting-stone click could crash the server. HandleMeetingStoneClick guarded
the role tally with "if (member)" and then dereferenced the same pointer on the
very next line regardless. A member slot outlives its player logging out, so
GetPlayer returns NULL for anyone offline, and one offline member in the
leader's group was enough. Pre-existing, not from the resident work, but it is a
live crash path so it is fixed here.
The roster doubled on every restart that did not wipe. CreateRandomBots treats
an account as full at ten characters while creating one per playable class, and
vanilla has nine -- 6 and 10 do not exist. An account already full at nine
therefore looked incomplete and received a second full set, taking 450 characters
to 900 at eighteen per account until the count finally cleared ten.
CreateRandomBot bypasses the normal per-account character limit, so nothing else
stopped it. The threshold is now the number of classes the loop will actually
create. This is also why bot-density comparisons between runs have been
untrustworthy.
And the new residency settings were taken on faith. Residency is decided by
(guid % 100) < pct, so a value above 100 quietly makes the entire roster
resident, and a negative in the file arrives as an enormous unsigned. It is
clamped now. Residency with RandomBotHomeZoneMaxLevel at zero is worse than
either alone -- the bot is placed at home once and then confined by nothing,
which reads as the feature working and then silently failing -- so that
combination is refused with a log line rather than half-applied.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* A random bot healing nobody should be fighting instead
Non-shadow priests are given "heal" and "flee" and nothing else, and
GetPlayerSpecTab answers 0 whenever no talent points are spent -- so every
untalented priest is a healer. Alone, with nothing to heal, it has no action to
take at all.
465b2f6b caused this. It stopped the random-bot damage override evicting the
build the talents had chosen, which was right for tanks and for casters who were
being handed a melee strategy, and wrong for healers: it left them holding a
build that does nothing outside a group. The previous behaviour was equally
wrong in the other direction, forcing every healer to damage even in a party, so
this trades neither for the other.
The branch is only reached when the bot has no group, so healers now take the
override and tanks and ranged builds still keep their spec. Nothing is lost by
it: the grouped branch a few lines above hands a HEAL build botHealStrategies,
and AcceptInvitationAction calls ResetStrategies, so joining a party rebuilds the
healer it was meant to be.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Report the talents a bot actually spent, instead of 0/0/0
Every bot answered "0/0/0" to a spec query however many points it had spent,
which is how this was noticed: a level 22 restoration druid cannot have nothing
in its trees.
GetPlayerSpecTabs counts into TalentTab DBC ids -- 161, 164 and 163 for a
warrior -- and the keys 0, 1 and 2 it seeds are never touched. formatClass read
those three seeded keys directly, so it always found zeros. Only the display was
wrong: GetPlayerSpecTab reads the same map correctly, converting the winning id
to a tree position, so spec SELECTION was never affected.
Worth recording why the ids are the right key, because the obvious change is a
bug. The shipped 1.12 TalentTab.dbc gives mage Fire (41) OrderIndex 0, the same
as Arcane (81) -- verified by parsing the client file this server loads. Keying
the counts by OrderIndex would merge two mage specs into one bucket, and it is
exactly why GetPlayerSpecTab carries a hand-written mage mapping.
That mapping is now TalentTabToIndex, shared by both callers, so the display and
the selection cannot drift apart again -- which is the whole shape of this bug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* The findings two independent reviews agreed were worth blocking a push
Both reviewers landed on the event cache. GetEventValue records a miss so
ProcessBot stops re-querying events most bots do not have, but IsRandomBot is
the one entry point map workers reach -- PlayerbotAI::UpdateAI, the trade and
grind values, AiFactory -- so it was adding unsynchronised writes to a std::map
the world thread reads and writes at the same time. A concurrent rebalance is a
crash, not a stale read. The miss-caching is now opt-out and IsRandomBot opts
out; the world-thread path keeps it.
A delayed shutdown emptied the world immediately. LogoutAllBots sat outside the
branch in ShutdownServ, so ".server shutdown 3600" logged every bot out the
moment it was typed and left the real players alone for the hour, and an idle
shutdown that then declined to stop because sessions remained had already
dispatched them for good. It now runs only where the shutdown actually proceeds.
InitTalents spilled leftover points into "2 - specNo", which is the opposite
tree for specs 0 and 2 and is spec 1 itself for spec 1 -- the tree the loop has
just failed to fill any further, so a middle-spec bot kept its leftovers
forever. It now tries the other two trees and stops when one accepts a point.
The random-bot combat override was skipped wholesale for tanks and casters when
only the damage entry needed skipping, so "+attack weak" -- which is in the
shipped default -- and anything else an operator had added were silently lost on
those builds. Only the offending entry is dropped now.
RandomTeleportHome was the one teleport path with no Refresh, leaving residents
with whatever health, mana, durability and combat references they arrived with.
And two code fallbacks still disagreed with the dist they were fixed in:
RandomBotMaps offered Outland and Northrend, RandomBotSpellIds taught Cold
Weather Flying, to anyone whose conf predates those keys.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Show the player their own whisper when the target is a bot
Whispering a bot printed nothing in your own chat window -- not the message, not
a confirmation, no indication it had gone anywhere. Player::Whisper is what
sends CHAT_MSG_WHISPER_INFORM back to the sender, the "To Name:" line, and the
bot branch of the whisper handler never called it: it forwarded the text to
HandleCommand and stopped.
It now whispers first and then hands the text over, which also restores the
whisper log and the AFK/DND handling every other whisper gets. Safe for a bot
target: SendPacket routes the bot's copy through HandleBotOutgoingPacket, then
returns on the missing socket.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Give a shadow priest something to cast before level 10
Watching one made the gap obvious: it pulled with a single Shadow Word: Pain and
then stood beside the target doing nothing for as long as anyone cared to watch.
The strategy's only default action is mind blast, which is level 10. Below that
the entire rotation is out of reach -- shadowform and vampiric embrace are
40-point talents, dispersion is a Wrath spell this core does not have, and the
one trigger that can fire is Shadow Word: Pain at level 4. So a young shadow
priest casts exactly one spell per pull and has nothing to follow it with.
Smite is level 1, is already registered, and is the filler Holy falls back on in
the same position. Added behind mind blast so it only gets its turn when the
better spell is unavailable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Let every combat build turn to face what it is fighting
Only melee strategies carried the "not facing target" trigger, so casters,
hunters and every healer running a solo damage build never turned at all.
Nothing else turns a bot in combat. The module moves with MovePoint, whose
generator carries no facing, and never with the chase generator that sets facing
to the target every tick. Orientation after a move is therefore the direction of
travel, and a bot that was already within contactDistance when the fight started
never moves and never turns -- which is a priest standing beside a nightsaber it
never once faced.
It costs a caster its auto-attack. Swings are refused outside a 120 degree arc
and the refusal re-arms the swing timer at 100ms, so a bot in melee range that
cannot face retries ten times a second indefinitely and lands nothing. Spells
mostly do not care, facing being per-spell through spell_facing, so this is
about the weapon and about not spinning on a check that cannot pass.
The trigger moves to CombatStrategy, which every combat build derives from
directly or through its class's generic strategy, and the three copies it
replaces are removed. PullStrategy keeps its own "set facing" because that one
is a sequenced step in the pull chain rather than the same trigger.
Both names were already registered, and SetFacingTargetAction runs on the bot's
own map worker, so this adds no cross-thread access.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Stop bots landing inside buildings and under the world
Teleporting to a bot could drop a player through the floor, because the bot
itself was standing inside a structure or beneath one.
The landing point is a creature spawn with x and y jittered by up to half a
grindDistance -- fifty yards as shipped -- while z was left at the anchor's
height. Every safety test then ran against that stale z, asking whether a point
fifty yards away and possibly well above or below the ground was outdoors and
dry. StaticFloor was called afterwards, and it searches a column centred on the
height it is given, so anchored on the wrong height it returned whatever surface
happened to be there: under a building, that is the ground beneath it.
The snap now happens first and every test runs at the height the bot will
actually stand at. IsOutdoors reads WMO group flags at the point given, so asked
there it is precisely the check that rejects a floor inside a building or the
ground under one -- asked at the old height it was answering about the open sky
above the roof and passing happily.
Every teleport path funnels through this function, so the spawn-query, home and
level searches are all covered.
Expect more candidates to be rejected than before, because the check finally
runs where it means something. Ten attempts and pools of hundreds to low
thousands of sites should absorb it; "Cannot teleport bot" in the log is the
thing to watch if they do not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Let a hunter back out of its own dead zone
A hunter with a mob on it stood still between seven and fifteen yards and did
nothing whatsoever until the mob reached melee.
HunterEnsureRangedPositionAction is the only action that can restore range, and
its isUseful disqualified itself whenever the target's victim was the bot -- so
it switched off at exactly the moment it was needed. Presumably it meant "let
the pet tank and reposition freely", and with a pet holding aggro the hunter
does recover properly. Self-aggro is the broken case.
Nothing else in the cascade covers that gap. Wing clip, mongoose bite, disengage
and the melee fallback all require five yards or less, flee requires seven, and
auto shot is refused by the core with SPELL_FAILED_TOO_CLOSE inside eight. So
between seven and fifteen yards, with aggro, every single option was skipped.
Walking while being hit costs casting pushback, which is plainly better than
standing in the dead zone eating free melee until the mob arrives.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* A cat druid whose only attack is a Burning Crusade ability, and a shaman with no way out
CatDpsDruidStrategy's sole default action is mangle (cat). Mangle arrived in
Burning Crusade and does not exist on this core, so the action could never be
cast and a cat druid fell through to nothing on every tick -- the same shape as
the shadow priest whose only default was a level 10 spell. Claw is the vanilla
equivalent, learned at 10, and melee sits behind it so a druid that is not in
form or has not learned claw still swings. This matters more now that solo
healers take a damage build, because a Restoration druid becomes a cat without
having spent a point in Feral.
The Enhancement shaman branch had no flee while the other two shaman branches
both do, so an Enhancement shaman -- and every untalented one, since
GetPlayerSpecTab answers -1 and lands in that same else -- fought to the death
with no escape at low health.
Both found by an outside review of the strategy layer, and both are the same
kind of fault: a rotation whose entries are individually plausible and
collectively unreachable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Fill the emptiest starting zone before drawing at random
A percentage decides who MAY live in a starting zone; nothing decided who
actually does. AddRandomBot draws uniformly from every free character of a
faction, so which zones end up populated is luck, and on a 450-character roster
with 101 active it left Teldrassil and Mulgore with none at all. Not a placement
failure -- no qualifying night elf or tauren was ever selected to log in. Raising
the percentage only moves the average; it cannot express "every starting zone
should have somebody in it".
Admission now checks how many residents each starting zone currently holds and,
when one is below RandomBotStarterZoneQuota (default 5), admits a resident of
the emptiest such zone instead of drawing at random. Everything else fills as
before. It runs in AddRandomBot on the world thread, where the roster is already
owned, and needs no new per-bot state: residency is still the guid predicate and
the start zone comes from playercreateinfo.
Two smaller things the same reviews found:
A ready check was never answer…