refactor(Core/Player): compute playable class count and mask dynamically via ClassMgr - #27104
refactor(Core/Player): compute playable class count and mask dynamically via ClassMgr#27104JSlomian wants to merge 2 commits into
Conversation
…lly via ClassMgr Add ClassMgr, mirroring the existing RaceMgr, which scans sChrClassesStore at boot to compute the max class id and playable class bitmask instead of relying on the hardcoded MAX_CLASSES/CLASSMASK_ALL_PLAYABLE literals. Migrate downstream consumers (ObjectMgr player-create/level-info loading, item AllowableClass validation, quest RequiredClasses validation, trainer class requirements, guild member data integrity checks, achievement criteria validation, class conditions, and mail template conditions) to read from ClassMgr instead of the static macros, so class validity now tracks what is actually loaded from the DBC rather than a value a human has to keep in sync by hand. MAX_CLASSES/CLASSMASK_ALL_PLAYABLE remain as compile-time sizing constants for fixed per-class data tables (stat caps, triggered spell tables) that still require literal per-class values.
📝 WalkthroughWalkthroughAdded Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82dacecc-b631-4953-948e-3a81dde4f181
📒 Files selected for processing (9)
src/server/game/Achievements/AchievementMgr.cppsrc/server/game/Conditions/ConditionMgr.cppsrc/server/game/Entities/Player/ClassMgr.cppsrc/server/game/Entities/Player/ClassMgr.hsrc/server/game/Globals/ObjectMgr.cppsrc/server/game/Globals/ObjectMgr.hsrc/server/game/Guilds/Guild.cppsrc/server/game/Mails/ServerMailMgr.cppsrc/server/game/World/World.cpp
| uint8 classId = classEntry->ClassID; | ||
|
|
||
| if (GetMaxClasses() <= classId) | ||
| SetMaxClasses(classId + 1); | ||
|
|
||
| uint32 classBit = (1 << (classId - 1)); | ||
|
|
||
| _playableClassMask |= classBit; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate ClassID before deriving its mask.
ClassID == 0 underflows the shift count, ClassID >= 33 has an invalid shift count, and ClassID == 32 shifts a signed 1 into its sign bit. Reject unsupported IDs and use uint32(1) for the shift. The runtime registry can otherwise publish an invalid mask and class limit.
Proposed fix
- uint8 classId = classEntry->ClassID;
+ uint32 classId = classEntry->ClassID;
+
+ if (classId == 0 || classId > std::numeric_limits<uint32>::digits)
+ {
+ LOG_ERROR("server.loading", "Invalid class ID {} in ChrClasses.dbc.", classId);
+ continue;
+ }
if (GetMaxClasses() <= classId)
- SetMaxClasses(classId + 1);
+ SetMaxClasses(static_cast<uint8>(classId + 1));
- uint32 classBit = (1 << (classId - 1));
+ uint32 classBit = uint32(1) << (classId - 1);#!/bin/bash
set -euo pipefail
dbc_file="$(fd -a -i '^ChrClasses\.dbc$' . | head -n 1)"
test -n "${dbc_file}" || { echo "ChrClasses.dbc not found"; exit 1; }
python - "${dbc_file}" <<'PY'
import struct
import sys
with open(sys.argv[1], "rb") as dbc:
magic, rows, fields, row_size, strings = struct.unpack("<4s4I", dbc.read(20))
assert magic == b"WDBC", f"Unexpected DBC header: {magic!r}"
class_ids = [struct.unpack("<I", dbc.read(row_size)[:4])[0] for _ in range(rows)]
print(f"class IDs: {class_ids}")
assert all(1 <= class_id <= 32 for class_id in class_ids), "Class ID is outside the uint32 mask range"
PY…le mask ChrClasses.dbc rows with a class ID of 0 or greater than 32 would either underflow the shift count or shift past the width of the bitmask, both undefined behavior. Reject out-of-range IDs and use an unsigned shift. Also widen the local class ID variable to uint32 to match the DBC field's type, avoiding a silent truncation for out-of-range values.
Changes Proposed:
This PR proposes changes to:
ClassMgris added, mirroring the existingRaceMgr: it scanssChrClassesStoreat boot to compute the max class id and the playable-class bitmask, instead of relying on the hardcodedMAX_CLASSES/CLASSMASK_ALL_PLAYABLEliterals. Every downstream consumer of those two literals for validity/mask checks (ObjectMgrplayer-create/level-info loading, itemAllowableClassvalidation, questRequiredClassesvalidation, trainer class requirements, guild member data integrity checks, achievement criteria validation, class conditions, mail template conditions) now reads fromClassMgrinstead.MAX_CLASSES/CLASSMASK_ALL_PLAYABLEremain as compile-time sizing constants for the handful of fixed per-class data tables (stat caps, triggered spell tables) that still require literal per-class values — those are unrelated to class validity and out of scope here.This is a pure refactor: behavior for all 9 existing classes is unchanged (verified — see Tests Performed).
AI-assisted Pull Requests
Important
Using AI tools to prepare pull requests is allowed, but it must be disclosed and it must follow our AC guidelines for AI Agentic Engineering (link below).
You are expected to fully understand the changes you submit and to be able to explain and justify them when maintainers ask.
Issues Addressed:
SOURCE:
The changes have been validated through:
N/A — this is a structural engine refactor (generalizing a hardcoded literal into a data-driven manager), not a gameplay/behavior fix, so it isn't backed by an external behavioral source. It's structurally cloned from this codebase's own existing
RaceMgr(src/server/game/Entities/Player/RaceMgr.h/.cpp), which already solves the identical problem for races.Tests Performed:
This PR has been:
Verified so far:
codestyle-cpp.pypasses.docker compose up -d --build) compiles clean with zero errors/warnings.readywith the newClassMgr::LoadClasses()call in the boot sequence;LoadPlayerInforeports the same counts as before the change (63 player create definitions, 1115 skills, 190 level stats definitions); zero occurrences of any of the migrated validation warnings (does not have any playable classes,invalid class requirement,non existing classmask,Wrong class ... ignoring) — confirming all 9 existing classes still validate exactly as before.Not yet done: an in-game click-through by a human (see reproduction steps below).
How to Test the Changes:
LoadPlayerInfopath this PR touches (race/class create-info, starting items, skills, spells, action bars, level stats).Confirm all of the above behaves identically to
master— this PR should be behaviorally invisible for the 9 existing classes.Known Issues and TODO List:
sTalentTabPagesinDBCStores.cppstill sizes/bounds off theMAX_CLASSESliteral rather thanClassMgr, becauseLoadDBCStores()runs beforeClassMgr::LoadClasses()in the boot sequence. Left as a follow-up since it requires its own fix (computing the max class id locally duringLoadDBCStores()), independent of this PR's scope.