refactor(Core/DB): normalize creature table by extracting multi-ID spawns - #25197
Conversation
…awns Move the rarely-used id2/id3 columns from the creature table into a new creature_multispawn table, and rename id1 back to id. Only ~737 out of 5.3M+ spawns use multiple entries, so this eliminates two wasted columns on 99.99% of rows while preserving full functionality. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Normalizes creature spawns by renaming creature.id1 back to id, moving multi-entry spawn variants into a new creature_multispawn table, and updating server code to load/consume the new schema while keeping existing random-entry behavior.
Changes:
- Renamed
CreatureData::id1→idand updated references across scripts/commands/core. - Updated creature loading to read base entry from
creature.idand load variant entries fromcreature_multispawnintoid2/id3. - Added SQL migration creating
creature_multispawn, migratingid2/id3data, and dropping old columns fromcreature.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/scripts/Northrend/IcecrownCitadel/instance_icecrown_citadel.cpp | Switch spawn entry lookup from id1 to id. |
| src/server/scripts/Northrend/IcecrownCitadel/icecrown_citadel.cpp | Update script logic to use CreatureData::id. |
| src/server/scripts/Northrend/FrozenHalls/PitOfSaron/instance_pit_of_saron.cpp | Switch spawn entry lookup from id1 to id. |
| src/server/scripts/Kalimdor/TempleOfAhnQiraj/temple_of_ahnqiraj.cpp | Use CreatureData::id when building HighGuid. |
| src/server/scripts/Commands/cs_wp.cpp | Update prepared statement argument count for new query. |
| src/server/scripts/Commands/cs_tele.cpp | Use id in in-memory filtering and SQL query joins. |
| src/server/scripts/Commands/cs_npc.cpp | Spawn creation uses data.id; adjust DB query field indices after schema change. |
| src/server/scripts/Commands/cs_go.cpp | Use CreatureData::id for entry matching. |
| src/server/game/OutdoorPvP/OutdoorPvP.cpp | Use CreatureData::id when resolving capture point spawns. |
| src/server/game/Maps/ZoneScript.h | Default entry resolver now returns data->id. |
| src/server/game/Handlers/AuctionHouseHandler.cpp | Auctioneer template lookup uses CreatureData::id. |
| src/server/game/Globals/ObjectMgr.cpp | Load creatures from new schema; add loading of variants from creature_multispawn. |
| src/server/game/Events/GameEventMgr.cpp | Update event vendor/model equip loading for new creature schema. |
| src/server/game/Entities/Creature/CreatureData.h | Rename id1 to id; annotate id2/id3 as multispawn variants. |
| src/server/game/Entities/Creature/Creature.cpp | Save/load/respawn logic updated to use data->id as primary template ID. |
| src/server/game/Conditions/ConditionMgr.cpp | Condition validation now compares against CreatureData::id. |
| src/server/game/AI/SmartScripts/SmartScriptMgr.cpp | SAI validation uses CreatureData::id. |
| src/server/database/Database/Implementation/WorldDatabase.cpp | Update prepared statements to match new creature schema + multispawn lookup. |
| data/sql/updates/pending_db_world/rev_8337814054007982.sql | Introduce creature_multispawn and migrate/drop old repeating columns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| INSERT INTO `creature_multispawn` (`spawnId`, `entry`) | ||
| SELECT `guid`, `id2` FROM `creature` WHERE `id2` != 0; | ||
|
|
||
| -- Migrate id3 entries | ||
| INSERT INTO `creature_multispawn` (`spawnId`, `entry`) |
There was a problem hiding this comment.
These inserts can fail and abort the migration if duplicates exist (e.g., id2 == id3 for a spawn, or repeated values across runs), because (spawnId, entry) is the primary key. Use INSERT IGNORE / ON DUPLICATE KEY handling, or filter duplicates in the SELECT (e.g., exclude id3 rows where id3 = id2) to make the migration idempotent and robust.
| INSERT INTO `creature_multispawn` (`spawnId`, `entry`) | |
| SELECT `guid`, `id2` FROM `creature` WHERE `id2` != 0; | |
| -- Migrate id3 entries | |
| INSERT INTO `creature_multispawn` (`spawnId`, `entry`) | |
| INSERT IGNORE INTO `creature_multispawn` (`spawnId`, `entry`) | |
| SELECT `guid`, `id2` FROM `creature` WHERE `id2` != 0; | |
| -- Migrate id3 entries | |
| INSERT IGNORE INTO `creature_multispawn` (`spawnId`, `entry`) |
There was a problem hiding this comment.
Fixed — using INSERT IGNORE now to handle potential id2 == id3 duplicates safely.
| // Load alternate entries from creature_multispawn | ||
| QueryResult variantResult = WorldDatabase.Query("SELECT entry FROM creature_multispawn WHERE spawnId = {}", spawnId); | ||
| if (variantResult) | ||
| { | ||
| do | ||
| { | ||
| uint32 variantEntry = variantResult->Fetch()[0].Get<uint32>(); | ||
| if (!GetCreatureTemplate(variantEntry)) | ||
| { | ||
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has creature (SpawnId: {}) with non-existing entry {}, skipped.", spawnId, variantEntry); | ||
| continue; | ||
| } | ||
| if (!creatureData.id2) | ||
| creatureData.id2 = variantEntry; | ||
| else if (!creatureData.id3) | ||
| creatureData.id3 = variantEntry; | ||
| } while (variantResult->NextRow()); | ||
| } |
There was a problem hiding this comment.
The lazy-load path (LoadCreatureDataFromDB) diverges from the main bulk load (LoadCreatures) validation/behavior: it does not check difficulty-entry constraints, does not validate equipment compatibility, and does not detect/log >2 variants (it silently ignores extras after filling id2/id3). This can lead to inconsistent runtime behavior depending on whether a spawn was loaded via bulk load or on-demand. Consider reusing the same validation logic as LoadCreatures (including logging when >2 variants exist) and making the variant row order deterministic (e.g., ORDER BY entry) so id2/id3 assignment is stable.
There was a problem hiding this comment.
Fixed — LoadCreatureDataFromDB now has the same validation as the bulk loader: difficulty entry checks, equipment validation, overflow logging, and ORDER BY entry for deterministic assignment.
| CreatureData* data = const_cast<CreatureData*>(GetCreatureData(spawnId)); | ||
| if (!data) | ||
| { | ||
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has entry for non-existing creature spawn (SpawnId: {}), skipped.", spawnId); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid const_cast here. Since variants are being applied during the load phase, prefer retrieving a mutable reference/pointer directly from _creatureDataStore (e.g., via find(spawnId) and modifying the stored CreatureData). This removes const-correctness hazards and makes it clearer that the data is intentionally mutable in this context.
| CreatureData* data = const_cast<CreatureData*>(GetCreatureData(spawnId)); | |
| if (!data) | |
| { | |
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has entry for non-existing creature spawn (SpawnId: {}), skipped.", spawnId); | |
| continue; | |
| } | |
| auto creatureDataIt = _creatureDataStore.find(spawnId); | |
| if (creatureDataIt == _creatureDataStore.end()) | |
| { | |
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has entry for non-existing creature spawn (SpawnId: {}), skipped.", spawnId); | |
| continue; | |
| } | |
| CreatureData* data = &creatureDataIt->second; |
There was a problem hiding this comment.
Fixed — using _creatureDataStore.find() directly now, no more const_cast.
| PrepareStatement(WORLD_SEL_WAYPOINT_SCRIPT_BY_ID, "SELECT guid, delay, command, datalong, datalong2, dataint, x, y, z, o FROM waypoint_scripts WHERE id = ?", CONNECTION_SYNCH); | ||
| PrepareStatement(WORLD_SEL_ITEM_TEMPLATE_BY_NAME, "SELECT entry FROM item_template WHERE name = ?", CONNECTION_SYNCH); | ||
| PrepareStatement(WORLD_SEL_CREATURE_BY_ID, "SELECT guid FROM creature WHERE id1 = ? OR id2 = ? OR id3 = ?", CONNECTION_SYNCH); | ||
| PrepareStatement(WORLD_SEL_CREATURE_BY_ID, "SELECT guid FROM creature WHERE id = ? UNION SELECT spawnId FROM creature_multispawn WHERE entry = ?", CONNECTION_SYNCH); |
There was a problem hiding this comment.
In the UNION, the second SELECT returns spawnId without aliasing it to match the first column name. While most consumers fetch by index, aliasing improves clarity and avoids subtle issues in any code path that relies on column names. Consider: ... UNION SELECT spawnId AS guid ....
There was a problem hiding this comment.
Fixed — aliased as spawnId AS guid.
| if (!data->id2) | ||
| data->id2 = entry; | ||
| else if (!data->id3) | ||
| data->id3 = entry; | ||
| else | ||
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has more than 2 variant entries for creature (SpawnId: {}), extra entry {} skipped.", spawnId, entry); | ||
|
|
||
| ++variantCount; |
There was a problem hiding this comment.
variantCount is incremented even when the variant entry is not actually stored (e.g., when a spawn already has both id2 and id3 and the entry is skipped). Increment only when an entry is successfully assigned to id2/id3 to keep the load log accurate.
| if (!data->id2) | |
| data->id2 = entry; | |
| else if (!data->id3) | |
| data->id3 = entry; | |
| else | |
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has more than 2 variant entries for creature (SpawnId: {}), extra entry {} skipped.", spawnId, entry); | |
| ++variantCount; | |
| bool stored = false; | |
| if (!data->id2) | |
| { | |
| data->id2 = entry; | |
| stored = true; | |
| } | |
| else if (!data->id3) | |
| { | |
| data->id3 = entry; | |
| stored = true; | |
| } | |
| else | |
| { | |
| LOG_ERROR("sql.sql", "Table `creature_multispawn` has more than 2 variant entries for creature (SpawnId: {}), extra entry {} skipped.", spawnId, entry); | |
| } | |
| if (stored) | |
| ++variantCount; |
There was a problem hiding this comment.
Fixed — variantCount now only increments when an entry is actually stored in id2/id3.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use INSERT IGNORE for migration to handle id2==id3 duplicates - Remove const_cast, use _creatureDataStore.find() directly - Only increment variantCount when entry is actually stored - Add validation parity to LoadCreatureDataFromDB (difficulty, equipment, overflow) - Add ORDER BY entry for deterministic id2/id3 assignment - Alias spawnId AS guid in UNION query Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
DB
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Will Claude provide fixes for the 100 broken modules 🫣 |
|
The change is a simple rename ( |
|
Only one module in the CI suite is affected — |
|
Alright, currently module ci is failing on build already, so it's not showing the failing sql files yet |
|
Module SQL fix PRs for the
|
|
Module fix PRs for removed
|
Resolve conflicts from the creature id1->id rename and creature_multispawn extraction against current master: - Creature::Respawn: keep master's compat/non-compat split; multi-ID random re-selection stays in compat mode (id1->id), non-compat recreates via ProcessRespawns/LoadCreatureFromDB which re-rolls the entry. - ObjectMgr::LoadCreatures: keep new-schema field indices, add master's data.spawnId assignment. - Update id1 references added to master after the PR was opened (cs_pool, cs_misc, cs_list, ConditionMgr) to the renamed field. - Fix .list creature SQL to query creature.id plus creature_multispawn instead of the dropped id1/id2/id3 columns.
📝 WalkthroughWalkthroughIntroduces a new Changescreature_multispawn schema migration and id1→id rename
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/game/Events/GameEventMgr.cpp (1)
615-643:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCollapse multispawn variants before storing model/equipment changes.
The updated query can return multiple rows for the same
(eventId, guid)throughcreature_multispawn, but_gameEventModelEquipstores only guid-level changes. Pushing one row per valid variant makesChangeEquipOrModel()apply the same guid multiple times, which can overwriteEquipementIdPrev/ModelIdPrevand restore the wrong equipment/model when the event ends.Proposed localized guard
+ std::unordered_set<uint64> loadedModelEquipKeys; uint32 count = 0; do { Field* fields = result->Fetch(); ObjectGuid::LowType guid = fields[0].Get<uint32>(); uint32 entry = fields[1].Get<uint32>(); uint16 eventId = fields[2].Get<uint8>(); @@ } } + uint64 const key = (uint64(eventId) << 32) | guid; + if (!loadedModelEquipKeys.insert(key).second) + continue; + equiplist.push_back(std::pair<ObjectGuid::LowType, ModelEquip>(guid, newModelEquipSet));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/game/Events/GameEventMgr.cpp` around lines 615 - 643, The query can now return multiple rows for the same (eventId, guid) pair due to creature_multispawn joins, but the current code pushes all variants into the equiplist within _gameEventModelEquip, causing ChangeEquipOrModel() to apply the same guid multiple times and overwrite EquipementIdPrev/ModelIdPrev. Deduplicate the results by tracking which (eventId, guid) pairs have already been processed in the current load iteration and skip pushing subsequent variants, ensuring only the first valid variant per guid is stored in the equiplist.
🧹 Nitpick comments (1)
data/sql/updates/pending_db_world/rev_8337814054007982.sql (1)
2-6: ⚡ Quick winAdd a secondary index for
entrylookups.The primary key (
spawnId,entry) does not efficiently supportWHERE entry = ?lookups. Since that predicate is now used in prepared statements, a dedicatedentryindex avoids unnecessary scans.🔧 Suggested fix
CREATE TABLE IF NOT EXISTS `creature_multispawn` ( `spawnId` int unsigned NOT NULL COMMENT 'creature.guid', `entry` int unsigned NOT NULL COMMENT 'creature_template.entry', - PRIMARY KEY (`spawnId`, `entry`) + PRIMARY KEY (`spawnId`, `entry`), + KEY `idx_entry` (`entry`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='Additional creature entries for multi-ID spawning';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/sql/updates/pending_db_world/rev_8337814054007982.sql` around lines 2 - 6, The primary key on (spawnId, entry) does not efficiently support lookups filtering by entry alone. Add a secondary index on the entry column to the creature_multispawn table to optimize WHERE entry = ? queries. This should be added after the PRIMARY KEY definition to provide efficient index coverage for entry-based lookups in prepared statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/sql/updates/pending_db_world/rev_8337814054007982.sql`:
- Around line 2-6: The `creature_multispawn` table definition lacks a foreign
key constraint to enforce referential integrity with the parent `creature`
table. Add a FOREIGN KEY constraint on the `spawnId` column that references
`creature(guid)` with `ON DELETE CASCADE` to automatically remove orphaned rows
from `creature_multispawn` when a creature is deleted from the `creature` table.
This constraint should be added as part of the CREATE TABLE statement for
`creature_multispawn` to ensure data consistency and prevent stale GUID lookups.
In `@src/server/game/Entities/Creature/Creature.cpp`:
- Line 2042: The ObjectGuid::Create line for the dbtableHighGuid variable in the
Creature.cpp file exceeds the project's 80-column limit. Split this line by
extracting the ternary operator expression (m_creatureData ? m_creatureData->id
: GetEntry()) to a separate variable or breaking the ObjectGuid::Create call
across multiple lines to ensure the total line length does not exceed 80 columns
as required by the coding guidelines.
In `@src/server/game/Globals/ObjectMgr.cpp`:
- Line 2369: In src/server/game/Globals/ObjectMgr.cpp at line 2369 and line
2599, after assigning the primary creatureId to data.id, also reset the
alternate entry fields data.id2 and data.id3 to clear any stale values. This is
necessary because these alternate fields are no longer selected from the
creature table, so reused CreatureData entries can retain stale alternates when
multispawn rows change. Add the reset of id2 and id3 alongside the data.id
assignment at both locations to ensure consistency across both load paths.
- Line 2501: The startup variant loading query in ObjectMgr.cpp at the
WorldDatabase.Query call for creature_multispawn needs to match the
deterministic ordering used by the lazy path. Currently, the query only orders
by spawnId, but it should order by both spawnId and entry to ensure consistent
variant assignment and prevent different "extra" variants from being selected
when handling duplicate or malformed data. Add entry to the ORDER BY clause in
the same query that selects spawnId and entry from creature_multispawn.
- Around line 2111-2112: The code calls GetCreatureData(guidLow) which can
return nullptr, but then immediately dereferences the master pointer without
validation on the next line when building the GUID via
ObjectGuid::Create<HighGuid::Unit>(master->id, guidLow). Add a null check for
the master pointer immediately after the GetCreatureData call and before the
ObjectGuid::Create dereference to handle the case where GetCreatureData returns
nullptr, preventing a potential server crash from invalid input.
In `@src/server/scripts/Commands/cs_list.cpp`:
- Around line 80-92: The three WorldDatabase.Query calls at lines 80–92 are
using raw SQL strings with inline parameters instead of prepared statements.
Convert each of these queries to use PreparedStatement objects from
WorldDatabase, binding the parameters (creatureId, count, player position
values) via the SetData method. This applies to the COUNT query, the
distance-ordered query with player position calculations (using POW and
player->GetPositionX/Y/Z), and the simpler query without position ordering.
In `@src/server/scripts/Commands/cs_npc.cpp`:
- Around line 733-735: The PSendSysMessage call in the NPC info command is
missing the id1 argument for the LANG_NPCINFO_CHAR format string, causing all
subsequent field values to shift and produce incorrect output. Add the missing
cData->id1 argument to the handler->PSendSysMessage call at the correct position
so that id1, id2, id3 are passed in the proper order after the initial
ObjectGuid argument, ensuring the format string placeholders align correctly
with their corresponding values.
In `@src/server/scripts/Commands/cs_tele.cpp`:
- Line 355: Replace the raw SQL query in the WorldDatabase.Query() call with a
prepared statement approach. Define a new prepared statement enum
WORLD_SEL_CREATURE_POS_BY_TEMPLATE_NAME in the WorldDatabase implementation to
encapsulate the creature position query (selecting position_x, position_y,
position_z, orientation, map, and creature template name), then use
PreparedStatement with the WorldDatabase instead of passing the raw query string
with inline parameter substitution. This ensures the query follows the server
database contract and protects against SQL injection through proper
parameterized query handling.
---
Outside diff comments:
In `@src/server/game/Events/GameEventMgr.cpp`:
- Around line 615-643: The query can now return multiple rows for the same
(eventId, guid) pair due to creature_multispawn joins, but the current code
pushes all variants into the equiplist within _gameEventModelEquip, causing
ChangeEquipOrModel() to apply the same guid multiple times and overwrite
EquipementIdPrev/ModelIdPrev. Deduplicate the results by tracking which
(eventId, guid) pairs have already been processed in the current load iteration
and skip pushing subsequent variants, ensuring only the first valid variant per
guid is stored in the equiplist.
---
Nitpick comments:
In `@data/sql/updates/pending_db_world/rev_8337814054007982.sql`:
- Around line 2-6: The primary key on (spawnId, entry) does not efficiently
support lookups filtering by entry alone. Add a secondary index on the entry
column to the creature_multispawn table to optimize WHERE entry = ? queries.
This should be added after the PRIMARY KEY definition to provide efficient index
coverage for entry-based lookups in prepared statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e6d76b7-73c4-4a83-addd-a20506b7fe95
📒 Files selected for processing (22)
data/sql/updates/pending_db_world/rev_8337814054007982.sqlsrc/server/database/Database/Implementation/WorldDatabase.cppsrc/server/game/AI/SmartScripts/SmartScriptMgr.cppsrc/server/game/Conditions/ConditionMgr.cppsrc/server/game/Entities/Creature/Creature.cppsrc/server/game/Entities/Creature/CreatureData.hsrc/server/game/Events/GameEventMgr.cppsrc/server/game/Globals/ObjectMgr.cppsrc/server/game/Handlers/AuctionHouseHandler.cppsrc/server/game/Maps/ZoneScript.hsrc/server/game/OutdoorPvP/OutdoorPvP.cppsrc/server/scripts/Commands/cs_go.cppsrc/server/scripts/Commands/cs_list.cppsrc/server/scripts/Commands/cs_misc.cppsrc/server/scripts/Commands/cs_npc.cppsrc/server/scripts/Commands/cs_pool.cppsrc/server/scripts/Commands/cs_tele.cppsrc/server/scripts/Commands/cs_wp.cppsrc/server/scripts/Kalimdor/TempleOfAhnQiraj/temple_of_ahnqiraj.cppsrc/server/scripts/Northrend/FrozenHalls/PitOfSaron/instance_pit_of_saron.cppsrc/server/scripts/Northrend/IcecrownCitadel/icecrown_citadel.cppsrc/server/scripts/Northrend/IcecrownCitadel/instance_icecrown_citadel.cpp
| CREATE TABLE IF NOT EXISTS `creature_multispawn` ( | ||
| `spawnId` int unsigned NOT NULL COMMENT 'creature.guid', | ||
| `entry` int unsigned NOT NULL COMMENT 'creature_template.entry', | ||
| PRIMARY KEY (`spawnId`, `entry`) | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='Additional creature entries for multi-ID spawning'; |
There was a problem hiding this comment.
Enforce parent-child cleanup for creature_multispawn.
Line 2 introduces a child table keyed by spawnId, but there is no referential cleanup to keep it in sync with creature deletions. That can leave orphan rows and make entry-based lookups return stale GUIDs.
🔧 Suggested fix
CREATE TABLE IF NOT EXISTS `creature_multispawn` (
- `spawnId` int unsigned NOT NULL COMMENT 'creature.guid',
- `entry` int unsigned NOT NULL COMMENT 'creature_template.entry',
- PRIMARY KEY (`spawnId`, `entry`)
+ `spawnId` int unsigned NOT NULL COMMENT 'creature.guid',
+ `entry` int unsigned NOT NULL COMMENT 'creature_template.entry',
+ PRIMARY KEY (`spawnId`, `entry`),
+ CONSTRAINT `fk_creature_multispawn_spawn`
+ FOREIGN KEY (`spawnId`) REFERENCES `creature` (`guid`)
+ ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='Additional creature entries for multi-ID spawning';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE IF NOT EXISTS `creature_multispawn` ( | |
| `spawnId` int unsigned NOT NULL COMMENT 'creature.guid', | |
| `entry` int unsigned NOT NULL COMMENT 'creature_template.entry', | |
| PRIMARY KEY (`spawnId`, `entry`) | |
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='Additional creature entries for multi-ID spawning'; | |
| CREATE TABLE IF NOT EXISTS `creature_multispawn` ( | |
| `spawnId` int unsigned NOT NULL COMMENT 'creature.guid', | |
| `entry` int unsigned NOT NULL COMMENT 'creature_template.entry', | |
| PRIMARY KEY (`spawnId`, `entry`), | |
| CONSTRAINT `fk_creature_multispawn_spawn` | |
| FOREIGN KEY (`spawnId`) REFERENCES `creature` (`guid`) | |
| ON DELETE CASCADE | |
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='Additional creature entries for multi-ID spawning'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/sql/updates/pending_db_world/rev_8337814054007982.sql` around lines 2 -
6, The `creature_multispawn` table definition lacks a foreign key constraint to
enforce referential integrity with the parent `creature` table. Add a FOREIGN
KEY constraint on the `spawnId` column that references `creature(guid)` with `ON
DELETE CASCADE` to automatically remove orphaned rows from `creature_multispawn`
when a creature is deleted from the `creature` table. This constraint should be
added as part of the CREATE TABLE statement for `creature_multispawn` to ensure
data consistency and prevent stale GUID lookups.
| return; | ||
|
|
||
| ObjectGuid dbtableHighGuid = ObjectGuid::Create<HighGuid::Unit>(m_creatureData ? m_creatureData->id1 : GetEntry(), m_spawnId); | ||
| ObjectGuid dbtableHighGuid = ObjectGuid::Create<HighGuid::Unit>(m_creatureData ? m_creatureData->id : GetEntry(), m_spawnId); |
There was a problem hiding this comment.
Wrap the linked-respawn GUID construction.
Line 2042 exceeds the project’s 80-column limit; split the ternary input before merge. As per coding guidelines, **/*.{cpp,h,hpp,cc,sql,json,yaml,yml,sh,ts,js} files must use “max 80 columns.”
Proposed formatting fix
- ObjectGuid dbtableHighGuid = ObjectGuid::Create<HighGuid::Unit>(m_creatureData ? m_creatureData->id : GetEntry(), m_spawnId);
+ uint32 const dbEntry = m_creatureData ? m_creatureData->id : GetEntry();
+ ObjectGuid dbtableHighGuid =
+ ObjectGuid::Create<HighGuid::Unit>(dbEntry, m_spawnId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ObjectGuid dbtableHighGuid = ObjectGuid::Create<HighGuid::Unit>(m_creatureData ? m_creatureData->id : GetEntry(), m_spawnId); | |
| uint32 const dbEntry = m_creatureData ? m_creatureData->id : GetEntry(); | |
| ObjectGuid dbtableHighGuid = | |
| ObjectGuid::Create<HighGuid::Unit>(dbEntry, m_spawnId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/game/Entities/Creature/Creature.cpp` at line 2042, The
ObjectGuid::Create line for the dbtableHighGuid variable in the Creature.cpp
file exceeds the project's 80-column limit. Split this line by extracting the
ternary operator expression (m_creatureData ? m_creatureData->id : GetEntry())
to a separate variable or breaking the ObjectGuid::Create call across multiple
lines to ensure the total line length does not exceed 80 columns as required by
the coding guidelines.
Source: Coding guidelines
| CreatureData const* master = GetCreatureData(guidLow); | ||
| ObjectGuid guid = ObjectGuid::Create<HighGuid::Unit>(master->id1, guidLow); | ||
| ObjectGuid guid = ObjectGuid::Create<HighGuid::Unit>(master->id, guidLow); |
There was a problem hiding this comment.
Validate master before building the GUID.
GetCreatureData(guidLow) can return nullptr, but Line 2112 dereferences master before any check. Invalid nonzero input can crash the server.
Proposed fix
CreatureData const* master = GetCreatureData(guidLow);
+ if (!master)
+ {
+ LOG_ERROR("sql.sql", "Creature '{}' does not exist.", guidLow);
+ return false;
+ }
+
ObjectGuid guid = ObjectGuid::Create<HighGuid::Unit>(master->id, guidLow);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CreatureData const* master = GetCreatureData(guidLow); | |
| ObjectGuid guid = ObjectGuid::Create<HighGuid::Unit>(master->id1, guidLow); | |
| ObjectGuid guid = ObjectGuid::Create<HighGuid::Unit>(master->id, guidLow); | |
| CreatureData const* master = GetCreatureData(guidLow); | |
| if (!master) | |
| { | |
| LOG_ERROR("sql.sql", "Creature '{}' does not exist.", guidLow); | |
| return false; | |
| } | |
| ObjectGuid guid = ObjectGuid::Create<HighGuid::Unit>(master->id, guidLow); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/game/Globals/ObjectMgr.cpp` around lines 2111 - 2112, The code
calls GetCreatureData(guidLow) which can return nullptr, but then immediately
dereferences the master pointer without validation on the next line when
building the GUID via ObjectGuid::Create<HighGuid::Unit>(master->id, guidLow).
Add a null check for the master pointer immediately after the GetCreatureData
call and before the ObjectGuid::Create dereference to handle the case where
GetCreatureData returns nullptr, preventing a potential server crash from
invalid input.
| data.unit_flags = fields[21].Get<uint32>(); | ||
| data.dynamicflags = fields[22].Get<uint32>(); | ||
| data.ScriptId = GetScriptId(fields[23].Get<std::string>()); | ||
| data.id = creatureId; |
There was a problem hiding this comment.
Reset alternate entries before loading creature_multispawn.
id2 and id3 are no longer selected from creature, so reused CreatureData entries can retain stale alternates when the multispawn rows change. Clear them with the primary id assignment in both load paths.
Proposed fix
data.id = creatureId;
+ data.id2 = 0;
+ data.id3 = 0;
data.mapid = fields[2].Get<uint16>(); creatureData.id = creatureId;
+ creatureData.id2 = 0;
+ creatureData.id3 = 0;
creatureData.mapid = fields[2].Get<uint16>();Also applies to: 2599-2599
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/game/Globals/ObjectMgr.cpp` at line 2369, In
src/server/game/Globals/ObjectMgr.cpp at line 2369 and line 2599, after
assigning the primary creatureId to data.id, also reset the alternate entry
fields data.id2 and data.id3 to clear any stale values. This is necessary
because these alternate fields are no longer selected from the creature table,
so reused CreatureData entries can retain stale alternates when multispawn rows
change. Add the reset of id2 and id3 alongside the data.id assignment at both
locations to ensure consistency across both load paths.
| } while (result->NextRow()); | ||
|
|
||
| // Load alternate entries from creature_multispawn | ||
| QueryResult variantResult = WorldDatabase.Query("SELECT spawnId, entry FROM creature_multispawn ORDER BY spawnId"); |
There was a problem hiding this comment.
Use deterministic ordering for startup variant loading.
The lazy path orders a spawn’s variants by entry, but startup only orders by spawnId; ties can assign id2/id3 differently and choose a different “extra” variant when bad data has more than two rows.
Proposed fix
- QueryResult variantResult = WorldDatabase.Query("SELECT spawnId, entry FROM creature_multispawn ORDER BY spawnId");
+ QueryResult variantResult = WorldDatabase.Query(
+ "SELECT spawnId, entry FROM creature_multispawn "
+ "ORDER BY spawnId, entry");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/game/Globals/ObjectMgr.cpp` at line 2501, The startup variant
loading query in ObjectMgr.cpp at the WorldDatabase.Query call for
creature_multispawn needs to match the deterministic ordering used by the lazy
path. Currently, the query only orders by spawnId, but it should order by both
spawnId and entry to ensure consistent variant assignment and prevent different
"extra" variants from being selected when handling duplicate or malformed data.
Add entry to the ORDER BY clause in the same query that selects spawnId and
entry from creature_multispawn.
| result = WorldDatabase.Query("SELECT COUNT(guid) FROM creature WHERE id = '{}' OR guid IN (SELECT spawnId FROM creature_multispawn WHERE entry = '{}')", uint32(creatureId), uint32(creatureId)); | ||
| if (result) | ||
| creatureCount = (*result)[0].Get<uint64>(); | ||
|
|
||
| if (handler->GetSession()) | ||
| { | ||
| Player* player = handler->GetSession()->GetPlayer(); | ||
| result = WorldDatabase.Query("SELECT guid, position_x, position_y, position_z, map, (POW(position_x - '{}', 2) + POW(position_y - '{}', 2) + POW(position_z - '{}', 2)) AS order_ FROM creature WHERE id1='{}' OR id2='{}' OR id3='{}' ORDER BY order_ ASC LIMIT {}", | ||
| player->GetPositionX(), player->GetPositionY(), player->GetPositionZ(), uint32(creatureId), uint32(creatureId), uint32(creatureId), count); | ||
| result = WorldDatabase.Query("SELECT guid, position_x, position_y, position_z, map, (POW(position_x - '{}', 2) + POW(position_y - '{}', 2) + POW(position_z - '{}', 2)) AS order_ FROM creature WHERE id = '{}' OR guid IN (SELECT spawnId FROM creature_multispawn WHERE entry = '{}') ORDER BY order_ ASC LIMIT {}", | ||
| player->GetPositionX(), player->GetPositionY(), player->GetPositionZ(), uint32(creatureId), uint32(creatureId), count); | ||
| } | ||
| else | ||
| result = WorldDatabase.Query("SELECT guid, position_x, position_y, position_z, map FROM creature WHERE id1='{}' OR id2='{}' OR id3='{}' LIMIT {}", | ||
| uint32(creatureId), uint32(creatureId), uint32(creatureId), count); | ||
| result = WorldDatabase.Query("SELECT guid, position_x, position_y, position_z, map FROM creature WHERE id = '{}' OR guid IN (SELECT spawnId FROM creature_multispawn WHERE entry = '{}') LIMIT {}", | ||
| uint32(creatureId), uint32(creatureId), count); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use prepared statements for the new creature listing queries.
Lines 80–92 introduce/modify raw SQL string queries in server C++ command code. Please move these to WorldDatabase prepared statements and bind parameters (creatureId, count, position values) via SetData to stay compliant and keep query contracts centralized.
As per coding guidelines, use PreparedStatement (via WorldDatabase, CharacterDatabase, or LoginDatabase) instead of raw query strings for database queries in C++.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/scripts/Commands/cs_list.cpp` around lines 80 - 92, The three
WorldDatabase.Query calls at lines 80–92 are using raw SQL strings with inline
parameters instead of prepared statements. Convert each of these queries to use
PreparedStatement objects from WorldDatabase, binding the parameters
(creatureId, count, player position values) via the SetData method. This applies
to the COUNT query, the distance-ordered query with player position calculations
(using POW and player->GetPositionX/Y/Z), and the simpler query without position
ordering.
Source: Coding guidelines
| handler->PSendSysMessage(LANG_NPCINFO_CHAR, lowGuid, ObjectGuid::Create<HighGuid::Unit>(cData->id, lowGuid).ToString(), cData->id, | ||
| cData->id2, cData->id3, cData->displayid, cData->displayid, cInfo->faction, | ||
| cData->npcflag); |
There was a problem hiding this comment.
LANG_NPCINFO_CHAR arguments are shifted in the DB-only NPC info path.
Line 733 drops the id1 slot value, so the remaining fields shift (id2 is shown as id1, etc.). This can produce incorrect output and format-argument mismatch behavior.
Proposed fix
- handler->PSendSysMessage(LANG_NPCINFO_CHAR, lowGuid, ObjectGuid::Create<HighGuid::Unit>(cData->id, lowGuid).ToString(), cData->id,
- cData->id2, cData->id3, cData->displayid, cData->displayid, cInfo->faction,
+ handler->PSendSysMessage(LANG_NPCINFO_CHAR, lowGuid, ObjectGuid::Create<HighGuid::Unit>(cData->id, lowGuid).ToString(), cData->id,
+ cData->id, cData->id2, cData->id3, cData->displayid, cData->displayid, cInfo->faction,
cData->npcflag);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/scripts/Commands/cs_npc.cpp` around lines 733 - 735, The
PSendSysMessage call in the NPC info command is missing the id1 argument for the
LANG_NPCINFO_CHAR format string, causing all subsequent field values to shift
and produce incorrect output. Add the missing cData->id1 argument to the
handler->PSendSysMessage call at the correct position so that id1, id2, id3 are
passed in the proper order after the initial ObjectGuid argument, ensuring the
format string placeholders align correctly with their corresponding values.
|
|
||
| // May need work //PussyWizardEliteMalcrom | ||
| QueryResult result = WorldDatabase.Query("SELECT c.position_x, c.position_y, c.position_z, c.orientation, c.map, ct.name FROM creature c INNER JOIN creature_template ct ON c.id1 = ct.entry WHERE ct.name LIKE '{}'", normalizedName); | ||
| QueryResult result = WorldDatabase.Query("SELECT c.position_x, c.position_y, c.position_z, c.orientation, c.map, ct.name FROM creature c INNER JOIN creature_template ct ON c.id = ct.entry WHERE ct.name LIKE '{}'", normalizedName); |
There was a problem hiding this comment.
Use a prepared statement instead of an inline SQL string.
Line 355 still executes a raw query built from command input. Even with escaping, this should follow the server DB contract and move to a PreparedStatement path.
As per coding guidelines, "Use PreparedStatement (via WorldDatabase, CharacterDatabase, or LoginDatabase and prepared-statement enums) instead of raw query strings for database queries in C++."
Suggested direction
- QueryResult result = WorldDatabase.Query("SELECT c.position_x, c.position_y, c.position_z, c.orientation, c.map, ct.name FROM creature c INNER JOIN creature_template ct ON c.id = ct.entry WHERE ct.name LIKE '{}'", normalizedName);
+ WorldDatabasePreparedStatement* stmt = WorldDatabase.GetPreparedStatement(WORLD_SEL_CREATURE_POS_BY_TEMPLATE_NAME);
+ stmt->SetData(0, normalizedName);
+ QueryResult result = WorldDatabase.Query(stmt);Also add WORLD_SEL_CREATURE_POS_BY_TEMPLATE_NAME in src/server/database/Database/Implementation/WorldDatabase.cpp.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/scripts/Commands/cs_tele.cpp` at line 355, Replace the raw SQL
query in the WorldDatabase.Query() call with a prepared statement approach.
Define a new prepared statement enum WORLD_SEL_CREATURE_POS_BY_TEMPLATE_NAME in
the WorldDatabase implementation to encapsulate the creature position query
(selecting position_x, position_y, position_z, orientation, map, and creature
template name), then use PreparedStatement with the WorldDatabase instead of
passing the raw query string with inline parameter substitution. This ensures
the query follows the server database contract and protects against SQL
injection through proper parameterized query handling.
Source: Coding guidelines
sudlud
left a comment
sudlud
left a comment
There was a problem hiding this comment.
Merged current master into the branch and resolved the two conflicts:
Creature::Respawn: kept master's compat / non-compat split. The multi-ID re-roll stays in compat mode; non-compat destroys and recreates viaProcessRespawns->LoadCreatureFromDB, which re-rolls the entry anyway.ObjectMgr::LoadCreatures: kept the new-schema field indices and master'sdata.spawnIdassignment.
Also updated id1 references that master added after this PR was opened and that the rename would otherwise break to compile (cs_pool, cs_misc, cs_list, ConditionMgr), and fixed the .list creature query to read creature.id plus creature_multispawn instead of the dropped id1/id2/id3 columns.
Verified the net diff against master is exactly the rename plus the creature_multispawn extraction, with no remaining id1 references. LGTM.
In the `creature` table, `id1` was renamed to `id` and `id2` and `id3` were moved to a `creature_multispawn` table upstream in AzerothCore in this PR: azerothcore/azerothcore-wotlk#25197 . This commit updates the module's SQL files, a .h file, and a .cpp file to work with those changes.
…::id PR azerothcore#25197 renamed CreatureData::id1 to id, so the despawner loop no longer compiled. Update the field access and clean up the loop per review: - Snapshot matching spawnIds first, then re-time outside the iteration to avoid mutating GetCreatureRespawnTimes() while iterating it. - Drop the redundant per-map mapid guard (the store is already per-map). - Trim the comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The id1, id2 and id3 got removed it azerothcore/azerothcore-wotlk#25197 its only id now.
The pending update was generated against the base-dump creature schema (id1/id2/id3), but 2026_06_16_00.sql (creature_multispawn, azerothcore#25197) renamed id1 to id and dropped id2/id3, so db-import failed with "Unknown column 'id1' in 'field list'". Rename the column in the INSERT; no other table in the update was altered post-base-dump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Changes Proposed:
This PR normalizes the
creaturetable by extracting the rarely-used multi-ID spawning system into a dedicated table.Database Normalization
The
creaturetable currently stores three creature template entry columns:id1,id2, andid3. This was introduced in PR #10115 to support spawn points that randomly pick between multiple creature templates on spawn/respawn.However, only ~737 out of 5.3M+ creature spawns (~0.014%) actually use
id2orid3. The remaining 99.99% of rows carry two permanently-zero columns, violating database normalization principles (specifically 1NF — the columns represent a repeating group of the same attribute type).This PR:
creature_multispawntable with a composite primary key(spawnId, entry)to store alternate creature template entries for the rare spawns that need themid1back toidin thecreaturetable, restoring the original column nameid2andid3from thecreaturetableid2/id3values are inserted intocreature_multispawnbefore the columns are droppedThe C++
CreatureDatastruct retainsid2/id3fields internally, populated from the new table during loading, so all downstream spawn logic (GetRandomId, respawn entry selection, etc.) remains unchanged.AI-assisted Pull Requests
Important
While the use of AI tools when preparing pull requests is not prohibited, contributors must clearly disclose when such tools have been used and specify the model involved.
Contributors are also expected to fully understand the changes they are submitting and must be able to explain and justify those changes when requested by maintainers.
Issues Addressed:
N/A — this is a schema improvement.
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
How to Test the Changes:
id2/id3values still randomly pick between entries on spawn and respawn.npc infoon both single-entry and multi-entry spawns.npc addto verify new creature spawns work correctlyKnown Issues and TODO List:
id1,id2,id3columns will need updatesHow to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.
Summary by CodeRabbit