5 files changed, 194 insertions, 248 deletions

diff --git a/src/BlockEntities/BlockEntity.cpp b/src/BlockEntities/BlockEntity.cpp
index 4b17f10a6..7c0660431 100644
--- a/src/BlockEntities/BlockEntity.cpp
+++ b/src/BlockEntities/BlockEntity.cpp
@@ -168,6 +168,15 @@ bool cBlockEntity::IsBlockEntityBlockType(const BLOCKTYPE a_BlockType)
 
 
 
+void cBlockEntity::OnAddToWorld(cWorld & a_World, cChunk & a_Chunk)
+{
+	m_World = &a_World;
+}
+
+
+
+
+
 void cBlockEntity::OnRemoveFromWorld()
 {
 }
diff --git a/src/BlockEntities/BlockEntity.h b/src/BlockEntities/BlockEntity.h
index 3e9cf38e7..e13600e8c 100644
--- a/src/BlockEntities/BlockEntity.h
+++ b/src/BlockEntities/BlockEntity.h
@@ -51,12 +51,19 @@ public:
 	Returns nullptr for unknown block types. */
 	static OwnedBlockEntity CreateByBlockType(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World = nullptr);
 
+	/** Called when this block entity's associated block is destroyed.
+	It is guaranteed that this function is called before OnRemoveFromWorld. */
 	virtual void Destroy();
 
 	/** Returns true if the specified blocktype is supposed to have an associated block entity. */
 	static bool IsBlockEntityBlockType(BLOCKTYPE a_BlockType);
 
-	/** Called when the block entity is removed from a world. */
+	/** Called when the block entity object is added to a world. */
+	virtual void OnAddToWorld(cWorld & a_World, cChunk & a_Chunk);
+
+	/** Called when the block entity object is removed from a world.
+	This occurs when the chunk it resides in is unloaded, or when the associated block is destroyed.
+	If it is the latter, Destroy() is guaranteed to be called first. */
 	virtual void OnRemoveFromWorld();
 
 	/** Sends the packet defining the block entity to the client specified.
diff --git a/src/BlockEntities/ChestEntity.cpp b/src/BlockEntities/ChestEntity.cpp
index 3c80a7aa3..c2c31b30a 100644
--- a/src/BlockEntities/ChestEntity.cpp
+++ b/src/BlockEntities/ChestEntity.cpp
@@ -2,6 +2,7 @@
 #include "Globals.h"  // NOTE: MSVC stupidness requires this to be the same across all modules
 
 #include "ChestEntity.h"
+#include "../Chunk.h"
 #include "../BlockInfo.h"
 #include "../Item.h"
 #include "../Entities/Player.h"
@@ -18,167 +19,144 @@ cChestEntity::cChestEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector
 	m_NumActivePlayers(0),
 	m_Neighbour(nullptr)
 {
-	auto chunkCoord = cChunkDef::BlockToChunk(a_Pos);
-	if (
-		(m_World != nullptr) &&
-		m_World->IsChunkValid(chunkCoord.m_ChunkX, chunkCoord.m_ChunkZ)
-	)
+}
+
+
+
+
+
+cChestEntity & cChestEntity::GetPrimaryChest()
+{
+	if (m_Neighbour == nullptr)
 	{
-		ScanNeighbours();
+		return *this;
 	}
+
+	// The primary chest should be the one with lesser X or Z coord:
+	return (
+		(m_Neighbour->GetPosX() < GetPosX()) ||
+		(m_Neighbour->GetPosZ() < GetPosZ())
+	) ? *m_Neighbour : *this;
 }
 
 
 
 
 
-void cChestEntity::CopyFrom(const cBlockEntity & a_Src)
+cChestEntity * cChestEntity::GetSecondaryChest()
 {
-	Super::CopyFrom(a_Src);
-	auto & src = static_cast<const cChestEntity &>(a_Src);
-	m_Contents.CopyFrom(src.m_Contents);
-
-	// Reset the neighbor and player count, there's no sense in copying these:
-	m_Neighbour = nullptr;
-	m_NumActivePlayers = 0;
+	// If we're the primary, then our neighbour is the secondary, and vice versa:
+	return (&GetPrimaryChest() == this) ? m_Neighbour : this;
 }
 
 
 
 
 
-void cChestEntity::OnRemoveFromWorld()
+bool cChestEntity::ScanNeighbour(cChunk & a_Chunk, Vector3i a_Position)
 {
-	if (m_Neighbour != nullptr)
+	const auto Chunk = a_Chunk.GetRelNeighborChunkAdjustCoords(a_Position);
+
+	if ((Chunk == nullptr) || !Chunk->IsValid())
 	{
-		// Neighbour may share a window with us, force the window shut:
-		m_Neighbour->DestroyWindow();
-		m_Neighbour->m_Neighbour = nullptr;
+		// If a chest was in fact there, they'll find us when their chunk loads.
+		return false;
 	}
 
-	DestroyWindow();
+	const auto BlockEntity = Chunk->GetBlockEntityRel(a_Position);
+
+	if ((BlockEntity == nullptr) || (BlockEntity->GetBlockType() != m_BlockType))
+	{
+		// Neighbouring block is not the same type of chest:
+		return false;
+	}
+
+	m_Neighbour = static_cast<cChestEntity *>(BlockEntity);
+	return true;
 }
 
 
 
 
 
-void cChestEntity::SendTo(cClientHandle & a_Client)
+void cChestEntity::DestroyWindow()
 {
-	// Send a dummy "number of players with chest open" packet to make the chest visible:
-	a_Client.SendBlockAction(m_Pos.x, m_Pos.y, m_Pos.z, 1, 0, m_BlockType);
+	const auto Window = GetWindow();
+	if (Window != nullptr)
+	{
+		Window->OwnerDestroyed();
+	}
 }
 
 
 
 
 
-bool cChestEntity::UsedBy(cPlayer * a_Player)
+bool cChestEntity::IsBlocked()
 {
-	if (IsBlocked())
-	{
-		// Obstruction, don't open
-		return true;
-	}
+	return (
+		(GetPosY() < cChunkDef::Height - 1) &&
+		(
+			!cBlockInfo::IsTransparent(GetWorld()->GetBlock(GetPos().addedY(1))) ||
+			!cOcelot::IsCatSittingOnBlock(GetWorld(), Vector3d(GetPos()))
+		)
+	);
+}
 
-	if (m_Neighbour == nullptr)
-	{
-		ScanNeighbours();
-	}
 
-	// The primary chest should be the one with lesser X or Z coord:
-	cChestEntity * PrimaryChest = this;
-	if (m_Neighbour != nullptr)
-	{
-		if (m_Neighbour->IsBlocked())
-		{
-			// Obstruction, don't open
-			return true;
-		}
 
-		if (
-			(m_Neighbour->GetPosX() > GetPosX()) ||
-			(m_Neighbour->GetPosZ() > GetPosZ())
-		)
-		{
-			PrimaryChest = m_Neighbour;
-		}
-	}
 
-	if (m_BlockType == E_BLOCK_CHEST)
-	{
-		a_Player->GetStatManager().AddValue(Statistic::OpenChest);
-	}
-	else  // E_BLOCK_TRAPPED_CHEST
-	{
-		a_Player->GetStatManager().AddValue(Statistic::TriggerTrappedChest);
-	}
 
-	// If the window is not created, open it anew:
-	cWindow * Window = PrimaryChest->GetWindow();
-	if (Window == nullptr)
+void cChestEntity::OpenNewWindow(void)
+{
+	if (m_Neighbour != nullptr)
 	{
-		PrimaryChest->OpenNewWindow();
-		Window = PrimaryChest->GetWindow();
-	}
+		ASSERT(&GetPrimaryChest() == this);  // Should only open windows for the primary chest.
 
-	// Open the window for the player:
-	if (Window != nullptr)
+		OpenWindow(new cChestWindow(this, m_Neighbour));
+	}
+	else
 	{
-		if (a_Player->GetWindow() != Window)
-		{
-			a_Player->OpenWindow(*Window);
-		}
+		// There is no chest neighbour, open a single-chest window:
+		OpenWindow(new cChestWindow(this));
 	}
-
-	// This is rather a hack
-	// Instead of marking the chunk as dirty upon chest contents change, we mark it dirty now
-	// We cannot properly detect contents change, but such a change doesn't happen without a player opening the chest first.
-	// The few false positives aren't much to worry about
-	auto chunkCoords = cChunkDef::BlockToChunk(m_Pos);
-	m_World->MarkChunkDirty(chunkCoords.m_ChunkX, chunkCoords.m_ChunkZ);
-	return true;
 }
 
 
 
 
 
-cChestEntity * cChestEntity::GetNeighbour()
+void cChestEntity::CopyFrom(const cBlockEntity & a_Src)
 {
-	return m_Neighbour;
+	Super::CopyFrom(a_Src);
+	auto & src = static_cast<const cChestEntity &>(a_Src);
+	m_Contents.CopyFrom(src.m_Contents);
+
+	// Reset the neighbor and player count, there's no sense in copying these:
+	m_Neighbour = nullptr;
+	m_NumActivePlayers = 0;
 }
 
 
 
 
 
-void cChestEntity::ScanNeighbours()
+void cChestEntity::OnAddToWorld(cWorld & a_World, cChunk & a_Chunk)
 {
-	// Callback for finding neighbouring chest.
-	auto FindNeighbour = [this](cBlockEntity & a_BlockEntity)
-	{
-		if (a_BlockEntity.GetBlockType() != m_BlockType)
-		{
-			// Neighboring block is not the same type of chest
-			return false;
-		}
-
-		m_Neighbour = static_cast<cChestEntity *>(&a_BlockEntity);
-		return true;
-	};
+	Super::OnAddToWorld(a_World, a_Chunk);
 
 	// Scan horizontally adjacent blocks for any neighbouring chest of the same type:
 	if (
-		m_World->DoWithBlockEntityAt(m_Pos.addedX(-1), FindNeighbour) ||
-		m_World->DoWithBlockEntityAt(m_Pos.addedX(+1), FindNeighbour) ||
-		m_World->DoWithBlockEntityAt(m_Pos.addedZ(-1), FindNeighbour) ||
-		m_World->DoWithBlockEntityAt(m_Pos.addedX(+1), FindNeighbour)
+		const auto Position = GetRelPos();
+
+		ScanNeighbour(a_Chunk, Position.addedX(-1)) ||
+		ScanNeighbour(a_Chunk, Position.addedX(+1)) ||
+		ScanNeighbour(a_Chunk, Position.addedZ(-1)) ||
+		ScanNeighbour(a_Chunk, Position.addedZ(+1))
 	)
 	{
 		m_Neighbour->m_Neighbour = this;
-		// Force neighbour's window shut. Does Mojang server do this or should a double window open?
-		m_Neighbour->DestroyWindow();
+		m_Neighbour->DestroyWindow();  // Force neighbour's window shut. Does Mojang server do this or should a double window open?
 	}
 }
 
@@ -186,49 +164,73 @@ void cChestEntity::ScanNeighbours()
 
 
 
-void cChestEntity::OpenNewWindow(void)
+void cChestEntity::OnRemoveFromWorld()
 {
 	if (m_Neighbour != nullptr)
 	{
-		ASSERT(  // This should be the primary chest
-			(m_Neighbour->GetPosX() < GetPosX()) ||
-			(m_Neighbour->GetPosZ() < GetPosZ())
-		);
-		OpenWindow(new cChestWindow(this, m_Neighbour));
-	}
-	else
-	{
-		// There is no chest neighbour, open a single-chest window:
-		OpenWindow(new cChestWindow(this));
+		// Neighbour may share a window with us, force the window shut:
+		m_Neighbour->DestroyWindow();
+		m_Neighbour->m_Neighbour = nullptr;
 	}
+
+	DestroyWindow();
 }
 
 
 
 
 
-void cChestEntity::DestroyWindow()
+void cChestEntity::SendTo(cClientHandle & a_Client)
 {
-	const auto Window = GetWindow();
-	if (Window != nullptr)
-	{
-		Window->OwnerDestroyed();
-	}
+	a_Client.SendUpdateBlockEntity(*this);
 }
 
 
 
 
 
-bool cChestEntity::IsBlocked()
+bool cChestEntity::UsedBy(cPlayer * a_Player)
 {
-	return (
-		(GetPosY() < cChunkDef::Height - 1) &&
-		(
-			!cBlockInfo::IsTransparent(GetWorld()->GetBlock(GetPos().addedY(1))) ||
-			!cOcelot::IsCatSittingOnBlock(GetWorld(), Vector3d(GetPos()))
-		)
-	);
+	if (IsBlocked())
+	{
+		// Obstruction, don't open
+		return true;
+	}
+
+	if ((m_Neighbour != nullptr) && m_Neighbour->IsBlocked())
+	{
+		return true;
+	}
+
+	if (m_BlockType == E_BLOCK_CHEST)
+	{
+		a_Player->GetStatManager().AddValue(Statistic::OpenChest);
+	}
+	else  // E_BLOCK_TRAPPED_CHEST
+	{
+		a_Player->GetStatManager().AddValue(Statistic::TriggerTrappedChest);
+	}
+
+	auto & PrimaryChest = GetPrimaryChest();
+	cWindow * Window = PrimaryChest.GetWindow();
+
+	// If the window is not created, open it anew:
+	if (Window == nullptr)
+	{
+		PrimaryChest.OpenNewWindow();
+		Window = PrimaryChest.GetWindow();
+	}
+
+	// Open the window for the player:
+	if (Window != nullptr)
+	{
+		if (a_Player->GetWindow() != Window)
+		{
+			a_Player->OpenWindow(*Window);
+		}
+	}
+
+	return true;
 }
 
 
@@ -239,11 +241,6 @@ void cChestEntity::OnSlotChanged(cItemGrid * a_Grid, int a_SlotNum)
 {
 	ASSERT(a_Grid == &m_Contents);
 
-	if (m_World == nullptr)
-	{
-		return;
-	}
-
 	// Have cBlockEntityWithItems update redstone and try to broadcast our window:
 	Super::OnSlotChanged(a_Grid, a_SlotNum);
 
@@ -259,9 +256,4 @@ void cChestEntity::OnSlotChanged(cItemGrid * a_Grid, int a_SlotNum)
 	{
 		Window->BroadcastWholeWindow();
 	}
-
-	m_World->MarkChunkDirty(GetChunkX(), GetChunkZ());
-
-	// Notify comparators:
-	m_World->WakeUpSimulators(m_Pos);
 }
diff --git a/src/BlockEntities/ChestEntity.h b/src/BlockEntities/ChestEntity.h
index 02b182481..ee3c59b8b 100644
--- a/src/BlockEntities/ChestEntity.h
+++ b/src/BlockEntities/ChestEntity.h
@@ -34,32 +34,20 @@ public:
 
 	// tolua_end
 
-	/** Constructor used for normal operation */
 	cChestEntity(BLOCKTYPE a_BlockType, NIBBLETYPE a_BlockMeta, Vector3i a_Pos, cWorld * a_World);
 
-	// cBlockEntity overrides:
-	virtual void CopyFrom(const cBlockEntity & a_Src) override;
-	virtual void OnRemoveFromWorld() override;
-	virtual void SendTo(cClientHandle & a_Client) override;
-	virtual bool UsedBy(cPlayer * a_Player) override;
+	/** Gets the number of players who currently have this chest open */
+	int GetNumberOfPlayers(void) const { return m_NumActivePlayers; }
 
-	/** Search horizontally adjacent blocks for neighbouring chests of the same type and links them together. */
-	void ScanNeighbours();
+	/** Returns the associated primary chest. */
+	cChestEntity & GetPrimaryChest();
 
-	/** Returns the value of m_Neighbour. */
-	cChestEntity * GetNeighbour();
+	/** Returns the associated secondary chest, if any. */
+	cChestEntity * GetSecondaryChest();
 
-	/** Opens a new chest window where this is the primary chest and any neighbour is the secondary. */
-	void OpenNewWindow();
-
-	/** Forces any players to close the owned window. */
-	void DestroyWindow();
-
-	/** Returns true if the chest should not be accessible by players. */
-	bool IsBlocked();
-
-	/** Gets the number of players who currently have this chest open */
-	int GetNumberOfPlayers(void) const { return m_NumActivePlayers; }
+	/** Search the given horizontally adjacent relative position for a neighbouring chest of the same type.
+	If found, links them together with ourselves and returns true. */
+	bool ScanNeighbour(cChunk & a_Chunk, Vector3i a_Position);
 
 	/** Sets the number of players who currently have this chest open */
 	void SetNumberOfPlayers(int a_NumActivePlayers) { m_NumActivePlayers = a_NumActivePlayers; }
@@ -72,6 +60,22 @@ private:
 	/** Neighbouring chest that links to form a double chest */
 	cChestEntity * m_Neighbour;
 
+	/** Forces any players to close the owned window. */
+	void DestroyWindow();
+
+	/** Returns true if the chest should not be accessible by players. */
+	bool IsBlocked();
+
+	/** Opens a new chest window where this is the primary chest and any neighbour is the secondary. */
+	void OpenNewWindow();
+
+	// cBlockEntity overrides:
+	virtual void CopyFrom(const cBlockEntity & a_Src) override;
+	virtual void OnAddToWorld(cWorld & a_World, cChunk & a_Chunk) override;
+	virtual void OnRemoveFromWorld() override;
+	virtual void SendTo(cClientHandle & a_Client) override;
+	virtual bool UsedBy(cPlayer * a_Player) override;
+
 	/** cItemGrid::cListener overrides: */
 	virtual void OnSlotChanged(cItemGrid * a_Grid, int a_SlotNum) override;
 } ;  // tolua_export
diff --git a/src/BlockEntities/HopperEntity.cpp b/src/BlockEntities/HopperEntity.cpp
index 5b273b13c..82e07f6a0 100644
--- a/src/BlockEntities/HopperEntity.cpp
+++ b/src/BlockEntities/HopperEntity.cpp
@@ -132,12 +132,6 @@ bool cHopperEntity::UsedBy(cPlayer * a_Player)
 		}
 	}
 
-	// This is rather a hack
-	// Instead of marking the chunk as dirty upon chest contents change, we mark it dirty now
-	// We cannot properly detect contents change, but such a change doesn't happen without a player opening the chest first.
-	// The few false positives aren't much to worry about
-	cChunkCoords ChunkPos = cChunkDef::BlockToChunk(GetPos());
-	m_World->MarkChunkDirty(ChunkPos.m_ChunkX, ChunkPos.m_ChunkZ);
 	return true;
 }
 
@@ -172,15 +166,15 @@ bool cHopperEntity::MoveItemsIn(cChunk & a_Chunk, const cTickTimeLong a_CurrentT
 	bool res = false;
 	switch (a_Chunk.GetBlock(GetRelPos().addedY(1)))
 	{
-		case E_BLOCK_TRAPPED_CHEST:
 		case E_BLOCK_CHEST:
+		case E_BLOCK_TRAPPED_CHEST:
 		{
 			// Chests have special handling because of double-chests
 			res = MoveItemsFromChest(a_Chunk);
 			break;
 		}
-		case E_BLOCK_LIT_FURNACE:
 		case E_BLOCK_FURNACE:
+		case E_BLOCK_LIT_FURNACE:
 		{
 			// Furnaces have special handling because only the output and leftover fuel buckets shall be moved
 			res = MoveItemsFromFurnace(a_Chunk);
@@ -331,15 +325,15 @@ bool cHopperEntity::MoveItemsOut(cChunk & a_Chunk, const cTickTimeLong a_Current
 	auto absCoord = destChunk->RelativeToAbsolute(relCoord);
 	switch (destChunk->GetBlock(relCoord))
 	{
-		case E_BLOCK_TRAPPED_CHEST:
 		case E_BLOCK_CHEST:
+		case E_BLOCK_TRAPPED_CHEST:
 		{
 			// Chests have special handling because of double-chests
 			res = MoveItemsToChest(*destChunk, absCoord);
 			break;
 		}
-		case E_BLOCK_LIT_FURNACE:
 		case E_BLOCK_FURNACE:
+		case E_BLOCK_LIT_FURNACE:
 		{
 			// Furnaces have special handling because of the direction-to-slot relation
 			res = MoveItemsToFurnace(*destChunk, absCoord, meta);
@@ -375,52 +369,22 @@ bool cHopperEntity::MoveItemsOut(cChunk & a_Chunk, const cTickTimeLong a_Current
 
 bool cHopperEntity::MoveItemsFromChest(cChunk & a_Chunk)
 {
-	auto ChestPos = GetPos().addedY(1);
-	auto MainChest = static_cast<cChestEntity *>(a_Chunk.GetBlockEntity(ChestPos));
-	if (MainChest == nullptr)
+	const auto ConnectedBlockEntity = a_Chunk.GetBlockEntityRel(GetRelPos().addedY(1));
+
+	if (ConnectedBlockEntity == nullptr)
 	{
-		FLOGWARNING("{0}: A chest entity was not found where expected, at {1}", __FUNCTION__, ChestPos);
 		return false;
 	}
-	auto SideChest = MainChest->GetNeighbour();
-	if (SideChest == nullptr)
-	{
-		if (MoveItemsFromGrid(*MainChest))
-		{
-			return true;
-		}
-	}
-	else
+
+	const auto ConnectedChest = static_cast<cChestEntity *>(ConnectedBlockEntity);
+
+	if (MoveItemsFromGrid(ConnectedChest->GetPrimaryChest()))
 	{
-		auto SideAbsCoords = SideChest->GetPos();
-		// the "primary" chest is the one with the higher z or x value
-		if (SideAbsCoords.z > ChestPos.z || SideAbsCoords.x > ChestPos.x)
-		{
-			// side is "primary" so pull from it first
-			if (MoveItemsFromGrid(*SideChest))
-			{
-				return true;
-			}
-			// main is secondary, pull from next
-			if (MoveItemsFromGrid(*MainChest))
-			{
-				return true;
-			}
-		}
-		else
-		{
-			if (MoveItemsFromGrid(*MainChest))
-			{
-				return true;
-			}
-			if (MoveItemsFromGrid(*SideChest))
-			{
-				return true;
-			}
-		}
+		return true;
 	}
-	// The chest was empty
-	return false;
+
+	const auto SecondaryChest = ConnectedChest->GetSecondaryChest();
+	return (SecondaryChest != nullptr) && MoveItemsFromGrid(*SecondaryChest);
 }
 
 
@@ -529,48 +493,22 @@ bool cHopperEntity::MoveItemsFromSlot(cBlockEntityWithItems & a_Entity, int a_Sl
 
 bool cHopperEntity::MoveItemsToChest(cChunk & a_Chunk, Vector3i a_Coords)
 {
-	// Try the chest directly connected to the hopper:
-	auto ConnectedChest = static_cast<cChestEntity *>(a_Chunk.GetBlockEntity(a_Coords));
-	if (ConnectedChest == nullptr)
+	const auto ConnectedBlockEntity = a_Chunk.GetBlockEntity(a_Coords);
+
+	if (ConnectedBlockEntity == nullptr)
 	{
-		FLOGWARNING("{0}: A chest entity was not found where expected, at {1}", __FUNCTION__, a_Coords);
 		return false;
 	}
-	auto SideChest = ConnectedChest->GetNeighbour();
-	if (SideChest == nullptr)
-	{
-		if (MoveItemsToGrid(*ConnectedChest))
-		{
-			return true;
-		}
-	}
-	else
+
+	const auto ConnectedChest = static_cast<cChestEntity *>(ConnectedBlockEntity);
+
+	if (MoveItemsToGrid(ConnectedChest->GetPrimaryChest()))
 	{
-		auto SideAbsCoords = SideChest->GetPos();
-		if (SideAbsCoords.z > a_Coords.z || SideAbsCoords.x > a_Coords.x)
-		{
-			if (MoveItemsToGrid(*SideChest))
-			{
-				return true;
-			}
-			if (MoveItemsToGrid(*ConnectedChest))
-			{
-				return true;
-			}
-		}
-		else
-		{
-			if (MoveItemsToGrid(*ConnectedChest))
-			{
-				return true;
-			}
-			if (MoveItemsToGrid(*SideChest))
-			{
-				return true;
-			}
-		}
+		return true;
 	}
-	return false;
+
+	const auto SecondaryChest = ConnectedChest->GetSecondaryChest();
+	return (SecondaryChest != nullptr) && MoveItemsToGrid(*SecondaryChest);
 }
 
 
@@ -661,7 +599,3 @@ bool cHopperEntity::MoveItemsToSlot(cBlockEntityWithItems & a_Entity, int a_DstS
 		return false;
 	}
 }
-
-
-
-