Remove blocking GetHeight (#4689)

* WIP: Remove blocking GetHeight

* Make CheckBasicStyle.lua happy

* Update comment and style

* Update lua bindings and docs

* Rework chunk loading in GenerateRandomSpawn

Explicitly use chunk stays instead of relying on chunks loaded by
`cSpawnPrepare` to not unload.

* Fix variable shadowing

* Update to C++17

* Add note about empty chunks to IsWeatherWetAtXYZ API docs

* Cleanup, fix some of the remaining merge issues.

---------

Co-authored-by: Alexander Harkness <me@bearbin.net>

Server/Plugins/APIDump/Classes/World.lua

@@ -1782,7 +1782,7 @@ function OnAllChunksAvailable()</pre> All return values from the callbacks are i
 						Type = "number",
 					},
 				},
-				Notes = "Returns the maximum height of the particula block column in the world. If the chunk is not loaded, it waits for it to load / generate. <b>WARNING</b>: Do not use, Use TryGetHeight() instead for a non-waiting version, otherwise you run the risk of a deadlock!",
+				Notes = "<b>DEPRECATED</b>, use TryGetHeight instead. Returns the maximum height of the particular block column in the world. If the chunk is not loaded, this function used to block until the chunk was loaded, leading to possible deadlock. Now it returns 0 instead.",
 			},
 			GetIniFileName =
 			{
@@ -2040,6 +2040,16 @@ function OnAllChunksAvailable()</pre> All return values from the callbacks are i
 				},
 				Notes = "Returns the Z coord of the default spawn",
 			},
+			GetSpawnPos =
+			{
+				Returns =
+				{
+					{
+						Type = "Vector3d"
+					},
+				},
+				Notes = "Returns the default spawn position",
+			},
 			GetStorageLoadQueueLength =
 			{
 				Returns =
@@ -2500,7 +2510,7 @@ function OnAllChunksAvailable()</pre> All return values from the callbacks are i
 						Type = "boolean",
 					},
 				},
-				Notes = "Returns true if the specified location has wet weather (rain or storm), using the same logic as IsWeatherWetAt, except that any rain-blocking blocks above the specified position will block the precipitation and this function will return false.",
+				Notes = "Returns true if the specified location has wet weather (rain or storm), using the same logic as IsWeatherWetAt, except that any rain-blocking blocks above the specified position will block the precipitation and this function will return false. Note if the chunk is unloaded then the weather state for the world will be returned.",
 			},
 			PickupsFromBlock =
 			{

src/Bindings/ManualBindings_World.cpp

@@ -1689,7 +1689,7 @@ static int tolua_cWorld_SpawnSplitExperienceOrbs(lua_State* tolua_S)
 
 static int tolua_cWorld_TryGetHeight(lua_State * tolua_S)
 {
-	/* Exported manually, because tolua would require the out-only param a_Height to be used when calling
+	/* Exported manually because we don't export optional<T>
 	Function signature: world:TryGetHeight(a_World, a_BlockX, a_BlockZ) -> IsValid, Height
 	*/
 
@@ -1716,12 +1716,11 @@ static int tolua_cWorld_TryGetHeight(lua_State * tolua_S)
 	}
 
 	// Call the implementation:
-	int Height = 0;
-	bool res = self->TryGetHeight(BlockX, BlockZ, Height);
-	L.Push(res);
-	if (res)
+	auto Height = self->GetHeight(BlockX, BlockZ);
+	L.Push(Height.has_value());
+	if (Height.has_value())
 	{
-		L.Push(Height);
+		L.Push(Height.value());
 		return 2;
 	}
 	return 1;
@@ -1731,6 +1730,39 @@ static int tolua_cWorld_TryGetHeight(lua_State * tolua_S)
 
 
 
+static int tolua_cWorld_GetHeight(lua_State * tolua_S)
+{
+	// Signature: world:GetHeight(a_World, a_BlockX, a_BlockZ) -> Height
+
+	// Check params:
+	cLuaState L(tolua_S);
+	if (
+		!L.CheckParamSelf("cWorld") ||
+		!L.CheckParamNumber(2, 3) ||
+		!L.CheckParamEnd(4)
+		)
+	{
+		return 0;
+	}
+
+	// Get params:
+	cWorld * self = nullptr;
+	int BlockX = 0;
+	int BlockZ = 0;
+	L.GetStackValues(1, self, BlockX, BlockZ);
+
+	// Call the implementation:
+	L.LogStackTrace();
+	FLOGWARN("cWorld:GetHeight is DEPRECATED, use TryGetHeight instead");
+	auto Height = self->GetHeight(BlockX, BlockZ);
+	L.Push(Height.value_or(0));
+	return 1;
+}
+
+
+
+
+
 void cManualBindings::BindWorld(lua_State * tolua_S)
 {
 	tolua_beginmodule(tolua_S, nullptr);
@@ -1776,6 +1808,7 @@ void cManualBindings::BindWorld(lua_State * tolua_S)
 			tolua_function(tolua_S, "GetBlockMeta",                 tolua_cWorld_GetBlockMeta);
 			tolua_function(tolua_S, "GetBlockSkyLight",             tolua_cWorld_GetBlockSkyLight);
 			tolua_function(tolua_S, "GetBlockTypeMeta",             tolua_cWorld_GetBlockTypeMeta);
+			tolua_function(tolua_S, "GetHeight",                    tolua_cWorld_GetHeight);
 			tolua_function(tolua_S, "GetSignLines",                 tolua_cWorld_GetSignLines);
 			tolua_function(tolua_S, "GetTimeOfDay",                 tolua_cWorld_GetTimeOfDay);
 			tolua_function(tolua_S, "GetWorldAge",                  tolua_cWorld_GetWorldAge);

src/Blocks/BlockCauldron.h

@@ -186,6 +186,7 @@ private:
 	) const override
 	{
 		auto WorldPos = a_Chunk.RelativeToAbsolute(a_RelPos);
+
 		if (!a_WorldInterface.IsWeatherWetAtXYZ(WorldPos.addedY(1)))
 		{
 			// It's not raining at our current location or we do not have a direct view of the sky

src/Blocks/WorldInterface.h

@@ -4,6 +4,8 @@
 #include "../FunctionRef.h"
 #include "../Mobs/MonsterTypes.h"
 
+#include <optional>
+
 class cBlockEntity;
 class cBroadcastInterface;
 class cItems;
@@ -76,7 +78,9 @@ public:
 	virtual bool IsWeatherWetAt(int a_BlockX, int a_BlockZ) = 0;
 
 	/** Returns true if it is raining or storming at the specified location,
-	and the rain reaches the specified block position. */
+	and the rain reaches the specified block position.
+	Returns the global weather state for unloaded chunks.
+	*/
 	virtual bool IsWeatherWetAtXYZ(Vector3i a_Pos) = 0;
 
 	/** Returns or sets the minumim or maximum netherportal width */
@@ -91,8 +95,8 @@ public:
 	virtual void SetMinNetherPortalHeight(int a_NewMinHeight) = 0;
 	virtual void SetMaxNetherPortalHeight(int a_NewMaxHeight) = 0;
 
-	/** Returns the world height at the specified coords; waits for the chunk to get loaded / generated */
-	virtual int GetHeight(int a_BlockX, int a_BlockZ) = 0;
+	/** Returns the world height at the specified coords; returns nullopt for unloaded / generated chunks */
+	virtual std::optional<int> GetHeight(int a_BlockX, int a_BlockZ) = 0;
 
 	/** Wakes up the simulators for the specified block */
 	virtual void WakeUpSimulators(Vector3i a_Block) = 0;

src/ChunkMap.cpp

@@ -380,42 +380,19 @@ bool cChunkMap::HasChunkAnyClients(int a_ChunkX, int a_ChunkZ) const
 
 
 
-int  cChunkMap::GetHeight(int a_BlockX, int a_BlockZ)
+std::optional<int> cChunkMap::GetHeight(int a_BlockX, int a_BlockZ)
 {
-	for (;;)
-	{
-		cCSLock Lock(m_CSChunks);
-		int ChunkX, ChunkZ, BlockY = 0;
-		cChunkDef::AbsoluteToRelative(a_BlockX, BlockY, a_BlockZ, ChunkX, ChunkZ);
-		auto & Chunk = GetChunk(ChunkX, ChunkZ);
-		if (Chunk.IsValid())
-		{
-			return Chunk.GetHeight(a_BlockX, a_BlockZ);
-		}
-
-		// The chunk is not valid, wait for it to become valid:
-		cCSUnlock Unlock(Lock);
-		m_evtChunkValid.Wait();
-	}  // while (true)
-}
-
-
-
-
-
-bool cChunkMap::TryGetHeight(int a_BlockX, int a_BlockZ, int & a_Height)
-{
-	// Returns false if chunk not loaded / generated
+	// Returns std::nullopt if chunk not loaded / generated.
 	cCSLock Lock(m_CSChunks);
 	int ChunkX, ChunkZ, BlockY = 0;
 	cChunkDef::AbsoluteToRelative(a_BlockX, BlockY, a_BlockZ, ChunkX, ChunkZ);
 	const auto Chunk = FindChunk(ChunkX, ChunkZ);
 	if ((Chunk == nullptr) || !Chunk->IsValid())
 	{
-		return false;
+		return std::nullopt;
 	}
-	a_Height = Chunk->GetHeight(a_BlockX, a_BlockZ);
-	return true;
+
+	return Chunk->GetHeight(a_BlockX, a_BlockZ);
 }
 
 

src/ChunkMap.h

@@ -5,6 +5,8 @@
 
 #pragma once
 
+#include <optional>
+
 #include "ChunkDataCallback.h"
 #include "EffectID.h"
 #include "FunctionRef.h"
@@ -91,10 +93,10 @@ public:
 	bool IsWeatherWetAt(int a_BlockX, int a_BlockZ) const;
 	bool IsWeatherWetAt(Vector3i a_Position) const;
 
-	bool      IsChunkValid       (int a_ChunkX, int a_ChunkZ) const;
-	bool      HasChunkAnyClients (int a_ChunkX, int a_ChunkZ) const;
-	int       GetHeight          (int a_BlockX, int a_BlockZ);  // Waits for the chunk to get loaded / generated
-	bool      TryGetHeight       (int a_BlockX, int a_BlockZ, int & a_Height);  // Returns false if chunk not loaded / generated
+	bool IsChunkValid       (int a_ChunkX, int a_ChunkZ) const;
+	bool HasChunkAnyClients (int a_ChunkX, int a_ChunkZ) const;
+
+	std::optional<int> GetHeight(int a_BlockX, int a_BlockZ);  // Returns nullopt if chunk not loaded / generated
 
 	/** Sets the block at the specified coords to the specified value.
 	The replacement doesn't trigger block updates, nor wake up simulators.

src/SpawnPrepare.h

@@ -1,6 +1,8 @@
 
 #pragma once
 
+#include "ChunkDef.h"
+
 class cWorld;
 
 
@@ -17,6 +19,11 @@ public:
 
 	static void PrepareChunks(cWorld & a_World, int a_SpawnChunkX, int a_SpawnChunkZ, int a_PrepareDistance);
 
+	static void PrepareChunks(cWorld & a_World, cChunkCoords a_SpawnChunk, int a_PrepareDistance)
+	{
+		PrepareChunks(a_World, a_SpawnChunk.m_ChunkX, a_SpawnChunk.m_ChunkZ, a_PrepareDistance);
+	}
+
 protected:
 	cWorld & m_World;
 	int m_SpawnChunkX;

src/World.cpp

@@ -711,27 +711,23 @@ void cWorld::GenerateRandomSpawn(int a_MaxSpawnRadius)
 	for (int BiomeCheckIndex = 0; BiomeCheckIndex < BiomeCheckCount; ++BiomeCheckIndex)
 	{
 		EMCSBiome Biome = GetBiomeAt(BiomeOffset.x, BiomeOffset.z);
-		if ((Biome == EMCSBiome::biOcean) || (Biome == EMCSBiome::biFrozenOcean))
+		if ((Biome != EMCSBiome::biOcean) && (Biome != EMCSBiome::biFrozenOcean))
 		{
-			BiomeOffset += Vector3d(cChunkDef::Width * 4, 0, 0);
-			continue;
+			// Found a usable biome
+			break;
 		}
 
-		// Found a usable biome
-		// Spawn chunks so we can find a nice spawn.
-		int ChunkX = 0, ChunkZ = 0;
-		cChunkDef::BlockToChunk(BiomeOffset.x, BiomeOffset.z, ChunkX, ChunkZ);
-		cSpawnPrepare::PrepareChunks(*this, ChunkX, ChunkZ, a_MaxSpawnRadius);
-		break;
+		BiomeOffset.x += cChunkDef::Width * 4;
 	}
 
 	// Check 0, 0 first.
-	int SpawnY = 0;
-	if (CanSpawnAt(BiomeOffset.x, SpawnY, BiomeOffset.z))
+	Vector3i BiomeSpawn = BiomeOffset;
+	if (CanSpawnAt(BiomeSpawn.x, BiomeSpawn.y, BiomeSpawn.z))
 	{
-		SetSpawn(BiomeOffset.x, SpawnY, BiomeOffset.z);
-
-		FLOGINFO("World \"{}\": Generated spawnpoint position at {}", m_WorldName, Vector3i{m_SpawnX, m_SpawnY, m_SpawnZ});
+		SetSpawn(BiomeSpawn.x, BiomeSpawn.y, BiomeSpawn.z);
+		auto ChunkPos = cChunkDef::BlockToChunk(BiomeSpawn);
+		cSpawnPrepare::PrepareChunks(*this, ChunkPos, a_MaxSpawnRadius);
+		FLOGINFO("World \"{}\": Generated spawnpoint position at {}", m_WorldName, BiomeSpawn);
 		return;
 	}
 
@@ -755,24 +751,25 @@ void cWorld::GenerateRandomSpawn(int a_MaxSpawnRadius)
 	{
 		for (int SearchGridIndex = 0; SearchGridIndex < PerRadiSearchCount; ++SearchGridIndex)
 		{
-			const Vector3i PotentialSpawn = BiomeOffset + (ChunkOffset[SearchGridIndex] * RadiusOffset);
+			auto PotentialSpawn = BiomeOffset + (ChunkOffset[SearchGridIndex] * RadiusOffset);
 
-			if (CanSpawnAt(PotentialSpawn.x, SpawnY, PotentialSpawn.z))
+			if (CanSpawnAt(PotentialSpawn.x, PotentialSpawn.y, PotentialSpawn.z))
 			{
-				SetSpawn(PotentialSpawn.x, SpawnY, PotentialSpawn.z);
+				SetSpawn(PotentialSpawn.x, PotentialSpawn.y, PotentialSpawn.z);
 
-				int ChunkX, ChunkZ;
-				cChunkDef::BlockToChunk(m_SpawnX, m_SpawnZ, ChunkX, ChunkZ);
-				cSpawnPrepare::PrepareChunks(*this, ChunkX, ChunkZ, a_MaxSpawnRadius);
+				auto ChunkPos = cChunkDef::BlockToChunk(PotentialSpawn);
+				cSpawnPrepare::PrepareChunks(*this, ChunkPos, a_MaxSpawnRadius);
 
-				FLOGINFO("World \"{}\":Generated spawnpoint position at {}", m_WorldName, Vector3i{m_SpawnX, m_SpawnY, m_SpawnZ});
+				FLOGINFO("World \"{}\":Generated spawnpoint position at {}", m_WorldName, PotentialSpawn);
 				return;
 			}
 		}
 	}
 
-	m_SpawnY = GetHeight(m_SpawnX, m_SpawnZ);
-	FLOGWARNING("World \"{}\": Did not find an acceptable spawnpoint. Generated a random spawnpoint position at {}", m_WorldName, Vector3i{m_SpawnX, m_SpawnY, m_SpawnZ});
+	SetSpawn(BiomeSpawn.x, BiomeSpawn.y, BiomeSpawn.z);
+	auto ChunkPos = cChunkDef::BlockToChunk(BiomeSpawn);
+	cSpawnPrepare::PrepareChunks(*this, ChunkPos, a_MaxSpawnRadius);
+	FLOGWARNING("World \"{}\": Did not find an acceptable spawnpoint. Generated a random spawnpoint position at {}", m_WorldName, BiomeSpawn);
 }
 
 
@@ -792,11 +789,57 @@ bool cWorld::CanSpawnAt(int a_X, int & a_Y, int a_Z)
 		E_BLOCK_NETHERRACK
 	};
 
+
+	class cCanSpawnChunkStay:
+		public cChunkStay
+	{
+		cEvent m_ChunksReady;
+	public:
+
+		cCanSpawnChunkStay(double a_WorldX, double a_WorldZ)
+		{
+			auto Chunk = cChunkDef::BlockToChunk(Vector3d{a_WorldX, 0.0, a_WorldZ}.Floor());
+			for (int XOffset = -1; XOffset != 2; ++XOffset)
+			{
+				for (int ZOffset = -1; ZOffset != 2; ++ZOffset)
+				{
+					Add(Chunk.m_ChunkX + XOffset, Chunk.m_ChunkZ + ZOffset);
+				}
+			}
+		}
+
+		virtual ~cCanSpawnChunkStay() override
+		{
+			Disable();
+		}
+
+		virtual bool OnAllChunksAvailable() override
+		{
+			m_ChunksReady.Set();
+			return false;  // Keep chunk stay active
+		}
+
+		virtual void OnChunkAvailable(int, int) override {}
+		virtual void OnDisabled() override {}
+
+		void Wait()
+		{
+			m_ChunksReady.Wait();
+		}
+	};
+
+	// Use chunk stay to load 3x3 chunk area around a_X, a_Z
+	cCanSpawnChunkStay ChunkStay(a_X, a_Z);
+	ChunkStay.Enable(m_ChunkMap);
+	ChunkStay.Wait();
+
 	static const int ValidSpawnBlocksCount = ARRAYCOUNT(ValidSpawnBlocks);
 
 	// Increase this by two, because we need two more blocks for body and head
-	static const int HighestSpawnPoint = GetHeight(a_X, a_Z) + 2;
-	static const int LowestSpawnPoint = HighestSpawnPoint / 2;
+	auto Height = GetHeight(static_cast<int>(a_X), static_cast<int>(a_Z));
+	ASSERT(Height.has_value());
+	static const int HighestSpawnPoint = *Height + 2;
+	static const int LowestSpawnPoint = static_cast<int>(HighestSpawnPoint / 2.0f);
 
 	for (int PotentialY = HighestSpawnPoint; PotentialY > LowestSpawnPoint; --PotentialY)
 	{
@@ -845,6 +888,8 @@ bool cWorld::CanSpawnAt(int a_X, int & a_Y, int a_Z)
 		return true;
 	}
 
+	// Always set a_Y so it can be used when we fail to find any spawn point
+	a_Y = HighestSpawnPoint - 2;
 	return false;
 }
 
@@ -2122,7 +2167,7 @@ void cWorld::SendBlockTo(int a_X, int a_Y, int a_Z, const cPlayer & a_Player)
 
 
 
-int cWorld::GetHeight(int a_X, int a_Z)
+std::optional<int> cWorld::GetHeight(int a_X, int a_Z)
 {
 	return m_ChunkMap.GetHeight(a_X, a_Z);
 }
@@ -2131,15 +2176,6 @@ int cWorld::GetHeight(int a_X, int a_Z)
 
 
 
-bool cWorld::TryGetHeight(int a_BlockX, int a_BlockZ, int & a_Height)
-{
-	return m_ChunkMap.TryGetHeight(a_BlockX, a_BlockZ, a_Height);
-}
-
-
-
-
-
 void cWorld::SendBlockEntity(int a_BlockX, int a_BlockY, int a_BlockZ, cClientHandle & a_Client)
 {
 	m_ChunkMap.SendBlockEntity(a_BlockX, a_BlockY, a_BlockZ, a_Client);

src/World.h

@@ -1,6 +1,8 @@
 
 #pragma once
 
+#include <optional>
+
 #include "Simulator/SimulatorManager.h"
 #include "ChunkMap.h"
 #include "WorldStorage/WorldStorage.h"
@@ -132,9 +134,6 @@ public:
 
 	virtual eDimension GetDimension(void) const override { return m_Dimension; }
 
-	/** Returns the world height at the specified coords; waits for the chunk to get loaded / generated */
-	virtual int GetHeight(int a_BlockX, int a_BlockZ) override;
-
 	// tolua_end
 
 	virtual cTickTime GetTimeOfDay(void) const override;
@@ -144,8 +143,8 @@ public:
 
 	virtual void SetTimeOfDay(cTickTime a_TimeOfDay) override;
 
-	/** Retrieves the world height at the specified coords; returns false if chunk not loaded / generated */
-	bool TryGetHeight(int a_BlockX, int a_BlockZ, int & a_Height);  // Exported in ManualBindings.cpp
+	/** Retrieves the world height at the specified coords; returns nullopt if chunk not loaded / generated */
+	virtual std::optional<int> GetHeight(int a_BlockX, int a_BlockZ) override;  // Exported in ManualBindings.cpp
 
 	// Broadcast respective packets to all clients of the chunk where the event is taking place
 	// Implemented in Broadcaster.cpp
@@ -585,6 +584,10 @@ public:
 	int GetSpawnX(void) const { return m_SpawnX; }
 	int GetSpawnY(void) const { return m_SpawnY; }
 	int GetSpawnZ(void) const { return m_SpawnZ; }
+	Vector3i GetSpawnPos() const
+	{
+		return {m_SpawnX, m_SpawnY, m_SpawnZ};
+	}
 
 	/** Wakes up the simulators for the specified block */
 	virtual void WakeUpSimulators(Vector3i a_Block) override;