Removed the create_npc wrapper and the last two NPC callbacks

When creating an NPC, you now provide its optional talk and update functions
directly rather than them being stored in a table on the Lua side and then
called in response to a global callback.

Also fixed an issue with a missing gender parameter to the delayed NPC
creation callback used by NPCs defined on the map (found by Erik while
reviewing this patch).

Reviewed-by: Erik Schilling

11 files changed, 156 insertions, 72 deletions

diff --git a/src/common/manaserv_protocol.h b/src/common/manaserv_protocol.h
index 6c34276d..a78c4734 100644
--- a/src/common/manaserv_protocol.h
+++ b/src/common/manaserv_protocol.h
@@ -461,7 +461,7 @@ inline ManaServ::BeingGender getGender(int gender)
         default:
             return ManaServ::GENDER_UNSPECIFIED;
     }
-};
+}
 
 /**
 * Helper function for getting gender by string
@@ -474,7 +474,7 @@ inline ManaServ::BeingGender getGender(std::string gender)
         return ManaServ::GENDER_FEMALE;
     else
         return ManaServ::GENDER_UNSPECIFIED;
-};
+}
 
 } // namespace ManaServ
 
diff --git a/src/game-server/character.cpp b/src/game-server/character.cpp
index bf16e268..ccd629e8 100644
--- a/src/game-server/character.cpp
+++ b/src/game-server/character.cpp
@@ -86,6 +86,7 @@ Character::Character(MessageIn &msg):
     mRecalculateLevel(true),
     mParty(0),
     mTransaction(TRANS_NONE),
+    mTalkNpcId(0),
     mNpcThread(0)
 {
     const AttributeManager::AttributeScope &attr =
@@ -696,6 +697,31 @@ AttribmodResponseCode Character::useCorrectionPoint(size_t attribute)
     return ATTRIBMOD_OK;
 }
 
+void Character::startNpcThread(Script::Thread *thread, int npcId)
+{
+    mNpcThread = thread;
+    mTalkNpcId = npcId;
+
+    resumeNpcThread();
+}
+
+void Character::resumeNpcThread()
+{
+    Script *script = ScriptManager::currentState();
+
+    assert(script->getCurrentThread() == mNpcThread);
+
+    if (script->resume())
+    {
+        MessageOut msg(GPMSG_NPC_CLOSE);
+        msg.writeInt16(mTalkNpcId);
+        gameHandler->sendTo(this, msg);
+
+        mTalkNpcId = 0;
+        mNpcThread = 0;
+    }
+}
+
 void Character::disconnected()
 {
     mConnected = false;
diff --git a/src/game-server/character.h b/src/game-server/character.h
index 1b31c611..1f82a101 100644
--- a/src/game-server/character.h
+++ b/src/game-server/character.h
@@ -348,12 +348,31 @@ class Character : public Being
         void setCorrectionPoints(int points) { mCorrectionPoints = points; }
         int getCorrectionPoints() const { return mCorrectionPoints; }
 
-        void setNpcThread(Script::Thread *thread)
-        { mNpcThread = thread; }
 
+        /**
+         * Starts the given NPC thread.
+         *
+         * Should be called immediately after creating the thread and pushing
+         * the NPC function and its parameters.
+         */
+        void startNpcThread(Script::Thread *thread, int npcId);
+
+        /**
+         * Resumes the given NPC thread of this character and sends the NPC
+         * close message to the player when the script is done.
+         *
+         * Should be called after preparing the current Script instance for
+         * resuming the thread and pushing the parameters the script expects.
+         */
+        void resumeNpcThread();
+
+        /**
+         * Returns the NPC thread in use by this character, if any.
+         */
         Script::Thread *getNpcThread() const
         { return mNpcThread; }
 
+
         /**
          * Gets the way the actor is blocked by other things on the map
          */
@@ -472,6 +491,8 @@ class Character : public Being
         int mParty;                  /**< Party id of the character */
         TransactionType mTransaction; /**< Trade/buy/sell action the character is involved in. */
         std::map<int, int> mKillCount;  /**< How many monsters the character has slain of each type */
+
+        int mTalkNpcId;              /**< Public ID of NPC the character is talking to, if any */
         Script::Thread *mNpcThread;  /**< Script thread executing NPC interaction, if any */
 
         static Script::Ref mDeathCallback;
diff --git a/src/game-server/mapcomposite.cpp b/src/game-server/mapcomposite.cpp
index a0d48f5b..6b2b74f9 100644
--- a/src/game-server/mapcomposite.cpp
+++ b/src/game-server/mapcomposite.cpp
@@ -769,12 +769,14 @@ void MapComposite::initializeContent()
         else if (utils::compareStrI(type, "NPC") == 0)
         {
             int npcId = utils::stringToInt(object->getProperty("NPC_ID"));
+            std::string gender = object->getProperty("GENDER");
             std::string scriptText = object->getProperty("SCRIPT");
 
             if (npcId && !scriptText.empty())
             {
                 Script *script = ScriptManager::currentState();
                 script->loadNPC(object->getName(), npcId,
+                                ManaServ::getGender(gender),
                                 object->getX(), object->getY(),
                                 scriptText.c_str());
             }
diff --git a/src/game-server/npc.cpp b/src/game-server/npc.cpp
index aa032c98..5583c3d5 100644
--- a/src/game-server/npc.cpp
+++ b/src/game-server/npc.cpp
@@ -19,13 +19,12 @@
  */
 
 #include "game-server/character.h"
+#include "game-server/gamehandler.h"
 #include "game-server/npc.h"
+#include "net/messageout.h"
 #include "scripting/script.h"
 #include "scripting/scriptmanager.h"
 
-Script::Ref NPC::mStartCallback;
-Script::Ref NPC::mUpdateCallback;
-
 NPC::NPC(const std::string &name, int id):
     Being(OBJECT_NPC),
     mID(id),
@@ -34,7 +33,14 @@ NPC::NPC(const std::string &name, int id):
     setName(name);
 }
 
-void NPC::enable(bool enabled)
+NPC::~NPC()
+{
+    Script *script = ScriptManager::currentState();
+    script->unref(mTalkCallback);
+    script->unref(mUpdateCallback);
+}
+
+void NPC::setEnabled(bool enabled)
 {
     mEnabled = enabled;
 }
@@ -52,7 +58,7 @@ void NPC::update()
 
 void NPC::prompt(Character *ch, bool restart)
 {
-    if (!mEnabled || !mStartCallback.isValid())
+    if (!mEnabled || !mTalkCallback.isValid())
         return;
 
     Script *script = ScriptManager::currentState();
@@ -61,12 +67,10 @@ void NPC::prompt(Character *ch, bool restart)
     {
         Script::Thread *thread = script->newThread();
         thread->mMap = getMap();
-        script->prepare(mStartCallback);
+        script->prepare(mTalkCallback);
         script->push(this);
         script->push(ch);
-
-        if (!script->resume())
-            ch->setNpcThread(thread);
+        ch->startNpcThread(thread, getPublicID());
     }
     else
     {
@@ -75,8 +79,7 @@ void NPC::prompt(Character *ch, bool restart)
             return;
 
         script->prepareResume(thread);
-        if (script->resume())
-            ch->setNpcThread(0);
+        ch->resumeNpcThread();
     }
 }
 
@@ -92,8 +95,7 @@ void NPC::select(Character *ch, int index)
     Script *script = ScriptManager::currentState();
     script->prepareResume(thread);
     script->push(index);
-    if (script->resume())
-        ch->setNpcThread(0);
+    ch->resumeNpcThread();
 }
 
 void NPC::integerReceived(Character *ch, int value)
@@ -108,8 +110,7 @@ void NPC::integerReceived(Character *ch, int value)
     Script *script = ScriptManager::currentState();
     script->prepareResume(thread);
     script->push(value);
-    if (script->resume())
-        ch->setNpcThread(0);
+    ch->resumeNpcThread();
 }
 
 void NPC::stringReceived(Character *ch, const std::string &value)
@@ -124,6 +125,17 @@ void NPC::stringReceived(Character *ch, const std::string &value)
     Script *script = ScriptManager::currentState();
     script->prepareResume(thread);
     script->push(value);
-    if (script->resume())
-        ch->setNpcThread(0);
+    ch->resumeNpcThread();
+}
+
+void NPC::setTalkCallback(Script::Ref function)
+{
+    ScriptManager::currentState()->unref(mTalkCallback);
+    mTalkCallback = function;
+}
+
+void NPC::setUpdateCallback(Script::Ref function)
+{
+    ScriptManager::currentState()->unref(mUpdateCallback);
+    mUpdateCallback = function;
 }
diff --git a/src/game-server/npc.h b/src/game-server/npc.h
index 4bff9af0..f62f72c7 100644
--- a/src/game-server/npc.h
+++ b/src/game-server/npc.h
@@ -22,8 +22,8 @@
 #define GAMESERVER_NPC_H
 
 #include "game-server/being.h"
+#include "scripting/script.h"
 
-class Script;
 class Character;
 
 /**
@@ -34,12 +34,27 @@ class NPC : public Being
     public:
         NPC(const std::string &name, int id);
 
+        ~NPC();
+
+        /**
+         * Sets the function that should be called when this NPC is talked to.
+         */
+        void setTalkCallback(Script::Ref function);
+
+        /**
+         * Sets the function that should be called each update.
+         */
+        void setUpdateCallback(Script::Ref function);
+
+        /**
+         * Calls the update callback, if any.
+         */
         void update();
 
         /**
-         * Enables the NPC
+         * Sets whether the NPC is enabled.
          */
-        void enable(bool enabled);
+        void setEnabled(bool enabled);
 
         /**
          * Prompts NPC.
@@ -73,12 +88,6 @@ class NPC : public Being
         virtual unsigned char getWalkMask() const
         { return 0x83; } // blocked like a monster by walls, monsters and characters ( bin 1000 0011)
 
-        static void setStartCallback(Script *script)
-        { script->assignCallback(mStartCallback); }
-
-        static void setUpdateCallback(Script *script)
-        { script->assignCallback(mUpdateCallback); }
-
     protected:
         /**
          * Gets the way a monster blocks pathfinding for other objects
@@ -90,8 +99,8 @@ class NPC : public Being
         unsigned short mID; /**< ID of the NPC. */
         bool mEnabled;      /**< Whether NPC is enabled */
 
-        static Script::Ref mStartCallback;
-        static Script::Ref mUpdateCallback;
+        Script::Ref mTalkCallback;
+        Script::Ref mUpdateCallback;
 };
 
 #endif // GAMESERVER_NPC_H
diff --git a/src/scripting/lua.cpp b/src/scripting/lua.cpp
index a416771f..273bdf42 100644
--- a/src/scripting/lua.cpp
+++ b/src/scripting/lua.cpp
@@ -112,20 +112,6 @@ static int on_update(lua_State *s)
     return 0;
 }
 
-static int on_npc_start(lua_State *s)
-{
-    luaL_checktype(s, 1, LUA_TFUNCTION);
-    NPC::setStartCallback(getScript(s));
-    return 0;
-}
-
-static int on_npc_update(lua_State *s)
-{
-    luaL_checktype(s, 1, LUA_TFUNCTION);
-    NPC::setUpdateCallback(getScript(s));
-    return 0;
-}
-
 static int on_create_npc_delayed(lua_State *s)
 {
     luaL_checktype(s, 1, LUA_TFUNCTION);
@@ -316,8 +302,10 @@ static int npc_ask_string(lua_State *s)
 }
 
 /**
- * mana.npc_create(string name, int id, int gender, int x, int y): NPC*
- * Callback for creating a NPC on the current map with the current script.
+ * mana.npc_create(string name, int id, int gender, int x, int y,
+ *                 function talk, function update): NPC*
+ *
+ * Callback for creating a NPC on the current map.
  */
 static int npc_create(lua_State *s)
 {
@@ -327,33 +315,36 @@ static int npc_create(lua_State *s)
     const int x = luaL_checkint(s, 4);
     const int y = luaL_checkint(s, 5);
 
+    if (!lua_isnoneornil(s, 6))
+        luaL_checktype(s, 6, LUA_TFUNCTION);
+    if (!lua_isnoneornil(s, 7))
+        luaL_checktype(s, 7, LUA_TFUNCTION);
+
     MapComposite *m = checkCurrentMap(s);
 
     NPC *q = new NPC(name, id);
     q->setGender(getGender(gender));
     q->setMap(m);
     q->setPosition(Point(x, y));
+
+    if (lua_isfunction(s, 6))
+    {
+        lua_pushvalue(s, 6);
+        q->setTalkCallback(luaL_ref(s, LUA_REGISTRYINDEX));
+    }
+
+    if (lua_isfunction(s, 7))
+    {
+        lua_pushvalue(s, 7);
+        q->setUpdateCallback(luaL_ref(s, LUA_REGISTRYINDEX));
+    }
+
     GameState::enqueueInsert(q);
     lua_pushlightuserdata(s, q);
     return 1;
 }
 
 /**
- * mana.npc_end(NPC*, Character*): void
- * Callback for ending a NPC conversation with the given character.
- */
-static int npc_end(lua_State *s)
-{
-    NPC *p = checkNPC(s, 1);
-    Character *q = checkCharacter(s, 2);
-
-    MessageOut msg(GPMSG_NPC_CLOSE);
-    msg.writeInt16(p->getPublicID());
-    gameHandler->sendTo(q, msg);
-    return 0;
-}
-
-/**
  * mana.npc_post(NPC*, Character*): void
  * Callback for sending a NPC_POST.
  */
@@ -376,7 +367,7 @@ static int npc_post(lua_State *s)
 static int npc_enable(lua_State *s)
 {
     NPC *p = checkNPC(s, 1);
-    p->enable(true);
+    p->setEnabled(true);
     GameState::enqueueInsert(p);
     return 0;
 }
@@ -388,7 +379,7 @@ static int npc_enable(lua_State *s)
 static int npc_disable(lua_State *s)
 {
     NPC *p = checkNPC(s, 1);
-    p->enable(false);
+    p->setEnabled(false);
     GameState::remove(p);
     return 0;
 }
@@ -2152,8 +2143,6 @@ LuaScript::LuaScript():
         { "on_being_death",                  &on_being_death                  },
         { "on_being_remove",                 &on_being_remove                 },
         { "on_update",                       &on_update                       },
-        { "on_npc_start",                    &on_npc_start                    },
-        { "on_npc_update",                   &on_npc_update                   },
         { "on_create_npc_delayed",           &on_create_npc_delayed           },
         { "on_map_initialize",               &on_map_initialize               },
         { "on_craft",                        &on_craft                        },
@@ -2243,7 +2232,6 @@ LuaScript::LuaScript():
         { "item_drop",                       &item_drop                       },
         { "item_get_name",                   &item_get_name                   },
         { "npc_ask_integer",                 &npc_ask_integer                 },
-        { "npc_end",                         &npc_end                         },
         { "npc_ask_string",                  &npc_ask_string                  },
         { "log",                             &log                             },
         { "get_distance",                    &get_distance                    },
diff --git a/src/scripting/luascript.cpp b/src/scripting/luascript.cpp
index e45588b9..36adb912 100644
--- a/src/scripting/luascript.cpp
+++ b/src/scripting/luascript.cpp
@@ -198,6 +198,15 @@ void LuaScript::assignCallback(Script::Ref &function)
     function.value = luaL_ref(mRootState, LUA_REGISTRYINDEX);
 }
 
+void LuaScript::unref(Ref &ref)
+{
+    if (ref.isValid())
+    {
+        luaL_unref(mRootState, LUA_REGISTRYINDEX, ref.value);
+        ref.value = -1;
+    }
+}
+
 void LuaScript::load(const char *prog, const char *name)
 {
     int res = luaL_loadbuffer(mRootState, prog, std::strlen(prog), name);
@@ -262,7 +271,7 @@ void LuaScript::getQuestCallback(Character *q,
 
     script->prepareResume(thread);
     script->push(value);
-    script->resume();
+    q->resumeNpcThread();
 }
 
 /**
@@ -280,7 +289,7 @@ void LuaScript::getPostCallback(Character *q,
     script->prepareResume(thread);
     script->push(sender);
     script->push(letter);
-    script->resume();
+    q->resumeNpcThread();
 }
 
 
diff --git a/src/scripting/luascript.h b/src/scripting/luascript.h
index 57b8dc1a..955fe21c 100644
--- a/src/scripting/luascript.h
+++ b/src/scripting/luascript.h
@@ -64,6 +64,8 @@ class LuaScript : public Script
 
         void assignCallback(Ref &function);
 
+        void unref(Ref &ref);
+
         static void getQuestCallback(Character *,
                                      const std::string &value,
                                      Script *);
diff --git a/src/scripting/script.cpp b/src/scripting/script.cpp
index 6312d1ce..074546ef 100644
--- a/src/scripting/script.cpp
+++ b/src/scripting/script.cpp
@@ -111,7 +111,10 @@ bool Script::loadFile(const std::string &name)
     }
 }
 
-void Script::loadNPC(const std::string &name, int id, int x, int y,
+void Script::loadNPC(const std::string &name,
+                     int id,
+                     ManaServ::BeingGender gender,
+                     int x, int y,
                      const char *prog)
 {
     if (!mCreateNpcDelayedCallback.isValid())
@@ -124,6 +127,7 @@ void Script::loadNPC(const std::string &name, int id, int x, int y,
     prepare(mCreateNpcDelayedCallback);
     push(name);
     push(id);
+    push(gender);
     push(x);
     push(y);
     execute();
diff --git a/src/scripting/script.h b/src/scripting/script.h
index b55f204e..f6d6d180 100644
--- a/src/scripting/script.h
+++ b/src/scripting/script.h
@@ -22,6 +22,7 @@
 #define SCRIPTING_SCRIPT_H
 
 #include "common/inventorydata.h"
+#include "common/manaserv_protocol.h"
 #include "game-server/eventlistener.h"
 
 #include <list>
@@ -61,6 +62,7 @@ class Script
         {
             public:
                 Ref() : value(-1) {}
+                Ref(int value) : value(value) {}
                 bool isValid() const { return value != -1; }
                 int value;
         };
@@ -111,7 +113,10 @@ class Script
          * Loads a chunk of text and considers it as an NPC handler. This
          * handler will later be used to create the given NPC.
          */
-        virtual void loadNPC(const std::string &name, int id, int x, int y,
+        virtual void loadNPC(const std::string &name,
+                             int id,
+                             ManaServ::BeingGender gender,
+                             int x, int y,
                              const char *);
 
         /**
@@ -187,6 +192,12 @@ class Script
         virtual void assignCallback(Ref &function) = 0;
 
         /**
+         * Unreferences the script object given by \a ref, if any, and sets
+         * \a ref to invalid.
+         */
+        virtual void unref(Ref &ref) = 0;
+
+        /**
          * Returns the currently executing thread, or null when no thread is
          * currently executing.
          */