From 0f51f7e35883936a64857a12ca5a97eaa1c9e190 Mon Sep 17 00:00:00 2001 From: Mattes D Date: Sun, 14 Aug 2016 16:26:31 +0200 Subject: [PATCH] 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 | 52 ++++------- src/Bindings/LuaChunkStay.h | 13 +-- src/Bindings/LuaState.cpp | 128 +++++++++++++++++++++++--- src/Bindings/LuaState.h | 79 ++++++++++++++-- src/Bindings/ManualBindings.cpp | 1 - src/Bindings/ManualBindings_World.cpp | 29 +++--- 6 files changed, 222 insertions(+), 80 deletions(-) diff --git a/src/Bindings/LuaChunkStay.cpp b/src/Bindings/LuaChunkStay.cpp index 145a9ee1b..d885470db 100644 --- a/src/Bindings/LuaChunkStay.cpp +++ b/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(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; } diff --git a/src/Bindings/LuaChunkStay.h b/src/Bindings/LuaChunkStay.h index 51356d5b7..175260978 100644 --- a/src/Bindings/LuaChunkStay.h +++ b/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; diff --git a/src/Bindings/LuaState.cpp b/src/Bindings/LuaState.cpp index e6a94091e..fd29e8e34 100644 --- a/src/Bindings/LuaState.cpp +++ b/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 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(); + a_Callback = cpp14::make_unique(); } 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(); + a_Callback = std::make_shared(); } - 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(*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(); + } + return a_TableRef->RefStack(*this, a_StackPos); +} + + + + + bool cLuaState::GetStackValue(int a_StackPos, cTrackedRef & a_Ref) { return a_Ref.RefStack(*this, a_StackPos); diff --git a/src/Bindings/LuaState.h b/src/Bindings/LuaState.h index bc88fbf1b..4def2c663 100644 --- a/src/Bindings/LuaState.h +++ b/src/Bindings/LuaState.h @@ -237,13 +237,43 @@ public: typedef SharedPtr 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 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 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 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 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); diff --git a/src/Bindings/ManualBindings.cpp b/src/Bindings/ManualBindings.cpp index b6a10adf3..20792393d 100644 --- a/src/Bindings/ManualBindings.cpp +++ b/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" diff --git a/src/Bindings/ManualBindings_World.cpp b/src/Bindings/ManualBindings_World.cpp index b3170a636..56a4ee65b 100644 --- a/src/Bindings/ManualBindings_World.cpp +++ b/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(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(); + 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; }