Plain pointer cChunkPtr finishing touches; removed cChunk's critical sections

git-svn-id: http://mc-server.googlecode.com/svn/trunk@325 0a769ca7-a7f5-676a-18bf-c427514a06d6

source/cChunk.cpp

@@ -97,7 +97,6 @@ cChunk::~cChunk()
 {
 	// LOGINFO("### delete cChunk() (%i, %i) from %p, thread 0x%x ###", m_PosX, m_PosZ, this, GetCurrentThreadId() );
 	
-	cCSLock Lock(m_CSEntities);
 	for (cBlockEntityList::iterator itr = m_BlockEntities.begin(); itr != m_BlockEntities.end(); ++itr)
 	{
 		delete *itr;
@@ -136,7 +135,6 @@ void cChunk::SetValid(bool a_SendToClients)
 		return;
 	}
 	
-	cCSLock Lock(m_CSClients);
 	if (m_LoadedByClient.empty())
 	{
 		return;
@@ -166,7 +164,6 @@ void cChunk::SetValid(bool a_SendToClients)
 
 bool cChunk::CanUnload(void)
 {
-	cCSLock Lock(m_CSClients);
 	return m_LoadedByClient.empty() && !m_IsDirty;
 }
 
@@ -210,7 +207,6 @@ void cChunk::GetAllData(cChunkDataCallback * a_Callback)
 {
 	a_Callback->BlockData(m_BlockData);
 	
-	cCSLock Lock(m_CSEntities);
 	for (cEntityList::iterator itr = m_Entities.begin(); itr != m_Entities.end(); ++itr)
 	{
 		a_Callback->Entity(*itr);
@@ -231,7 +227,6 @@ void cChunk::SetAllData(const char * a_BlockData, cEntityList & a_Entities, cBlo
 	memcpy(m_BlockData, a_BlockData, sizeof(m_BlockData));
 
 	// Clear the internal entities:
-	cCSLock Lock(m_CSEntities);
 	for (cEntityList::iterator itr = m_Entities.begin(); itr != m_Entities.end(); ++itr)
 	{
 		if ((*itr)->GetEntityType() == cEntity::E_PLAYER)
@@ -1053,8 +1048,6 @@ void cChunk::UseBlockEntity(cPlayer * a_Player, int a_X, int a_Y, int a_Z)
 
 void cChunk::CollectPickupsByPlayer(cPlayer * a_Player)
 {
-	cCSLock Lock(m_CSEntities);
-
 	double PosX = a_Player->GetPosX();
 	double PosY = a_Player->GetPosY();
 	double PosZ = a_Player->GetPosZ();
@@ -1084,7 +1077,6 @@ void cChunk::CollectPickupsByPlayer(cPlayer * a_Player)
 void cChunk::UpdateSign(int a_PosX, int a_PosY, int a_PosZ, const AString & a_Line1, const AString & a_Line2, const AString & a_Line3, const AString & a_Line4)
 {
 	// Also sends update packets to all clients in the chunk
-	cCSLock Lock(m_CSEntities);
 	for (cBlockEntityList::iterator itr = m_BlockEntities.begin(); itr != m_BlockEntities.end(); ++itr)
 	{
 		if (
@@ -1121,20 +1113,16 @@ void cChunk::RemoveBlockEntity( cBlockEntity* a_BlockEntity )
 
 bool cChunk::AddClient(cClientHandle* a_Client)
 {
+	for (cClientHandleList::iterator itr = m_LoadedByClient.begin(); itr != m_LoadedByClient.end(); ++itr)
 	{
-		cCSLock Lock(m_CSClients);
+		if (a_Client == *itr)
-		for (cClientHandleList::iterator itr = m_LoadedByClient.begin(); itr != m_LoadedByClient.end(); ++itr)
 		{
-			if (a_Client == *itr)
+			// Already there, nothing needed
-			{
+			return false;
-				// Already there, nothing needed
-				return false;
-			}
 		}
-		m_LoadedByClient.push_back( a_Client );
 	}
+	m_LoadedByClient.push_back( a_Client );
 
-	cCSLock Lock(m_CSEntities);
 	for (cEntityList::iterator itr = m_Entities.begin(); itr != m_Entities.end(); ++itr )
 	{
 		LOGD("cChunk: Entity #%d (%s) at [%i, %i, %i] spawning for player \"%s\"", (*itr)->GetUniqueID(), (*itr)->GetClass(), m_PosX, m_PosY, m_PosZ, a_Client->GetUsername().c_str() );
@@ -1149,14 +1137,10 @@ bool cChunk::AddClient(cClientHandle* a_Client)
 
 void cChunk::RemoveClient( cClientHandle* a_Client )
 {
-	{
+	m_LoadedByClient.remove( a_Client );
-		cCSLock Lock(m_CSClients);
-		m_LoadedByClient.remove( a_Client );
-	}
 
 	if ( !a_Client->IsDestroyed() )
 	{
-		cCSLock Lock(m_CSEntities);
 		for (cEntityList::iterator itr = m_Entities.begin(); itr != m_Entities.end(); ++itr )
 		{
 			LOG("chunk [%i, %i] destroying entity #%i for player \"%s\"", m_PosX, m_PosZ, (*itr)->GetUniqueID(), a_Client->GetUsername().c_str() );
@@ -1172,7 +1156,6 @@ void cChunk::RemoveClient( cClientHandle* a_Client )
 
 bool cChunk::HasClient( cClientHandle* a_Client )
 {
-	cCSLock Lock(m_CSClients);
 	for (cClientHandleList::const_iterator itr = m_LoadedByClient.begin(); itr != m_LoadedByClient.end(); ++itr)
 	{
 		if ((*itr) == a_Client)
@@ -1189,7 +1172,6 @@ bool cChunk::HasClient( cClientHandle* a_Client )
 
 bool cChunk::HasAnyClients(void)
 {
-	cCSLock Lock(m_CSClients);
 	return !m_LoadedByClient.empty();
 }
 
@@ -1199,7 +1181,6 @@ bool cChunk::HasAnyClients(void)
 
 void cChunk::AddEntity( cEntity * a_Entity)
 {
-	cCSLock Lock(m_CSEntities);
 	if (a_Entity->GetEntityType() != cEntity::E_PLAYER)
 	{
 		MarkDirty();
@@ -1213,13 +1194,10 @@ void cChunk::AddEntity( cEntity * a_Entity)
 
 void cChunk::RemoveEntity(cEntity * a_Entity)
 {
-	size_t SizeBefore, SizeAfter;
+	size_t SizeBefore = m_Entities.size();
-	{
+	m_Entities.remove(a_Entity);
-		cCSLock Lock(m_CSEntities);
+	size_t SizeAfter = m_Entities.size();
-		SizeBefore = m_Entities.size();
+	
-		m_Entities.remove(a_Entity);
-		SizeAfter = m_Entities.size();
-	}
 	if (SizeBefore != SizeAfter)
 	{
 		// Mark as dirty if it was a server-generated entity:
@@ -1256,6 +1234,8 @@ char cChunk::GetBlock( int a_BlockIdx )
 
 
 
+/*
+// _X 2012_02_23: Loading in old format not supported anymore
 /// Loads the chunk from the old-format disk file, erases the file afterwards. Returns true if successful
 bool cChunk::LoadFromDisk()
 {
@@ -1274,9 +1254,7 @@ bool cChunk::LoadFromDisk()
 		return false;
 	}
 
-	// Now load Block Entities
+	// Now load Block Entities:
-	cCSLock Lock(m_CSEntities);
-
 	ENUM_BLOCK_ID BlockType;
 	while (f.Read(&BlockType, sizeof(ENUM_BLOCK_ID)) == sizeof(ENUM_BLOCK_ID))
 	{
@@ -1329,7 +1307,6 @@ bool cChunk::LoadFromDisk()
 			}
 		}
 	}
-	Lock.Unlock();
 	f.Close();
 
 	// Delete old format file
@@ -1344,6 +1321,7 @@ bool cChunk::LoadFromDisk()
 	m_IsDirty = false;
 	return true;
 }
+*/
 
 
 
@@ -1351,7 +1329,6 @@ bool cChunk::LoadFromDisk()
 
 void cChunk::Broadcast( const cPacket * a_Packet, cClientHandle* a_Exclude)
 {
-	cCSLock Lock(m_CSClients);
 	for (cClientHandleList::const_iterator itr = m_LoadedByClient.begin(); itr != m_LoadedByClient.end(); ++itr )
 	{
 		if (*itr == a_Exclude)
@@ -1377,131 +1354,6 @@ void cChunk::CopyBlockDataFrom(const char * a_NewBlockData)
 
 
 
-void cChunk::LoadFromJson( const Json::Value & a_Value )
-{
-	cCSLock Lock(m_CSEntities);
-
-	// Load chests
-	Json::Value AllChests = a_Value.get("Chests", Json::nullValue);
-	if (!AllChests.empty())
-	{
-		for ( Json::Value::iterator itr = AllChests.begin(); itr != AllChests.end(); ++itr )
-		{
-			Json::Value & Chest = *itr;
-			cChestEntity* ChestEntity = new cChestEntity(0, 0, 0, m_World);
-			if ( !ChestEntity->LoadFromJson( Chest ) )
-			{
-				LOGERROR("ERROR READING CHEST FROM JSON!" );
-				delete ChestEntity;
-			}
-			else m_BlockEntities.push_back( ChestEntity );
-		}
-	}
-
-	// Load furnaces
-	Json::Value AllFurnaces = a_Value.get("Furnaces", Json::nullValue);
-	if ( !AllFurnaces.empty() )
-	{
-		for ( Json::Value::iterator itr = AllFurnaces.begin(); itr != AllFurnaces.end(); ++itr )
-		{
-			Json::Value & Furnace = *itr;
-			cFurnaceEntity* FurnaceEntity = new cFurnaceEntity(0, 0, 0, m_World);
-			if ( !FurnaceEntity->LoadFromJson( Furnace ) )
-			{
-				LOGERROR("ERROR READING FURNACE FROM JSON!" );
-				delete FurnaceEntity;
-			}
-			else m_BlockEntities.push_back( FurnaceEntity );
-		}
-	}
-
-	// Load signs
-	Json::Value AllSigns = a_Value.get("Signs", Json::nullValue);
-	if ( !AllSigns.empty() )
-	{
-		for ( Json::Value::iterator itr = AllSigns.begin(); itr != AllSigns.end(); ++itr )
-		{
-			Json::Value & Sign = *itr;
-			cSignEntity* SignEntity = new cSignEntity( E_BLOCK_SIGN_POST, 0, 0, 0, m_World);
-			if ( !SignEntity->LoadFromJson( Sign ) )
-			{
-				LOGERROR("ERROR READING SIGN FROM JSON!" );
-				delete SignEntity;
-			}
-			else m_BlockEntities.push_back( SignEntity );
-		}
-	}
-}
-
-
-
-
-
-void cChunk::SaveToJson( Json::Value & a_Value )
-{
-	Json::Value AllChests;
-	Json::Value AllFurnaces;
-	Json::Value AllSigns;
-	cCSLock Lock(m_CSEntities);
-	for (cBlockEntityList::iterator itr = m_BlockEntities.begin(); itr != m_BlockEntities.end(); ++itr)
-	{
-		cBlockEntity * BlockEntity = *itr;
-		switch ( BlockEntity->GetBlockType() )
-		{
-			case E_BLOCK_CHEST:
-			{
-				cChestEntity* ChestEntity = reinterpret_cast< cChestEntity* >( BlockEntity );
-				Json::Value NewChest;
-				ChestEntity->SaveToJson( NewChest );
-				AllChests.append( NewChest );
-				break;
-			}
-			
-			case E_BLOCK_FURNACE:
-			{
-				cFurnaceEntity* FurnaceEntity = reinterpret_cast< cFurnaceEntity* >( BlockEntity );
-				Json::Value NewFurnace;
-				FurnaceEntity->SaveToJson( NewFurnace );
-				AllFurnaces.append( NewFurnace );
-				break;
-			}
-			
-			case E_BLOCK_SIGN_POST:
-			case E_BLOCK_WALLSIGN:
-			{
-				cSignEntity* SignEntity = reinterpret_cast< cSignEntity* >( BlockEntity );
-				Json::Value NewSign;
-				SignEntity->SaveToJson( NewSign );
-				AllSigns.append( NewSign );
-				break;
-			}
-			
-			default:
-			{
-				ASSERT(!"Unhandled blocktype in BlockEntities list while saving to JSON");
-				break;
-			}
-		}  // switch (BlockEntity->GetBlockType())
-	}  // for itr - BlockEntities[]
-
-	if( !AllChests.empty() )
-	{
-		a_Value["Chests"] = AllChests;
-	}
-	if( !AllFurnaces.empty() )
-	{
-		a_Value["Furnaces"] = AllFurnaces;
-	}
-	if( !AllSigns.empty() )
-	{
-		a_Value["Signs"] = AllSigns;
-	}
-}
-
-
-
-
-
 void cChunk::PositionToWorldPosition(int a_ChunkX, int a_ChunkY, int a_ChunkZ, int & a_X, int & a_Y, int & a_Z)
 {
 	a_Y = a_ChunkY;

source/cChunk.h

@@ -107,6 +107,8 @@ typedef std::list< sSetBlock > sSetBlockList;
 
 
 
+// This class is not to be used directly
+// Instead, call actions on cChunkMap (such as cChunkMap::SetBlock() etc.)
 class cChunk
 {
 public:
@@ -182,13 +184,10 @@ public:
 	void CalculateLighting(); // Recalculate right now
 	void CalculateHeightmap();
 
-	bool LoadFromDisk();
-
 	// Broadcasts to all clients that have loaded this chunk
 	void Broadcast( const cPacket & a_Packet, cClientHandle * a_Exclude = NULL) {Broadcast(&a_Packet, a_Exclude); }
 	void Broadcast( const cPacket * a_Packet, cClientHandle * a_Exclude = NULL);
 
-	// TODO: These functions are dangerous - rewrite to:
 	//   Loaded(blockdata, lightdata, blockentities, entities),
 	//   Generated(blockdata, lightdata, blockentities, entities),
 	//   GetBlockData(blockdatadest) etc.
@@ -200,10 +199,6 @@ public:
 	
 	void CopyBlockDataFrom(const char * a_NewBlockData);  // Copies all blockdata, recalculates heightmap (used by chunk loaders)
 	
-	// TODO: Move this into the specific WSSchema:
-	void LoadFromJson( const Json::Value & a_Value );
-	void SaveToJson( Json::Value & a_Value );
-
 	char GetLight(char* a_Buffer, int a_BlockIdx);
 	char GetLight(char* a_Buffer, int x, int y, int z);
 	void SetLight(char* a_Buffer, int a_BlockIdx, char a_Light);
@@ -238,13 +233,9 @@ private:
 	std::map< unsigned int, int > m_ToTickBlocks;
 	std::vector< unsigned int >   m_PendingSendBlocks;
 	
-	// TODO: This CS will soon not be needed, because all chunk access is protected by its parent ChunkMap's csLayers
+	// A critical section is not needed, because all chunk access is protected by its parent ChunkMap's csLayers
-	cCriticalSection  m_CSClients;
+	cClientHandleList  m_LoadedByClient;
-	cClientHandleList m_LoadedByClient;
+	cClientHandleList  m_UnloadQuery;
-	cClientHandleList m_UnloadQuery;
-
-	// TODO: This CS will soon not be needed, because all chunk access is protected by its parent ChunkMap's csLayers
-	cCriticalSection   m_CSEntities;
 	cEntityList        m_Entities;
 	cBlockEntityList   m_BlockEntities;
 

source/cChunkMap.cpp

@@ -810,6 +810,18 @@ cChunkMap::cChunkLayer::cChunkLayer(int a_LayerX, int a_LayerZ, cChunkMap * a_Pa
 
 
 
+cChunkMap::cChunkLayer::~cChunkLayer()
+{
+	for (int i = 0; i < ARRAYCOUNT(m_Chunks); ++i)
+	{
+		delete m_Chunks[i];
+	}  // for i - m_Chunks[]
+}
+
+
+
+
+
 cChunkPtr cChunkMap::cChunkLayer::GetChunk( int a_ChunkX, int a_ChunkY, int a_ChunkZ )
 {
 	// Always returns an assigned chunkptr, but the chunk needn't be valid (loaded / generated) - callers must check
@@ -891,6 +903,9 @@ void cChunkMap::cChunkLayer::UnloadUnusedChunks(void)
 	{
 		if ((m_Chunks[i] != NULL) && (m_Chunks[i]->CanUnload()))
 		{
+			// The chunk destructor calls our GetChunk() while removing its entities
+			// so we still need to be able to return the chunk. Therefore we first delete, then NULLify
+			// Doing otherwise results in bug http://forum.mc-server.org/showthread.php?tid=355
 			delete m_Chunks[i];
 			m_Chunks[i] = NULL;
 		}

source/cChunkMap.h

@@ -128,6 +128,7 @@ private:
 	{
 	public:
 		cChunkLayer(int a_LayerX, int a_LayerZ, cChunkMap * a_Parent);
+		~cChunkLayer();
 
 		/// Always returns an assigned chunkptr, but the chunk needn't be valid (loaded / generated) - callers must check
 		cChunkPtr GetChunk( int a_ChunkX, int a_ChunkY, int a_ChunkZ );