Refactor|Plugins: Fixed hidden assumption in plugin ID assignment

The mechanism for tracking the plugin ID for the currently executing
plugin hooks was a massive kludge that relied on the order in which
Plug_AddHooks() had been called. Now the plugin IDs are tracked in a
sensible manner: an ID is assigned to each plugin upon loading, after
which the engine is responsible for letting everyone know which plugin
execution has been passed to, if any.

In practice, whenever Plug_AddHook() is called from a plugin, the
engine must make sure that DD_SetActivePluginId() has previously been
called. DD_SetActivePluginId() should also be called whenever control
is passed to the functions exported from the game (gx).

Plugins can now be loaded in any order.

Todo: Ditto for Windows (use the same code).

Todo for later: Make a nice mechanism for calling a function in gx
while automatically setting the active plugin ID.

doomsday/engine/api/dd_plugin.h

@@ -62,7 +62,8 @@ enum {
     NUM_HOOK_TYPES
 };
 
-/// Unique identifier attributed to each plugin during initial startup.
+/// Unique identifier assigned to each plugin during initial startup.
+/// Zero is not a valid ID.
 typedef int pluginid_t;
 
 /// Paramaters for HOOK_FINALE_EVAL_IF
@@ -84,9 +85,8 @@ typedef struct {
 } ddhook_viewport_reshape_t;
 
 /**
- * Registers a new hook function.
- * A plugin should call this to add a hook function to be executed at the time
- * specified by @a hook_type.
+ * Registers a new hook function. A plugin can call this to add a hook
+ * function to be executed at the time specified by @a hook_type.
  *
  * @param hook_type  Hook type.
  * @param hook       Pointer to hook function.

doomsday/engine/portable/include/dd_main.h

@@ -91,8 +91,20 @@ void DD_UpdateEngineState(void);
  */
 int DD_CallHooks(int hook_type, int parm, void* data);
 
-/// @return  Unique identified of the plugin responding to active hook callback.
-pluginid_t DD_PluginIdForActiveHook(void);
+/**
+ * Sets the ID of the currently active plugin. Note that plugin hooks are
+ * executed in a single-threaded manner; only one can be active at a time.
+ *
+ * @param id  Plugin id.
+ */
+void DD_SetActivePluginId(pluginid_t id);
+
+/**
+ * @return Unique identifier of the currently active plugin. Note that plugin
+ * hooks are executed in a single-threaded manner; only one is active at a
+ * time.
+ */
+pluginid_t DD_ActivePluginId(void);
 
 /**
  * Locate the address of the named, exported procedure in the plugin.

doomsday/engine/portable/src/dd_games.cpp

@@ -506,7 +506,7 @@ gameid_t DD_DefineGame(GameDef const* def)
     Game* game = addGame(Game_FromDef(def));
     if(game)
     {
-        Game_SetPluginId(game, DD_PluginIdForActiveHook());
+        Game_SetPluginId(game, DD_ActivePluginId());
         return Games_Id(game);
     }
     return 0; // Invalid id.

doomsday/engine/portable/src/dd_main.c

@@ -573,7 +573,13 @@ static int DD_ActivateGameWorker(void* parameters)
 
     // Now that resources have been located we can begin to initialize the game.
     if(!Games_IsNullObject(theGame) && gx.PreInit)
+    {
+        DENG_ASSERT(Game_PluginId(theGame) != 0);
+
+        DD_SetActivePluginId(Game_PluginId(theGame));
         gx.PreInit(Games_Id(theGame));
+        DD_SetActivePluginId(0);
+    }
 
     if(p->initiatedBusyMode)
         Con_SetProgress(100);
@@ -647,14 +653,17 @@ static int DD_ActivateGameWorker(void* parameters)
 
     if(gx.PostInit)
     {
+        DD_SetActivePluginId(Game_PluginId(theGame));
         gx.PostInit();
+        DD_SetActivePluginId(0);
     }
 
     if(p->initiatedBusyMode)
     {
         Con_SetProgress(200);
         BusyMode_WorkerEnd();
     }
+
     return 0;
 }
 
@@ -758,7 +767,9 @@ boolean DD_ChangeGame2(Game* game, boolean allowReload)
         { // Tell the plugin it is being unloaded.
             void* unloader = DD_FindEntryPoint(Game_PluginId(theGame), "DP_Unload");
             DEBUG_Message(("DD_ChangeGame2: Calling DP_Unload (%p)\n", unloader));
+            DD_SetActivePluginId(Game_PluginId(theGame));
             if(unloader) ((pluginfunc_t)unloader)();
+            DD_SetActivePluginId(0);
         }
 
         // The current game is now the special "null-game".
@@ -855,7 +866,9 @@ boolean DD_ChangeGame2(Game* game, boolean allowReload)
             /// @todo Must this be done in the main thread?
             void* loader = DD_FindEntryPoint(Game_PluginId(theGame), "DP_Load");
             DEBUG_Message(("DD_ChangeGame2: Calling DP_Load (%p)\n", loader));
+            DD_SetActivePluginId(Game_PluginId(theGame));
             if(loader) ((pluginfunc_t)loader)();
+            DD_SetActivePluginId(0);
         }
 
         /// @kludge Use more appropriate task names when unloading a game.
@@ -882,6 +895,8 @@ boolean DD_ChangeGame2(Game* game, boolean allowReload)
         }
     }
 
+    DENG_ASSERT(DD_ActivePluginId() == 0);
+
     /**
      * Clear any input events we may have accumulated during this process.
      * @note Only necessary here because we might not have been able to use

doomsday/engine/portable/src/dd_plugin.c

@@ -40,16 +40,26 @@
 
 typedef struct {
     int exclude;
-    hookfunc_t list[MAX_HOOKS];
+    struct {
+        hookfunc_t func;
+        pluginid_t pluginId;
+    } list[MAX_HOOKS];
 } hookreg_t;
 
 static hookreg_t hooks[NUM_HOOK_TYPES];
-static pluginid_t currentPlugin = 0;
+static pluginid_t currentPlugin = 0; // none
 
 int Plug_AddHook(int hookType, hookfunc_t hook)
 {
     int i, type = HOOKMASK(hookType);
 
+    /**
+     * The current plugin must be set before calling this. The engine has the
+     * responsibility to call DD_SetActivePluginId() whenever it passes control
+     * to a plugin, and then set it back to zero after it gets control back.
+     */
+    DENG_ASSERT(DD_ActivePluginId() != 0);
+
     // The type must be good.
     if(type < 0 || type >= NUM_HOOK_TYPES)
         return false;
@@ -66,35 +76,39 @@ int Plug_AddHook(int hookType, hookfunc_t hook)
         return false;
     }
 
-    for(i = 0; i < MAX_HOOKS && hooks[type].list[i]; ++i) {};
+    for(i = 0; i < MAX_HOOKS && hooks[type].list[i].func; ++i) {};
     if(i == MAX_HOOKS)
         return false; // No more hooks allowed!
 
-    // Add the hook.
-    hooks[type].list[i] = hook;
+    // Add the hook. If the plugin is unidentified the ID will be zero.
+    hooks[type].list[i].func     = hook;
+    hooks[type].list[i].pluginId = DD_ActivePluginId();
     return true;
 }
 
 int Plug_RemoveHook(int hookType, hookfunc_t hook)
 {
     int i, type = HOOKMASK(hookType);
 
+    /*
     if(currentPlugin)
     {
         LegacyCore_PrintfLogFragmentAtLevel(DE2_LOG_WARNING,
             "Plug_RemoveHook: Failed to remove hook %p of type %i; currently processing a hook.\n",
             hook, hookType);
         return false;
     }
+    */
 
     // The type must be good.
     if(type < 0 || type >= NUM_HOOK_TYPES)
         return false;
     for(i = 0; i < MAX_HOOKS; ++i)
     {
-        if(hooks[type].list[i] != hook)
+        if(hooks[type].list[i].func != hook)
             continue;
-        hooks[type].list[i] = 0;
+        hooks[type].list[i].func = 0;
+        hooks[type].list[i].pluginId = 0;
         if(hookType & HOOKF_EXCLUSIVE)
         {   // Exclusive hook removed; allow normal hooks.
             hooks[type].exclude = false;
@@ -108,43 +122,50 @@ int Plug_CheckForHook(int hookType)
 {
     size_t i;
     for(i = 0; i < MAX_HOOKS; ++i)
-        if(hooks[hookType].list[i])
+        if(hooks[hookType].list[i].func)
             return true;
     return false;
 }
 
+void DD_SetActivePluginId(pluginid_t id)
+{
+    currentPlugin = id;
+}
+
 int DD_CallHooks(int hookType, int parm, void *data)
 {
     int ret = 0;
     boolean allGood = true;
+    pluginid_t oldPlugin = DD_ActivePluginId();
 
     // Try all the hooks.
     { int i;
     for(i = 0; i < MAX_HOOKS; ++i)
     {
-        if(!hooks[hookType].list[i])
+        if(!hooks[hookType].list[i].func)
             continue;
 
-        currentPlugin = i+1;
-        if(hooks[hookType].list[i] (hookType, parm, data))
+        DD_SetActivePluginId(hooks[hookType].list[i].pluginId);
+
+        if(hooks[hookType].list[i].func(hookType, parm, data))
         {   // One hook executed; return nonzero from this routine.
             ret = 1;
         }
         else
             allGood = false;
     }}
 
-    currentPlugin = 0;
+    DD_SetActivePluginId(oldPlugin);
 
     if(ret && allGood)
         ret |= 2;
 
     return ret;
 }
 
-pluginid_t DD_PluginIdForActiveHook(void)
+pluginid_t DD_ActivePluginId(void)
 {
-    return (pluginid_t)currentPlugin;
+    return currentPlugin;
 }
 
 void* DD_FindEntryPoint(pluginid_t pluginId, const char* fn)
@@ -165,6 +186,7 @@ void* DD_FindEntryPoint(pluginid_t pluginId, const char* fn)
         }
     }
     return adr;
+
 #elif UNIX
     void* addr = 0;
     int plugIndex = pluginId - 1;

doomsday/engine/unix/src/dd_uinit.c

@@ -91,7 +91,7 @@ static PluginHandle* findFirstUnusedPluginHandle(application_t* app)
         if(!app->hInstPlug[i])
             return &app->hInstPlug[i];
     }
-    return 0;
+    return 0; // failed
 }
 
 static int loadPlugin(const char* fileName, const char* pluginPath, void* param)
@@ -101,6 +101,7 @@ static int loadPlugin(const char* fileName, const char* pluginPath, void* param)
     PluginHandle* handle;
     void (*initializer)(void);
     filename_t name;
+    pluginid_t plugId;
 
     assert(app && pluginPath && pluginPath[0]);
 
@@ -128,7 +129,9 @@ static int loadPlugin(const char* fileName, const char* pluginPath, void* param)
         return 0; // Continue iteration.
     }
 
+    // Assign a handle and ID to the plugin.
     handle = findFirstUnusedPluginHandle(app);
+    plugId = handle - app->hInstPlug + 1;
     if(!handle)
     {
 #if _DEBUG
@@ -140,10 +143,13 @@ static int loadPlugin(const char* fileName, const char* pluginPath, void* param)
 
     // This seems to be a Doomsday plugin.
     _splitpath(pluginPath, NULL, NULL, name, NULL);
-    Con_Message("  %s\n", name);
+    Con_Message("  %s (id:%i)\n", name, plugId);
 
     *handle = plugin;
+
+    DD_SetActivePluginId(plugId);
     initializer();
+    DD_SetActivePluginId(0);
 
     return 0; // Continue iteration.
 }