Core/Instances: Recycle instance IDs. Should fix instance ID shifting and related issues with instance binding.

fixes #736

src/server/game/Battlegrounds/BattlegroundMgr.cpp

@@ -110,6 +110,10 @@ void BattlegroundMgr::Update(uint32 diff)
                 m_Battlegrounds[i].erase(itr);
                 if (!m_ClientBattlegroundIds[i][bg->GetBracketId()].empty())
                     m_ClientBattlegroundIds[i][bg->GetBracketId()].erase(bg->GetClientInstanceID());
+
+                // Free up the instance id and allow it to be reused
+                sMapMgr->FreeInstanceId(bg->GetInstanceID());
+
                 delete bg;
             }
         }
@@ -603,7 +607,10 @@ Battleground * BattlegroundMgr::CreateNewBattleground(BattlegroundTypeId bgTypeI
     bg->SetBracket(bracketEntry);
 
     // generate a new instance id
-    bg->SetInstanceID(sMapMgr->GenerateInstanceId()); // set instance id
+    uint32 instanceId = sMapMgr->GenerateInstanceId();
+    // set instance id
+    bg->SetInstanceID(instanceId);
+
     bg->SetClientInstanceID(CreateClientVisibleInstanceId(isRandom ? BATTLEGROUND_RB : bgTypeId, bracketEntry->GetBracketId()));
 
     // reset the new bg (set status to status_wait_queue from status_none)

src/server/game/Instances/InstanceSaveMgr.cpp

@@ -257,13 +257,10 @@ void InstanceSaveManager::_DelHelper(const char *fields, const char *table, cons
     }
 }
 
-void InstanceSaveManager::CleanupAndPackInstances()
+void InstanceSaveManager::LoadInstances()
 {
     uint32 oldMSTime = getMSTime();
 
-    // load reset times and clean expired instances
-    sInstanceSaveMgr->LoadResetTimes();
-
     // Delete invalid character_instance and group_instance references
     CharacterDatabase.DirectExecute("DELETE ci.* FROM character_instance AS ci LEFT JOIN characters AS c ON ci.guid = c.guid WHERE c.guid IS NULL");
     CharacterDatabase.DirectExecute("DELETE gi.* FROM group_instance     AS gi LEFT JOIN groups     AS g ON gi.guid = g.guid WHERE g.guid IS NULL");
@@ -281,26 +278,13 @@ void InstanceSaveManager::CleanupAndPackInstances()
     CharacterDatabase.DirectExecute("UPDATE corpse     AS tmp LEFT JOIN instance ON tmp.instance    = instance.id SET tmp.instance    = 0 WHERE tmp.instance    > 0 AND instance.id IS NULL");
     CharacterDatabase.DirectExecute("UPDATE characters AS tmp LEFT JOIN instance ON tmp.instance_id = instance.id SET tmp.instance_id = 0 WHERE tmp.instance_id > 0 AND instance.id IS NULL");
 
-    // Create new index
+    // Initialize instance id storage (Needs to be done after the trash has been clean out)
-    CharacterDatabase.DirectExecute("ALTER TABLE instance ADD newid INT UNSIGNED AUTO_INCREMENT, ADD INDEX(newid)");
+    sMapMgr->InitInstanceIds();
 
-    // Update old ids
+    // Load reset times and clean expired instances
-    CharacterDatabase.DirectExecute("UPDATE account_instance_times  AS tmp LEFT JOIN instance ON tmp.instanceId  = instance.id SET tmp.instanceId  = instance.newid WHERE tmp.instanceId  > 0 AND instance.newid IS NOT NULL"); // can be null and must not be cleaned! its an "already reset" but still limited case
+    sInstanceSaveMgr->LoadResetTimes();
-    CharacterDatabase.DirectExecute("UPDATE corpse                  AS tmp LEFT JOIN instance ON tmp.instance    = instance.id SET tmp.instance    = instance.newid WHERE tmp.instance    > 0");
-    CharacterDatabase.DirectExecute("UPDATE character_instance      AS tmp LEFT JOIN instance ON tmp.instance    = instance.id SET tmp.instance    = instance.newid WHERE tmp.instance    > 0");
-    CharacterDatabase.DirectExecute("UPDATE group_instance          AS tmp LEFT JOIN instance ON tmp.instance    = instance.id SET tmp.instance    = instance.newid WHERE tmp.instance    > 0");
-    CharacterDatabase.DirectExecute("UPDATE characters              AS tmp LEFT JOIN instance ON tmp.instance_id = instance.id SET tmp.instance_id = instance.newid WHERE tmp.instance_id > 0");
-    CharacterDatabase.DirectExecute("UPDATE creature_respawn        AS tmp LEFT JOIN instance ON tmp.instance    = instance.id SET tmp.instance    = instance.newid WHERE tmp.instance    > 0");
-    CharacterDatabase.DirectExecute("UPDATE gameobject_respawn      AS tmp LEFT JOIN instance ON tmp.instance    = instance.id SET tmp.instance    = instance.newid WHERE tmp.instance    > 0");
 
-    // Update instance too
+    sLog->outString(">> Loaded instances in %u ms", GetMSTimeDiffToNow(oldMSTime));
-    CharacterDatabase.DirectExecute("UPDATE instance SET id = newid");
-
-    // Finally drop the no longer needed column
-    CharacterDatabase.DirectExecute("ALTER TABLE instance DROP COLUMN newid");
-
-    // Bake some cookies for click
-    sLog->outString(">> Cleaned up and packed instances in %u ms", GetMSTimeDiffToNow(oldMSTime));
     sLog->outString();
 }
 
@@ -322,20 +306,30 @@ void InstanceSaveManager::LoadResetTimes()
     typedef std::multimap<uint32 /*PAIR32(map,difficulty)*/, uint32 /*instanceid*/ > ResetTimeMapDiffInstances;
     ResetTimeMapDiffInstances mapDiffResetInstances;
 
-    QueryResult result = CharacterDatabase.Query("SELECT id, map, difficulty, resettime FROM instance WHERE resettime > 0");
+    QueryResult result = CharacterDatabase.Query("SELECT id, map, difficulty, resettime FROM instance ORDER BY id ASC");
     if (result)
     {
         do
         {
             Field* fields = result->Fetch();
 
+            uint32 instanceId = fields[0].GetUInt32();
+
+            // Instances are pulled in ascending order from db and nextInstanceId is initialized with 1,
+            // so if the instance id is used, increment until we find the first unused one for a potential new instance
+            if (sMapMgr->GetNextInstanceId() == instanceId)
+                sMapMgr->SetNextInstanceId(instanceId + 1);
+
+            // Mark instance id as being used
+            sMapMgr->RegisterInstanceId(instanceId);
+
             if (time_t resettime = time_t(fields[3].GetUInt32()))
             {
-                uint32 id = fields[0].GetUInt32();
                 uint32 mapid = fields[1].GetUInt16();
                 uint32 difficulty = fields[2].GetUInt8();
-                instResetTime[id] = ResetTimeMapDiffType(MAKE_PAIR32(mapid, difficulty), resettime);
+
-                mapDiffResetInstances.insert(ResetTimeMapDiffInstances::value_type(MAKE_PAIR32(mapid, difficulty), id));
+                instResetTime[instanceId] = ResetTimeMapDiffType(MAKE_PAIR32(mapid, difficulty), resettime);
+                mapDiffResetInstances.insert(ResetTimeMapDiffInstances::value_type(MAKE_PAIR32(mapid, difficulty), instanceId));
             }
         }
         while (result->NextRow());
@@ -564,6 +558,9 @@ void InstanceSaveManager::_ResetInstance(uint32 mapid, uint32 instanceId)
         ((InstanceMap*)iMap)->Reset(INSTANCE_RESET_RESPAWN_DELAY);
     else
         sObjectMgr->DeleteRespawnTimeForInstance(instanceId);   // even if map is not loaded
+
+    // Free up the instance id and allow it to be reused
+    sMapMgr->FreeInstanceId(iMap->GetInstanceId());
 }
 
 void InstanceSaveManager::_ResetOrWarnAll(uint32 mapid, Difficulty difficulty, bool warn, time_t resetTime)

src/server/game/Instances/InstanceSaveMgr.h

@@ -140,7 +140,7 @@ class InstanceSaveManager
         };
         typedef std::multimap<time_t /*resetTime*/, InstResetEvent> ResetTimeQueue;
 
-        void CleanupAndPackInstances();
+        void LoadInstances();
 
         void LoadResetTimes();
         time_t GetResetTimeFor(uint32 mapid, Difficulty d) const

src/server/game/Maps/MapManager.cpp

@@ -62,8 +62,6 @@ void MapManager::Initialize()
     // Start mtmaps if needed.
     if (num_threads > 0 && m_updater.activate(num_threads) == -1)
         abort();
-
-    InitMaxInstanceId();
 }
 
 void MapManager::InitializeVisibilityDistanceInfo()
@@ -328,15 +326,6 @@ void MapManager::UnloadAll()
     Map::DeleteStateMachine();
 }
 
-void MapManager::InitMaxInstanceId()
-{
-    i_MaxInstanceId = 0;
-
-    QueryResult result = CharacterDatabase.Query("SELECT MAX(id) FROM instance");
-    if (result)
-        i_MaxInstanceId = result->Fetch()[0].GetUInt32();
-}
-
 uint32 MapManager::GetNumInstances()
 {
     ACE_GUARD_RETURN(ACE_Thread_Mutex, Guard, Lock, NULL);
@@ -371,3 +360,73 @@ uint32 MapManager::GetNumPlayersInInstances()
     }
     return ret;
 }
+
+void MapManager::InitInstanceIds()
+{
+    _nextInstanceId = 1;
+
+    QueryResult result = CharacterDatabase.Query("SELECT MAX(id) FROM instance");
+    if (result)
+    {
+        uint32 maxId = (*result)[0].GetUInt32();
+
+        // Resize to multiples of 32 (vector<bool> allocates memory the same way)
+        _instanceIds.resize(floor(maxId / 32.0f) * 32 + (maxId % 32 > 0 ? 32 : 0));
+    }
+}
+
+void MapManager::RegisterInstanceId(uint32 instanceId)
+{
+    // Allocation and sizing was done in InitInstanceIds()
+    _instanceIds[instanceId] = true;
+}
+
+uint32 MapManager::GenerateInstanceId()
+{
+    uint32 newInstanceId = _nextInstanceId;
+
+    // Find the lowest available id starting from the current NextInstanceId (which should be the lowest according to the logic in FreeInstanceId()
+    for (uint32 i = ++_nextInstanceId; i < 0xFFFFFFFF; ++i)
+    {
+        if ((i < _instanceIds.size() && !_instanceIds[i]) || i >= _instanceIds.size())
+        {
+            _nextInstanceId = i;
+            break;
+        }
+    }
+
+    if (newInstanceId == _nextInstanceId)
+    {
+        sLog->outError("Instance ID overflow!! Can't continue, shutting down server. ");
+        World::StopNow(ERROR_EXIT_CODE);
+    }
+
+    // Allocate space if necessary
+    if (newInstanceId >= uint32(_instanceIds.size()))
+    {
+        // DEBUG CODE - TO BE REMOVED OR ENABLED DEPENDING ON THIS ASSERT TRIGGERING
+        ASSERT(_instanceIds.size() == _instanceIds.capacity());
+
+        /*
+        if (_instanceIds.size() < _instanceIds.capacity())
+        {
+            _instanceIds.resize(_instanceIds.capacity());
+        }
+        else
+        */
+            _instanceIds.resize(floor(newInstanceId / 32.0f) * 32 + (newInstanceId % 32 > 0 ? 32 : 0));
+    }
+
+    _instanceIds[newInstanceId] = true;
+
+    return newInstanceId;
+}
+
+void MapManager::FreeInstanceId(uint32 instanceId)
+{
+    // If freed instance id is lower than the next id available for new instances, use the freed one instead
+    if (instanceId < _nextInstanceId)
+        SetNextInstanceId(instanceId);
+
+    _instanceIds[instanceId] = false;
+}

src/server/game/Maps/MapManager.h

@@ -34,6 +34,7 @@ class MapManager
 {
     friend class ACE_Singleton<MapManager, ACE_Thread_Mutex>;
     typedef UNORDERED_MAP<uint32, Map*> MapMapType;
+    typedef std::vector<bool> InstanceIds;
 
     public:
 
@@ -132,14 +133,21 @@ class MapManager
         TransportMap m_TransportsByMap;
 
         bool CanPlayerEnter(uint32 mapid, Player* player, bool loginCheck = false);
-        uint32 GenerateInstanceId() { return ++i_MaxInstanceId; }
-        void InitMaxInstanceId();
         void InitializeVisibilityDistanceInfo();
 
         /* statistics */
         uint32 GetNumInstances();
         uint32 GetNumPlayersInInstances();
 
+        // Instance ID management
+        void InitInstanceIds();
+        uint32 GenerateInstanceId();
+        void RegisterInstanceId(uint32 instanceId);
+        void FreeInstanceId(uint32 instanceId);
+
+        uint32 GetNextInstanceId() { return _nextInstanceId; };
+        void SetNextInstanceId(uint32 nextInstanceId) { _nextInstanceId = nextInstanceId; };
+
     private:
         // debugging code, should be deleted some day
         void checkAndCorrectGridStatesArray();              // just for debugging to find some memory overwrites
@@ -164,7 +172,8 @@ class MapManager
         MapMapType i_maps;
         IntervalTimer i_timer;
 
-        uint32 i_MaxInstanceId;
+        InstanceIds _instanceIds;
+        uint32 _nextInstanceId;
         MapUpdater m_updater;
 };
 #define sMapMgr ACE_Singleton<MapManager, ACE_Thread_Mutex>::instance()

src/server/game/World/World.cpp

@@ -1253,9 +1253,9 @@ void World::SetInitialWorldSettings()
     sLog->outString("Loading SkillLineAbilityMultiMap Data...");
     sSpellMgr->LoadSkillLineAbilityMap();
 
-    ///- Clean up and pack instances
+    // Must be called before `creature_respawn`/`gameobject_respawn` tables
-    sLog->outString("Cleaning up and packing instances...");
+    sLog->outString("Loading instances...");
-    sInstanceSaveMgr->CleanupAndPackInstances();                // must be called before `creature_respawn`/`gameobject_respawn` tables
+    sInstanceSaveMgr->LoadInstances();
 
     sLog->outString("Loading Localization strings...");
     uint32 oldMSTime = getMSTime();