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)
The client-side cast timer expires ~50-200ms before the server sends
SMSG_SPELL_GO (float precision + frame timing). Previously the fallback
called resetCastState() which set casting_=false and currentCastSpellId_
=0. When SMSG_SPELL_GO arrived moments later, wasInTimedCast evaluated
to false (false && spellId==0), so the loot path (CMSG_LOOT via
lastInteractedGoGuid_) was never taken. Quest chests never opened.
Now the fallback skips resetCastState() for GO interaction casts, letting
the cast bar sit at 100% until SMSG_SPELL_GO arrives and handles cleanup
properly with wasInTimedCast=true.
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_).
pendingGameObjectInteractGuid_ was always cleared to 0 right before
the interaction, which defeated the cancel-protection guard in
cancelCast(). Any positional movement (WASD, jump) during a GO
interaction cast (e.g., "Opening" on a quest chest) sent
CMSG_CANCEL_CAST to the server, aborting the interaction and
preventing quest objective credit.
Now sets pendingGameObjectInteractGuid_ to the GO guid so:
1. cancelCast() skips CMSG_CANCEL_CAST for GO-triggered casts
2. The cast-completion fallback can re-trigger loot after timer expires
3. isGameObjectInteractionCasting() returns true during GO casts
CMSG_GAMEOBJ_REPORT_USE was only sent for non-chest GOs. Chest-type
(type=3) and name-matched chest-like GOs (Bundle of Wood, etc.) went
through a separate path that sent CMSG_GAMEOBJ_USE + CMSG_LOOT but
skipped REPORT_USE. On AzerothCore, REPORT_USE triggers the server-side
HandleGameobjectReportUse which calls GossipHello on the GO script —
this is where many quest objective scripts grant credit.
Restructured so CMSG_GAMEOBJ_USE is sent first for all GO types,
then chest-like GOs additionally send CMSG_LOOT, and REPORT_USE fires
for everything except mailboxes.
The click region for targeting via nameplates was bounded by the name
text (nameX to nameX+textSize.x). Short names like "Wolf" produced a
~30px clickable strip, while the health bar below was 80px wide. Clicks
on the bar outside the name text bounds were ignored. Now uses the wider
of name text or health bar for the horizontal hit area.
The Packet::skipAll() method was introduced to replace the verbose
setReadPos(getSize()) pattern. 186 instances were migrated earlier,
but 20 survived in domain handler files created after the migration.
Also removes a redundant single-element for-loop wrapper around
SMSG_LOOT_CLEAR_MONEY registration.
Same class of bug as inventory_handler fix b9ecc26f. The for-loop over
{SMSG_INSTANCE_DIFFICULTY, MSG_SET_DUNGEON_DIFFICULTY} was missing its
closing brace, so GUILD_DECLINE, RAF_EXPIRED, RAF_FAILURE, and
PVP_AFK_RESULT registrations executed inside the loop body — each
registered twice (once per opcode). Currently harmless since duplicate
registration is idempotent, but structurally wrong.
The while(true) loop retried av_read_frame after seeking to the start
on error. A corrupt file where read fails but seek succeeds would loop
forever, blocking the main thread. Bounded to 500 attempts with a
warning log on exhaustion.
If generateNormalHeightMapCPU threw (e.g., bad_alloc), the pending
counter was never decremented, causing shutdown() to block forever
waiting for a count that would never reach zero. Added try-catch to
guarantee the decrement. Also strengthened the increment from relaxed
to acq_rel so shutdown()'s acquire load sees the count before the
thread body begins executing.
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.
The two fireAddonEvent calls were indented as if conditional on
repChangeCallback_ but actually execute unconditionally (no braces).
Fixed indentation and added clarifying comment.
queryTimeMs and queryCallCount on WMORenderer and M2Renderer were plain
mutable doubles/uint32s written by getFloorHeight (dispatched on async
threads from CameraController) and read by the main thread. This is
undefined behavior per C++ — thread sanitizer would flag it. Changed to
std::atomic with relaxed ordering (adequate for diagnostics) and updated
QueryTimer to use atomic fetch_add/compare_exchange.
std::future::get() re-throws any exception from the async task. The 6
call sites in the render pipeline (terrain/WMO/M2 workers + animation
worker) and 2 floor-query sites in camera_controller were unguarded,
so a single bad_alloc in any worker would terminate the process with
no recovery. Now wrapped in try-catch with error logging.
SDL2 requires video/window functions to be called from the main thread
(the one that called SDL_Init). The watchdog thread was calling
SDL_SetRelativeMouseMode, SDL_ShowCursor, and SDL_SetWindowGrab directly
on stall detection — undefined behavior on macOS (Cocoa requires main-
thread UI calls) and unsafe on other platforms.
Now the watchdog sets an atomic flag, and the main loop checks it at the
top of each iteration, executing the SDL calls on the correct thread.
zone_manager.cpp used std::rand() for music track selection with modulo
bias and global state. game_screen.cpp used std::rand() for rain/snow
particle positions. Both now use local std::mt19937 seeded from
random_device. Also removes the global srand(time(nullptr)) call since
no code depends on the C rand() seed anymore.
No std::rand() or srand() calls remain in the codebase.
8 rand() calls in weather.cpp used C rand() which defaults to seed 1.
Weather intensity rolls, cycle durations, and particle Y positions were
identical on every launch. Replaced with a file-local mt19937 seeded
from random_device, matching the RNG already present in getRandomPosition.
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.
All 8 rand() calls for animation time offsets and variation timers in
m2_renderer.cpp used C rand() which defaults to seed 1 without srand(),
producing identical sequences every launch. Trees, torches, and grass
all swayed in sync. Replaced with std::mt19937 seeded from
random_device. Same fix for 4 mount idle fidget/sound timer sites in
renderer.cpp which mixed rand() with the mt19937 already present.
offset + length was computed in uint32_t before comparing to size_t.
A crafted M2 with offset=0xFFFFFFFF, length=2 wraps to 1 in uint32,
passing the check and reading out of bounds. Now uses size_t arithmetic,
matching the readArray fix from an earlier round.
count * sizeof(T) was computed in uint32_t — a large count value from a
crafted WMO file could wrap to a small number, pass the bounds check,
then attempt a multi-GB allocation causing OOM/crash. Now uses 64-bit
arithmetic with a 64MB sanity cap, matching the M2 loader pattern.
All four functions read UNIT_FIELD_FLAGS instead of PLAYER_FLAGS.
- AFK (0x01) hit UNIT_FLAG_SERVER_CONTROLLED — vendors flagged as AFK
- DND (0x02) hit UNIT_FLAG_NON_ATTACKABLE — guards flagged as DND
- Ghost (0x100) hit UNIT_FLAG_IMMUNE_TO_PC — immune NPCs flagged as ghost
- FFA PvP (0x80000) hit UNIT_FLAG_PACIFIED — pacified mobs flagged FFA
All now correctly read PLAYER_FLAGS with the right bit masks (0x01,
0x02, 0x10, 0x80 respectively), matching entity_controller.cpp which
already uses the correct field.
Final sweep across mpq_manager, application, auth_screen, wmo_renderer,
character_renderer, and terrain_manager. All now use the unsigned char
cast pattern. No remaining bare ::tolower/::toupper or std::tolower(c)
calls on signed char in the codebase.
tavernTrackIndex was initialized to 0 but never modified, so the player
always heard TavernAlliance01.mp3. Added post-increment to rotate
through the 3 available tracks on each tavern entry.
std::strlen on raw MWMO chunk data has no upper bound if the chunk
lacks a null terminator (truncated/corrupt WDT file). Replaced with
strnlen bounded by chunkSize, matching the ADT parser fix in d776226f.
static int logCount/batchLogCount inside the per-group parse loop
accumulated globally, so after the first WMO with many sub-chunks
loaded, no subsequent WMO group would ever log. Changed to function-
local / loop-index-based throttle so each group gets its own window.
elapsedTime_ was float (32-bit, ~7 significant digits). At 16384
seconds the float can only represent integers, so elapsedTime_*1000
jumps in 1-second steps — ships and elevators visibly jerk. Changed to
double (53-bit mantissa) which maintains sub-millisecond precision for
~285 million years. Also changed lastServerUpdate to double to match.
vmaCreateBuffer return value was silently discarded in both loadTerrain
and loadTerrainIncremental. If allocation failed (OOM/fragmentation),
the chunk proceeded with a VK_NULL_HANDLE UBO, causing the GPU to read
from an invalid descriptor on the next draw call. Now checks the return
value and skips the chunk on failure.
The doodad set name read used raw memcpy(20 bytes) bypassing the safe
read<T> template that returns {} on OOB. A truncated WMO file would
read past the vector's storage. Added bounds check before the memcpy.
ARGB8888 decompression read pixelCount*4 bytes from mipData without
checking that mipSize was large enough — a truncated BLP caused heap
OOB reads. Also, 'int pixelCount = width * height' overflowed for
large dimensions (signed int UB). Now validates dimensions <= 4096,
uses uint32_t arithmetic, and checks mipSize >= required for ARGB8888.
std::toupper(int) and std::tolower(int) have undefined behavior when
passed a negative value. These sites passed raw signed char without
casting to unsigned char first, unlike the rest of the codebase which
already uses the correct pattern. Affects auth (account names), world
packets, and mount sound path matching.
1. MCNK sub-chunk bounds checks didn't account for the 8-byte header
skip, so parseMCVT/parseMCNR could read up to 8 bytes past the
validated buffer when sub-chunk headers are present (the common case).
2. parseMTEX/parseMMDX/parseMWMO used unbounded strlen on raw chunk
data. A truncated file without a null terminator would read past the
chunk boundary. Replaced with strnlen bounded by remaining size.
Also removes dead debug code: empty magic buffer copy, cathedral WMO
search, and Stormwind placement dump (which also had ::toupper UB).
pendingPlayerSpawns_, deferredEquipmentQueue_, and
pendingGameObjectSpawns_ are consumed from the front via erase(begin()),
which is O(n) on vector (shifts all elements). With many spawns queued
(entering a city), this made the processing loop O(n²). deque supports
O(1) front erasure. pendingCreatureSpawns_ already used deque.
readArray was called inside the loop on every iteration, re-parsing the
entire flat key array via memcpy. For a model with 200 sequences and
10k keys this produced ~24MB of redundant copying. Now reads once before
the loop (matching how allTimestamps was already handled).
WotLK format is min(4)+max(4)+result(4)+guid(8)=20 bytes. The parser
read guid(8) first (treating min|max as a uint64), then targetGuid(8)
(non-existent field), then the actual values at wrong offsets. Every
/roll message showed garbled numbers and a bogus roller identity.
Also adds a hasRemaining guard for the 64 bytes of damage/armor/resist
fields in the item query parser — previously read past end with silent
zero-fill on truncated packets.
The comment would lead a maintainer to "fix" the working code to read
4-byte RGBA instead of 3-float C3Vector. Updated to match the actual
M2 particle FBlock color format (3 floats, values 0-255, per WoWDev).
std::tolower(int) has undefined behavior when passed a negative value,
which signed char produces for bytes > 127. The rest of the codebase
correctly casts to unsigned char first; these 3 sites were missed.
The golden tint rect was drawn before rendering with a hardcoded single-
line height. Multi-line wrapped messages only had the first line
highlighted. Now drawn after EndGroup() using GetItemRectMin/Max so the
rect covers all wrapped lines.
Also fixes std::tolower(char) UB at two call sites — negative char
values (extended ASCII) are undefined behavior without unsigned cast.
Both operands are size_t (unsigned), so if readPos > data.size() the
subtraction wrapped to ~0 instead of returning 0. This could happen
via setReadPos() which has no bounds check. Downstream hasRemaining()
was already safe but getRemainingSize() callers (e.g. hasFullPackedGuid)
would see billions of bytes available.
A single send() that returned fewer bytes than requested was logged but
not retried, leaving the server with a truncated packet. This causes an
irreversible TCP framing desync (next header lands mid-payload) that
manifests as a disconnect under network pressure. Added a retry loop
that handles EWOULDBLOCK with a brief yield.
Also rejects payloads > 64KB instead of silently truncating the 16-bit
CMSG size field, which would have written a wrong header while still
appending all bytes.
The two emit calls were indented 12 spaces (suggesting a nested block)
instead of 8 (matching the enclosing if). Same class of maintenance
trap as the PLAYER_ALIVE/PLAYER_UNGHOST fix in b3abf04d.
1. Contradictory condition (!numStrings && numStrings >= 1) was always
false, so unknown guild event messages never included the server's
context string. Fixed to just numStrings >= 1.
2. leaveParty() only sent the packet without clearing partyData or
firing addon events, so /leave left party frames visible until the
server pushed an update. Now delegates to leaveGroup() which handles
both the packet and local state cleanup.
The cast falsely suggests reasonType is unused, but it's read on lines
3699-3702 for AFK/vote-kick differentiation. Same class of issue as
the (void)isPlayerTarget fix in commit 6731e584.
