Memory leak fix: Make chunks unload properly (#1406)

* Fix chunk unload and cleanup logic, fixes #1347

* Applying formatting to code I edited 😝

Minecraft.Client/MultiPlayerChunkCache.cpp

@@ -139,19 +139,26 @@ bool MultiPlayerChunkCache::reallyHasChunk(int x, int z)
 	return hasData[idx];
 }
 
-void MultiPlayerChunkCache::drop(int x, int z)
+void MultiPlayerChunkCache::drop(const int x, const int z)
 {
-	// 4J Stu - We do want to drop any entities in the chunks, especially for the case when a player is dead as they will
+	const int ix = x + XZOFFSET;
-	// not get the RemoveEntity packet if an entity is removed.
+	const int iz = z + XZOFFSET;
-	LevelChunk *chunk = getChunk(x, z);
+	if ((ix < 0) || (ix >= XZSIZE)) return;
-	if (!chunk->isEmpty())
+	if ((iz < 0) || (iz >= XZSIZE)) return;
+	const int idx = ix * XZSIZE + iz;
+	LevelChunk* chunk = cache[idx];
+
+	if (chunk != nullptr && !chunk->isEmpty())
 	{
-		// Added parameter here specifies that we don't want to delete tile entities, as they won't get recreated unless they've got update packets
+		// Unload chunk but keep tile entities
-		// The tile entities are in general only created on the client by virtue of the chunk rebuild
 		chunk->unload(false);
 
-		// 4J - We just want to clear out the entities in the chunk, but everything else should be valid
+		const auto it = std::find(loadedChunkList.begin(), loadedChunkList.end(), chunk);
-		chunk->loaded = true;
+		if (it != loadedChunkList.end()) loadedChunkList.erase(it);
+
+		cache[idx] = nullptr;
+		hasData[idx] = false;
+		chunk->loaded = false;
 	}
 }
 

Minecraft.Client/PlayerChunkMap.cpp

@@ -792,6 +792,14 @@ void PlayerChunkMap::setRadius(int newRadius)
 				int xc = static_cast<int>(player->x) >> 4;
 				int zc = static_cast<int>(player->z) >> 4;
 
+				for (auto it = addRequests.begin(); it != addRequests.end(); )
+				{
+					if (it->player == player)
+						it = addRequests.erase(it);
+					else
+						++it;
+				}
+
 				for (int x = xc - newRadius; x <= xc + newRadius; x++)
 					for (int z = zc - newRadius; z <= zc + newRadius; z++)
 					{
@@ -801,9 +809,26 @@ void PlayerChunkMap::setRadius(int newRadius)
 							getChunkAndAddPlayer(x, z, player);
 						}
 					}
+
+				// Remove chunks that are outside the new radius
+				for (int x = xc - radius; x <= xc + radius; x++)
+				{
+					for (int z = zc - radius; z <= zc + radius; z++)
+					{
+						if (x < xc - newRadius || x > xc + newRadius || z < zc - newRadius || z > zc + newRadius)
+						{
+							getChunkAndRemovePlayer(x, z, player);
+						}
+					}
+				}
 			}
 		}
 
+		if (newRadius < radius)
+		{
+			level->cache->dropAll();
+		}
+
 		assert(radius <= MAX_VIEW_DISTANCE);
 		assert(radius >= MIN_VIEW_DISTANCE);
 		this->radius = newRadius;

Minecraft.Client/PlayerList.cpp

@@ -1690,7 +1690,16 @@ bool PlayerList::isXuidBanned(PlayerUID xuid)
 }
 
 // AP added for Vita so the range can be increased once the level starts
-void PlayerList::setViewDistance(int newViewDistance)
+void PlayerList::setViewDistance(const int newViewDistance)
 {
 	viewDistance = newViewDistance;
+
+	for (size_t i = 0; i < server->levels.length; i++)
+	{
+		ServerLevel* level = server->levels[i];
+		if (level != nullptr)
+		{
+			level->getChunkMap()->setRadius(newViewDistance);
+		}
+	}
 }

Minecraft.Client/ServerChunkCache.cpp

@@ -80,54 +80,31 @@ vector<LevelChunk *> *ServerChunkCache::getLoadedChunkList()
 	return &m_loadedChunkList;
 }
 
-void ServerChunkCache::drop(int x, int z)
+void ServerChunkCache::drop(const int x, const int z)
 {
-	// 4J - we're not dropping things anymore now that we have a fixed sized cache
+	const int ix = x + XZOFFSET;
-#ifdef _LARGE_WORLDS
+	const int iz = z + XZOFFSET;
+	if ((ix < 0) || (ix >= XZSIZE)) return;
+	if ((iz < 0) || (iz >= XZSIZE)) return;
+	const int idx = ix * XZSIZE + iz;
+	LevelChunk* chunk = cache[idx];
 
-	bool canDrop = false;
+	if (chunk != nullptr)
-//	if (level->dimension->mayRespawn())
-//	{
-//		Pos *spawnPos = level->getSharedSpawnPos();
-//		int xd = x * 16 + 8 - spawnPos->x;
-//		int zd = z * 16 + 8 - spawnPos->z;
-//		delete spawnPos;
-//		int r = 128;
-//		if (xd < -r || xd > r || zd < -r || zd > r)
-//		{
-//			canDrop = true;
-//}
-//	}
-//	else
 	{
-		canDrop = true;
+		const auto it = std::find(m_loadedChunkList.begin(), m_loadedChunkList.end(), chunk);
-	}
+		if (it != m_loadedChunkList.end()) m_loadedChunkList.erase(it);
-	if(canDrop)
-	{
-		int ix = x + XZOFFSET;
-		int iz = z + XZOFFSET;
-		// Check we're in range of the stored level
-		if( ( ix < 0 ) || ( ix >= XZSIZE ) ) return;
-		if( ( iz < 0 ) || ( iz >= XZSIZE ) ) return;
-		int idx = ix * XZSIZE + iz;
-		LevelChunk *chunk = cache[idx];
 
-		if(chunk)
+		cache[idx] = nullptr;
-		{
+		chunk->loaded = false;
-			m_toDrop.push_back(chunk);
-		}
 	}
-#endif
 }
 
 void ServerChunkCache::dropAll()
 {
-#ifdef _LARGE_WORLDS
 	for (LevelChunk *chunk : m_loadedChunkList)
 	{
 		drop(chunk->x, chunk->z);
-}
+	}
-#endif
 }
 
 // 4J - this is the original (and virtual) interface to create
@@ -957,6 +934,10 @@ bool ServerChunkCache::tick()
 						m_unloadedCache[idx] = chunk;
 						cache[idx] = nullptr;
 					}
+					else
+					{
+						continue;
+					}
 				}
 				m_toDrop.pop_front();
 			}