The per-tick GPU upload budget check ran before consuming async futures,
so after 1 upload ALL remaining ready results were deferred — including
permanent failures and cache hits that need zero GPU work. Moved the
budget gate after failure/cache-hit processing so only actual uploads
count. Re-queues over-budget results as pending spawns for next frame.
Callbacks and addons querying the current target during this event saw
the old (stale) target instead of null. setTarget correctly updates the
GUID before firing — clearTarget now does the same.
The spellbook tab dirty check used a function-local static, meaning
switching to a character with the same spell count would skip the
rebuild and return the previous character's tabs. Changed to an
instance member so each SpellHandler tracks its own count.
Freshly spawned entities have maxHealth=0 before fields populate.
0/0 produces NaN which propagates through all geometry calculations
for that nameplate frame. Guard with a maxHealth>0 check.
All five force-ACK handlers (speed, root, flag, collision-height,
knockback) repeated the same ~25-line GUID+counter+movementInfo+coord-
conversion+send sequence. Extracted into buildForceAck() which returns
a ready-to-send packet with the movement payload already written.
This also fixes a transport coordinate conversion bug: the collision-
height handler was the only one that omitted the ONTRANSPORT check,
causing position desync when riding boats/zeppelins. buildForceAck
handles transport coords uniformly for all callers.
Net -80 lines.
Loop iterated 20 hair geoset lookups for Human Male but the if-body
was empty — the LOG statement that was presumably there was removed
but the loop skeleton was left behind.
A function for taxi/movement cleanup was resetting 10 death-related
fields (playerDead_, releasedSpirit_, resurrectPending_, etc.), which
could cancel a pending resurrection or mark a dead player as alive
when called during taxi dismount. Death state is owned by
entity_controller and resurrect packet handlers, not movement cleanup.
The auto-refresh after successful bid/buyout was gated on
lastAuctionSearch_.name.length() > 0, so a browse-all search (empty
name) would never refresh. Replaced with a hasAuctionSearch_ flag
that's set on any search regardless of the name filter.
The character-list level update loop used 'return' instead of 'break',
exiting the handler lambda before the level-up chat message, sound
effect, callback, and PLAYER_LEVEL_UP event could fire. Since the
player GUID is always in the character list, the notification code
was effectively dead — players never saw "You have reached level N!".
The variable is used earlier in the function for hostile attacker
tracking, so the (void) cast falsely suggests it was unused. Leftover
from a prior refactor.
Icon==6 and text=="GOSSIP_OPTION_BANKER" both sent BANKER_ACTIVATE
independently. Banking NPCs match both, so the packet was sent twice —
some servers toggle the bank window open then closed. Added sentBanker
guard so only one packet is sent.
Also extracts classifyGossipQuests() from two identical 30-line blocks
in handleGossipMessage and handleQuestgiverQuestList. The icon→status
mapping (5/6/10=completable, 3/4=incomplete, 2/7/8=available) is now
in one place with a why-comment explaining these are protocol-defined.
If the server sent a NaN or out-of-range speed, the client echoed it
back in the ACK (confirming it to the server) but then rejected it
locally. This left the server believing the client accepted the speed
while the client used the old value — a desync only fixable by relog.
Moved validation before the ACK so bad speeds are rejected outright.
selectCharacter had 30+ if(spellHandler_) guards reaching into
SpellHandler internals (knownSpells_, spellCooldowns_, playerAuras_,
etc.) to clear per-character state. Consolidated into resetAllState()
so SpellHandler owns its own reset logic and new state fields don't
require editing GameHandler.
reinterpret_cast<float*> on raw packet bytes is undefined behavior per
the C++ strict aliasing rule — compilers can optimize assuming uint8_t
and float never alias. Replaced with packet.readFloat() which uses
memcpy internally. Also switched to hasRemaining() for consistency.
The 10s silence warning used a one-shot bool guard, but the 15s warning
used a 500ms time window — firing every frame (~30 times at 60fps).
Added rxSilence15sLogged_ guard consistent with the 10s pattern.
!packet.hasRemaining(4) + 4 + 4 evaluated as (!hasRemaining(4))+8
due to ! binding tighter than +, making the check always truthy and
breaking out of the loop after the first pet. Hunters with multiple
stabled pets would see only one in the stable master UI.
handleSpellStart and handleSpellGo duplicated the player/target/focus/
pet GUID-to-unitId mapping that already exists in guidToUnitId(). If a
new unit-id category is added (e.g. mouseover), these inline copies
would not pick it up.
SMSG_PET_GUIDS, SMSG_PET_DISMISS_SOUND, and SMSG_PET_ACTION_SOUND were
registered with the same handler as SMSG_PET_UNLEARN_CONFIRM. Their
different formats (GUID lists, sound IDs with position) were misread as
unlearn cost, potentially triggering a bogus unlearn confirmation dialog.
Also extracts resetWardenState() from 13 lines duplicated verbatim
between connect() and disconnect().
Both buyBackItem() and the retry path in handleBuyFailed constructed
packets with a raw opcode constant instead of using the expansion-aware
BuybackItemPacket::build(). This would silently break if any expansion's
CMSG_BUYBACK_ITEM wire mapping diverges from 0x290.
The heartbeat throttle bitmask was missing SWIMMING and DESCENDING,
treating swimming/descending players as stationary and using a slower
heartbeat interval. The identical bitmask in movement_handler.cpp
already included SWIMMING — this inconsistency could cause the server
to miss position updates during swim combat.
Only ASCENDING was cleared — the DESCENDING flag was never toggled,
so outgoing movement packets during flight descent had incorrect flags.
Also clears DESCENDING on start-ascend and stop-ascend for symmetry.
Replaces static heartbeat log counter with member variable (was shared
across instances and not thread-safe) and demotes to LOG_DEBUG.
SpellHandler::updateTimers() (added in 209c2577) already ticks down
castTimeRemaining_, unitCastStates_, and spellCooldowns_. But the
GameHandler::update() loop also ticked them manually — causing casts to
complete at 2x speed and cooldowns to expire twice as fast.
Removed the duplicate tick-downs from update(). The GO interaction
completion check remains (client-timed casts need this fallback).
Also uses resetCastState() instead of manually clearing 4 fields,
adds missing castTimeTotal_ reset, and adds loadSpellNameCache()
to getSpellName/getSpellRank (every other DBC getter had it).
SMSG_ENVIRONMENTALDAMAGELOG (alias) registration at line 173 silently
overwrote the canonical SMSG_ENVIRONMENTAL_DAMAGE_LOG handler at line
108. The alias handler discarded envType (fall/lava/drowning), so the
UI couldn't differentiate environmental damage sources. Removed the
dead alias handler and its method; the canonical inline handler with
envType forwarding is now the sole registration.
Quest log had a redundant pos=0 right after initialization. Chat handler
logged every incoming/outgoing message at WARNING level, flooding the
log and obscuring genuine warnings.
taxiRecoverPending_ was unconditionally reset to false in the general
state cleanup, 39 lines before the recovery check that reads it. The
recovery block could never execute. Removed the premature clear so
mid-flight disconnect recovery can actually trigger.
The packet only contains uint8 count + count×uint64 GUIDs, but the
handler called readString() after each GUID. This consumed raw bytes of
subsequent GUIDs as a string, corrupting all entries after the first.
Now stores GUIDs in ignoreListGuids_ and resolves names asynchronously
via SMSG_NAME_QUERY_RESPONSE, matching the friends list pattern.
Also fixes unsafe static_pointer_cast in ready check (no type guard)
and removes redundant packetHasRemaining wrapper (duplicates Packet API).
All spline speed opcodes share the same PackedGuid+float format,
differing only in which member receives the value. Replaced 8 identical
lambdas (~55 lines) with a makeSplineSpeedHandler factory that captures
a member pointer, cutting duplication and making it trivial to add new
speed types.
Both the health==0 and dynFlags UNIT_DYNFLAG_DEAD paths duplicated the
same corpse-position caching and death-state logic with a subtle
asymmetry (only health path called stopAutoAttack). Extracted into
markPlayerDead() so coordinate swapping and state changes happen in one
place. stopAutoAttack remains at the health==0 call site since the
dynFlags path doesn't need it.
Three identical copies (game_handler.cpp, spell_handler.cpp,
quest_handler.cpp) plus two forward declarations (inventory_handler.cpp,
social_handler.cpp) replaced with a single inline definition in
game_utils.hpp. All affected files already include this header, so
quality color table changes now propagate from one source of truth.
The dismount path wiped every aura with maxDurationMs < 0, which
includes racial passives, tracking, and zone buffs — not just the mount
spell. Now only clears the specific mountAuraSpellId_ so the buff bar
stays accurate without waiting for a server aura resync.
The range check (c > 0x7E) rejected UTF-8 multi-byte sequences, so quest
titles on localized servers (French, German, Russian, etc.) were treated
as unreadable binary and replaced with 'Quest #ID' placeholders. Now
allows bytes >= 0x80 while still requiring at least one ASCII letter to
distinguish real text from binary garbage.
The emit calls were indented at a level suggesting they were outside the
if/else blocks, but braces placed them inside. Fixed to match the actual
control flow, preventing a future maintainer from "correcting" the
indentation and accidentally changing the logic.
Charge already computed facing and sent SET_FACING, but then fell through
to both the melee-ability facing block and the generic targeted-spell
facing block — sending up to 3 SET_FACING + 1 HEARTBEAT per cast. Added
facingHandled flag so only one block sends facing, reducing redundant
network traffic that could trigger server-side movement validation.
The for-loop over {SMSG_LOOT_CLEAR_MONEY} was missing its closing brace,
so SMSG_READ_ITEM_OK and SMSG_READ_ITEM_FAILED registrations were inside
the loop body. Works by accident (single iteration) but fragile and
misleading — future additions to the loop would re-register book handlers.
Two fixes for item name resolution:
1. Clear entry from pendingItemQueries_ even when response parsing fails.
Previously a malformed response left the entry stuck in pending forever,
blocking all retries so the UI permanently showed "Item 12345".
2. Add 5-second periodic cleanup of pendingItemQueries_ so lost/dropped
responses don't permanently block item info resolution.
The handler read an extra uint8 (bag) after bagSlot, shifting all
subsequent fields by 1 byte. This caused count to straddle the count
and countInInventory fields — e.g. count=1 read as 0x03000000 (50M).
Also removes cast bar diagnostic overlay and demotes debug logs.
The cast bar window used ImGuiCond_FirstUseEver for positioning, so
ImGui's .ini state restored a stale off-screen position from a prior
session. Switch to ImGuiCond_Always and add NoSavedSettings flag so
the bar always renders centered near the bottom of the screen.
Also demotes remaining diagnostic logs to LOG_DEBUG.
SpellHandler::updateTimers() was never called after PR #23 extraction,
so cast bar timers, spell cooldowns, and unit cast state timers never
ticked. Also removes duplicate cast/queue/spell members left in
GameHandler that shadowed the SpellHandler versions, and fixes
MovementHandler writing to those stale members on world portal.
Demotes SMSG_SPELL_START/CAST_RESULT debug logs to LOG_DEBUG.
- Restore 0x02→0x80 Classic harmful-to-WotLK debuff bit mapping in
syncClassicAurasFromFields so downstream checks work across expansions
- Extract handleDisplayIdChange helper to deduplicate identical logic
in onValuesUpdateUnit and onValuesUpdatePlayer
- Remove unused newItemCreated parameter from handleValuesUpdate
- Fix indentation on PLAYER_DEAD/PLAYER_ALIVE/PLAYER_UNGHOST emit calls
- split applyUpdateObjectBlock into handleCreateObject,
handleValuesUpdate, handleMovementUpdate
- extract concern helpers — createEntityFromBlock,
applyPlayerTransportState, applyUnitFieldsOnCreate/OnUpdate,
applyPlayerStatFields, dispatchEntitySpawn, trackItemOnCreate,
updateItemOnValuesUpdate, syncClassicAurasFromFields,
detectPlayerMountChange, updateNonPlayerTransportAttachment
- UnitFieldIndices, PlayerFieldIndices, UnitFieldUpdateResult
structs with static resolve() — eliminate repeated fieldIndex() calls
- IObjectTypeHandler strategy interface; concrete handlers
UnitTypeHandler, PlayerTypeHandler, GameObjectTypeHandler,
ItemTypeHandler, CorpseTypeHandler registered in typeHandlers_ map;
handleCreateObject and handleValuesUpdate now dispatch via
getTypeHandler() — adding a new object type requires zero changes
to existing handler methods
- PendingEvents member bus; all 27 inline owner_.fireAddonEvent()
calls in the update path replaced with pendingEvents_.emit(); events
flushed via flushPendingEvents() at the end of each handler, decoupling
field-parse logic from the addon callback system
entity_controller.cpp: 1520-line monolith → longest method ~200 lines,
cyclomatic complexity ~180 → ~5; zero duplicated CREATE/VALUES blocks
Moves entity lifecycle, name/creature/game-object caches, transport GUID
tracking, and the entire update-object pipeline out of GameHandler into a
new EntityController class (friend-class pattern, same as CombatHandler
et al.).
What moved:
- applyUpdateObjectBlock() — 1,520-line core of all entity creation,
field updates, and movement application
- processOutOfRangeObjects() / finalizeUpdateObjectBatch()
- handleUpdateObject() / handleCompressedUpdateObject() / handleDestroyObject()
- handleNameQueryResponse() / handleCreatureQueryResponse()
- handleGameObjectQueryResponse() / handleGameObjectPageText()
- handlePageTextQueryResponse()
- enqueueUpdateObjectWork() / processPendingUpdateObjectWork()
- playerNameCache, playerClassRaceCache_, pendingNameQueries
- creatureInfoCache, pendingCreatureQueries
- gameObjectInfoCache_, pendingGameObjectQueries_
- transportGuids_, serverUpdatedTransportGuids_
- EntityManager (accessed by other handlers via getEntityManager())
8 opcodes re-registered by EntityController::registerOpcodes():
SMSG_UPDATE_OBJECT, SMSG_COMPRESSED_UPDATE_OBJECT, SMSG_DESTROY_OBJECT,
SMSG_NAME_QUERY_RESPONSE, SMSG_CREATURE_QUERY_RESPONSE,
SMSG_GAMEOBJECT_QUERY_RESPONSE, SMSG_GAMEOBJECT_PAGETEXT,
SMSG_PAGE_TEXT_QUERY_RESPONSE
Other handler files (combat, movement, social, spell, inventory, quest,
chat) updated to access EntityManager via getEntityManager() and the
name cache via getPlayerNameCache() — no logic changes.
Also included:
- .clang-tidy: add modernize-use-nodiscard,
modernize-use-designated-initializers; set -std=c++20 in ExtraArgs
- test.sh: prepend clang's own resource include dir before GCC's to
silence xmmintrin.h / ia32intrin.h conflicts during clang-tidy runs
Line counts:
entity_controller.hpp 147 lines (new)
entity_controller.cpp 2172 lines (new)
game_handler.cpp 8095 lines (was 10143, −2048)
Build: 0 errors, 0 warnings.
The canonical yaw convention (documented in coordinates.hpp) is
atan2(-dy, dx) where X=north, Y=west. North=0, East=+PI/2.
The spell facing code used atan2(dy, dx) (no negation on dy), producing
a yaw ~77° off from the correct server orientation. The server rejected
every cast with "unit not in front" because the sent orientation pointed
in the wrong direction.
Fixed in all 3 locations: charge facing, melee facing, and general
pre-cast facing.
Only melee abilities sent MSG_MOVE_SET_FACING before the cast packet.
Ranged spells like Smite used whatever orientation was in movementInfo
from the last movement, causing "target not in front" server rejection.
Now sends a facing update toward the target entity before ANY targeted
spell cast. The server checks a ~180° frontal arc for most spells.
CreatureDisplayInfo.dbc (691KB, 24K+ entries) exists at Data/db/ but
the loader only checked DBFilesClient\ (MPQ manifest) and expansion CSV.
The CSV had only 13248 entries (malformed export), so TBC+ creatures
(Mana Wyrms, Blood Elf area) had no display data and were invisible.
Now checks Data/db/ as a fallback for binary DBCs. This path contains
pre-extracted DBCs shared across expansions. Binary DBCs have complete
record data including proper IDs.
