- Add Inventory::FIRST_BAG_EQUIP_SLOT = 19 constant with why-comment
explaining WoW equip slot layout (bags occupy slots 19-22)
- Replace all 19 occurrences of magic number 19 in bag slot calculations
across inventory_handler, spell_handler, inventory, and game_handler
- Add UNIT_FIELD_FLAGS / UNIT_FLAG_PVP comment in combat_handler
- Add why-comment on network packet budget constants (prevent server
data bursts from starving the render loop)
- Replace all 11 occurrences of magic number 23 in backpack slot
calculations with Inventory::NUM_EQUIP_SLOTS across inventory_handler,
spell_handler, and inventory.cpp
- Add why-comment to NUM_EQUIP_SLOTS explaining WoW slot layout
(equipment 0-22, backpack starts at 23 in bag 0xFF)
- Add why-comment on 0x80000000 bit mask in item query response
(high bit flags negative/missing entry response)
- Replace manual channel membership loops with std::find in
chat_handler.cpp (YOU_JOINED and PLAYER_ALREADY_MEMBER cases)
- Add why-comment on PLAYER_ALREADY_MEMBER reconnect edge case
- spell_handler: extract duplicated item on-use spell lookup into
findOnUseSpellId() — was copy-pasted in useItemBySlot and useItemInBag
- warden_handler: add why-comment explaining the door model HMAC-SHA1
hash table (wall-hack detection for unmodified 3.3.5a client data)
- spell_handler.cpp: replace goto-done with do/while(false) for pet
spell packet parsing — bail on truncated data while always firing
events afterward
- water_renderer.cpp: replace goto-found_neighbor with immediately
invoked lambda to break out of nested neighbor search loops
Introduce `GameServices` struct — an explicit dependency bundle that
`Application` populates and passes to `GameHandler` at construction time.
Eliminates all 47 hidden `Application::getInstance()` calls in
`src/game/*.cpp`, completing SOLID-D (dependency-inversion) cleanup.
Changes:
- New `include/game/game_services.hpp` — `struct GameServices` carrying
pointers to `Renderer`, `AssetManager`, `ExpansionRegistry`, and two
taxi-mount display IDs
- `GameHandler(GameServices&)` replaces default constructor; exposes
`services() const` accessor for domain handlers
- `Application` holds `game::GameServices gameServices_`; populates it
after all subsystems are created, then constructs `GameHandler`
(fixes latent init-order bug: `GameHandler` was previously created
before `AssetManager` / `ExpansionRegistry`)
- `game_handler.cpp`: duplicate `isActiveExpansion` / `isClassicLikeExpansion` /
`isPreWotlk` anonymous-namespace helpers removed; `game_utils.hpp`
included instead
- All domain handlers (`InventoryHandler`, `SpellHandler`, `MovementHandler`,
`CombatHandler`, `QuestHandler`, `SocialHandler`, `WardenHandler`) replace
`Application::getInstance().getXxx()` with `owner_.services().xxx`
Adds [GO-DIAG] WARNING-level logs at:
- Right-click dispatch (raypick hit / re-interact with target)
- interactWithGameObject entry + all BLOCKED paths
- SMSG_SPELL_GO (wasInTimedCast, lastGoGuid, pendingGoGuid state)
- SMSG_LOOT_RESPONSE (items, gold, guid)
- Raypick candidate GO positions (entity pos + hit center + radius)
These logs will pinpoint exactly where the interaction fails:
- No GO-DIAG lines = GOs not in entity manager / not visible
- Raypick GO pos=(0,0,0) = GO position not set from update block
- BLOCKED = guard condition preventing interaction
- SPELL_GO wasInTimedCast=false = timer race (already fixed)
When the client-side cast timer expired slightly before SMSG_SPELL_GO
arrived, the fallback at update():1367 called performGameObjectInteraction
Now which sent a DUPLICATE CMSG_GAMEOBJ_USE to the server (confusing its
GO state machine), then resetCastState() cleared lastInteractedGoGuid_.
When SMSG_SPELL_GO finally arrived, the guid was gone so CMSG_LOOT was
never sent — quest chests produced no loot window.
Fix: the fallback no longer re-sends USE (server drives the interaction
via SMSG_SPELL_GO). resetCastState() no longer clears
lastInteractedGoGuid_ so the SMSG_SPELL_GO handler can still send LOOT.
Two remaining GO interaction bugs:
1. pendingGameObjectInteractGuid_ was never cleared after SMSG_SPELL_GO
or SMSG_CAST_FAILED, leaving it stale. This suppressed CMSG_CANCEL_CAST
for ALL subsequent spell casts (not just GO casts), causing the server
to think the player was still casting when they weren't.
2. For chest-like GOs, CMSG_LOOT was sent simultaneously with
CMSG_GAMEOBJ_USE. If the server starts a timed cast ("Opening"),
the GO isn't lootable until the cast completes — the premature LOOT
gets an empty response or is dropped, potentially corrupting the
server's loot state. Now defers LOOT to handleSpellGo which sends it
after the cast completes (via lastInteractedGoGuid_).
Long achievement names combined with sender name could exceed 256
bytes, silently cutting the message mid-word in chat. Replaced with
std::string concatenation which grows dynamically.
All domain handler files used 'packet.getSize() - packet.getReadPos()'
which underflows to ~2^64 when readPos exceeds size (documented in
commit ed63b029). The game_handler.cpp and packet_parsers were migrated
to hasRemaining(N) in an earlier cleanup, but the domain handlers were
created after that migration by the PR #23 split, copying the old
unsafe patterns back in. Now uses hasRemaining(N) for comparisons and
getRemainingSize() for assignments across all 7 handler files.
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.
!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.
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).
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.
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 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.
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.
Extract domain-specific logic from the monolithic GameHandler into
dedicated handler classes, each owning its own opcode registration,
state, and packet parsing:
- CombatHandler: combat, XP, kill, PvP, loot roll (~26 methods)
- SpellHandler: spells, auras, pet stable, talent (~3+ methods)
- SocialHandler: friends, guild, groups, BG, RAF, PvP AFK (~14+ methods)
- ChatHandler: chat messages, channels, GM tickets, server messages,
defense/area-trigger messages (~7+ methods)
- InventoryHandler: items, trade, loot, mail, vendor, equipment sets,
read item (~3+ methods)
- QuestHandler: gossip, quests, completed quest response (~5+ methods)
- MovementHandler: movement, follow, transport (~2 methods)
- WardenHandler: Warden anti-cheat module
Each handler registers its own dispatch table entries via
registerOpcodes(DispatchTable&), called from
GameHandler::registerOpcodeHandlers(). GameHandler retains core
orchestration: auth/session handshake, update-object parsing,
opcode routing, and cross-handler coordination.
game_handler.cpp reduced from ~10,188 to ~9,432 lines.
Also add a POST_BUILD CMake step to symlink Data/ next to the
executable so expansion profiles and opcode tables are found at
runtime when running from build/bin/.
