feat(Maps): implement MapPartitioned for parallel continent map updates#26445
feat(Maps): implement MapPartitioned for parallel continent map updates#26445hanu-14 wants to merge 7 commits into
Conversation
Add MapPartitioned class that splits continent maps into N partitions by X coordinate. Each partition runs its own Update on the MapUpdater thread pool, enabling parallel entity processing on large continents. Key changes: - MapPartitioned: new class extending Map; creates N child Map instances at construction, routes players by position, schedules partition updates on MapUpdater - MapPartition: thin Map subclass with position-guarded respawn handling to prevent duplicate creature/GO respawns across partitions - Collision data shared via existing MapCollisionData(parent) mechanism (static tree + nav mesh shared, dynamic tree per-partition) - DoForAllMaps/DoForAllMapsWithMapId iterates over partitions - CONFIG_MAP_PARTITION_COUNT (default: 4) controls partition count Configuration: MapUpdate.PartitionCount = 4
github-actions
Bot
added
CORE
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MapPartitioned implementation intended to parallelize continent (“world map”) updates by splitting a base map into multiple X-coordinate partitions and scheduling each partition’s Map::Update() on the existing MapUpdater thread pool.
Changes:
- Added
MapPartitioned/MapPartitionclasses and integrated them into base-map creation for non-instanceable maps. - Updated
MapMgriteration helpers to treat partitions as transparent map units for commands/stats/scripts. - Added a new configuration option
MapUpdate.PartitionCount(default4) to control partition count.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/game/World/WorldConfig.h | Adds config enum for partition count. |
| src/server/game/World/WorldConfig.cpp | Registers MapUpdate.PartitionCount with default value. |
| src/server/game/Maps/MapPartitioned.h | Declares MapPartitioned and MapPartition map types. |
| src/server/game/Maps/MapPartitioned.cpp | Implements partition creation, routing, update scheduling, and respawn filtering logic. |
| src/server/game/Maps/MapMgr.h | Updates DoForAllMaps* helpers to iterate partitions. |
| src/server/game/Maps/MapMgr.cpp | Creates partitioned maps for non-instanceable maps and extends FindMap. |
| src/server/game/Maps/Map.h | Adds partition-type helpers and makes respawn hooks overridable/accessible for partition filtering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!IsPositionInPartition(data->posX)) | ||
| { | ||
| RemoveCreatureRespawnTime(spawnId); | ||
| return; | ||
| } |
| if (!IsPositionInPartition(data->posX)) | ||
| { | ||
| RemoveGORespawnTime(spawnId); | ||
| return; | ||
| } |
| #include "MapPartitioned.h" | ||
| #include "MapMgr.h" | ||
| #include "Player.h" | ||
| #include "Log.h" | ||
| #include "ObjectMgr.h" | ||
| #include "SpawnData.h" | ||
| #include "ScriptMgr.h" | ||
|
|
| void MapPartitioned::LoadRespawnTimes() | ||
| { | ||
| for (Map* partition : m_Partitions) | ||
| { | ||
| partition->LoadRespawnTimes(); | ||
| } | ||
|
|
||
| Map::LoadRespawnTimes(); | ||
| } |
| Map* MapPartitioned::GetPartitionForPosition(float x, float y) const | ||
| { | ||
| uint32 index = GetPartitionIndex(x); | ||
| if (index < m_Partitions.size()) | ||
| return m_Partitions[index]; | ||
| return m_Partitions[0]; | ||
| } |
| uint32 MapPartitioned::GetPartitionIndex(float x) const | ||
| { | ||
| float const MAP_HALF_SIZE = 17066.0f; | ||
| float clampedX = std::max(-MAP_HALF_SIZE, std::min(x, MAP_HALF_SIZE)); | ||
| float normalizedX = (clampedX + MAP_HALF_SIZE) / (2.0f * MAP_HALF_SIZE); | ||
| return uint32(normalizedX * m_partitionCount) % m_partitionCount; | ||
| } |
| if (entry->Instanceable()) | ||
| map = new MapInstanced(id); | ||
| else | ||
| map = new Map(id, 0, REGULAR_DIFFICULTY); | ||
| map = new MapPartitioned(id, sWorld->getIntConfig(CONFIG_MAP_PARTITION_COUNT)); | ||
|
|
📝 WalkthroughWalkthroughThis PR adds map partitioning support to AzerothCore. New ChangesMap Partitioning
Estimated code review effort: 4 (Complex) | ~60 minutes Related issues: Directly addresses "Feature: Map Partitioning Support for AzerothCore" ( Suggested labels: enhancement, performance, maps Suggested reviewers: Core maintainers familiar with the Maps subsystem and MapMgr threading model 🐰 A map once whole, now cut in four, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/server/game/Maps/MapPartitioned.cpp (1)
196-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant guard and braces in
src/server/game/Maps/MapPartitioned.cpp:196-203
AddPlayerToMap()already assigns the child partition toplayer->SetMap(partition), sopartition != thisis effectively always true here. Remove the braces to match the style rule, and either drop the check or replace it with a real membership test if you meant to guard against an unrelated map.🤖 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/Maps/MapPartitioned.cpp` around lines 196 - 203, Remove the redundant partition guard in MapPartitioned::RemovePlayerFromMap and match the brace/style convention. Since AddPlayerToMap() already sets the child partition via player->SetMap(partition), the partition != this check is unnecessary here; either call partition->RemovePlayerFromMap(player, remove) directly after a null check, or replace the condition with a real membership validation if that was the intent.Source: Coding guidelines
src/server/game/Maps/Map.h (1)
645-645: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueKeep the
protectedscope narrow. This change makes the rest ofMap’s internals and helper state available to every subclass, not just the respawn bookkeeping. Split the new state into a smaller protected block or keep unrelated members private.🤖 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/Maps/Map.h` at line 645, The Map class exposes too much internal state under a broad protected scope, making unrelated members available to all subclasses. Narrow the access in Map by keeping only the respawn-related state in a small protected section and moving the rest of the internals/helper members back to private, using the Map class and its respawn bookkeeping members to locate the affected block.
🤖 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 `@src/server/game/Maps/MapPartitioned.cpp`:
- Around line 46-49: MapPartitioned::CreatePartition is hardcoding instanceId to
0, which prevents MapMgr::FindMap from ever matching a partition in its
partition lookup path. Update CreatePartition so each MapPartition gets a unique
non-zero instance id derived from the partition index (for example, based on
index) while keeping the existing MapPartition constructor call and identifiers
like GetId(), m_partitionCount, and static_cast<uint8>(index) intact.
- Around line 125-133: The trailing Map::LoadRespawnTimes() call in
MapPartitioned::LoadRespawnTimes() is redundant because the base Map object is
never ticked by MapPartitioned::Update(), so its respawn data is never used.
Remove that base-object load and keep only the per-partition loading loop in
MapPartitioned::LoadRespawnTimes() so respawn times are loaded only for the maps
that actually run Update().
- Around line 87-121: Initial grid loading still bypasses partition filtering,
so update GridObjectLoader::LoadCreatures() and
GridObjectLoader::LoadGameObjects() to skip spawns whose coordinates are not in
the current partition using IsPositionInPartition, matching the respawn-time
checks already added in MapPartition::ProcessCreatureRespawn and
MapPartition::ProcessGameObjectRespawn. Make sure the partition-aware load path
filters both creature and gameobject spawns before instantiation so
MapPartitioned::LoadRespawnTimes does not cause duplicate off-partition objects
across partitions.
- Around line 189-194: Remove the eager player map assignment in
MapPartitioned::AddPlayerToMap and let Map::AddPlayerToMap handle SetMap only
after a successful add. Update the MapPartitioned::AddPlayerToMap flow so it
just finds the partition with GetPartitionForPlayer and forwards the player to
partition->AddPlayerToMap, avoiding any pre-set state that could leave Player
pointing at the partition when the add fails.
In `@src/server/game/Maps/MapPartitioned.h`:
- Around line 34-38: The partition index calculation is duplicated between
MapPartition and MapPartitioned, so extract the shared X-to-partition math into
a single helper (for example, a free function or static utility near
MapPartitioned) and have both GetPartitionIndex implementations call it. Keep
the normalization logic and MAP_HALF_SIZE handling in one place so
IsPositionInPartition and the respawn/player routing code cannot drift apart.
In `@src/server/game/World/WorldConfig.cpp`:
- Line 566: Add validation for CONFIG_MAP_PARTITION_COUNT in
WorldConfig::SetConfigValue so only safe non-zero values within the uint8 range
are accepted. The current MapUpdate.PartitionCount setting is passed into
MapPartitioned’s uint8 partitionCount and can truncate large values or allow 0,
which later breaks MapPartitioned::GetPartitionIndex. Update the config
registration for MapUpdate.PartitionCount to use a validator lambda like the
other numeric options in this file and reject 0 and values above 255.
---
Nitpick comments:
In `@src/server/game/Maps/Map.h`:
- Line 645: The Map class exposes too much internal state under a broad
protected scope, making unrelated members available to all subclasses. Narrow
the access in Map by keeping only the respawn-related state in a small protected
section and moving the rest of the internals/helper members back to private,
using the Map class and its respawn bookkeeping members to locate the affected
block.
In `@src/server/game/Maps/MapPartitioned.cpp`:
- Around line 196-203: Remove the redundant partition guard in
MapPartitioned::RemovePlayerFromMap and match the brace/style convention. Since
AddPlayerToMap() already sets the child partition via player->SetMap(partition),
the partition != this check is unnecessary here; either call
partition->RemovePlayerFromMap(player, remove) directly after a null check, or
replace the condition with a real membership validation if that was the intent.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7a12367-b092-40bf-b58a-72a5262de5f2
📒 Files selected for processing (7)
src/server/game/Maps/Map.hsrc/server/game/Maps/MapMgr.cppsrc/server/game/Maps/MapMgr.hsrc/server/game/Maps/MapPartitioned.cppsrc/server/game/Maps/MapPartitioned.hsrc/server/game/World/WorldConfig.cppsrc/server/game/World/WorldConfig.h
| Map* MapPartitioned::CreatePartition(uint32 index) | ||
| { | ||
| return new MapPartition(GetId(), 0, REGULAR_DIFFICULTY, this, m_partitionCount, static_cast<uint8>(index)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Every partition is created with instanceId = 0, so MapMgr::FindMap's partition lookup can never match.
CreatePartition hardcodes 0 for instanceId:
return new MapPartition(GetId(), 0, REGULAR_DIFFICULTY, this, m_partitionCount, static_cast<uint8>(index));All partitions therefore have GetInstanceId() == 0. In MapMgr::FindMap (see MapMgr.cpp), instanceId == 0 already short-circuits to return the base MapPartitioned map before the partition-search loop runs, and for any non-zero instanceId the loop can never find a match since no partition has a non-zero id. The new partition-resolution code path added to FindMap is effectively dead.
Give each partition a unique, non-zero instance id (e.g. index + 1).
🐛 Proposed fix
Map* MapPartitioned::CreatePartition(uint32 index)
{
- return new MapPartition(GetId(), 0, REGULAR_DIFFICULTY, this, m_partitionCount, static_cast<uint8>(index));
+ return new MapPartition(GetId(), index + 1, REGULAR_DIFFICULTY, this, m_partitionCount, static_cast<uint8>(index));
}🤖 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/Maps/MapPartitioned.cpp` around lines 46 - 49,
MapPartitioned::CreatePartition is hardcoding instanceId to 0, which prevents
MapMgr::FindMap from ever matching a partition in its partition lookup path.
Update CreatePartition so each MapPartition gets a unique non-zero instance id
derived from the partition index (for example, based on index) while keeping the
existing MapPartition constructor call and identifiers like GetId(),
m_partitionCount, and static_cast<uint8>(index) intact.
| void MapPartition::ProcessCreatureRespawn(ObjectGuid::LowType spawnId) | ||
| { | ||
| CreatureData const* data = sObjectMgr->GetCreatureData(spawnId); | ||
| if (!data) | ||
| { | ||
| RemoveCreatureRespawnTime(spawnId); | ||
| return; | ||
| } | ||
|
|
||
| if (!IsPositionInPartition(data->posX)) | ||
| { | ||
| RemoveCreatureRespawnTime(spawnId); | ||
| return; | ||
| } | ||
|
|
||
| Map::ProcessCreatureRespawn(spawnId); | ||
| } | ||
|
|
||
| void MapPartition::ProcessGameObjectRespawn(ObjectGuid::LowType spawnId) | ||
| { | ||
| GameObjectData const* data = sObjectMgr->GetGameObjectData(spawnId); | ||
| if (!data) | ||
| { | ||
| RemoveGORespawnTime(spawnId); | ||
| return; | ||
| } | ||
|
|
||
| if (!IsPositionInPartition(data->posX)) | ||
| { | ||
| RemoveGORespawnTime(spawnId); | ||
| return; | ||
| } | ||
|
|
||
| Map::ProcessGameObjectRespawn(spawnId); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how creature/GO grid loading decides to spawn vs. respawn, to check whether it bypasses ProcessCreatureRespawn/ProcessGameObjectRespawn
ast-grep outline src/server/game/Maps/Map.cpp --view expanded | head -100
rg -n 'GetCreatureRespawnTime|LoadGridObjects|ObjectGridLoader' src/server/game/Maps/Map.cpp src/server/game/Grids -A3 -B3Repository: azerothcore/azerothcore-wotlk
Length of output: 1772
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- MapPartitioned.cpp outline ---'
ast-grep outline src/server/game/Maps/MapPartitioned.cpp --view expanded | head -200
echo
echo '--- Map.cpp outline (relevant symbols) ---'
ast-grep outline src/server/game/Maps/Map.cpp --view expanded | head -250
echo
echo '--- Search for grid-load spawn paths and respawn table usage ---'
rg -n 'ObjectGridLoader|LoadGrid|LoadCreatures|LoadGameObjects|GetCreatureRespawnTime|GetGORespawnTime|_creatureRespawnTimes|_gameObjectRespawnTimes|Respawn' src/server/game/Maps src/server/game/Grids src/server/game -g '!*.o' -A3 -B3Repository: azerothcore/azerothcore-wotlk
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- GridObjectLoader outline ---'
ast-grep outline src/server/game/Grids/GridObjectLoader.h --view expanded
echo
ast-grep outline src/server/game/Grids/GridObjectLoader.cpp --view expanded | head -200
echo
echo '--- GridObjectLoader implementation slices ---'
sed -n '1,240p' src/server/game/Grids/GridObjectLoader.cpp | cat -n
echo
echo '--- Map.cpp grid-load / respawn slices ---'
sed -n '180,240p' src/server/game/Maps/Map.cpp | cat -n
echo
sed -n '2480,2865p' src/server/game/Maps/Map.cpp | cat -n
echo
echo '--- MapPartitioned respawn/load slices ---'
sed -n '1,170p' src/server/game/Maps/MapPartitioned.cpp | cat -nRepository: azerothcore/azerothcore-wotlk
Length of output: 31126
Initial grid loading still bypasses partition filtering
GridObjectLoader::LoadCreatures() / LoadGameObjects() still load every spawn in a grid without checking IsPositionInPartition, so this change only guards the respawn-timer path. Since MapPartitioned::LoadRespawnTimes() loads respawn state into every partition, boundary grids can still instantiate off-partition creatures/GOs and duplicate them across partitions.
🤖 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/Maps/MapPartitioned.cpp` around lines 87 - 121, Initial grid
loading still bypasses partition filtering, so update
GridObjectLoader::LoadCreatures() and GridObjectLoader::LoadGameObjects() to
skip spawns whose coordinates are not in the current partition using
IsPositionInPartition, matching the respawn-time checks already added in
MapPartition::ProcessCreatureRespawn and MapPartition::ProcessGameObjectRespawn.
Make sure the partition-aware load path filters both creature and gameobject
spawns before instantiation so MapPartitioned::LoadRespawnTimes does not cause
duplicate off-partition objects across partitions.
| void MapPartitioned::LoadRespawnTimes() | ||
| { | ||
| for (Map* partition : m_Partitions) | ||
| { | ||
| partition->LoadRespawnTimes(); | ||
| } | ||
|
|
||
| Map::LoadRespawnTimes(); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Map::LoadRespawnTimes() on the un-ticked base object loads data that is never used.
MapPartitioned::Update() (below) only schedules updates for m_Partitions; it never calls Map::Update() on this. Since respawn processing happens inside Map::Update()'s tick loop, the respawn times loaded into this via the trailing Map::LoadRespawnTimes(); call are never consumed — this is a full, redundant DB query and in-memory table for every partitioned continent that has no functional effect.
⚡ Proposed fix
void MapPartitioned::LoadRespawnTimes()
{
for (Map* partition : m_Partitions)
{
partition->LoadRespawnTimes();
}
-
- Map::LoadRespawnTimes();
}📝 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.
| void MapPartitioned::LoadRespawnTimes() | |
| { | |
| for (Map* partition : m_Partitions) | |
| { | |
| partition->LoadRespawnTimes(); | |
| } | |
| Map::LoadRespawnTimes(); | |
| } | |
| void MapPartitioned::LoadRespawnTimes() | |
| { | |
| for (Map* partition : m_Partitions) | |
| { | |
| partition->LoadRespawnTimes(); | |
| } | |
| } |
🤖 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/Maps/MapPartitioned.cpp` around lines 125 - 133, The trailing
Map::LoadRespawnTimes() call in MapPartitioned::LoadRespawnTimes() is redundant
because the base Map object is never ticked by MapPartitioned::Update(), so its
respawn data is never used. Remove that base-object load and keep only the
per-partition loading loop in MapPartitioned::LoadRespawnTimes() so respawn
times are loaded only for the maps that actually run Update().
| bool MapPartitioned::AddPlayerToMap(Player* player) | ||
| { | ||
| Map* partition = GetPartitionForPlayer(player); | ||
| player->SetMap(partition); | ||
| return partition->AddPlayerToMap(player); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== MapPartitioned.cpp ==\n'
git ls-files 'src/server/game/Maps/MapPartitioned.cpp'
wc -l src/server/game/Maps/MapPartitioned.cpp
cat -n src/server/game/Maps/MapPartitioned.cpp | sed -n '170,210p'
printf '\n== Search AddPlayerToMap implementations ==\n'
rg -n "AddPlayerToMap\\(" src/server/game/Maps src/server/game -g'*.cpp' -g'*.h'
printf '\n== Map class outline ==\n'
ast-grep outline src/server/game/Maps/Map.h --view expanded || true
ast-grep outline src/server/game/Maps/Map.cpp --view expanded || trueRepository: azerothcore/azerothcore-wotlk
Length of output: 3574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== MapPartitioned.cpp ==\n'
wc -l src/server/game/Maps/MapPartitioned.cpp
cat -n src/server/game/Maps/MapPartitioned.cpp | sed -n '180,205p'
printf '\n== Relevant Map APIs ==\n'
rg -n "class Map|bool AddPlayerToMap|void SetMap|SetMap\\(" src/server/game/Maps src/server/game/Entities src/server/game -g'*.h' -g'*.cpp' | sed -n '1,200p'Repository: azerothcore/azerothcore-wotlk
Length of output: 10989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/server/game/Maps/MapPartitioned.cpp
sed -n '180,205p' src/server/game/Maps/MapPartitioned.cpp
rg -n "bool Map::AddPlayerToMap|AddPlayerToMap\\(Player\\*|SetMap\\(" src/server/game/Maps src/server/game -g'*.cpp' -g'*.h'Repository: azerothcore/azerothcore-wotlk
Length of output: 4939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '245,310p' src/server/game/Maps/Map.cpp
printf '\n== InstanceMap::AddPlayerToMap ==\n'
sed -n '1980,2038p' src/server/game/Maps/Map.cpp
printf '\n== BattlegroundMap::AddPlayerToMap ==\n'
sed -n '2310,2345p' src/server/game/Maps/Map.cppRepository: azerothcore/azerothcore-wotlk
Length of output: 5732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '255,290p' src/server/game/Maps/Map.cppRepository: azerothcore/azerothcore-wotlk
Length of output: 1170
Remove the eager SetMap here
Map::AddPlayerToMap already sets the player’s map on success, so doing it before the add can leave the player pointing at a partition when AddPlayerToMap returns false.
🤖 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/Maps/MapPartitioned.cpp` around lines 189 - 194, Remove the
eager player map assignment in MapPartitioned::AddPlayerToMap and let
Map::AddPlayerToMap handle SetMap only after a successful add. Update the
MapPartitioned::AddPlayerToMap flow so it just finds the partition with
GetPartitionForPlayer and forwards the player to partition->AddPlayerToMap,
avoiding any pre-set state that could leave Player pointing at the partition
when the add fails.
| bool IsPositionInPartition(float x) const; | ||
| uint32 GetPartitionIndex(float x) const; | ||
|
|
||
| uint8 m_partitionCount; | ||
| uint8 m_partitionIndex; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate partition-index math across MapPartition and MapPartitioned.
Both classes independently declare GetPartitionIndex(float x) (and, in the .cpp, duplicate the MAP_HALF_SIZE constant and normalization formula). If the two implementations ever diverge, player routing (MapPartitioned) and respawn gating (MapPartition) would silently disagree about which partition owns a given X coordinate, causing missed/duplicated respawns at partition boundaries.
Extract the shared math into one free function or static helper (e.g. in MapPartitioned.h) and have both classes call it.
Also applies to: 71-74
🤖 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/Maps/MapPartitioned.h` around lines 34 - 38, The partition
index calculation is duplicated between MapPartition and MapPartitioned, so
extract the shared X-to-partition math into a single helper (for example, a free
function or static utility near MapPartitioned) and have both GetPartitionIndex
implementations call it. Keep the normalization logic and MAP_HALF_SIZE handling
in one place so IsPositionInPartition and the respawn/player routing code cannot
drift apart.
| SetConfigValue<bool>(CONFIG_SHOW_MUTE_IN_WORLD, "ShowMuteInWorld", false); | ||
| SetConfigValue<bool>(CONFIG_SHOW_BAN_IN_WORLD, "ShowBanInWorld", false); | ||
| SetConfigValue<uint32>(CONFIG_NUMTHREADS, "MapUpdate.Threads", 1); | ||
| SetConfigValue<uint32>(CONFIG_MAP_PARTITION_COUNT, "MapUpdate.PartitionCount", 4); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing validation on MapUpdate.PartitionCount; also implicitly truncated to uint8.
Unlike most numeric options in this file (e.g. CONFIG_COMPRESSION, CONFIG_MAX_OVERSPEED_PINGS), this has no validator lambda. A value of 0 set by an operator reaches MapPartitioned's uint8 partitionCount constructor parameter (truncating any value ≥ 256), and MapPartitioned::GetPartitionIndex performs % m_partitionCount — 0 triggers UB/crash on every position lookup.
🛡️ Proposed fix
- SetConfigValue<uint32>(CONFIG_MAP_PARTITION_COUNT, "MapUpdate.PartitionCount", 4);
+ SetConfigValue<uint32>(CONFIG_MAP_PARTITION_COUNT, "MapUpdate.PartitionCount", 4, ConfigValueCache::Reloadable::No, [](uint32 const& value) { return value > 0 && value <= 255; }, "> 0 && <= 255");📝 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.
| SetConfigValue<uint32>(CONFIG_MAP_PARTITION_COUNT, "MapUpdate.PartitionCount", 4); | |
| SetConfigValue<uint32>(CONFIG_MAP_PARTITION_COUNT, "MapUpdate.PartitionCount", 4, ConfigValueCache::Reloadable::No, [](uint32 const& value) { return value > 0 && value <= 255; }, "> 0 && <= 255"); |
🤖 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/World/WorldConfig.cpp` at line 566, Add validation for
CONFIG_MAP_PARTITION_COUNT in WorldConfig::SetConfigValue so only safe non-zero
values within the uint8 range are accepted. The current MapUpdate.PartitionCount
setting is passed into MapPartitioned’s uint8 partitionCount and can truncate
large values or allow 0, which later breaks MapPartitioned::GetPartitionIndex.
Update the config registration for MapUpdate.PartitionCount to use a validator
lambda like the other numeric options in this file and reject 0 and values above
255.
…g validation, deduplication
|
I would love to see the resource increase running this PR |
|
We need a DJ Khaled label |
|
All review feedback has been addressed in the latest code:
All Copilot and CodeRabbit issues resolved. Ready for re-review. |
|
Okay let's be real. This is way too complex of a system to be purely AI built just for a quick bounty cashin. I will close this, as many other AI generated Map partitioning PRs. |
Changes Proposed:
This PR proposes changes to:
Adds MapPartitioned class that splits continent maps into N partitions by X coordinate. Each partition runs its own Update on the MapUpdater thread pool, enabling parallel entity processing on large continents.
New classes
Collision data
Shared via existing MapCollisionData(parent) mechanism: static tree + nav mesh via shared_ptr; dynamic tree is per-partition.
Integration
New config
MapUpdate.PartitionCount(default: 4) - number of partitions per continentAI-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.
opencode
Issues Addressed:
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
Builds successfully with GCC 14 and Clang 18 on Ubuntu (pending CI re-run after latest fixes).
How to Test the Changes:
MapUpdate.PartitionCountin worldserver.conf (default: 4)Known Issues and TODO List: