Skip to content

Commit

Permalink
VMAP: Resolve VMAP manager race conditions with iInstanceMapTrees
Browse files Browse the repository at this point in the history 
Closes #584

Adapted from TrinityCore/TrinityCore@87a5ba0
  • Loading branch information
@killerwife
killerwife committed Aug 17, 2024
1 parent 431adc5 commit aac413d
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 31 deletions.
10 changes: 10 additions & 0 deletions src/game/World/World.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,16 @@ void World::SetInitialWorldSettings()
DetectDBCLang();
sObjectMgr.SetDbc2StorageLocaleIndex(GetDefaultDbcLocale()); // Get once for all the locale index of DBC language (console/broadcasts)

if (VMAP::IVMapManager* vmmgr2 = VMAP::VMapFactory::createOrGetVMapManager()) // after map store init
{
std::vector<uint32> mapIds;
for (uint32 mapId = 0; mapId < sMapStore.GetNumRows(); mapId++)

This comment has been minimized.

Sign in to view

Copy link
@insunaa

insunaa Sep 1, 2024

Contributor

Is this right? The number of maps is much smaller than the highest map id, so higher map IDs would not get initialized, right?

This comment has been minimized.

Sign in to view

Copy link
@insunaa

insunaa Sep 1, 2024

Contributor

In WotLK:
image
@killerwife
if (sMapStore.LookupEntry(mapId))
mapIds.push_back(mapId);

vmmgr2->InitializeThreadUnsafe(mapIds);
}

// Loading cameras for characters creation cinematic
sLog.outString("Loading cinematic...");
LoadM2Cameras(m_dataPath);
Expand Down
2 changes: 2 additions & 0 deletions src/game/vmap/IVMapManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ namespace VMAP

virtual ~IVMapManager(void) {}

virtual void InitializeThreadUnsafe(const std::vector<uint32>& mapIds) = 0;

virtual VMAPLoadResult loadMap(const char* pBasePath, unsigned int pMapId, int x, int y) = 0;

virtual bool existsMap(const char* pBasePath, unsigned int pMapId, int x, int y) = 0;
Expand Down
13 changes: 3 additions & 10 deletions src/game/vmap/VMapFactory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ namespace VMAP
}
}

std::mutex m_vmapMutex;
IVMapManager* gVMapManager = nullptr;

//===============================================
Expand Down Expand Up @@ -83,22 +82,16 @@ namespace VMAP
// just return the instance
IVMapManager* VMapFactory::createOrGetVMapManager()
{
if (!gVMapManager)
{
std::lock_guard<std::mutex> lock(m_vmapMutex);
if (!gVMapManager)
gVMapManager = new VMapManager2(); // should be taken from config ... Please change if you like :-)
}
if (!gVMapManager) // called once on load
gVMapManager = new VMapManager2(); // should be taken from config ... Please change if you like :-)
return gVMapManager;
}

//===============================================
// delete all internal data structures
void VMapFactory::clear()
{
std::lock_guard<std::mutex> lock(m_vmapMutex);

delete gVMapManager;
delete gVMapManager; // called once on world destruction where single threaded is guaranteed
gVMapManager = nullptr;
}
}
69 changes: 48 additions & 21 deletions src/game/vmap/VMapManager2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace VMAP
{
//=========================================================

VMapManager2::VMapManager2()
VMapManager2::VMapManager2() : m_thread_safe_environment(true)
{
}

Expand All @@ -50,6 +50,15 @@ namespace VMAP
}
}

void VMapManager2::InitializeThreadUnsafe(const std::vector<uint32>& mapIds)
{
// the caller must pass the list of all mapIds that will be used in the VMapManager2 lifetime
for (const uint32& mapId : mapIds)
iInstanceMapTrees.insert(InstanceTreeMap::value_type(mapId, nullptr));

m_thread_safe_environment = false;
}

//=========================================================

Vector3 VMapManager2::convertPositionToInternalRep(float x, float y, float z) const
Expand All @@ -63,6 +72,16 @@ namespace VMAP
return pos;
}

InstanceTreeMap::const_iterator VMapManager2::GetMapTree(uint32 mapId) const
{
// return the iterator if found or end() if not found/NULL
InstanceTreeMap::const_iterator itr = iInstanceMapTrees.find(mapId);
if (itr != iInstanceMapTrees.cend() && !itr->second)
itr = iInstanceMapTrees.cend();

return itr;
}

// move to MapTree too?
std::string VMapManager2::getMapFileName(unsigned int pMapId)
{
Expand Down Expand Up @@ -92,21 +111,29 @@ namespace VMAP
bool VMapManager2::IsTileLoaded(uint32 mapId, uint32 x, uint32 y) const
{
InstanceTreeMap::const_iterator instanceTree = iInstanceMapTrees.find(mapId);
if (instanceTree == iInstanceMapTrees.end())
if (instanceTree == iInstanceMapTrees.end() || instanceTree->second == nullptr)
return false;
return instanceTree->second->IsTileLoaded(x, y);
}

//=========================================================
// load one tile (internal use only)

bool VMapManager2::_loadMap(unsigned int pMapId, const std::string& basePath, uint32 tileX, uint32 tileY)
bool VMapManager2::_loadMap(unsigned int mapId, const std::string& basePath, uint32 tileX, uint32 tileY)
{
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(pMapId);
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(mapId);
if (instanceTree == iInstanceMapTrees.end())
{
std::string mapFileName = getMapFileName(pMapId);
StaticMapTree* newTree = new StaticMapTree(pMapId, basePath);
if (m_thread_safe_environment)
instanceTree = iInstanceMapTrees.insert(InstanceTreeMap::value_type(mapId, nullptr)).first;
else
MANGOS_ASSERT(false && "Invalid mapId passed to VMapManager2 after startup in thread unsafe environment");
}

if (!instanceTree->second)
{
std::string mapFileName = getMapFileName(mapId);
StaticMapTree* newTree = new StaticMapTree(mapId, basePath);
if (!newTree->InitMap(mapFileName, this))
{
delete newTree;
Expand All @@ -116,7 +143,7 @@ namespace VMAP
// insert new data
{
std::lock_guard<std::mutex> lock(m_vmStaticMapMutex);
instanceTree = iInstanceMapTrees.insert(InstanceTreeMap::value_type(pMapId, newTree)).first;
instanceTree->second = newTree;
}
}
return instanceTree->second->LoadMapTile(tileX, tileY, this);
Expand All @@ -127,13 +154,13 @@ namespace VMAP
void VMapManager2::unloadMap(unsigned int pMapId)
{
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(pMapId);
if (instanceTree != iInstanceMapTrees.end())
if (instanceTree != iInstanceMapTrees.end() && instanceTree->second)
{
instanceTree->second->UnloadMap(this);
if (instanceTree->second->numLoadedTiles() == 0)
{
delete instanceTree->second;
iInstanceMapTrees.erase(pMapId);
instanceTree->second = nullptr;
}
}
}
Expand All @@ -143,24 +170,24 @@ namespace VMAP
void VMapManager2::unloadMap(unsigned int pMapId, int x, int y)
{
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(pMapId);
if (instanceTree != iInstanceMapTrees.end())
if (instanceTree != iInstanceMapTrees.end() && instanceTree->second)
{
instanceTree->second->UnloadMapTile(x, y, this);
if (instanceTree->second->numLoadedTiles() == 0)
{
delete instanceTree->second;
iInstanceMapTrees.erase(pMapId);
instanceTree->second = nullptr;
}
}
}

//==========================================================

bool VMapManager2::isInLineOfSight(unsigned int pMapId, float x1, float y1, float z1, float x2, float y2, float z2, bool ignoreM2Model)
bool VMapManager2::isInLineOfSight(unsigned int mapId, float x1, float y1, float z1, float x2, float y2, float z2, bool ignoreM2Model)
{
if (!isLineOfSightCalcEnabled()) return true;
bool result = true;
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(pMapId);
InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
if (instanceTree != iInstanceMapTrees.end())
{
Vector3 pos1 = convertPositionToInternalRep(x1, y1, z1);
Expand All @@ -177,15 +204,15 @@ namespace VMAP
get the hit position and return true if we hit something
otherwise the result pos will be the dest pos
*/
bool VMapManager2::getObjectHitPos(unsigned int pMapId, float x1, float y1, float z1, float x2, float y2, float z2, float& rx, float& ry, float& rz, float pModifyDist)
bool VMapManager2::getObjectHitPos(unsigned int mapId, float x1, float y1, float z1, float x2, float y2, float z2, float& rx, float& ry, float& rz, float pModifyDist)
{
bool result = false;
rx = x2;
ry = y2;
rz = z2;
if (isLineOfSightCalcEnabled())
{
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(pMapId);
InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
if (instanceTree != iInstanceMapTrees.end())
{
Vector3 pos1 = convertPositionToInternalRep(x1, y1, z1);
Expand All @@ -206,12 +233,12 @@ namespace VMAP
get height or INVALID_HEIGHT if no height available
*/

float VMapManager2::getHeight(unsigned int pMapId, float x, float y, float z, float maxSearchDist)
float VMapManager2::getHeight(unsigned int mapId, float x, float y, float z, float maxSearchDist)
{
float height = VMAP_INVALID_HEIGHT_VALUE; // no height
if (isHeightCalcEnabled())
{
InstanceTreeMap::iterator instanceTree = iInstanceMapTrees.find(pMapId);
InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
if (instanceTree != iInstanceMapTrees.end())
{
Vector3 pos = convertPositionToInternalRep(x, y, z);
Expand All @@ -227,10 +254,10 @@ namespace VMAP

//=========================================================

bool VMapManager2::getAreaInfo(unsigned int pMapId, float x, float y, float& z, uint32& flags, int32& adtId, int32& rootId, int32& groupId) const
bool VMapManager2::getAreaInfo(unsigned int mapId, float x, float y, float& z, uint32& flags, int32& adtId, int32& rootId, int32& groupId) const
{
bool result = false;
InstanceTreeMap::const_iterator instanceTree = iInstanceMapTrees.find(pMapId);
InstanceTreeMap::const_iterator instanceTree = GetMapTree(mapId);
if (instanceTree != iInstanceMapTrees.end())
{
Vector3 pos = convertPositionToInternalRep(x, y, z);
Expand Down Expand Up @@ -319,8 +346,8 @@ namespace VMAP
}
//=========================================================

bool VMapManager2::existsMap(const char* pBasePath, unsigned int pMapId, int x, int y)
bool VMapManager2::existsMap(const char* pBasePath, unsigned int mapId, int x, int y)
{
return StaticMapTree::CanLoadMap(std::string(pBasePath), pMapId, x, y);
return StaticMapTree::CanLoadMap(std::string(pBasePath), mapId, x, y);
}
} // namespace VMAP
5 changes: 5 additions & 0 deletions src/game/vmap/VMapManager2.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ namespace VMAP
std::mutex m_vmStaticMapMutex;
std::mutex m_vmModelMutex;

bool m_thread_safe_environment;

protected:
// Tree to check collision
ModelFileMap iLoadedModelFiles;
Expand All @@ -80,11 +82,14 @@ namespace VMAP
public:
// public for debug
G3D::Vector3 convertPositionToInternalRep(float x, float y, float z) const;
InstanceTreeMap::const_iterator GetMapTree(uint32 mapId) const;
static std::string getMapFileName(unsigned int pMapId);

VMapManager2();
~VMapManager2();

void InitializeThreadUnsafe(const std::vector<uint32>& mapIds);

VMAPLoadResult loadMap(const char* pBasePath, unsigned int pMapId, int x, int y) override;
bool IsTileLoaded(uint32 mapId, uint32 x, uint32 y) const override;

Expand Down

2 comments on commit aac413d

@al3xc1985
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to reextract vmaps?

@killerwife
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Please sign in to comment.