Separate raw client DifficultyID from internal Difficulty - #81
Merged
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 minor |
🟢 Metrics 61 complexity · 0 duplication
Metric Results Complexity 61 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…nstance was enterable
Every instance in the game refused entry. Stockades answered
AREA_LOCKSTATUS_MISSING_DIFFICULTY, and so would every other dungeon and raid.
MapDifficulty.dbc is keyed on Difficulty.dbc ids, and for instances those START AT
1. The core's Difficulty enum is the 0-based WotLK-era one, so every instance
lookup missed. Stockades (map 34) has exactly one row, DifficultyID 1, and
GetMapDifficultyData(34, DUNGEON_DIFFICULTY_NORMAL = 0) found nothing.
Measured across the shipped DBCs, which is also why continents were unaffected:
world / battleground / arena only ever DifficultyID 0
dungeon (5-man) only ever 1, 2, 8 -- never 0
raid only ever 3-7, 9, 14 -- never 0
Zero dungeon or raid maps carry a 0 row, and zero world/bg/arena maps carry a
nonzero one. So the translation must apply ONLY to instance maps: for everything
else identity is required, not merely tolerable, and translating them would break
every continent.
The mapping is read out of Difficulty.dbc rather than invented. Each row carries an
instance type and a legacy 0-based index, giving dungeon {0->1, 1->2} and raid
{0->3, 1->4, 2->5, 3->6}.
Translated inside GetMapDifficultyData rather than at the call sites. There are
eleven callers, and the failure mode of missing one is silent.
Two gaps are deliberate. Challenge mode is DifficultyID 8, whose legacy index is
-1; it is reachable only by knowing it is the third 5-man row, so it is mapped
explicitly. Scenarios use DifficultyID 11 and 12, both legacy -1, and the
Difficulty enum has no member for either -- they still miss, exactly as before.
Fixing that needs new enum members, not a new mapping, and is out of scope here.
Two call sites were checked first because they could have depended on the old
behaviour. MapManager.cpp's "some instances only have one difficulty" fallback
currently fires for every dungeon because every lookup misses; afterwards it fires
only when a difficulty genuinely does not exist. Map.cpp's GetMaxPlayers asks for
REGULAR_DIFFICULTY explicitly, which is why the translation has to be type-aware:
regular is id 1 for a 5-man but id 3 for a raid.
Guarded by a source gate with five mutation arms, all verified failing, including
one that drops the instance-type gate and one that reverts to the untranslated
lookup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rework after review rejected the first attempt. That version was both incomplete and actively harmful, and neither fault was visible from its own tests. A static per-type translation left one-row raids unreachable. Some raids have no client ID 3 at all: seven 25-player-only raids carry only ID 4, and four legacy 40-player raids only ID 9. Translating REGULAR_DIFFICULTY to a fixed 3 missed all eleven, so they stayed unenterable while the commit claimed the blocker fixed. CreateDungeonMap's fallback cannot recover it either, and the area trigger refuses before map creation anyway. Worse, it corrupted instance resets. The reset scheduler walks sMapDifficultyMap directly, so it holds RAW client ids, and it persists them into `instance_reset` and carries them in reset events. Sending those back through a translating lookup turned raw raid ID 3 into ID 6 -- raid 25 heroic instead of raid 10 normal -- and raw dungeon 1 and 2 into 2 and 8. So there are now two key spaces, named apart so they cannot be confused: sMapDifficultyMap raw client DifficultyID GetMapDifficultyDataByClientId sMapDifficultyLegacyMap internal 0-based mode GetMapDifficultyData The legacy index is built once at load, per map, applying the same widening BuildMapSpawnModeMasks already uses: client ids map to internal modes, the lower client id wins a tie, and a raid with no regular row inherits its 25-player row. Measured against the shipped DBCs, 113 of 116 instance maps now resolve their regular tier, including all 7 ID-4-only raids and all 4 ID-9 raids. The three that still miss -- 938, 939, 940 -- are 5-man maps whose only row is client ID 2, i.e. heroic only. A player on normal cannot enter those on retail either, so widening them would admit players to heroic content on normal. Left deliberately. The reset scheduler keeps its raw ids and its raw lookup, so `instance_reset` retains its existing meaning and needs no migration. The raw/internal conflation in that file predates this change and is left as a follow-up rather than widened into a data migration here. Call sites are classified individually rather than converted wholesale: `instance` holds internal modes and keeps the internal lookup, while `instance_reset` and reset events hold raw ids and move to the raw one. Source gate rewritten with six arms, all verified failing, one per defect above. One arm was dead on first run -- it checked for the bare function name, which the forward declaration and definition also satisfy, so deleting the call still passed. It is now anchored on the indented call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-review caught a regression the rework introduced. The first version mapped DUNGEON_DIFFICULTY_CHALLENGE to client id 8; inverting the direction to client -> internal, I never wrote the reverse, so id 8 fell to the default -1 and all nine challenge dungeons -- 959, 960, 961, 962, 994, 1001, 1004, 1007, 1011 -- silently lost internal mode 2. Fixed by mapping client 8 back to mode 2, verified 9 of 9 against the shipped DBCs. The gate certified that regression because its mapping list simply omitted id 8. Adding it, plus an arm that deletes exactly that case, so the same slip cannot pass again. That is the second time today a gate has been blind to the defect it was written alongside, both times because the gate and the code were written from the same idea. Two further gate holes from the same review: it protected only two of the three raw scheduler lookups, leaving _ResetOrWarnAll unguarded, and its status line under-reported the mapping count. Both corrected. Also tidied the doc comment above ToInternalDifficulty, which still described the old internal -> client direction, and fixed the indentation of the new lines in _ResetOrWarnAll. Coverage unchanged at 113 of 116 instance maps resolving their regular tier; 938, 939 and 940 remain heroic-only 5-mans and are correctly refused on normal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round three kept TWO difficulty key spaces alive -- raw client DifficultyIDs
for the reset scheduler, internal 0-based modes for everything else -- and
added GetMapDifficultyDataByClientId to serve the former. Both reviewers
rejected it, independently and for the same reason.
The scheduler does not merely look rows up. Iterating sMapDifficultyMap keys
its own writes on raw ids: m_resetTimeByMapDifficulty, `instance_reset` and
every scheduled DungeonResetEvent. Those are read back with INTERNAL modes by
AddPersistentState (MapPersistentStateMgr.cpp:831), by MovementHandler's reset
warning (:236), by the DungeonPersistentState::GetDifficulty() comparison in
_ResetOrWarnAll (:1195), and by the instance/instance_reset SQL join in
_CleanupExpiredInstancesAtTime (:1276).
No raw id except 0 equals its own internal mode. Derived from the shipped DBCs:
of the 143 reset-bearing (map, internal mode) tiers the fix makes enterable,
129 missed the reset table outright and got resetTime 0 -- which SendRaidInfo
transmits as uint32(0 - now), a ~79-year raid lockout that survives restart --
while the other 14 numerically matched raw id 3 and silently took raid
10-normal's row while claiming to be 25-heroic. Auditing callers of the two
accessors could not see any of this, because the defect was in the key space,
not in the lookups. The gate certified it for the same reason.
So: translate once, at DBC load, and keep one key space.
- LoadResetTimes now enumerates the internal-mode index via the new
GetMapDifficultyLegacyMap(); the three scheduler sites revert to
GetMapDifficultyData. All 143 tiers now key with what reads them.
- GetMapDifficultyDataByClientId is deleted. Raw ids no longer escape
DBCStores.cpp, and the gate bans the symbol so the design cannot return.
- BuildMapSpawnModeMasks gains client id 8. It dropped challenge mode while
ToInternalDifficulty admitted it, so a player at dungeon difficulty 2 on
the nine challenge maps passed the area trigger and then instantiated with
no spawns filed under mask bit 2. Index modes and spawn masks now agree on
all 253 maps, where before they diverged on nine.
- Comments corrected: the header still said id 8 had no internal equivalent,
and two comments claimed the widening predicate was identical to
BuildMapSpawnModeMasks'. It is not -- that one fires on an exact (1 << 1)
mask, this one whenever mode 0 is absent. They agree on all shipped data;
the comment now says so rather than asserting identity.
Gate reworked around the key space rather than the accessors. Adds
case 0 -- the one id that equals its own internal mode, so its removal was
invisible to every existing assertion -- plus arms for the spawn-mask
divergence, the scheduler iterating the raw map, and the deleted accessor
returning. Ten arms, each verified live.
Verified: gate 11/11; mangosd builds clean; derived from
MapDifficulty/Map/Difficulty.dbc -- 143/143 reset tiers key correctly (was
14/143), 113/116 instance maps resolve the regular tier (938/939/940 are
heroic-only and correctly refused), 9/9 challenge dungeons at internal mode 2
and spawn-mask bit 2, widening fires on exactly the seven rows=[4] raids,
132/132 world/bg/arena maps unchanged.
Operational note: `instance_reset` rows written by an earlier build hold raw
client ids. Rows that no longer resolve are deleted at load with an error log;
the 14 raid rows that coincidentally validate carry a same-duration weekly
time, so they are harmless. `DELETE FROM instance_reset;` on the characters DB
clears them outright if a clean slate is preferred.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…_reset
Codex blocked d598d4b80 on two counts, both correct, and both about claims the
commit made rather than the scheduler rework itself.
1. The `instance_reset` migration was incomplete, and my note calling the
leftovers harmless was wrong. Of the 136 rows the old raw-keyed scheduler
could persist, 113 fail the new lookup and are deleted, 14 raid rows are
duration-compatible -- and NINE are not. A raw id 2 row (5-man heroic,
86400s) on maps 959/960/961/962/994/1001/1004/1007/1011 resolves as internal
mode 2, which on those maps is CHALLENGE, whose row has no global reset. The
enumeration skips RaidDuration == 0, so the stale timestamp is never
overwritten and a challenge instance inherits an old heroic lockout.
LoadResetTimes now rejects any row whose mapped tier has no RaidDuration:
the scheduler only ever writes rows for tiers that have one, so a row that
does not cannot have come from us. The table is self-migrating again.
2. The commit claimed internal modes were the server's only difficulty key
space. They were not. Two more DBCs store raw client ids and both were cast
directly, and both PERSIST the result:
- LfgDungeons.dbc. LFGMgr::CreateDungeonGroup cast DifficultyID straight to
Difficulty, so LFG normal (id 1) selected internal mode 1 -- HEROIC -- and
LFG heroic (id 2) selected CHALLENGE. Group::SetDungeonDifficulty writes
that to `groups`.`difficulty` and through the members to
`characters`.`dungeon_difficulty`, so one LFG run left every member's
saved difficulty wrong. Now translated, with the 77 rows that have no
internal mode (LFR 7, scenarios 11/12, flexible 14) falling back to
regular rather than persisting something unreadable.
- DungeonEncounter.dbc. Its DifficultyID was compared directly with an
internal mode before writing `instance`.`encountersMask`, at both the
credit site and in CONDITION_COMPLETED_ENCOUNTER. Of 699 shipped rows only
the 238 carrying id 0 ever matched, and for the wrong reason; the 264
5-man normal rows (id 1) were tested against internal mode 1, HEROIC, so a
normal clear credited nothing and a heroic clear credited the normal
encounter. Ids 5 and 6 (Sinestra, Ra-den) exceed MAX_DIFFICULTY as raw
values and could never match. Both sites now share
EncounterDifficultyMatches.
Id 0 there is a WILDCARD, not internal mode 0 -- derived, not assumed: 41
maps carry nothing but id 0 rows and 36 of those have more than one
difficulty tier, so reading it as normal-only would stop every heroic run
on them from ever crediting an encounter.
The gate grew to match. The symbol ban is now genuinely whole-tree (586 files,
with a floor check so a broken glob cannot pass trivially instead of scanning
nothing), block comments are stripped as well as line comments, and the three
new boundaries plus the migration guard are each pinned with their own arm.
Sixteen arms, all verified live. Its own macro was rewritten to take a variable
NAME: passing a file body to a CMake macro pastes it into the source, and any
quote or backslash in the tree then re-parses as code.
BuildMapSpawnModeMasks now has two more arms pinned, not just challenge mode.
Verified: gate baseline green with all 16 arms failing at their intended
assertion; full build clean; ctest 1141/1141; difficulty subset 17/17.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tier First of the three Codex BLOCK findings on this branch, and the one I caused. EncounterDifficultyMatches compared the translated DungeonEncounter difficulty against the internal mode for equality. Difficulty.dbc defines fallback chains, so a map that tags its encounters only for a lower tier credited nothing at all on the higher one. Derived from the shipped DBCs rather than from the review text, and the numbers came out differently. The review said 139 encounters across 37 maps; the real figure is 32 rows across five map/tier pairs -- maps 189, 289, 309 and 598 at heroic (6, 13, 10 and 1 rows, all tagged id 1) and map 994 at challenge mode (2 rows). Most maps escape because they use the id 0 wildcard, which already matched everything. The obvious fix would have been worse than the bug. 33 map/tier pairs ship BOTH an exact row and a fallback-reachable one, so walking the chain unconditionally would credit every boss on them twice. The chain is therefore consulted only when the map has no row of its own for the tier being asked about, which is what the new sEncounterExactTiers index records -- and which is why the predicate now needs mapId. Wildcard rows are deliberately excluded from that index: they apply everywhere, so a map carrying only wildcards should still be allowed to fall back. Map 994 is the case that shows the walk must run to completion: challenge mode reaches its rows only through the full 8 -> 2 -> 1 chain, and my own first probe missed it by stopping at the first fallback step. ClientDifficultyFallback is hardcoded for the same reason ToInternalDifficulty is -- Difficulty.dbc is never loaded by this core, and the table is twelve static rows. Values read from field 1 of the shipped file. Verified by modelling the implemented predicate against Difficulty.dbc, DungeonEncounter.dbc and MapDifficulty.dbc: 32 rows gained, 0 rows lost, and 0 lower-tier rows admitted where an exact row exists. Gate extended with six arms, each verified failing: equality-only, unconditional fallback, a truncated 8 -> 2 chain, a broken 2 -> 1 link, a missing index build, and dropping mapId from the signature. Three existing assertions and two existing arms quoted the old two-argument call and went stale when the signature changed; the arms had died silently, which the gate's dead-arm guard caught. Full suite 1171/1171. Gate baseline green, 22 arms all failing on their own assertions, none dead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second of the three Codex BLOCK findings. CreateDungeonGroup called Group::SetDungeonDifficulty for every LFG group. Raids and dungeons keep their difficulty in different fields and the setters persist to different columns: SetDungeonDifficulty writes `groups`.`difficulty` and each member's `characters`.`dungeon_difficulty`, SetRaidDifficulty writes `groups`.`raiddifficulty` and `characters`.`raid_difficulty`. So every LFG raid filed its tier in the dungeon slot and left the raid slot untouched. Measured from LfgDungeons.dbc: 61 of the 343 rows are TypeID 2 (LFG_TYPE_RAID), and 57 of those carry a tier that translates to an internal mode. Their tiers span internal 0..3 -- client 3 and 9 to 0, client 4 to 1, client 5 to 2, client 6 to 3. Internal 3 is 25-player heroic, which no 5-man tier corresponds to, so the group did not merely have the value in the wrong column: it had a dungeon difficulty outside the range dungeon difficulties can hold. The remaining 4 raid rows are client 14 (flexible), which has no internal mode; those follow the existing untranslatable path and now take REGULAR_DIFFICULTY on the raid field rather than the dungeon one. dungeon->TypeID == LFG_TYPE_RAID is the test the rest of LFG already uses -- LFGMgr.cpp:330 and LFGMgrQueue.cpp:113 -- so this adds no new notion of raid-ness. Gate extended with two arms, both verified failing: routing raids back through the dungeon setter, and dropping the TypeID test entirely. Full suite 1173/1173. Gate baseline green, 24 arms all failing on their own assertions, none dead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rrect four claims Response to a read-only review that returned BLOCK, plus an independent multi-agent verification pass. The blocking finding was real and is fixed; three of the corrections are to claims I had written and could not support. BLOCKING -- challenge mode admitted an empty instance. ToInternalDifficulty translated client id 8 to internal mode 2 and BuildMapSpawnModeMasks agreed, so the two switches were consistent with each other. The world database still cannot populate that tier. All nine challenge maps ship a MapDifficulty id 8 row, but maps 959/960/961/962 carry 347/177/433/561 creatures with every one at spawnMask 3 (bits 0 and 1), and 994/1001/1004/1007/1011 have no creature spawns at all in any mode. Nothing anywhere on those maps has bit 2, so a player passed the area trigger and arrived somewhere empty. That is the divergence this file warns about, one layer below where the warning was aimed: the earlier revision checked the two switches against EACH OTHER, and agreeing about a mode the DATA never populates still yields an empty dungeon. BuildMapSpawnModeMasks could not have rescued it either -- its output is a validation permission mask, not a spawn source. Client id 8 is now untranslated in both switches, with the measurement and the exact query to re-enable it recorded. Refusing entry beats admitting a player to an empty dungeon. Knock-on: the encounter fallback gain drops from 32 rows across five map/tier pairs to 30 across four, because map 994's two rows were reachable only through the 8 -> 2 -> 1 chain that no longer starts anywhere. IMPORTANT -- two more LFG sites compared raw ids against the internal enum. Neither was touched by the earlier LFG commit. GetDungeonType tested dungeon->DifficultyID against DUNGEON_DIFFICULTY_NORMAL/HEROIC, so raw 1 (5-man normal) matched HEROIC because both are 1, and raw 2 (5-man heroic) matched neither and fell through -- into the NEXT case label, since both arms were also missing a terminating return. Every TBC and WotLK dungeon was therefore typed as heroic or as unknown, never as normal. The role-count initialiser had the same comparison, so the 90 normal-dungeon rows never initialised neededTanks, neededHealers or neededDps -- the one case that code claims to handle. CORRECTIONS TO MY OWN CLAIMS. The double-credit justification for the encounter fallback guard was false. DungeonPersistentState::UpdateEncounterState returns immediately after the first matching row, so no kill can credit two encounters. The guard is still right, for a narrower reason: the first matching row wins, so without it a lower-tier row could answer ahead of the map's own and set the wrong Bit. The guard's granularity was challenged as too coarse -- a mixed-tier map might carry an encounter existing only at the lower tier, which a per-map-tier guard rejects. Measured per encounter Bit: of the 109 fallback-reachable rows the guard blocks across those 33 pairs, every one has its Bit already covered by an exact or wildcard row on the same map and tier. Zero encounters are lost, so per-map-tier and per-encounter agree on all 5.4.8 data. The concern is legitimate for future data and the query to re-check is recorded. The LFG comment claimed SetRaidDifficulty persists to `characters`.`raid_difficulty`. That column does not exist. Verified against the live schema: `characters` has only `dungeon_difficulty`, and the sole raid column anywhere is `groups`.`raiddifficulty`. The raid tier reaches a character through Player::_LoadGroup at next login. The 41 base-less spell-effect keys were attributed to "Sinestra and Ra-den style" rows. They are predominantly WotLK ICC and ToC spells; that attribution belongs to the DungeonEncounter.dbc discussion elsewhere in the file. GATE. The challenge-mode checks are inverted -- two arms now RE-ADD it and must fail -- and both absence checks are scoped to a single function body, because "case 8: return 2;" is not unique in this file: ClientDifficultyFallback legitimately contains it, since Difficulty.dbc really does record 8 -> 2. A first draft searched the whitespace-flattened text for the two-space spelling, which matches nothing after flattening; it was vacuous and its arm passed. A second draft of the spawn-mode arm spanned the newline after "case 9:", where stripping the trailing comment leaves the separating spaces behind, and died silently -- the same hazard a note already in this gate warns about. Full suite 1175/1175. Gate baseline green, 26 arms all failing on their own assertions, none dead. git diff --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nt re-measurement Re-probed DungeonEncounter.dbc and MapDifficulty.dbc from scratch, transcribing EncounterDifficultyMatches and ClientDifficultyFallback into a standalone script rather than reusing the earlier working, because the original "0 encounters lost" figure had no independent check. Three corrections, none of them behavioural: * "32 rows gained" was wrong; it is 30. Checked by row/tier pair, by distinct DBC row Id, and without requiring the map to offer the tier -- every counting gives 30. * 0 encounters lost is CONFIRMED. 109 fallback-reachable rows are blocked across 33 map/tier pairs, and every one has its Bit already covered by an exact or wildcard row on the same map and tier, so no boss becomes uncreditable. The per-map-tier granularity and a per-Bit one agree on all shipped 5.4.8 data. * Which means the guard is INERT at boss granularity on this data, and the code comment now says so. It is kept for intent -- an exact row for a tier settles that tier -- not because it fixes an observable symptom. The gate arm that pins the guard had the worst of it: it justified itself by claiming the unconditional variant double-credits every boss. That cannot happen, UpdateEncounterState returns after the first match, and it was already withdrawn once in review. The arm stays, because without it the guard reads as dead code and would be deleted, but the comment now states what the arm actually pins and records both wrong justifications so neither comes back. The fallback walk also has a cycle assertion in the probe; the shipped chains terminate.
… tier refusal Cross-model review returned BLOCK on this branch. Both blocking findings were real and are fixed here, along with two important ones and a set of comment corrections. `instance_reset` is now validated as a WHOLE TABLE, not row by row. The old build keyed that column on raw client DifficultyIDs, and the two key spaces overlap, so per-row validation is not a migration. Re-measured from MapDifficulty.dbc: the old scheduler could write 136 rows across 88 maps, with stored values 2, 3, 4, 5, 6 and 9. Read as internal modes, 122 are detectable -- 4/5/6/9 exceed MAX_DIFFICULTY, and raw 2 resolves to internal 2, which has had no legacy-index entry since challenge mode stopped being translated. Exactly 14 are NOT detectable, all of them stored value 3: raw 3 is raid 10-normal, internal 3 is raid 25-heroic, and on the 14 maps carrying both with a global reset a stale 10-normal timestamp validates perfectly, is applied by SetResetTimeFor, and then suppresses the fresh initialisation the scheduler would have done -- because that loop only fills tiers that have no reset time yet. No per-row test can separate those 14 from rows we wrote ourselves. So rows are collected first and applied only after the whole table validates; one bad row condemns all of it. That is sufficient because every one of those 14 maps also carries at least one DETECTABLE stale row -- a 10/25 raid ships raw 3, 4, 5 and 6, all with a global reset -- and measured, zero of the 14 lack one. The trigger is deliberately broad: any invalid row condemns the table, including a map dropped from the DBC or a hand-edited row. These rows are a cache the scheduler rebuilds in the same startup, so the cost is that global lockout boundaries are recomputed once. The previous comment here claimed nine raw-2 challenge rows survived validation and that the RaidDuration test was what caught them. Both halves were obsolete once challenge mode stopped being translated; `!resetDiff` already rejects them. Untranslatable LFG tiers are now REFUSED, at admission. ToInternalDifficulty returning -1 meant nothing, because CreateDungeonGroup turned every negative into REGULAR_DIFFICULTY and then registered the group and teleported it. An LFR row put a 25-player queue into the 10-normal tier of the same raid. Refusing there is not possible -- the group exists by then and its members have been pulled out of their previous groups -- so JoinLFG now returns ERR_LFG_INVALID_SLOT for any slot whose tier has no internal mode. Measured against LfgDungeons.dbc that refuses 70 of 210 DUNGEON rows (tiers 7/11/12/14), 4 of 61 RAID (tier 14) and 2 of 12 RANDOM_DUNGEON (tiers 11/12); QUEST and ZONE were already refused by the existing default arm. The fallback in CreateDungeonGroup stays as a safety net and now logs at error level. Checking the selected row was not enough. All 10 random rows that pass admission have Group_ID 0, and Group_ID 0 matches 273 rows including 20 at scenario tier 12, so an untranslatable dungeon reached CreateDungeonGroup behind a perfectly valid random queue. The expanded candidate set is filtered too -- dropped rather than refused, so random queueing keeps working -- and the queue fails if filtering empties it. Raid rows no longer get five-man semantics. Translating the difficulty made this newly possible: raid rows carry raw 3, 4 and 9, which translate to internal 0 and 1, exactly what both sites test for, whereas before they compared raw against internal and missed. GetDungeonType now returns DUNGEON_UNKNOWN for LFG_TYPE_RAID before any tier test -- DungeonTypes has no raid member, it classifies five-man dungeons for daily rewards -- and the role-count initialiser requires TypeID != LFG_TYPE_RAID before applying the hardcoded 1/1/3 composition. LfgDungeons.dbc does carry per-row Count_tank/Count_healer/Count_damage, which is where raid compositions should come from; reading them would move the five-man numbers too, so it is recorded at the site as follow-up. The ClientDifficultyFallback walk is bounded and logs a cycle. No DBC can introduce one, since the core never loads Difficulty.dbc, so the bound exists to make a bad hand-edit a wrong answer instead of a startup hang. Five contradictory comments corrected: the ToInternalDifficulty declaration claiming challenge mode has an internal equivalent, the "id 8 was added to both switches" claim, BuildMapSpawnModeMasks' "challenge mode (8) -> 2" line, the gate's double-credit claim, and the gate's assertion that `characters` has a matching raid column. Not fixed, deliberately: the exact-tier guard's per-Bit invariant is a property of the DBC data, and a source gate cannot check it. It is recorded as a KNOWN LIMITATION with the measurement and a note that a DBC change needs re-measuring by hand. Gate 24 -> 31 arms, all failing as intended with none dead; it now reads LFGMgrQueue.cpp and LFGMgr.cpp too. One new arm initially passed because its assertion's substring also matched a pre-existing `else if (dungeon->TypeID == LFG_TYPE_RAID)` in the same file -- it now matches the guard together with its body on a flattened view. Build clean, suite 1184/1184.
…not fix
Second review round returned BLOCK again, and it took apart the reasoning rather than the code.
The whole-table condemnation was justified on "every map with an ambiguous raw-3 row also carries a
detectable stale row". That is DBC co-occurrence, not TABLE co-occurrence, and the table is allowed
to be incomplete -- the scheduler rebuilds missing rows by design. A legacy table can hold a raw-3
row with its raw-4/5/6 companions already gone: after a partial startup, after manual cleanup, or
after one run of the earlier row-by-row version of this very migration, which deleted precisely
those detectable rows one at a time. Such a table validates, the stale raid 10-normal timestamp is
applied as 25-heroic, and the rebuild is suppressed.
That case cannot be fixed by inspecting row contents. It needs a durable marker, and this repository
does not carry the characters schema, so the marker is a one-time migration in the database repo:
DELETE FROM `instance_reset`;
It is written up and handed over with this change. Until it is applied, an upgraded database whose
detectable rows were already cleaned can still hold up to 14 wrong 25-heroic reset times. Saying so
here rather than leaving it implied.
What IS fixed is the blast radius, which was the second finding and also correct. Condemning the
table on ANY invalid row was far too broad: the rebuild computes `today + period + diff`, so
discarding a valid row moves that lockout boundary by up to a full reset period. Describing this
table as a semantically free cache was wrong. The trigger is now DEFINITIVE evidence only -- a
stored value >= MAX_DIFFICULTY, which nothing but the raw key space can produce. Invalid rows with
any other cause are deleted individually, exactly as this code always did, and leave the rest of the
table alone. Re-measured: all 14 ambiguous maps do receive a >= MAX_DIFFICULTY row from the old
scheduler, so a table the old build wrote in full is still fully covered.
The random-expansion filter claimed a guarantee it does not provide. Both the party and solo paths
replace the expanded set with randomDungeonID alone before the queued LFGPlayers state is built, and
SendDungeonProposal reads *dungeonList.begin() from that -- so the proposal always carries the random
row, which the admission check has already validated. The admission refusal is the guarantee; the
filter only keeps untranslatable ids out of roleCheck.dungeonList, which goes to the client and is
re-read on a role-check rejoin. The comment now says that, and the dungeons.empty() branch is gone
because the admitted random row is translatable and always survives filtering.
Gate 31 -> 32 arms, with one pinning the narrow trigger specifically: mutating it to fire on any
invalid row now fails. Build clean, suite 1185/1185.
…its trigger Two loose ends from the previous commit, found uncommitted in the worktree and committed here so they are not lost. Sniffing the table cannot be sufficient. DBC co-occurrence is not TABLE co-occurrence: a legacy table can hold an ambiguous raw-3 row with its raw-4/5/6 companions already deleted, in which case it passes every check, the stale 10-normal timestamp is applied as 25-heroic, and the rebuild is suppressed. CHAR_DB_STRUCTURE_NR therefore goes to 2. A structure mismatch is fatal in Database::CheckDatabaseVersion, so an un-migrated database cannot start. CONTENT would not do -- a content lag is only a warning there. The condemnation trigger also stops using `difficulty >= MAX_DIFFICULTY`. Out-of-internal-range is not evidence of the raw key space; an arbitrary hand edit is out of range too, and treating that as proof condemns a whole table of valid reset times over one junk row. IsLegacyRawResetKey answers the precise question instead -- is this a raw reset-bearing (map, tier) pair the old scheduler could have written -- and returns no row, so it is not a way around the tree-wide ban on raw-keyed lookups. Requires the matching characters-database update that drops `instance_reset` and advances db_version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch predates a4a7fd7, which removed every *_source_test.cmake under src/game/Server/tests. The file is new on this branch, so rebasing reintroduced it with no merge conflict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MadMaxMangos
force-pushed
the
fix/mop-instance-difficulty-lookup
branch
from
August 4, 2026 17:02
0e4d5ef to
66ccb32
Compare
LfgDungeonsEntryfmt began with 'i' rather than 'n'. DBC_FF_IND is 'n' and marks the INDEX column; DBC_FF_INT is 'i' and is just a uint32. With no index column declared, DBCFileLoader::GetFormatRecordSize returns index_pos -1 and AutoProduceData falls into its fallback branch, numbering indexTable 0..recordCount-1 in FILE ORDER. Every LookupEntry on this store was positional. Reading the shipped DBC shows what that produced: LookupEntry(6) -> row 6 = id 12 "Stormwind Stockade" LookupEntry(12) -> row 12 = id 26 "Maraudon", MapID 349 So a player queueing Deadmines (id 6) was offered Stockade, and the group was then built for id 12 and sent to Maraudon's map. Confirmed live before the fix: "chose 6 (entry 0x0100000C)" and "DENIED, dungeon 12 map 349"; after it, "chose 6 (entry 0x01000006)" and "dungeon 6 map 36". The second consequence is quieter and larger. Without an index column GetNumRows() reports recordCount (343) instead of maxId+1 (776), so every loop of the form `for (id = 0; id < GetNumRows(); ++id) LookupEntry(id)` -- three of them in the LFG code, including FindRandomDungeonsNotForPlayer -- walked less than half the id space and mis-identified every row it did visit. This belongs in THIS pull request rather than a later one. The branch's own LFG code calls LookupEntry by id, and one of its comments was measured through the broken lookup and had to be withdrawn (see the following commit). Separating the difficulty key-space fix from this one would leave the dungeon finder reading the wrong rows, so instance entry would still not work end to end. Of the 129 format strings in DBCfmt.h, 86 start with 'n', 29 with 'd', 10 with 'x' and only 4 with 'i'. Two of those four (AreaTable, ScalingStatValues) carry an 'n' later in the string and are properly indexed. Of the remaining two, ChatChannels is genuinely unindexed and its only consumer knows it -- it linear scans comparing ch->ID, with a comment saying so. LfgDungeons was the only store looked up by ID while indexed by row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch separates the raw 5.4.8 client DifficultyID from the core's internal
0-based Difficulty. It did that correctly inside the core but left the two
boundaries where the key spaces actually meet the client unconverted.
WIRE, OUTBOUND -- live. Player::SendDungeonDifficulty and SendRaidDifficulty put
the internal value straight on the wire, so a character on normal difficulty
reported 0. Three independent oracles agree the wire carries the RAW client id:
- corpus, build 18414: SMSG_SET_DUNGEON_DIFFICULTY has 954 packets carrying
only 1 (x512) and 2 (x424). Zero occurrences of 0.
- FrameXML/Constants.lua: DIFFICULTY_DUNGEON_NORMAL = 1, _HEROIC = 2,
_CHALLENGE = 8.
- an observed SMSG_SET_RAID_DIFFICULTY body carries 9, a legacy 40-player
raid, which cannot be an internal mode at all -- those stop at 3.
Both opcodes are admitted by IsEnterWorldConverted, so this was happening on
every send. ToClientDifficulty(Difficulty, bool isRaid) sits beside
ToInternalDifficulty; the inverse is not a function, since several raw ids
collapse to one internal mode, which is exactly why the client keeps dungeon and
raid difficulty in separate fields with separate opcodes.
WIRE, INBOUND -- latent. HandleSetDungeonDifficultyOpcode and
HandleSetRaidDifficultyOpcode read the client's raw id and used it as a
Difficulty directly, so Normal (raw 1) would have stored HEROIC, Heroic (raw 2)
CHALLENGE, and Challenge (raw 8) would have been rejected outright since
MAX_DUNGEON_DIFFICULTY is 3. Neither handler has a DefC registration, so nothing
reaches them today -- but wiring them in that state would silently invert the
setting. They now translate, and are deliberately left dormant: the inbound
opcode values are not established for 18414. There is no
CMSG_SET_DUNGEON_DIFFICULTY declared at all, and MSG_SET_RAID_DIFFICULTY 0x0614
is a 4.3.4 carry-over tagged "no client leaf".
DEFECT -- every number in the random-expansion comment was wrong. It claimed
Group_ID 0 matches 273 rows, that 20 of them carry scenario tier 12, and that
all 10 admissible random rows have Group_ID 0. Re-measured against the shipped
LfgDungeons.dbc: 79 rows, ZERO at tier 12, and no admissible random row has
Group_ID 0 -- the ten carry 1, 2, 3, 4, 5, 12, 13, 33, 36 and 37, one per
expansion tier. They were measured through the row-ordinal LookupEntry fixed in
the previous commit. The filter is harmless and kept as a guard, but it removes
nothing for any shipped random and must not be cited as load-bearing.
The branch's other measurements were re-checked and all hold exactly: 343 rows,
77 carrying an untranslatable tier, 4 of those raids, 61 TypeID 2 rows. Those
were computed by ITERATING rows, which never needed a by-id lookup -- which is
why only the Group_ID claim was corrupted.
DEFECT -- CHAR_DB_UPDATE_DESCRIPT was 43 characters against a varchar(30)
column, so it could never equal any row the matching update writes. Only version
and structure gate startup and the description is merely printed, so this was
cosmetic, but it made the mismatch banner advertise an expected value no
database could hold. It now matches the characters update exactly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MadMaxMangos
marked this pull request as ready for review
August 5, 2026 12:48
Member
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc90d089be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follows the wire-boundary conversion. Every value here was verified before the
comments were written, and the evidence tier in each comment is what was
actually established rather than what was assumed.
SMSG_SET_DUNGEON_DIFFICULTY 0x1283 -- CONFIRMED, and the identification is now
recorded properly. It is dispatcher case 57, and case 57 has exactly ONE member:
brute-forced over all 65536 u16 inputs against a hash validated first on the
known 0x183B -> case 493 -> SMSG_MOTD anchor. Handler sub_6D8F3E has exactly one
xref, inside that case.
The old comment credited a "direct reader/handler", which overstated it. The
reader sub_6D9F28 is the SHARED constructor for every single-uint32 SMSG and has
50+ xrefs, so it is not an opcode fingerprint; the case index and the handler
are what identify this opcode. A future investigator should not treat that
reader as unique to it.
Payload is the raw client DifficultyID. Across 954 build-18414 captures the
value set is exactly {1 x512, 2 x424, 8 x18} -- the three DIFFICULTY_DUNGEON_*
constants in FrameXML/Constants.lua -- with ZERO occurrences of 0. The 18
occurrences of 8 are decisive on their own: no internal 0-based dungeon mode can
produce 8.
SMSG_SET_RAID_DIFFICULTY 0x0591 -- CONFIRMED, and the reason it looked doubtful
is now recorded so nobody repeats the doubt. The value is BIDIRECTIONAL: the
same number carries the CMSG and the SMSG. It is absent from the perfect-hash
SMSG dispatcher because this client has a second SMSG delivery subsystem that
bypasses it, so that absence is not evidence against the value. The corpus rows
flagged directionConflict are explained by the shared value rather than being
catalogue defects, which is what they were previously assumed to be.
MSG_SET_RAID_DIFFICULTY 0x0614 -- REFUTED and removed. A 4.3.4 carry-over tagged
"no client leaf", with no leaf in the 18414 binary and zero occurrences across
1079 captures in either direction. Nothing referenced the enum; only a stale
DEBUG_LOG string, corrected here.
The two real inbound opcodes are declared in its place, each a bare uint32 raw
client DifficultyID: CMSG_SET_DUNGEON_DIFFICULTY 0x1A36 and
CMSG_SET_RAID_DIFFICULTY 0x0591. Both agree with the values already recorded in
Opcodes_reference.h, which were derived independently.
They are declared and NOT registered. The handlers translate correctly now, but
registering them is a behaviour change that belongs with a live test of the
difficulty UI, not with a key-space fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both from the PR review, both real, both in the boundary work rather than the branch's original content. The inbound handlers accepted ids from the OTHER key space. ToInternalDifficulty is context-free by design -- the reset scheduler and the encounter predicate must translate whatever a DBC row carries -- but an inbound packet is different: the opcode tells us which field is being set, so a raid id in a dungeon request is malformed rather than translatable. A range check cannot catch it. Raw 5 is 10-player heroic RAID; it translates to internal 2, passes `< MAX_DUNGEON_DIFFICULTY`, and would set DUNGEON_DIFFICULTY_CHALLENGE -- a mode ToInternalDifficulty deliberately never produces, because no spawn on a challenge map carries bit 2 and admitting it opens an empty instance. So the wrong-space id lands on exactly the value the branch went out of its way to refuse. The mirror holds for raids: raw 2 is 5-man heroic and would have been accepted as 25-player normal. ToInternalDifficultyChecked(id, isRaid) now validates against the space being set, mirroring ToClientDifficulty's shape. Dungeon accepts raw 1 and 2 only; raid accepts 3, 4, 5, 6 and 9. Raw 8 stays absent, matching ToInternalDifficulty, and raw 0 is excluded because continents are not a tier a player sets. IsLegacyRawResetKey truncated its input. MAKE_PAIR32 packs the difficulty into the low 16 bits, so a hand-edited 65538 becomes 2 -- a real raw DifficultyID. On a map carrying a legacy raw-2 reset row, that junk would then be accepted as PROOF the table is raw-keyed, and LoadResetTimes would delete every reset row in it. That is exactly the blast radius this predicate exists to avoid: it was already narrowed once, from "any invalid row condemns the table" to "only a row that looks raw-keyed does", and the truncation defeated that narrowing via the same single junk row. Values above 0xFFFF are now rejected before the pair is built -- 0xFFFF because it is precisely what MAKE_PAIR32 can carry, which is the actual failure being prevented; the shipped DBC only uses 0..14. Both handlers remain unregistered, so neither defect was live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Thanks — both P3s are real and are fixed in P1 — the migration exists, in the database repoThe search was of the server repo, and MaNGOS migrations don't live here — they live in the database repo alongside the other
Verified against a restored copy of a live characters database (never the live one): applies clean reporting The underlying point stands and the PR body already carries it: this server revision cannot be rolled out by itself. The database PR must land with or before it. The reverse is safe — a database carrying the update is harmless to an older server, which compares only version and structure and reports a content lag as a warning. Worth adding that for a database written by master this migration is belt-and-braces rather than load-bearing: 122 of the 136 raw rows are detectable and the runtime detector wipes and rebuilds the table on first start. The residual is 14 ambiguous raw-3 rows whose stale timestamps would suppress re-initialisation of one raid tier. That is why the bump is STRUCTURE (fatal) rather than CONTENT (warning) — an un-migrated database must not start silently. P3 — dungeon setter accepting raid IDsCorrect, and the failure mode is worse than a range slip: raw 5 translates to internal 2 and lands on Added P3 — MAKE_PAIR32 truncationAlso correct, and it defeated a narrowing that had already been made deliberately. The predicate was tightened once from "any invalid row condemns the table" to "only a row that looks raw-keyed does"; truncating 65538 → 2 let a single junk row reach that conclusion anyway on any map with a legacy raw-2 row. Values above Both handlers remain unregistered, so neither P3 was live. |
Member
Author
|
@codex review the last commits since your last review only, did they fix the issues raised. |
Codacy flagged LoadResetTimes on PR #81: it went from 162 lines to 275, because the instance_reset key-space migration was folded in alongside the two things that function already did -- loading per-instance reset times, and rebuilding the global schedule from the difficulty index. The migration is a self-contained concern. It reads one table, validates it as a whole, decides whether the stored key space is the raw client one, and either applies the rows or discards them. Nothing else in LoadResetTimes depends on how that decision is reached, only on the reset times it leaves behind. It also carries by far the longest rationale in the file, since the trigger had to be narrowed twice under review and the reasoning for both the narrow trigger and the whole-table response has to survive the next person to read it. So it moves to DungeonResetScheduler::LoadGlobalResetTimes, taking its comment with it. The `if (result)` wrapper becomes an early return, which is why the body reindents; that reindent is the whole of the apparent churn. No statement is added, removed or reordered, and the configured reset hour is passed in rather than recomputed so the two callers cannot drift. LoadResetTimes is now 148 lines -- below where it started -- and the migration is 140, most of it comment. The two log lines inside the moved block named MapPersistentStateManager:: LoadResetTimes, which was already the wrong class and is now the wrong function as well. They name DungeonResetScheduler::LoadGlobalResetTimes instead. The second of those tells an admin which statement to run by hand, so a prefix that does not lead back to the emitting code is a real, if small, cost. Verified: game.lib builds clean, no new warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c3267838
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The key-space split reaches saved player state as well as the reset schedule. The old dungeon finder cast a raw client DifficultyID straight into the internal enum, and Group::SetDungeonDifficulty persisted that to `groups`.`difficulty` and to every member's `characters`.`dungeon_difficulty`. It was invisible while the lookup was raw-keyed too -- a stored raw 2 found the heroic row and worked -- but this branch moves the lookup to the internal space, where 2 is CHALLENGE, and ordinary heroic dungeons have no challenge row. Those characters would be refused at the portal, and could not select their way out, since the difficulty setter opcode still has no registered handler. mangosfour/Database#3 now clamps both columns as part of the same update, so the description here follows it. `groups`.`raiddifficulty` is deliberately left alone: it had no pre-split writer. Only the description string changes; version 23, structure 2 and content 1 are unchanged, and the description is printed rather than compared, so this is a labelling fix rather than a gate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found that the widening in BuildMapDifficultyLegacyIndex copies rather
than moves. The first pass has already keyed these raids at internal mode 1, and
the widening then adds mode 0 without withdrawing mode 1, so one physical lockout
ends up with two key spaces. Seven maps are affected -- Hyjal (534), Magtheridon
(544), SSC (548), The Eye (550), Black Temple (564), Gruul (565) and Sunwell
(580), verified as exactly the raid maps whose whole raw DifficultyID row set is
{4}. Legacy keys 360 -> 367.
Both halves are live on shipped data, and this is code the branch itself adds --
neither sMapDifficultyLegacyMap nor this function exists at the merge-base, so it
is not a pre-existing defect the branch merely fails to fix.
LoadResetTimes iterates this index, so each of the seven wrote TWO instance_reset
rows and TWO scheduled reset events at every startup, with no player involved.
That fires unconditionally.
The worse arm needs a player but is far more damaging. A group set to 25 Player
resolves internal mode 1, finds the duplicate, and is admitted -- and because the
lookup SUCCEEDS, the existing fold in MapManager::CreateDungeonMap for instances
that only have one difficulty never runs. The map instantiates at spawn mode 1,
where 2226 creatures and 254 gameobjects do not exist: every spawn on those maps
carries spawnMask 1, which is mode 0 only. The group binds itself to an empty
Black Temple.
Two changes, and both are needed:
* The widening becomes a move. It is guarded on identity rather than erasing
blind, because two raw rows can in principle map to one internal mode and
only the promoted row should be withdrawn. No shipped raid does that.
* Player::GetAreaTriggerLockStatus folds a missing tier back to
REGULAR_DIFFICULTY before refusing. Without it the move alone would be a
regression of its own: erasing mode 1 would leave a 25 Player group refused
outright. This is not a new rule -- MapManager::CreateDungeonMap,
Group::GetBoundInstance and Player::GetBoundInstance all already fold; only
the admission gate did not, so it refused what the paths behind it would have
opened. isRegularTargetMap is deliberately untouched, since it is computed
from the player's own difficulty and drives the key and quest requirements.
Also corrects two comments that quoted 143 reset-bearing tiers and 129 misses.
Those were measured while the duplicates existed, so they counted this bug's own
rows as real tiers. The true figures are 136 and 122; the 14 ambiguous internal-3
maps are unchanged.
Verified at runtime, not by inspection. Booted the built server against a scratch
character database restored from the live one and migrated to structure 2, with
instance_reset empty: it wrote 136 rows (0->32, 1->76, 2->14, 3->14), and each of
the seven raids has exactly ONE row, at difficulty 0. Before this change the same
table stood at 143 with 83 at difficulty 1 -- a difference of exactly the seven.
Live database untouched; scratch database and test binary removed.
ctest: 100/100 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found that the key-space split reaches saved player state, not only the reset schedule, and that this branch owns the consequence. The old dungeon finder cast a raw client DifficultyID straight into the internal enum, and Group::SetDungeonDifficulty is not session-local: it writes groups.difficulty and pushes the value through every member, which the character save persists to characters.dungeon_difficulty. One completed LFG run left the whole party holding a raw id. That was self-consistent while the lookup was raw-keyed too. It is worth being exact about what "worked" meant, because it sets the severity: a stored raw 2 hit on 67 of the 83 five-man maps and the player passed the area trigger -- into an instance created at spawn mode 2, where nothing exists. No five-man map carries a single bit-2 creature or gameobject spawn. So the old behaviour was not working; it was broken quietly. This branch makes it loud, refusing entry outright, and that is the regression it owns. There is no way out from inside the game. The difficulty setter opcode still has no registered handler, so an affected character cannot select another tier. Both load clamps already normalise anything at or past MAX_DUNGEON_DIFFICULTY. They are narrowed by one to reject CHALLENGE as well, which is the only stale value that slips through -- everything the old cast could write above 2 was already caught. This branch deliberately makes CHALLENGE unproducible (ToInternalDifficulty refuses raw 8, because no spawn on a challenge map carries bit 2), so a value that can never be produced should not be loadable either. If challenge mode is ever wired up, both bounds have to move together. mangosfour/Database#3 clamps the stored columns in the same characters update. The two are not redundant: the migration also catches stale raw 1, which is a legal internal value and so passes these clamps while silently meaning HEROIC rather than the NORMAL that was queued; and these clamps cover a hand-edited row or a database that never ran the update. groups.raiddifficulty is left alone in both, since it had no pre-split writer at all. ctest: 100/100 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing NULL Indexing LfgDungeons.dbc by id rather than by row ordinal is correct, but it changes what LookupEntry returns for an unknown id, and JoinLFG was relying on the old behaviour without knowing it. While the store was indexed by ordinal, every id below the record count found some row, so the missing NULL check was survivable by accident. Indexed by id the space is sparse -- it runs to 774 with 432 holes -- and these ids come off the wire, so a client sending an id in a hole dereferences NULL at the TypeID switch. Refused rather than skipped: an id the DBC does not know is not a slot this server can offer, and ERR_LFG_INVALID_SLOT is what the surrounding loop already returns for a slot it will not take. Not reachable today -- JoinLFG has no call sites and HandleLfgJoinOpcode is still a parse-and-log stub -- so this is placed ahead of the dungeon finder work rather than fixing a live crash. It belongs on this branch regardless, because the branch is what makes the NULL possible. ctest: 100/100 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b9c992694
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review found that SMSG_GROUP_LIST writes its two difficulty fields transposed, and that converting them to raw ids made the defect active rather than inert. The builder wrote them in struct declaration order, raid then dungeon. Retail sends dungeon then raid. Two 18414 bodies settle it, both with the pair sitting immediately after the leader-GUID byte and before memberData: capture-000006 seq 1424 ... d0 | 02 00 00 00 | 03 00 00 00 | 04 02 01 05 ... capture-000020 seq 2096 ... 63 | 01 00 00 00 | 03 00 00 00 | 04 02 01 04 ... Raw 1 and 2 are five-man tiers and raw 3 is a raid tier, so the first field is the dungeon difficulty and the second the raid one. There is no other reading: a party difficulty of 3 does not exist. Harmless until this branch made the values meaningful. While both fields carried internal modes they were 0 for nearly every group, and 0 matches no instance row, so the client read them as unset -- wrong, but inert. Converting them to raw ids put two VALID ids into swapped slots, so GetDungeonDifficultyID would answer 3, a raid-only id, and GetRaidDifficultyID would answer 1, a party-only one. That is worse than the state it replaced, which is why it is fixed here rather than deferred. The struct field names are left alone; only the wire order changes. The fixture could not have caught this. mop_party_update_packets_test.cpp fed raid 3 and dungeon 2 and expected 3 then 2 -- an inverse of the builder rather than an independent check, so it certified the transposition. Its expectation now reads 02 00 00 00 03 00 00 00, which is byte-identical to capture-000006 seq 1424, so it is pinned to captured bytes instead of to our own writer. Effect is client-side only: the party context menu and the ready-check difficulty label. Nothing persisted was affected -- the server keeps internal values throughout and converts only at this boundary. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing them Review found that the upgrade would silently destroy character_instance rows, and it would hit exactly the characters that had been able to run dungeons. `instance`.`difficulty` holds an internal mode, but a build predating the split could put a RAW id there. The old dungeon finder cast an LfgDungeons DifficultyID straight into Difficulty, Group::SetDungeonDifficulty pushed it through every member, and MapManager::CreateDungeonMap then created and SAVED the instance under it. Raw 2 means five-man HEROIC -- and it was the only value that could get a character into a five-man at all on such a build, because the old raw-keyed lookup accepted it while the default 0 matched nothing. Read as an internal mode, 2 is CHALLENGE, and no ordinary heroic dungeon has a challenge row. Both validators treat that as a corrupt bind and DELETE it: Player::_LoadBoundInstances at login, and the `instance` sweep in LoadResetTimes at startup. character_instance carries no difficulty of its own -- it joins the value from `instance` -- so one poisoned row takes the player and group binds with it. This is the destructive one of the two stale-value cases and it is treated differently for that reason. The earlier clamp on `characters`.`dungeon_difficulty` sends the value to NORMAL, because there the alternative is a lockout: with the difficulty setter still unregistered, a character left at HEROIC cannot enter a normal-only dungeon and has no way to change it, whereas NORMAL is entry-safe everywhere. Here the alternative is deletion, so the value is preserved as what it always meant. Rewritten at startup rather than in the characters update because the decision needs the DBCs: telling a five-man from a raid is what makes it safe, and SQL in the characters database cannot. It is also unconditionally safe rather than safe only during an upgrade -- challenge mode is unreachable in this core by design, since ToInternalDifficulty refuses raw 8 and both load clamps reject internal 2, so a dungeon-map instance at internal 2 is a value this build cannot produce whenever it was written. Raids are deliberately untouched. Internal 2 there is 10-player heroic, a real tier on 14 shipped maps, so the same numeric value is legitimate data. Ordering matters and is asserted by placement: the rewrite runs at the top of LoadResetTimes, before the `instance` sweep below it and before any character can log in. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review last commits since last review. |
Review re-measured the factual claims in the recent commits. Four are wrong, and
three of those were introduced by this branch's own review-response commits. This
branch has already had to retract counts three times, so they are corrected with
the measurement rather than quietly reworded.
LFGMgr.cpp, the requirements helper: the map names were swapped. Map.dbc gives
632 = The Forge of Souls and 668 = Halls of Reflection; the comment had them the
other way round.
LFGMgr.cpp, same block: the claim that row-ordinal indexing made the lookup "miss
on noise", so that correcting the index turned the tier shift systematic, is
false. FindRandomDungeonsNotForPlayer enumerates 0..GetNumRows()-1 and reads
MapID, DifficultyID and TypeID off ONE row pointer, and LfgDungeons.dbc has 343
rows with no duplicate ids, so both index modes visit every row exactly once --
the same 33 rows reach `dungeonfinder_requirements` with the same wrong tier key
either way. The defect is PRE-EXISTING; this branch fixes it because it is the
branch that separates the key spaces, not because it created it. The claim was
carried over from CreateDungeonGroup, which looks a row up by a client-supplied
id and IS index-sensitive -- the same sentence in the database update refers to
that path and is correct there.
LFGMgr.cpp, the role-count comment: "the 59 continent-style rows" is 60. The
comparison being described had no TypeID filter, so its match set included id
358, 10v10 Rated Battleground, which is raid-typed. 59 is the non-raid subset,
which is not what that code matched.
PlayerInstance.cpp and DBCStores.h: both said 954 captures carry "only 1 (x512)
and 2 (x424)", which is 936. The set is {1 x512, 2 x424, 8 x18}, as Opcodes.h
already had it. The omission mattered -- 8 is the value that proves the payload
cannot be an internal mode, since no internal dungeon mode produces it, and
DBCStores.h is the evidence paragraph a reader would check that against.
Comments only; no behaviour change. ctest: 105/105 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A scoped review of the fix commits found that "convert the remaining outbound difficulty fields" overclaimed. Two sites still put the internal mode on the wire, and four independent reviewers converged on them. Player::SendInstanceResetWarning writes SMSG_RAID_INSTANCE_MESSAGE. Its difficulty field is keyed into Difficulty.dbc by the client, so an internal mode either misses or names another tier -- internal 0 on a raid reads as "5 Player" rather than the 10-player normal it means, in the message that tells a player their lockout is about to expire. Player::SendTransferAbortedByLockStatus passes the difficulty as the argument of TRANSFER_ABORT_DIFFICULTY, on both the MISSING_ITEM and MISSING_DIFFICULTY arms. The client keys that the same way, to name the tier in the refusal, so a player turned away from a raid was told the wrong one. Both name a map, so both take the map-aware conversion. The clamp on the MISSING_DIFFICULTY arm deliberately stays on the INTERNAL value. RAID_DIFFICULTY_10MAN_HEROIC is a meaningful ceiling only in the internal space; converting first and then comparing would be a cross-key-space comparison, which is the whole class of bug this branch exists to remove. It is lifted out of the ternary so the ordering is visible rather than implied. Also withdraws a claim in the KNOWN GAP comment added one commit earlier. It said "every creature and gameobject on those three maps carries spawnMask 2". Seven creatures carry spawnMask 0 -- five on Well of Eternity, two on Hour of Twilight. The measurement was in the output I read at the time and I wrote past it. It does not change the conclusion, because a spawnMask of 0 sets no bit and therefore appears in no mode, mode 0 included, so those seven would not populate a mode-0 instance either. The comment now gives the real figures: 1447 creatures and 81 gameobjects at mask 2, seven at mask 0. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re counts Closes the last of the scoped review's findings. All four reviewers agreed the fixes themselves are correct; what remained were an inconsistency and three claims that do not reproduce. The three calendar conversions hardcoded isRaid = true, where the sibling site written one commit later derives it. The lockout loop walks m_boundInstances across every internal mode, so it can carry a five-man bind as well as a raid one. It is unreachable today -- the argument only decides the FALLBACK, taken when the map has no row for the tier, and no shipped bind reaches it -- but a hardcoded true answers a dungeon from the raid table, and the four sites now agree rather than three of them being right by accident. ToClientDifficultyForMap is documented as differing from the fixed table on SEVEN maps. It is eleven. Alongside the seven raw-4 TBC raids there are four legacy 40-player raids -- Emerald Dream 169, Molten Core 409, Blackwing Lair 469 and Ahn'Qiraj Temple 531 -- which ship a single raw-9 row and now report 9 where the fixed table said 3. Difficulty.dbc row 9 is "40 Player", so this is the map-aware answer being MORE right rather than merely different: Molten Core is a live 40-man lockout and calling it 10-player normal is wrong in the raid browser and the calendar alike. That the correction was undersold is worth recording, because the seven were the entire stated justification for the function. Non-instanceable maps differ numerically too -- their raw rows carry 0 while the dungeon table starts at 1 -- but no lockout, bind or calendar entry names one, so they never reach a caller. Noted so the next re-measurement does not report 143 pairs and think something is wrong. The requirements comment listed four five-man maps carrying two rows and missed Opening of the Dark Portal 269, which is the clearest exhibit of the five: its difficulty-1 row is the Black Morass heroic attunement, so a raw-keyed table would have had to carry it at 2. The same comment said a player was held to heroic "item level and achievements". No five-man row in the table carries an achievement. The only two that do are Icecrown Citadel's (631,2) and (631,3), on a raid map that never reaches the lookup because LFG_FORBIDDEN_RAID short-circuits above it. The live five-man gates are min_item_level -- 180 on 12 rows, 200 on 6, 219 on 2 -- and attunement quests. Comments and one derived argument; no functional change. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment explaining why CMSG_SET_DUNGEON_DIFFICULTY and CMSG_SET_RAID_DIFFICULTY stay unregistered was false on both clauses, and on merge it would have read as an invitation to register them. It said the handlers "cast one onto the other with Difficulty(mode)". They call ToInternalDifficultyChecked, which converts and rejects an id from the other key space. It then said the translation "lives in the unmerged instance-difficulty work; these two are held until it lands" -- this branch IS that work, so the stated precondition is satisfied the moment it merges while the opcodes stay absent, which is exactly the trap a maintainer walks into. The blocker that actually remains is unrelated to translation. Both handlers call ResetInstances(INSTANCE_RESET_CHANGE_DIFFICULTY, ...), so accepting either one tears down the player's or group's instance binds -- and the two packets that report the outcome, SMSG_INSTANCE_RESET and SMSG_INSTANCE_RESET_FAILED, are not admitted by IsEnterWorldConverted, so they are built and dropped. Registering these today would destroy binds silently and tell the player nothing on either success or failure. That is the same reason CMSG_RESET_INSTANCES is held a few lines below. Verified rather than restated: the handlers call ToInternalDifficultyChecked at MiscHandlerInstance.cpp:104 and :161, both reach ResetInstances at :139/:145 and :196/:202, and neither feedback opcode appears in the WorldSession.cpp allow-list. Comment only; no behaviour change. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… lookup A cross-model review found two things five earlier rounds missed, one of them on the most reachable path in the change. SMSG_WORLD_SERVER_INFO carried the internal mode. Player::SendInitialPackets- BeforeAddToMap wrote GetMap()->GetDifficulty() straight into the field, and that function runs on every login and every map change, so this is the single most frequently sent difficulty in the server. The client treats the field as a DifficultyID and feeds it to GetDifficultyInfo(), so on the seven 25-player-only TBC raids internal 0 arrived as 0 rather than raw 4 and the client read a 25-man raid as "5 Player". Map-aware, because the packet is about the map the player is entering. Two sweeps of the outbound boundaries missed it, including one that claimed to have closed "the remaining outbound difficulty fields". Worth stating plainly: the sweeps were done by grepping for difficulty-shaped expressions near packet builders, and this site passes the value as a bare function argument, so it did not match. The lesson is about the method, not the site. Group::GetBoundInstance(Map*, Difficulty) did not fold a missing tier to REGULAR_DIFFICULTY, while its sibling overload and Player::GetBoundInstance both do -- all three under the same "some instances only have one difficulty" comment. It is not reachable today, because MapManager::CreateDungeonMap folds before a DungeonMap exists, so every caller of this overload already holds a supported difficulty. It is closed anyway: the inconsistency is the hazard, and if a DungeonMap were ever built at an unsupported tier the group-bind check would miss and a player could be bound to a second instance of a raid they are already locked to -- the exact failure the sibling fold was added to prevent. That also makes two of my own comments false, and they are corrected here: there are FIVE bind and admission lookups, not four, because Group::GetBoundInstance has two overloads and the earlier count only covered the one that had been checked. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The conversion added in the previous commit was justified from FrameXML -- the
client feeds this field to GetDifficultyInfo() -- which is inference, not observed
bytes. That is the wrong evidence tier for a wire claim on this campaign, and it
was challenged on exactly that ground.
Checked against the corpus, and the change stands on much better footing than it
did. Across 2554 build-18414 captures of SMSG_WORLD_SERVER_INFO the field carries
{0, 1, 2, 3, 4, 5, 7, 8, 9, 11, 12, 14}.
7 (LFR), 11 and 12 (scenarios) and 14 (flexible) settle it: no internal mode can
produce any of them, because internal modes stop at 3. 8 and 9 are raw-only as
well. The field is the raw client DifficultyID, so writing GetMap()->GetDifficulty()
into it was wrong -- the same shape of proof as the 18 occurrences of raw 8 that
settled SMSG_SET_DUNGEON_DIFFICULTY.
The 10/14/18-byte size split in the corpus was worth ruling out as a second
transmission path with a different layout. It is not: the leading bits are
optional-field presence flags and the optionals sit AFTER this field, so the
offset and the key space are identical in all three shapes. One 14-byte body,
80 00 10 94 bc 53 | 02 00 00 00 | 05 00 00 00, decodes as one optional present,
difficulty 2, then the optional.
Comment only; the conversion itself is unchanged. ctest: 105/105 pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
@codex review last commits since last review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83c251d0b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MopWorldEntryPackets::BuildWorldServerInfo(data, 0, | ||
| uint32(sWorld.GetNextWeeklyQuestsResetTime() - WEEK), | ||
| uint32(GetMap()->GetDifficulty())); | ||
| ToClientDifficultyForMap(GetMap()->GetId(), GetMap()->GetDifficulty(), GetMap()->IsRaid())); |
There was a problem hiding this comment.
When a player logs in or changes maps on Eastern Kingdoms (map 0), ToClientDifficultyForMap finds no MapDifficulty row because this branch notes map 0 is absent from the 4.x DBCs, then falls back with isRaid == false to ToClientDifficulty(..., false), which returns raw dungeon difficulty 1. The old code sent the map's internal difficulty 0, and the new comment says SMSG_WORLD_SERVER_INFO legitimately carries raw 0 for non-instance maps, so this regresses the most common outdoor map by telling the 5.4.8 client it is on a 5-man-normal tier instead of a world map; handle missing/non-instance map rows as raw 0 rather than using the dungeon fallback.
Useful? React with 👍 / 👎.
| // into Difficulty.dbc, so an internal mode either misses or resolves to another tier: | ||
| // internal 0 on a raid would read as "5 Player" rather than the 10-player normal it means. | ||
| MapEntry const* warnMap = sMapStore.LookupEntry(mapid); | ||
| data << uint32(ToClientDifficultyForMap(mapid, difficulty, warnMap && warnMap->IsRaid())); |
There was a problem hiding this comment.
When a player or group is left on an unsupported raid tier and enters a single-tier raid through the new fold-to-regular path (for example Sunwell while the selected raid difficulty is 10H/25H), callers still pass the player's selected difficulty into this helper. There is no map row for those tiers, so this line falls back to the fixed raid table and sends raw 5/6 in SMSG_RAID_INSTANCE_MESSAGE even though the actual folded lockout is the map's only raw 4 row; use the dungeon map/persistent state's folded difficulty, or fold here before converting.
Useful? React with 👍 / 👎.
…he map's tier Two review findings, one of them a regression the previous commit introduced on the busiest map in the game. ToClientDifficultyForMap fell through to the fixed dungeon table whenever a map had no MapDifficulty row for the tier. That is right for an instance holding a tier it does not define, and wrong for a map that has no tiers at all: the raw id for "not an instance" is 0, not whatever the dungeon table returns for the internal mode it was handed. It matters because Eastern Kingdoms, map 0, has NO MapDifficulty row. Most continents do carry a raw-0 row -- Kalimdor, Outland, Northrend, Pandaria, Gilneas, Deepholm, Tol Barad and the Wandering Isle all answer from the index -- but map 0 does not, so SMSG_WORLD_SERVER_INFO started reporting 1 there, telling the client Stormwind was a five-man normal tier on every login and every map change. Four of the 116 non-instanceable maps have no row, as do 29 of the 34 scenario maps; 38 maps in total were reaching the fallback. The corpus already said so and I did not read it carefully enough: 1890 of the 2554 sampled bodies carry 0, which is what an outdoor map reports. Verified against the real post-canonicalisation index rather than by inspection. Continents now return 0, and every instance value is unchanged -- Sunwell 4, Black Temple 4, Molten Core 9, Icecrown 3 and 4, Forge of Souls 1 and 2. Separately, DungeonMap::SendResetWarnings passed the PLAYER's selected difficulty into the reset warning rather than the map's own. Those can differ now that the area trigger folds a missing tier: a group left on 10-heroic that entered Sunwell -- a single-tier raid whose only row is raw 4 -- stands in a REGULAR instance while still selecting internal 2, so the conversion missed the map's row, hit the fixed raid table and announced raw 5 for a lockout that is raw 4. The warning is about that instance, so it now takes the instance's difficulty. MovementHandler's reset warning is left alone: it guards on GetMapDifficultyData(mapId, diff) before sending, so it only ever warns about a tier the map really has. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63f78b8251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+700
to
+702
| CharacterDatabase.DirectPExecute( | ||
| "UPDATE `instance` SET `difficulty` = '%u' WHERE `id` = '%u'", | ||
| uint32(DUNGEON_DIFFICULTY_HEROIC), stale[i]); |
There was a problem hiding this comment.
When upgrading a database that has stale raw-2 five-man instance saves, those rows were originally stored with a normal-instance resettime because GetResetTimeForDB() only zeroed internal heroic difficulty 1. This normalizer changes only the difficulty, so the later SELECT ... WHERE resettime > 0 path schedules the converted heroic save as a per-instance RESET_EVENT_NORMAL_DUNGEON, and AddPersistentState keeps that old per-instance reset instead of using the heroic global reset. Please clear resettime to 0 in the same rewrite so preserved heroic lockouts use the global heroic reset boundary.
Useful? React with 👍 / 👎.
Review found that the normalizer added earlier today leaves the row in a state this codebase never produces. DungeonPersistentState::GetResetTimeForDB zeroes `instance`.`resettime` only for raids and for DUNGEON_DIFFICULTY_HEROIC. A five-man save stored at internal 2 -- the stale raw-2 the normalizer exists to rescue -- is neither, so it was written with a NON-ZERO per-instance reset time. Rewriting only the difficulty therefore produced (heroic, resettime > 0). The first query in LoadResetTimes selects `WHERE resettime > 0`, so the converted save was picked up and scheduled as a RESET_EVENT_NORMAL_DUNGEON, and AddPersistentState would then honour that stale per-instance boundary instead of the heroic global reset the lockout actually belongs to. Cleared in the same statement. Zero is exactly what GetResetTimeForDB would have written had the save been stored as heroic to begin with, which is the state this rewrite reconstructs -- so the row now looks like one this build produced rather than a hybrid of two key spaces. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The conversion added for this field was right for instances and wrong for everything else, and the guard added afterwards fixed one half of that and made the other half differently wrong. A review pass caught the remainder. Off an instance, the map's spawn mode ALREADY is the value retail sends, so translating it cannot help and can only distort: continents and battlegrounds run at spawn mode 0 and retail sends 0, but several have no MapDifficulty row to convert against -- Eastern Kingdoms has none at all -- so the conversion fell through to the five-man table and sent 1; Tol'Viron Arena, map 980, is the single exception in PvpDifficulty.dbc. One of its 173 rows carries a nonzero Difficulty -- (map 980, level 90, Difficulty 1) -- and CreateBattleGroundMap copies that straight into the map's spawn mode, so the map genuinely runs at 1 and retail sends 1. Converting sent 2; forcing non-instanceable maps to 0, as the previous commit did, sent 0. The value went 1 (correct) -> 2 -> 0 across two commits without ever being right again. So the spawn mode now goes out unconverted off an instance, and only an instance takes the map-aware conversion. That reproduces every correlated observation: 0 on continents and on every other arena and battleground, 1 on Tol'Viron, and the raw tier on instances. Verified rather than reasoned: PvpDifficulty.dbc decoded directly -- 173 rows, exactly one with a nonzero Difficulty, map 980 -- and Map.dbc gives map 980 InstanceType 4, an arena, which is why the IsDungeon guard caught it. The guard inside ToClientDifficultyForMap stays. It still protects the other callers, all of which name instances, and its comment no longer claims that nothing off an instance can reach it -- that claim was made false by the commit this one repairs. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the remaining findings from the tail review. All three are comments; the code is unchanged. The offset-stability claim gave the wrong reason. It said the optional fields sit AFTER the difficulty, so the offset is the same in all three body shapes. The optionals do not all follow it: sub_6F470B reads the one gated by read-order bit 1 BEFORE the u8 and both u32s, so a body with that bit set would shift the field. The stability is OBSERVED rather than structural -- the leading byte is only 0x00, 0x80 or 0x90 across all 2554 samples, so the bit is clear everywhere we have looked -- and the comment now says that, with a decoded 14-byte and 18-byte body to show the offset rather than assert it. "2554 captures" is 2554 PACKETS, spread over 246 of the 263 build-18414 captures. The fold's own justification was wrong, which matters more than the other two because it is the reasoning a future reader would rely on. It claimed MapManager::CreateDungeonMap folds first, so every caller already holds a supported difficulty. That fold is unconditional and hands internal 0 to End Time (938), Well of Eternity (939) and Hour of Twilight (940), whose single raw-2 rows give them an internal key of 1 and nothing at 0. What actually makes the new branch dead is the REGULAR_DIFFICULTY guard in the fold itself: for those three maps the second lookup misses too and it returns NULL exactly as before. The conclusion was right and the argument for it was not. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex found that the stale-save rescue preserved the bind in a form nothing can reach, because two fixes made earlier today disagree about which tier the owner is on. A bind is stored under the INSTANCE's tier -- Player::BindToInstance indexes m_boundInstances[state->GetDifficulty()] -- but looked up under the PLAYER's selected tier, in GetBoundInstanceSaveForSelfOrGroup. The normalizer rewrites the instance from the stale internal 2 to heroic, while the characters update clamps a stale selection of 2 to NORMAL. So the instance and its bind end at 1 and the owner asks at 0. On a dungeon that has a normal row -- which is most of them -- the lookup then succeeds and does NOT fold, so it searches tier 0 and misses the bind entirely. The character is treated as having no state, is relocated out of the instance at load, and can go on to create a second instance for a lockout they already hold. That is the outcome the normalizer exists to prevent, reached by a different route. The rescue now carries the owner with it: any character or group actually bound to a rewritten save is moved to heroic in the same pass. Deliberately narrow. The blanket clamp to NORMAL stays correct for everyone else, because a character left at HEROIC cannot enter a normal-only dungeon while the difficulty setter is unregistered, and NORMAL is entry-safe. Only an owner of one of these saves needs heroic, and for them it is not a preference -- it is the tier their lockout lives at. The group update joins on leaderGuid rather than groupId: `group_instance` keys on the leader, and ObjectMgrInstanceData's own loader joins `groups`.`leaderGUID` to `group_instance`.`leaderGUID`. The first draft of this used groupId and would have updated the wrong rows or none. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cross-model review found that the previous commit's claim -- that the rescue "carries the owner with it" -- is too broad. The mechanics it identified are real, but the two cases it misses are both correct to miss, so the claim is narrowed rather than the code widened. Player::LoadFromDB clamps anyone under LEVELREQUIREMENT_HEROIC (70) back to NORMAL, and Player::_LoadGroup replaces the loaded value with the group's tier for anyone at or above it. So the characters update persists only for an ungrouped character of level 70 or more. Neither clamp should be worked around. A sub-70 character cannot enter a heroic dungeon at all, so a reachable heroic bind would buy them nothing and NORMAL is the state they should be in. A grouped character follows their group by design -- GetBoundInstanceSaveForSelfOrGroup consults the group bind when the personal one does not answer -- and the group's own bind is rewritten by the adjacent statement anyway. So the honest scope is: this converts a case that was previously broken for every bound owner into one that works for ungrouped level-70+ owners, which is the only cohort for whom a heroic five-man lockout is usable at all. A strict improvement, not a complete one. Recorded in the comment because the next reader would otherwise see the gap and "fix" it by defeating two clamps that are doing their job -- which is how five of today's defects were introduced. Comment only; no behaviour change. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cross-model review raised this as a MINOR and it is broader than it looked, because IsDungeon() is not what its name suggests: it covers MAP_INSTANCE, MAP_RAID and MAP_SCENARIO. The non-instance guard added earlier therefore does not catch scenarios, and all 34 of them took the convert branch. None of them resolves. Only 5 carry a MapDifficulty row at all, those rows carry raw 11 and 12, and ToInternalDifficulty has no internal mode for either, so no scenario map appears in the internal index at any tier. Every one fell through to the fixed five-man table and reported raw 1 -- "party, Normal" -- on a scenario. So the fallback is now conditional on the map having some resolvable tier. The fixed table is an answer for a map that HAS tiers but not the one asked for; it is not an answer for a map with none. 0 rather than the real scenario id, deliberately: this core cannot represent a scenario tier, so it has no internal value to convert and would be inventing one. 0 says "no tier", which is what a map we cannot represent honestly reports. Retail does send 11 and 12 in this field -- both are in the corpus for SMSG_WORLD_SERVER_INFO -- so if scenarios are ever implemented this becomes a real conversion rather than a floor. Latent today: ToInternalDifficulty refusing 11 and 12 is also what keeps scenario maps out of every entry path, so only an admin teleport reaches one. Worth recording how this was missed. Every simulation used to verify the earlier commits treated "is a dungeon" as InstanceType 1 or 2, so none of them modelled scenario maps, and the checks came back clean while this was wrong. The conclusions those runs reached about continents, battlegrounds, arenas and instances are unaffected -- re-verified here -- but the predicate was not the code's. Verified against the shipped DBCs: all 34 scenario maps now report 0 where they reported 1, and every other value is unchanged -- Sunwell 4, Black Temple 4, Molten Core 9, Icecrown 3 and 4, Forge of Souls 1 and 2, End Time 2, continents and Tol'Viron 0. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nestly A second review round showed the previous commit's reasoning wrong in both directions, so the cohort is restated from the code rather than from argument. Wrong in the first direction: it claimed a grouped character follows their group by design, so the group case needed no help. It does not. GetBoundInstanceSaveForSelfOrGroup does fall through to the group bind, but Group::GetBoundInstance keys that lookup on the PLAYER's tier, so a personal heroic bind is invisible the moment _LoadGroup moves the player to a different one. Wrong in the second: it then described the reached cohort as "ungrouped level-70+ owners", which is too narrow. A grouped character of 70+ whose GROUP is bound to the same save is also reached, because the `groups` statement sets that group to heroic and _LoadGroup syncs every member of 70+ from it. Also wrong: the claim that a sub-70 character cannot enter a heroic five-man. They can, taken in by a heroic group -- GetAreaTriggerLockStatus evaluates its heroic level check against the PLAYER's selected tier, which for a sub-70 is always NORMAL, so the check is bypassed and the instance is created from the group's difficulty. What survives is that neither miss is corruption: the clamped value is written back by the next character save, so the row self-corrects to a tier the core will honour rather than holding one nothing loads. The sub-70 gap is left open deliberately and the reason is recorded. Such a character can only have acquired the bind by being taken in by a group, and the way back is the same group, whose own bind this pass repairs. Alone they are clamped to NORMAL, which is the tier a sub-70 should be on. Making a personal bind reachable regardless of selected tier is NOT the fix -- it would also drag a 70+ character with a heroic lockout into their heroic instance when they had deliberately chosen to run the place on normal. Closing it properly means teaching the login clamps that a difficulty backed by a permanent bind is not a mere UI preference, which changes shared login ordering and does not belong here. Comment only; no behaviour change. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit said "every other value is unchanged" and listed the maps it had checked. Review pointed out that is false, and it is -- because the check was a hand-picked list rather than a sweep. Swept exhaustively over every map and every internal tier: 36 maps change, not 34. The extra two are MAP_INSTANCE maps that ship no MapDifficulty row at all -- 627 "unused" and 637 "Abyssal Maw Exterior" -- which previously reported the fixed table's 1, 2 or 8 and now report 0. That is the same correction for the same reason, so the code stands: a map with no tier has no tier to name, and reporting a five-man Normal for one was the bug. What was wrong was the claim about the blast radius, not the blast radius. Everything else is confirmed unchanged by the same sweep: Sunwell, Black Temple, Molten Core, Icecrown, the ordinary five-mans, End Time, the continents and Tol'Viron. Recording the method failure as well, because it is the third time today the same one has cost a correction. Twice a claim was verified against a hand-chosen list of maps and once against a predicate -- InstanceType 1 or 2 -- that was not the one the code uses. Both times the check passed while the claim was wrong. The sweep is cheap; the list is what keeps failing. Comment only; no behaviour change. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 520afc5376
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return true; // applies to every tier of the map | ||
| } | ||
|
|
||
| if (ToInternalDifficulty(encounterDifficultyId) == int32(difficulty)) |
There was a problem hiding this comment.
When a 25-player-only TBC raid is folded to REGULAR_DIFFICULTY, its only MapDifficulty row is still raw DifficultyID 4, but this context-free check translates that encounter tier to internal 1 instead of the canonical internal 0 used to run the map. For encounters on those maps tagged with raw 4, the direct check fails and the fallback chain from raw 4 is empty, so boss kills never set the completed encounter mask; compare the encounter row to GetMapDifficultyData(mapId, difficulty)->DifficultyID before falling back to the generic raw-to-internal mapping.
Useful? React with 👍 / 👎.
…hanism An independent adjudication panel ruled the code correct on all three disputed points and found three claims in the comments that are not. The sub-70 justification was wrong, and it was the sentence that made the gap look benign. It said the way back in is the same group. It is not: Group::GetBoundInstance keys its lookup on the PLAYER's tier, not the group's, so both legs of GetBoundInstanceSaveForSelfOrGroup miss for a sub-70 sitting at NORMAL. No fold rescues it either -- 64 five-man maps carry both raw 1 and raw 2, so the NORMAL lookup succeeds and nothing folds. MapManager then resolves at the player's tier and CREATES at the group's, a duplicate instance, and DungeonMap::Add re-keys on the map's tier and reaches a MANGOS_ASSERT, which in a Release build is a log line -- so the player is silently added to the duplicate. That is a pre-existing core defect this rescue re-exposes for one upgrade cohort, not one it invents. The fix is a one-liner in Group::GetBoundInstance, deferred to its own commit because it wants a live check first: a sub-70 in a 70+ heroic group must land in the SAME instance id as the group. The proposed "proper fix" is withdrawn as a regression. Teaching the login clamps that a bind-backed difficulty outranks the level clamp would raise a sub-70 to HEROIC globally -- dungeon_difficulty is one selector for the whole character -- so the area-trigger level gate would then refuse them every five-man of that expansion, locking them out of normal dungeons they can run today. The cost to the cohort the rescue DOES reach was undocumented and now is: a rescued owner is pinned at HEROIC with no way out alone, because the difficulty setter is unregistered and GetAreaTriggerLockStatus derives isRegularTargetMap from the player's own tier. One group join clears it. And "only an admin teleport reaches a scenario" is false. Nothing reaches one: MapManager::IsValidMAP is false for a dungeon-type map with no instance_template, and the shipped world data has 119 such rows and none for any scenario map, so Player::TeleportTo fails its first check for a GM as readily as anyone else. Verified against mangos4 rather than asserted, and flagged as a world-DB fact rather than a code invariant. Comments only; no behaviour change. ctest: 105/105 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a14cd46030
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| MapDifficultyMap::const_iterator itr = sMapDifficultyMap.find(MAKE_PAIR32(mapId, difficulty)); | ||
| return itr != sMapDifficultyMap.end() ? itr->second : NULL; | ||
| MapDifficultyMap::const_iterator itr = sMapDifficultyLegacyMap.find(MAKE_PAIR32(mapId, difficulty)); |
There was a problem hiding this comment.
When upgrading a database that went through the old LFG path, normal dungeon proposals wrote the raw LfgDungeonsEntry::DifficultyID value 1 into the group/member difficulty and any created five-man instance; that value used to hit the raw normal MapDifficulty row, but after this lookup switches to the internal index it now means heroic. The new LFG code fixes future writes and the normalizer only handles stale raw 2, so existing normal-LFG players or binds are silently promoted to heroic and, with the difficulty setter still unregistered, cannot select normal again. Add a one-time remap for stale non-raid raw-1 rows/settings to internal 0 before relying on the new index.
Useful? React with 👍 / 👎.
MadMaxMangos
added a commit
that referenced
this pull request
Aug 5, 2026
Rebasing this branch onto merged master brought master's difficulty translation
into CreateDungeonGroup, and that block carries its own lookups of `dungeon` and
`groupGuid`. This branch already has both, earlier and deliberately:
* `dungeon` is looked up at the top of the function, BEFORE anything is
created, because the original order returned on an unknown id having already
new'd a Group, run Create (a group id plus an INSERT) and registered it with
ObjectMgr -- leaking the object and stranding its rows.
* `groupGuid` is taken once as `ObjectGuid const`.
Keeping both copies was a redefinition and did not compile. The earlier ones are
the right ones to keep, so the later pair is removed and the reason each exists
is noted where it would otherwise look redundant.
Build clean; ctest 110/110.
MadMaxMangos
added a commit
that referenced
this pull request
Aug 5, 2026
The diagnostics that located the LfgDungeons row-ordinal bug. They print what a
join actually stored and what the proposal actually chose:
LFG JoinLFG: solo entry for Humanwarrior stores dungeons={6}
LFG SendDungeonProposal: entry dungeons={6} -> chose 6 (entry 0x0100000C)
LFG TeleportToDungeon: Humanwarrior DENIED, dungeon 12 map 349, player error 6
Read together those three lines are what made the bug obvious: the queue entry was
correct throughout -- the player asked for dungeon 6 and we chose dungeon 6 -- yet
GetDungeonEntry(6) returned 0x0100000C, which is id 12. That is a lookup returning
the Nth ROW rather than the row with that id.
The fix itself is no longer here. It was a one-character change to
LfgDungeonsEntryfmt ('i' -> 'n', the DBC index marker) and it landed with the
instance-difficulty work in PR #81, so rebasing onto that left only the
instrumentation behind. Kept rather than dropped: the finder has a lot of state
between a join and a teleport, and these three lines are what make it legible.
This commit was previously titled for the fix it carried before the rebase.
MadMaxMangos
added a commit
that referenced
this pull request
Aug 5, 2026
Rebasing this branch onto merged master brought master's difficulty translation
into CreateDungeonGroup, and that block carries its own lookups of `dungeon` and
`groupGuid`. This branch already has both, earlier and deliberately:
* `dungeon` is looked up at the top of the function, BEFORE anything is
created, because the original order returned on an unknown id having already
new'd a Group, run Create (a group id plus an INSERT) and registered it with
ObjectMgr -- leaking the object and stranding its rows.
* `groupGuid` is taken once as `ObjectGuid const`.
Keeping both copies was a redefinition and did not compile. The earlier ones are
the right ones to keep, so the later pair is removed and the reason each exists
is noted where it would otherwise look redundant.
Build clean; ctest 110/110.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Separates the raw 5.4.8 client
DifficultyIDkey space from the core's internal 0-basedDifficulty. The two were compared directly, so every instance lookup missed and nothing in the game was enterable — by any route, walking in or through the dungeon finder.The defect
DBCStores.cpp:740builds the index on the raw client id:DBCStores.cpp:1671queries it with the internal enum:The two spaces agree on no id except 0, and raw 0 exists only on continents, battlegrounds and arenas.
PlayerAreaTrigger.cpp:175treats a NULL row asAREA_LOCKSTATUS_MISSING_DIFFICULTY, soTeleportTorefuses and the client prints "World transfer aborted". A missing arm is silent: nothing is logged.Measured from the shipped DBCs: internal difficulty 0 — the default for every character and every group — resolves on 0 of 121 instance maps. Of 116 instanceable maps carrying MapDifficulty rows, zero have a raw-0 row. 78 (map, tier) pairs resolve at some tier, but none returns the semantically correct row, so nothing is both enterable and correct.
Blocker 1 — RESOLVED: the database migration now exists
mangosfour/Database#3 adds
Character/Updates/Rel23/Rel23_02_001_Instance_Reset_Difficulty_Key_Space.sql, which emptiesinstance_resetand advancesdb_versionto structure 2.Verified against a restored copy of a live characters database: applies clean, 136 rows → 0,
db_versiongains(23,2,1,'instance_reset key space'), and re-running reportsUPDATE SKIPPED— idempotent.Merge order: the database PR must land with or before this one. A server carrying the bump will not start without it; a database carrying it is harmless to an older server.
Blocker 2 — RESOLVED: the founding claim holds on current master
Re-verified statically against master
7375e0ecf, not the original 2026-07-30 measurement, and confirmed live: without this branch a level-appropriate character accepted a Deadmines proposal with everyTeleportToinput valid —validCoord=1, alive, out of combat, not on a taxi, entrance trigger resolving to the correct map and coordinates — andTeleportTostill returned false. With this branch merged, the same character is teleported in.Added since the original review
The
LfgDungeons.dbcindex marker.LfgDungeonsEntryfmtbegan with'i'(plain uint32) rather than'n'(DBC_FF_IND), soAutoProduceDatanumberedindexTablein file order and everyLookupEntryon that store was positional:LookupEntry(6)returned row 6 = id 12 Stormwind Stockade, andLookupEntry(12)returned row 12 = id 26 Maraudon, map 349. It also cappedGetNumRows()at 343 instead of maxId+1, so three loops walking the id space saw less than half of it.This is in this PR rather than a follow-up because the branch's own LFG code looks rows up by id — without it, instance entry still would not work end to end.
Both wire boundaries.
SendDungeonDifficulty/SendRaidDifficultyput the internal value on the wire, so a character on normal difficulty reported 0. Three oracles agree the wire carries the raw id: 954 build-18414 captures ofSMSG_SET_DUNGEON_DIFFICULTYcarry only 1 (×512) and 2 (×424) — never 0;FrameXML/Constants.luadeclaresDIFFICULTY_DUNGEON_NORMAL = 1,_HEROIC = 2,_CHALLENGE = 8; and an observedSMSG_SET_RAID_DIFFICULTYbody carries 9, a legacy 40-player raid, which cannot be an internal mode at all.The inbound handlers had the mirror fault — Normal (raw 1) would have stored HEROIC — but are latent, since neither is registered. They now translate and are deliberately left dormant: there is no
CMSG_SET_DUNGEON_DIFFICULTYdeclared for 18414, andMSG_SET_RAID_DIFFICULTY 0x0614is a 4.3.4 carry-over tagged "no client leaf".Two defects corrected in the branch's own work
Group_ID 0matches 273 rows, 20 at scenario tier 12, and that all 10 admissible random rows haveGroup_ID 0. Re-measured: 79 rows, zero at tier 12, and no admissible random row hasGroup_ID 0— the ten carry 1, 2, 3, 4, 5, 12, 13, 33, 36, 37. They had been measured through the row-ordinalLookupEntryabove. The filter is harmless and kept as a guard, but removes nothing for any shipped random and is no longer cited as load-bearing.CHAR_DB_UPDATE_DESCRIPTwas 43 characters against avarchar(30)column, so it could never equal any row the update writes — the banner advertised an impossible value. Now identical to the SQL.The branch's other measurements were re-checked and all hold exactly: 343 rows, 77 carrying an untranslatable tier, 4 of those raids, 61 TypeID 2 rows. Those were computed by iterating rows, which never needed a by-id lookup — which is why only the
Group_IDclaim was corrupted.Verification
gamebuilds Release, 100/100 tests pass.Known limitation
BuildMapSpawnModeMasks(added to master after this branch forked) performs its own raw→internal translation. Verified arm-for-arm identical toToInternalDifficulty, and it readsmapDiff->DifficultyID— the raw field on the entry, not the map key — so re-keying the index cannot double-translate it.