Fixed cWorld:ChunkStay bindings. (#3319)

Introduced new cLuaState::cOptionalCallback for representing optional callbacks (nil from Lua side).
Introduced new cLuaState::cStackTable class for easy access to Lua table's elements.

Fixes #3305.

src/Bindings/LuaChunkStay.cpp

@@ -11,9 +11,7 @@
 
 
 
-cLuaChunkStay::cLuaChunkStay(cPluginLua & a_Plugin) :
-	m_Plugin(a_Plugin),
-	m_LuaState(nullptr)
+cLuaChunkStay::cLuaChunkStay()
 {
 }
 
@@ -21,49 +19,33 @@ cLuaChunkStay::cLuaChunkStay(cPluginLua & a_Plugin) :
 
 
 
-bool cLuaChunkStay::AddChunks(int a_ChunkCoordTableStackPos)
+bool cLuaChunkStay::AddChunks(const cLuaState::cStackTable & a_ChunkCoordsTable)
 {
 	// This function is expected to be called just once, with all the coords in a table
 	ASSERT(m_Chunks.empty());
 
-	cPluginLua::cOperation Op(m_Plugin);
-	cLuaState & L = Op();
-
-	// Check that we got a table:
-	if (!lua_istable(L, a_ChunkCoordTableStackPos))
-	{
-		LOGWARNING("%s: The parameter is not a table of coords (got %s). Ignoring the call.",
-			__FUNCTION__, lua_typename(L, lua_type(L, a_ChunkCoordTableStackPos))
-		);
-		L.LogStackTrace();
-		return false;
-	}
-
 	// Add each set of coords:
-	int NumChunks = luaL_getn(L, a_ChunkCoordTableStackPos);
-	m_Chunks.reserve(static_cast<size_t>(NumChunks));
-	for (int idx = 1; idx <= NumChunks; idx++)
-	{
-		// Push the idx-th element of the array onto stack top, check that it's a table:
-		lua_rawgeti(L, a_ChunkCoordTableStackPos, idx);
-		if (!lua_istable(L, -1))
+	a_ChunkCoordsTable.ForEachArrayElement([=](cLuaState & a_LuaState, int a_Index)
 		{
-			LOGWARNING("%s: Element #%d is not a table (got %s). Ignoring the element.",
-				__FUNCTION__, idx, lua_typename(L, -1)
-			);
-			L.LogStackTrace();
-			lua_pop(L, 1);
-			continue;
+			if (!lua_istable(a_LuaState, -1))
+			{
+				LOGWARNING("%s: Element #%d is not a table (got %s). Ignoring the element.",
+					__FUNCTION__, a_Index, lua_typename(a_LuaState, -1)
+				);
+				a_LuaState.LogStackTrace();
+				lua_pop(a_LuaState, 1);
+				return false;
+			}
+			AddChunkCoord(a_LuaState, a_Index);
+			return false;
 		}
-		AddChunkCoord(L, idx);
-		lua_pop(L, 1);
-	}
+	);
 
 	// If there are no chunks, log a warning and return failure:
 	if (m_Chunks.empty())
 	{
-		LOGWARNING("%s: Zero chunks to stay.", __FUNCTION__);
-		L.LogStackTrace();
+		LOGWARNING("%s: No valid chunk coords.", __FUNCTION__);
+		a_ChunkCoordsTable.GetLuaState().LogStackTrace();
 		return false;
 	}
 

src/Bindings/LuaChunkStay.h

@@ -30,24 +30,19 @@ class cLuaChunkStay
 	typedef cChunkStay super;
 
 public:
-	cLuaChunkStay(cPluginLua & a_Plugin);
+	cLuaChunkStay();
 
 	~cLuaChunkStay() { }
 
-	/** Adds chunks in the specified on-stack Lua table.
+	/** Adds chunks in the specified Lua table.
+	Can be called only once.
 	Returns true if any chunk added, false (plus log warning) if none. */
-	bool AddChunks(int a_ChunkCoordTableStackPos);
+	bool AddChunks(const cLuaState::cStackTable & a_ChunkCoords);
 
 	/** Enables the ChunkStay for the specified chunkmap, with the specified Lua callbacks. */
 	void Enable(cChunkMap & a_ChunkMap, cLuaState::cCallbackPtr a_OnChunkAvailable, cLuaState::cCallbackPtr a_OnAllChunksAvailable);
 
 protected:
-	/** The plugin which has created the ChunkStay, via cWorld:ChunkStay() binding method.  */
-	cPluginLua & m_Plugin;
-
-	/** The Lua state associated with the callbacks. Only valid when enabled. */
-	cLuaState * m_LuaState;
-
 	/** The Lua function to call in OnChunkAvailable. Only valid when enabled. */
 	cLuaState::cCallbackPtr m_OnChunkAvailable;
 

src/Bindings/LuaState.cpp

@@ -264,6 +264,26 @@ bool cLuaState::cCallback::RefStack(cLuaState & a_LuaState, int a_StackPos)
 
 
 
+////////////////////////////////////////////////////////////////////////////////
+// cLuaState::cOptionalCallback:
+
+bool cLuaState::cOptionalCallback::RefStack(cLuaState & a_LuaState, int a_StackPos)
+{
+	// If the stack pos is nil, make this an empty callback:
+	if (lua_isnil(a_LuaState, a_StackPos))
+	{
+		Clear();
+		return true;
+	}
+
+	// Use default cCallback implementation:
+	return Super::RefStack(a_LuaState, a_StackPos);
+}
+
+
+
+
+
 ////////////////////////////////////////////////////////////////////////////////
 // cLuaState::cTableRef:
 
@@ -282,6 +302,45 @@ bool cLuaState::cTableRef::RefStack(cLuaState & a_LuaState, int a_StackPos)
 
 
 
+////////////////////////////////////////////////////////////////////////////////
+// cLuaState::cStackTable:
+
+cLuaState::cStackTable::cStackTable(cLuaState & a_LuaState, int a_StackPos):
+	m_LuaState(a_LuaState),
+	m_StackPos(a_StackPos)
+{
+	ASSERT(lua_istable(a_LuaState, a_StackPos));
+}
+
+
+
+
+
+void cLuaState::cStackTable::ForEachArrayElement(std::function<bool(cLuaState & a_LuaState, int a_Index)> a_ElementCallback) const
+{
+	auto numElements = luaL_getn(m_LuaState, m_StackPos);
+	#ifdef _DEBUG
+		auto stackTop = lua_gettop(m_LuaState);
+	#endif
+	for (int idx = 1; idx <= numElements; idx++)
+	{
+		// Push the idx-th element of the array onto stack top and call the callback:
+		lua_rawgeti(m_LuaState, m_StackPos, idx);
+		auto shouldAbort = a_ElementCallback(m_LuaState, idx);
+		ASSERT(lua_gettop(m_LuaState) == stackTop + 1);  // The element callback must not change the Lua stack below the value
+		lua_pop(m_LuaState, 1);
+		if (shouldAbort)
+		{
+			// The callback wants to abort
+			return;
+		}
+	}
+}
+
+
+
+
+
 ////////////////////////////////////////////////////////////////////////////////
 // cLuaState:
 
@@ -1086,11 +1145,20 @@ bool cLuaState::GetStackValue(int a_StackPos, cCallbackPtr & a_Callback)
 
 
 
-bool cLuaState::GetStackValue(int a_StackPos, cCallbackSharedPtr & a_Callback)
+bool cLuaState::GetStackValue(int a_StackPos, cOptionalCallback & a_Callback)
+{
+	return a_Callback.RefStack(*this, a_StackPos);
+}
+
+
+
+
+
+bool cLuaState::GetStackValue(int a_StackPos, cOptionalCallbackPtr & a_Callback)
 {
 	if (a_Callback == nullptr)
 	{
-		a_Callback = std::make_shared<cCallback>();
+		a_Callback = cpp14::make_unique<cOptionalCallback>();
 	}
 	return a_Callback->RefStack(*this, a_StackPos);
 }
@@ -1099,22 +1167,13 @@ bool cLuaState::GetStackValue(int a_StackPos, cCallbackSharedPtr & a_Callback)
 
 
 
-bool cLuaState::GetStackValue(int a_StackPos, cTableRef & a_TableRef)
+bool cLuaState::GetStackValue(int a_StackPos, cCallbackSharedPtr & a_Callback)
 {
-	return a_TableRef.RefStack(*this, a_StackPos);
-}
-
-
-
-
-
-bool cLuaState::GetStackValue(int a_StackPos, cTableRefPtr & a_TableRef)
-{
-	if (a_TableRef == nullptr)
+	if (a_Callback == nullptr)
 	{
-		a_TableRef = cpp14::make_unique<cTableRef>();
+		a_Callback = std::make_shared<cCallback>();
 	}
-	return a_TableRef->RefStack(*this, a_StackPos);
+	return a_Callback->RefStack(*this, a_StackPos);
 }
 
 
@@ -1145,6 +1204,45 @@ bool cLuaState::GetStackValue(int a_StackPos, cRef & a_Ref)
 
 
 
+bool cLuaState::GetStackValue(int a_StackPos, cStackTablePtr & a_StackTable)
+{
+	// Only allow tables to be stored in a_StackTable:
+	if (!lua_istable(m_LuaState, a_StackPos))
+	{
+		return false;
+	}
+
+	// Assign the StackTable to the specified stack position:
+	a_StackTable = cpp14::make_unique<cStackTable>(*this, a_StackPos);
+	return true;
+}
+
+
+
+
+
+bool cLuaState::GetStackValue(int a_StackPos, cTableRef & a_TableRef)
+{
+	return a_TableRef.RefStack(*this, a_StackPos);
+}
+
+
+
+
+
+bool cLuaState::GetStackValue(int a_StackPos, cTableRefPtr & a_TableRef)
+{
+	if (a_TableRef == nullptr)
+	{
+		a_TableRef = cpp14::make_unique<cTableRef>();
+	}
+	return a_TableRef->RefStack(*this, a_StackPos);
+}
+
+
+
+
+
 bool cLuaState::GetStackValue(int a_StackPos, cTrackedRef & a_Ref)
 {
 	return a_Ref.RefStack(*this, a_StackPos);

src/Bindings/LuaState.h

@@ -237,13 +237,43 @@ public:
 	typedef SharedPtr<cCallback> cCallbackSharedPtr;
 
 
+	/** Same thing as cCallback, but GetStackValue() won't fail if the callback value is nil.
+	Used for callbacks that are optional - they needn't be present and in such a case they won't get called. */
+	class cOptionalCallback:
+		public cCallback
+	{
+		typedef cCallback Super;
+
+	public:
+
+		cOptionalCallback(void) {}
+
+		/** Set the contained callback to the function in the specified Lua state's stack position.
+		If a callback has been previously contained, it is unreferenced first.
+		Returns true on success, false on failure (not a function at the specified stack pos). */
+		bool RefStack(cLuaState & a_LuaState, int a_StackPos);
+
+	protected:
+
+		/** This class cannot be copied, because it is tracked in the LuaState by-ptr.
+		Use cCallbackPtr for a copyable object. */
+		cOptionalCallback(const cOptionalCallback &) = delete;
+
+		/** This class cannot be moved, because it is tracked in the LuaState by-ptr.
+		Use cCallbackPtr for a copyable object. */
+		cOptionalCallback(cOptionalCallback &&) = delete;
+	};
+	typedef UniquePtr<cOptionalCallback> cOptionalCallbackPtr;
+
+
 	/** Represents a stored Lua table with callback functions that C++ code can call.
 	Is thread-safe and unload-safe.
 	When Lua state is unloaded, the CallFn() will return failure instead of calling into non-existent code.
-	To receive the callback instance from the Lua side, use RefStack() or (better) cLuaState::GetStackValue()
-	with a cCallbackPtr. Note that instances of this class are tracked in the canon LuaState instance, so that
-	they can be invalidated when the LuaState is unloaded; due to multithreading issues they can only be tracked
-	by-ptr, which has an unfortunate effect of disabling the copy and move constructors. */
+	To receive the cTableRef instance from the Lua side, use RefStack() or (better) cLuaState::GetStackValue()
+	with a cTableRefPtr.
+	Note that instances of this class are tracked in the canon LuaState instance, so that they can be
+	invalidated when the LuaState is unloaded; due to multithreading issues they can only be tracked by-ptr,
+	which has an unfortunate effect of disabling the copy and move constructors. */
 	class cTableRef:
 		public cTrackedRef
 	{
@@ -342,6 +372,40 @@ public:
 	};
 
 
+	/** Represents a table on the Lua stack.
+	Provides easy access to the elements in the table.
+	Note that this class doesn't have cTrackedRef's thread-safeness and unload-safeness, it is expected to be
+	used for immediate queries in API bindings. */
+	class cStackTable
+	{
+	public:
+		cStackTable(cLuaState & a_LuaState, int a_StackPos);
+
+		/** Iterates over all array elements in the table consecutively and calls the a_ElementCallback for each.
+		The callback receives the LuaState in which the table resides, and the element's index. The LuaState has
+		the element on top of its stack. If the callback returns true, the iteration is aborted, if it returns
+		false, the iteration continues with the next element. */
+		void ForEachArrayElement(std::function<bool(cLuaState & a_LuaState, int a_Index)> a_ElementCallback) const;
+
+		/** Iterates over all dictionary elements in the table in random order, and calls the a_ElementCallback for
+		each of them.
+		The callback receives the LuaState in which the table reside. The LuaState has the element on top of its
+		stack, and the element's key just below it. If the callback returns true, the iteration is aborted, if it
+		returns false, the iteration continues with the next element. */
+		void ForEachElement(std::function<bool(cLuaState & a_LuaState)> a_ElementCallback) const;
+
+		cLuaState & GetLuaState(void) const { return m_LuaState; }
+
+	protected:
+		/** The Lua state in which the table resides. */
+		cLuaState & m_LuaState;
+
+		/** The stack index where the table resides in the Lua state. */
+		int m_StackPos;
+	};
+	typedef UniquePtr<cStackTable> cStackTablePtr;
+
+
 	/** Creates a new instance. The LuaState is not initialized.
 	a_SubsystemName is used for reporting problems in the console, it is "plugin %s" for plugins,
 	or "LuaScript" for the cLuaScript template
@@ -439,10 +503,13 @@ public:
 	bool GetStackValue(int a_StackPos, cCallback & a_Callback);
 	bool GetStackValue(int a_StackPos, cCallbackPtr & a_Callback);
 	bool GetStackValue(int a_StackPos, cCallbackSharedPtr & a_Callback);
-	bool GetStackValue(int a_StackPos, cTableRef & a_TableRef);
-	bool GetStackValue(int a_StackPos, cTableRefPtr & a_TableRef);
+	bool GetStackValue(int a_StackPos, cOptionalCallback & a_Callback);
+	bool GetStackValue(int a_StackPos, cOptionalCallbackPtr & a_Callback);
 	bool GetStackValue(int a_StackPos, cPluginManager::CommandResult & a_Result);
 	bool GetStackValue(int a_StackPos, cRef & a_Ref);
+	bool GetStackValue(int a_StackPos, cStackTablePtr & a_StackTable);
+	bool GetStackValue(int a_StackPos, cTableRef & a_TableRef);
+	bool GetStackValue(int a_StackPos, cTableRefPtr & a_TableRef);
 	bool GetStackValue(int a_StackPos, cTrackedRef & a_Ref);
 	bool GetStackValue(int a_StackPos, cTrackedRefPtr & a_Ref);
 	bool GetStackValue(int a_StackPos, cTrackedRefSharedPtr & a_Ref);

src/Bindings/ManualBindings.cpp

@@ -12,7 +12,6 @@
 #include "PluginLua.h"
 #include "PluginManager.h"
 #include "LuaWindow.h"
-#include "LuaChunkStay.h"
 #include "../Root.h"
 #include "../World.h"
 #include "../Entities/Player.h"

src/Bindings/ManualBindings_World.cpp

@@ -79,33 +79,34 @@ static int tolua_cWorld_ChunkStay(lua_State * tolua_S)
 		return 0;
 	}
 
-	cPluginLua * Plugin = cManualBindings::GetLuaPlugin(tolua_S);
-	if (Plugin == nullptr)
+	// Read the params:
+	cWorld * world;
+	cLuaState::cStackTablePtr chunkCoords;
+	cLuaState::cOptionalCallbackPtr onChunkAvailable, onAllChunksAvailable;  // Callbacks may be unassigned at all - as a request to load / generate chunks
+	if (!L.GetStackValues(1, world, chunkCoords, onChunkAvailable, onAllChunksAvailable))
 	{
+		LOGWARNING("cWorld:ChunkStay(): Cannot read parameters, bailing out.");
+		L.LogStackTrace();
+		L.LogStackValues("Values on the stack");
 		return 0;
 	}
-
-	// Read the params:
-	cWorld * World = reinterpret_cast<cWorld *>(tolua_tousertype(tolua_S, 1, nullptr));
-	if (World == nullptr)
+	if (world == nullptr)
 	{
 		LOGWARNING("World:ChunkStay(): invalid world parameter");
 		L.LogStackTrace();
 		return 0;
 	}
+	ASSERT(chunkCoords != nullptr);  // If the table was invalid, GetStackValues() would have failed
 
-	cLuaChunkStay * ChunkStay = new cLuaChunkStay(*Plugin);
-
-	if (!ChunkStay->AddChunks(2))
+	// Read the chunk coords:
+	auto chunkStay = cpp14::make_unique<cLuaChunkStay>();
+	if (!chunkStay->AddChunks(*chunkCoords))
 	{
-		delete ChunkStay;
-		ChunkStay = nullptr;
 		return 0;
 	}
 
-	cLuaState::cCallbackPtr onChunkAvailable, onAllChunksAvailable;
-	L.GetStackValues(3, onChunkAvailable, onAllChunksAvailable);  // Callbacks may be unassigned at all - as a request to load / generate chunks
-	ChunkStay->Enable(*World->GetChunkMap(), std::move(onChunkAvailable), std::move(onAllChunksAvailable));
+	// Activate the ChunkStay:
+	chunkStay.release()->Enable(*world->GetChunkMap(), std::move(onChunkAvailable), std::move(onAllChunksAvailable));
 	return 0;
 }