** Cleanup in ObjectAccessor/HashMapHolder:

* Correctly lock the HashMapHolder on access.
* Use ObjectAccessor wrappers (GetPlayers, GetCreatures, GetGameObjects) instead of raw HashMapHolder<Type>::GetContainer.
* General code style cleanup.

--HG--
branch : trunk

src/game/LFGHandler.cpp

@@ -32,7 +32,7 @@ static void AttemptJoin(Player* _player)
     if(!_player->m_lookingForGroup.canAutoJoin() || _player->GetGroup())
         return;
 
-    //TODO: Guard Player Map
+    ObjectAccessor::Guard guard(*HashMapHolder<Player>::GetLock());
     HashMapHolder<Player>::MapType const& players = ObjectAccessor::Instance().GetPlayers();
     for (HashMapHolder<Player>::MapType::const_iterator iter = players.begin(); iter != players.end(); ++iter)
     {
@@ -92,7 +92,7 @@ static void AttemptAddMore(Player* _player)
     if(!_player->m_lookingForGroup.more.canAutoJoin())
         return;
 
-    //TODO: Guard Player map
+    ObjectAccessor::Guard guard(*HashMapHolder<Player>::GetLock());
     HashMapHolder<Player>::MapType const& players = ObjectAccessor::Instance().GetPlayers();
     for (HashMapHolder<Player>::MapType::const_iterator iter = players.begin(); iter != players.end(); ++iter)
     {

src/game/Level0.cpp

@@ -165,9 +165,9 @@ bool ChatHandler::HandleGMListIngameCommand(const char* /*args*/)
 {
     bool first = true;
 
-    HashMapHolder<Player>::MapType &m = HashMapHolder<Player>::GetContainer();
+    ObjectAccessor::Guard guard(*HashMapHolder<Player>::GetLock());
-    HashMapHolder<Player>::MapType::const_iterator itr = m.begin();
+    HashMapHolder<Player>::MapType &m = ObjectAccessor::Instance().GetPlayers();
-    for (; itr != m.end(); ++itr)
+    for (HashMapHolder<Player>::MapType::const_iterator itr = m.begin(); itr != m.end(); ++itr)
     {
         AccountTypes itr_sec = itr->second->GetSession()->GetSecurity();
         if ((itr->second->isGameMaster() || (itr_sec > SEC_PLAYER && itr_sec <= sWorld.getConfig(CONFIG_GM_LEVEL_IN_GM_LIST))) &&

src/game/Level3.cpp

@@ -5361,6 +5361,8 @@ bool ChatHandler::HandleResetAllCommand(const char * args)
     }
 
     CharacterDatabase.PExecute("UPDATE characters SET at_login = at_login | '%u' WHERE (at_login & '%u') = '0'",atLogin,atLogin);
+    
+    ObjectAccessor::Guard guard(*HashMapHolder<Player>::GetLock());
     HashMapHolder<Player>::MapType const& plist = ObjectAccessor::Instance().GetPlayers();
     for (HashMapHolder<Player>::MapType::const_iterator itr = plist.begin(); itr != plist.end(); ++itr)
         itr->second->SetAtLoginFlag(atLogin);

src/game/MiscHandler.cpp

@@ -220,7 +220,7 @@ void WorldSession::HandleWhoOpcode( WorldPacket & recv_data )
     data << clientcount;                                    // clientcount place holder
     data << clientcount;                                    // clientcount place holder
 
-    //TODO: Guard Player map
+    ObjectAccessor::Guard guard(*HashMapHolder<Player>::GetLock());
     HashMapHolder<Player>::MapType& m = ObjectAccessor::Instance().GetPlayers();
     for (HashMapHolder<Player>::MapType::const_iterator itr = m.begin(); itr != m.end(); ++itr)
     {

src/game/ObjectAccessor.cpp

@@ -45,7 +45,10 @@
 INSTANTIATE_SINGLETON_2(ObjectAccessor, CLASS_LOCK);
 INSTANTIATE_CLASS_MUTEX(ObjectAccessor, ACE_Thread_Mutex);
 
-ObjectAccessor::ObjectAccessor() {}
+ObjectAccessor::ObjectAccessor()
+{
+}
+
 ObjectAccessor::~ObjectAccessor()
 {
     for (Player2CorpsesMapType::const_iterator itr = i_player2corpse.begin(); itr != i_player2corpse.end(); ++itr)
@@ -55,8 +58,7 @@ ObjectAccessor::~ObjectAccessor()
     }
 }
 
-Creature*
+Creature* ObjectAccessor::GetCreatureOrPetOrVehicle(WorldObject const& u, uint64 guid)
-ObjectAccessor::GetCreatureOrPetOrVehicle(WorldObject const &u, uint64 guid)
 {
     if (IS_PLAYER_GUID(guid))
         return NULL;
@@ -67,30 +69,19 @@ ObjectAccessor::GetCreatureOrPetOrVehicle(WorldObject const &u, uint64 guid)
     return u.IsInWorld() ? u.GetMap()->GetCreature(guid) : NULL;
 }
 
-/*
+Corpse* ObjectAccessor::GetCorpse(WorldObject const& u, uint64 guid)
-Unit*
-ObjectAccessor::GetUnit(WorldObject const &u, uint64 guid)
-{
-    if(!guid)
-        return NULL;
-
-    if(IS_PLAYER_GUID(guid))
-        return FindPlayer(guid);
-
-    return GetCreatureOrPetOrVehicle(u, guid);
-}
-*/
-
-Corpse*
-ObjectAccessor::GetCorpse(WorldObject const &u, uint64 guid)
 {
     Corpse* ret = GetObjectInWorld(guid, (Corpse*)NULL);
+
     if (!ret)
         return NULL;
+
     if (ret->GetMapId() != u.GetMapId())
         return NULL;
+
     if (ret->GetInstanceId() != u.GetInstanceId())
         return NULL;
+
     return ret;
 }
 
@@ -107,10 +98,8 @@ WorldObject* ObjectAccessor::GetWorldObject(WorldObject const &p, uint64 guid)
         case HIGHGUID_TRANSPORT:     return NULL;
         case HIGHGUID_CORPSE:        return GetCorpse(p,guid);
         case HIGHGUID_MO_TRANSPORT:  return NULL;
-        default: break;
+        default:                     return NULL;
     }
-
-    return NULL;
 }
 
 Object* ObjectAccessor::GetObjectByTypeMask(WorldObject const& p, uint64 guid, uint32 typemask)
@@ -151,8 +140,7 @@ Object* ObjectAccessor::GetObjectByTypeMask(WorldObject const &p, uint64 guid, u
     return NULL;
 }
 
-Player*
+Player* ObjectAccessor::FindPlayer(uint64 guid)
-ObjectAccessor::FindPlayer(uint64 guid)
 {
     Player* plr = GetObjectInWorld(guid, (Player*)NULL);
     if (!plr || !plr->IsInWorld())
@@ -161,50 +149,44 @@ ObjectAccessor::FindPlayer(uint64 guid)
     return plr;
 }
 
-Player*
+Player* ObjectAccessor::FindPlayerByName(const char* name)
-ObjectAccessor::FindPlayerByName(const char *name)
 {
-    //TODO: Player Guard
     Guard guard(*HashMapHolder<Player>::GetLock());
     HashMapHolder<Player>::MapType& m = HashMapHolder<Player>::GetContainer();
-    HashMapHolder<Player>::MapType::iterator iter = m.begin();
+    for (HashMapHolder<Player>::MapType::iterator iter = m.begin(); iter != m.end(); ++iter)
-    for (; iter != m.end(); ++iter)
+        if (iter->second->IsInWorld() && strcmp(name, iter->second->GetName()) == 0)
-        if(iter->second->IsInWorld() && ( ::strcmp(name, iter->second->GetName()) == 0 ))
             return iter->second;
+
     return NULL;
 }
 
-void
+void ObjectAccessor::SaveAllPlayers()
-ObjectAccessor::SaveAllPlayers()
 {
     Guard guard(*HashMapHolder<Player>::GetLock());
     HashMapHolder<Player>::MapType& m = HashMapHolder<Player>::GetContainer();
-    HashMapHolder<Player>::MapType::iterator itr = m.begin();
+    for (HashMapHolder<Player>::MapType::iterator itr = m.begin(); itr != m.end(); ++itr)
-    for (; itr != m.end(); ++itr)
         itr->second->SaveToDB();
 }
 
-Pet*
+Pet* ObjectAccessor::GetPet(uint64 guid)
-ObjectAccessor::GetPet(uint64 guid)
 {
     return GetObjectInWorld(guid, (Pet*)NULL);
 }
 
-Corpse*
+Corpse* ObjectAccessor::GetCorpseForPlayerGUID(uint64 guid)
-ObjectAccessor::GetCorpseForPlayerGUID(uint64 guid)
 {
     Guard guard(i_corpseGuard);
 
     Player2CorpsesMapType::iterator iter = i_player2corpse.find(guid);
-    if( iter == i_player2corpse.end() ) return NULL;
+    if (iter == i_player2corpse.end())
+        return NULL;
 
     assert(iter->second->GetType() != CORPSE_BONES);
 
     return iter->second;
 }
 
-void
+void ObjectAccessor::RemoveCorpse(Corpse* corpse)
-ObjectAccessor::RemoveCorpse(Corpse *corpse)
 {
     assert(corpse && corpse->GetType() != CORPSE_BONES);
 
@@ -213,9 +195,12 @@ ObjectAccessor::RemoveCorpse(Corpse *corpse)
     else
         corpse->RemoveFromWorld();
 
+    // Critical section
+    {
         Guard guard(i_corpseGuard);
+
         Player2CorpsesMapType::iterator iter = i_player2corpse.find(corpse->GetOwnerGUID());
-    if( iter == i_player2corpse.end() ) // i do not know when it happens but it happens
+        if (iter == i_player2corpse.end()) // TODO: Fix this
             return;
 
         // build mapid*cellid -> guid_set map
@@ -226,13 +211,16 @@ ObjectAccessor::RemoveCorpse(Corpse *corpse)
 
         i_player2corpse.erase(iter);
     }
+}
 
-void
+void ObjectAccessor::AddCorpse(Corpse* corpse)
-ObjectAccessor::AddCorpse(Corpse *corpse)
 {
     assert(corpse && corpse->GetType() != CORPSE_BONES);
 
+    // Critical section
+    {
         Guard guard(i_corpseGuard);
+
         assert(i_player2corpse.find(corpse->GetOwnerGUID()) == i_player2corpse.end());
         i_player2corpse[corpse->GetOwnerGUID()] = corpse;
 
@@ -242,37 +230,36 @@ ObjectAccessor::AddCorpse(Corpse *corpse)
 
         objmgr.AddCorpseCellData(corpse->GetMapId(), cell_id, corpse->GetOwnerGUID(), corpse->GetInstanceId());
     }
+}
 
-void
+void ObjectAccessor::AddCorpsesToGrid(GridPair const& gridpair, GridType& grid, Map* map)
-ObjectAccessor::AddCorpsesToGrid(GridPair const& gridpair,GridType& grid,Map* map)
 {
     Guard guard(i_corpseGuard);
+
     for (Player2CorpsesMapType::iterator iter = i_player2corpse.begin(); iter != i_player2corpse.end(); ++iter)
+    {
         if (iter->second->GetGrid() == gridpair)
         {
             // verify, if the corpse in our instance (add only corpses which are)
             if (map->Instanceable())
             {
                 if (iter->second->GetInstanceId() == map->GetInstanceId())
-            {
                     grid.AddWorldObject(iter->second);
             }
-        }
             else
-        {
                 grid.AddWorldObject(iter->second);
         }
     }
 }
 
-Corpse*
+Corpse* ObjectAccessor::ConvertCorpseForPlayer(uint64 player_guid, bool insignia)
-ObjectAccessor::ConvertCorpseForPlayer(uint64 player_guid, bool insignia)
 {
     Corpse* corpse = GetCorpseForPlayerGUID(player_guid);
     if(!corpse)
     {
         //in fact this function is called from several places
         //even when player doesn't have a corpse, not an error
+        // TODO: really, now...
         //sLog.outError("Try remove corpse that not in map for GUID %ul", player_guid);
         return NULL;
     }
@@ -314,7 +301,7 @@ ObjectAccessor::ConvertCorpseForPlayer(uint64 player_guid, bool insignia)
 
         bones->SetGrid(corpse->GetGrid());
         // bones->m_time = m_time;                              // don't overwrite time
-        // bones->m_inWorld = m_inWorld;                        // don't overwrite world state
+        // bones->m_inWorld = m_inWorld;                        // don't overwrite in-world state
         // bones->m_type = m_type;                              // don't overwrite type
         bones->Relocate(corpse->GetPositionX(), corpse->GetPositionY(), corpse->GetPositionZ(), corpse->GetOrientation());
         bones->SetPhaseMask(corpse->GetPhaseMask(), false);
@@ -338,12 +325,14 @@ ObjectAccessor::ConvertCorpseForPlayer(uint64 player_guid, bool insignia)
     return bones;
 }
 
-void
+void ObjectAccessor::Update(uint32 diff)
-ObjectAccessor::Update(uint32 diff)
 {
     UpdateDataMapType update_players;
+
+    // Critical section
     {
         Guard guard(i_updateGuard);
+
         while (!i_objects.empty())
         {
             Object* obj = *i_objects.begin();
@@ -362,28 +351,7 @@ ObjectAccessor::Update(uint32 diff)
     }
 }
 
-
+/// Define the static members of HashMapHolder
-/*void ObjectAccessor::UpdateVisibilityForPlayer( Player* player )
-{
-    WorldObject const* viewPoint = player->GetViewPoint();
-    Map* m = player->GetMap();
-
-    CellPair p = Trinity::ComputeCellPair(player->GetPositionX(), player->GetPositionY());
-    Cell cell(p);
-
-    m->UpdatePlayerVisibility(player, cell, p);
-    if (player!=viewPoint)
-    {
-        CellPair pView(Trinity::ComputeCellPair(viewPoint->GetPositionX(), viewPoint->GetPositionY()));
-        Cell cellView(pView);
-
-        m->UpdateObjectsVisibilityFor(player, cellView, pView);
-    }
-    else
-        m->UpdateObjectsVisibilityFor(player, cell, p);
-}*/
-
-/// Define the static member of HashMapHolder
 
 template <class T> UNORDERED_MAP< uint64, T* > HashMapHolder<T>::m_objectMap;
 template <class T> ACE_Thread_Mutex HashMapHolder<T>::i_lock;

src/game/ObjectAccessor.h

@@ -74,6 +74,7 @@ class HashMapHolder
         static MapType& GetContainer() { return m_objectMap; }
 
         static LockType* GetLock() { return &i_lock; }
+
     private:
 
         //Non instanceable only static
@@ -85,7 +86,6 @@ class HashMapHolder
 
 class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassLevelLockable<ObjectAccessor, ACE_Thread_Mutex> >
 {
-
     friend class Trinity::OperatorNew<ObjectAccessor>;
     ObjectAccessor();
     ~ObjectAccessor();
@@ -93,6 +93,7 @@ class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassL
     ObjectAccessor& operator=(const ObjectAccessor&);
 
     public:
+
         typedef UNORDERED_MAP<uint64, Corpse*> Player2CorpsesMapType;
         typedef UNORDERED_MAP<Player*, UpdateData>::value_type UpdateDataValueType;
 
@@ -134,14 +135,16 @@ class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassL
 
                 return u;
             }
+
             // Other object types than player are unloaded while out of world
-            return GetObjectInWorld(guid, ((Unit*)NULL));
+            return GetObjectInWorld(guid, (Unit*)NULL);
         }
 
         template<class T> static T* GetObjectInWorld(uint32 mapid, float x, float y, uint64 guid, T* /*fake*/)
         {
             T* obj = HashMapHolder<T>::Find(guid);
-            if(!obj || obj->GetMapId() != mapid) return NULL;
+            if (!obj || obj->GetMapId() != mapid)
+                return NULL;
 
             CellPair p = Trinity::ComputeCellPair(x, y);
             if (p.x_coord >= TOTAL_NUMBER_OF_CELLS_PER_MAP || p.y_coord >= TOTAL_NUMBER_OF_CELLS_PER_MAP )
@@ -160,8 +163,10 @@ class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassL
             int32 dx = int32(p.x_coord) - int32(q.x_coord);
             int32 dy = int32(p.y_coord) - int32(q.y_coord);
 
-            if (dx > -2 && dx < 2 && dy > -2 && dy < 2) return obj;
+            if (dx > -2 && dx < 2 && dy > -2 && dy < 2)
-            else return NULL;
+                return obj;
+            else
+                return NULL;
         }
 
         static WorldObject* GetWorldObject(WorldObject const&, uint64);
@@ -177,16 +182,19 @@ class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassL
 
         Player* FindPlayerByName(const char* name) ;
 
+        // when using this, you must use the hashmapholder's lock
         HashMapHolder<Player>::MapType& GetPlayers()
         {
             return HashMapHolder<Player>::GetContainer();
         }
 
+        // when using this, you must use the hashmapholder's lock
         HashMapHolder<Creature>::MapType& GetCreatures()
         {
             return HashMapHolder<Creature>::GetContainer();
         }
 
+        // when using this, you must use the hashmapholder's lock
         HashMapHolder<GameObject>::MapType& GetGameObjects()
         {
             return HashMapHolder<GameObject>::GetContainer();
@@ -205,9 +213,7 @@ class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassL
         void RemoveObject(Player* pl)
         {
             HashMapHolder<Player>::Remove(pl);
-
+            RemoveUpdateObject((Object*)pl);
-            Guard guard(i_updateGuard);
-            i_objects.erase((Object *)pl);
         }
 
         void SaveAllPlayers();
@@ -232,21 +238,20 @@ class ObjectAccessor : public Trinity::Singleton<ObjectAccessor, Trinity::ClassL
         void AddCorpsesToGrid(GridPair const& gridpair, GridType& grid, Map* map);
         Corpse* ConvertCorpseForPlayer(uint64 player_guid, bool insignia = false);
 
-        //static void UpdateVisibilityForPlayer(Player* player);
+        typedef ACE_Thread_Mutex LockType;
+        typedef Trinity::GeneralLock<LockType> Guard;
+
     private:
 
         Player2CorpsesMapType i_player2corpse;
 
-        typedef ACE_Thread_Mutex LockType;
-        typedef Trinity::GeneralLock<LockType > Guard;
-
         static void _buildChangeObjectForPlayer(WorldObject*, UpdateDataMapType&);
         static void _buildPacket(Player*, Object*, UpdateDataMapType&);
-        void _update(void);
+        void _update();
+
         std::set<Object*> i_objects;
-        LockType i_playerGuard;
+
         LockType i_updateGuard;
         LockType i_corpseGuard;
-        LockType i_petGuard;
 };
 #endif