• πŸ† Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. πŸ”—Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

[Lua] Hook

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Hook moves Lua's core development forward by providing users with vJass-esque hooks that allows manipulation of parameters, return values, the ability to call the original native, plays well with other hooks, and beautifies and simplifies the syntax as much as possible.


Hooking "BJDebugMsg" and, instead of allowing the original function to be called, simply call "print":
Lua:
Hook.add("BJDebugMsg", print)

A very simple timer recycling system:
Lua:
    local recycledTimers = {}
    function Hook:CreateTimer()
        if recycledTimers[1] then
            return table.remove(recycledTimers, #recycledTimers)
        else
            return self.old() --Call the CreateTimer native and return its timer
        end
    end
    function Hook:DestroyTimer(whichTimer)
        if #recycledTimers < 100 then
            table.insert(recycledTimers, whichTimer)
        else
            self.old(whichTimer) --This will not trigger recursively (but calling "DestroyTimer" again will cause recursion).
        end
    end

To add a hook to CreateUnit to show a message that appears before creation and another that shows after creation:
Lua:
function Hook:CreateUnit(...)

    local returnValue --AddHook needs to return the unit that is created, or return something else if preferred.

    print("Unit is about to be created")
 
    returnValue = self.old(...) --Calls and returns the original native (but first calls any other hooks with a lower priority)
 
    print("Unit has been created")
 
    return returnValue --Be mindful that whatever this function returns should coincide with CreateUnit's own return values, unless you WANT to overwrite the return value of course.
end



  • Priorities run from highest number down to lowest number. Not passing a priority defaults it to "0".
  • If you want to use an "after hook" which runs after the other hooks have run, you should also use a priority below zero (e.g. -1).
  • If you want to skip the original function but allow other hooks to run beforehand, you should use a priority below zero (e.g. -2).
  • If you want to change the parameters of a function to force subsequent (lower priority) hooks to run using those new parameters, you should use a priority above zero (e.g. 1).


Lua:
if Debug then Debug.beginFile "Hook" end
--β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”
-- Hook version 7.1.0.1
-- Created by: Bribe
-- Contributors: Eikonium, Jampion, MyPad, Wrda
--β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”
---@class Hook.property
---@field next function|Hook.property   --Call the next/native function. Also works with any given name (old/native/original/etc.). The args and return values align with the original function.
---@field remove fun(all?: boolean)     --Remove the hook. Pass the boolean "true" to remove all hooks.
---@field package tree HookTree         --Reference to the tree storing each hook on that particular key in that particular host.
---@field package priority number
---@field package index integer
---@field package hookAsBasicFn? function
----@field package debugId? string
----@field package debugNext? string

---@class Hook: {[integer]: Hook.property, [string]: function}
Hook = {}

do
    local looseValuesMT = { __mode = "v" }
    local hostKeyTreeMatrix = ---@type table<table, table<any, HookTree>>
        setmetatable({
            --Already add a hook matrix for _G right away.
            [_G] = setmetatable({}, looseValuesMT)
        }, looseValuesMT)

    ---@class HookTree: { [number]: Hook.property }
    ---@field host table
    ---@field key unknown --What the function was indexed to (_G items are typically indexed via strings)
    ---@field hasHookAsBasicFn boolean

    ---Reindexes a HookTree, inserting or removing a hook and updating the properties of each hook.
    ---@param tree HookTree
    ---@param index integer
    ---@param newHook? table
    local function reindexTree(tree, index, newHook)
        if newHook then
            table.insert(tree, index, newHook)
        else
            table.remove(tree, index)
        end

        local top = #tree
        local prevHook = tree[index - 1]

        -- `table.insert` and `table.remove` shift the elements upwards or downwards,
        -- so this loop manually aligns the tree elements with this shift.
        for i = index, top do
            local currentHook = tree[i]
            currentHook.index = i
            currentHook.next = (i > 1) and
                rawget(prevHook, 'hookAsBasicFn') or
                prevHook
            --currentHook.debugNext = tostring(currentHook.next)
            prevHook = currentHook
        end
        local topHookBasicFn = rawget(tree[top], 'hookAsBasicFn')
        if topHookBasicFn then
            if not tree.hasHookAsBasicFn or rawget(tree.host, tree.key) ~= topHookBasicFn then
                tree.hasHookAsBasicFn = true

                --a different basic function should be called for this hook
                --instead of the one that was previously there.
                tree.host[tree.key] = topHookBasicFn
            end
        else
            --The below comparison rules out 'nil' and 'true'.
            --Q: Why rule out nil?
            --A: There is no need to reassign a host hook handler if there is already one in place.
            if tree.hasHookAsBasicFn ~= false then
                tree.host[tree.key] = function(...)
                    return tree[#tree](...)
                end
            end
            tree.hasHookAsBasicFn = false
        end
    end

    ---@param hookProperty Hook.property
    ---@param deleteAllHooks? boolean
    function Hook.delete(hookProperty, deleteAllHooks)
        local tree = hookProperty.tree
        hookProperty.tree = nil
        if deleteAllHooks or #tree == 1 then
            --Reset the host table's native behavior for the hooked key.
            tree.host[tree.key] =
                (tree[0] ~= DoNothing) and
                    tree[0] or
                    nil

            hostKeyTreeMatrix[tree.host][tree.key] = nil
        else
            reindexTree(tree, hookProperty.index)
        end
    end

    ---@param hostTableToHook? table
    ---@param defaultNativeBehavior? function
    ---@param hookedTableIsMetaTable? boolean
    local function setupHostTable(hostTableToHook, defaultNativeBehavior, hookedTableIsMetaTable)
        hostTableToHook = hostTableToHook or _G
        if hookedTableIsMetaTable or
            (defaultNativeBehavior and hookedTableIsMetaTable == nil)
        then
            hostTableToHook = getmetatable(hostTableToHook) or
                getmetatable(setmetatable(hostTableToHook, {}))
        end
        return hostTableToHook
    end

    ---@param tree HookTree
    ---@param priority number
    local function huntThroughPriorityList(tree, priority)
        local index = 1
        local topIndex = #tree
        repeat
            if priority <= tree[index].priority then
                break
            end
            index = index + 1
        until index > topIndex
        return index
    end

    ---@param hostTableToHook table
    ---@param key unknown
    ---@param defaultNativeBehavior? function
    ---@return HookTree | nil
    local function createHookTree(hostTableToHook, key, defaultNativeBehavior)
        local nativeFn = rawget(hostTableToHook, key) or
            defaultNativeBehavior or
            ((hostTableToHook ~= _G or type(key) ~= "string") and
            DoNothing)

        if not nativeFn then
            --Logging is used here instead of directly throwing an error, because
            --no one can be sure that we're running within a debug-friendly thread.
            (Debug and Debug.throwError or print)("Hook Error: No value found for key: " .. tostring(key))
            return
        end

        ---@class HookTree
        local tree = {
            host = hostTableToHook,
            key = key,
            [0] = nativeFn,
            --debugNativeId = tostring(nativeFn)
        }
        hostKeyTreeMatrix[hostTableToHook][key] = tree
        return tree
    end

    ---@param self Hook.property
    local function __index(self)
        return self.next
    end

    ---@param key        unknown                Usually `string` (the name of the native you wish to hook)
    ---@param callbackFn fun(Hook, ...):any     The function you want to run when the native is called. The first parameter is type "Hook", and the remaining parameters (and return value(s)) align with the original function.
    ---@param priority?  number                 Defaults to 0. Hooks are called in order of highest priority down to lowest priority. The native itself has the lowest priority.
    ---@param hostTableToHook? table            Defaults to _G (the table that stores all global variables).
    ---@param defaultNativeBehavior? function   If the native does not exist in the host table, use this default instead.
    ---@param hookedTableIsMetaTable? boolean   Whether to store into the host's metatable instead. Defaults to true if the "default" parameter is given.
    ---@param hookAsBasicFn? boolean            When adding a hook instance, the default behavior is to use the __call metamethod in metatables to govern callbacks. If this is `true`, it will instead use normal function callbacks.
    ---@return Hook.property
    function Hook.add(
        key,
        callbackFn,
        priority,
        hostTableToHook,
        defaultNativeBehavior,
        hookedTableIsMetaTable,
        hookAsBasicFn
    )
        priority = priority or 0

        hostTableToHook = setupHostTable(hostTableToHook, defaultNativeBehavior, hookedTableIsMetaTable)

        hostKeyTreeMatrix[hostTableToHook] =
            hostKeyTreeMatrix[hostTableToHook] or
            setmetatable({}, looseValuesMT)

        local index = 1
        local tree = hostKeyTreeMatrix[hostTableToHook][key]
        if tree then
            index = huntThroughPriorityList(tree, priority)
        else
            ---@diagnostic disable-next-line: cast-local-type
            tree = createHookTree(hostTableToHook, key, defaultNativeBehavior)
            if not tree then
                return ---@diagnostic disable-line: missing-return-value
            end
        end
        local new = {
            priority = priority,
            tree = tree
        }
        function new.remove(deleteAllHooks)
            Hook.delete(new, deleteAllHooks)
        end
        --new.debugId = tostring(callbackFn) .. ' and ' .. tostring(new)
        if hookAsBasicFn then
            new.hookAsBasicFn = callbackFn
        else
            setmetatable(new, {
                __call = callbackFn,
                __index = __index
            })
        end
        reindexTree(tree, index, new)
        return new
    end
end

---Hook.basic avoids creating a metatable for the hook.
---This is necessary for adding hooks to metatable methods such as __index.
---The main difference versus Hook.add is in the parameters passed to callbackFn;
---Hook.add has a 'self' argument which points to the hook, whereas Hook.basic does not.
---@param key        unknown
---@param callbackFn fun(Hook, ...):any
---@param priority?  number
---@param hostTableToHook? table
---@param defaultNativeBehavior? function
---@param hookedTableIsMetaTable? boolean
function Hook.basic(key, callbackFn, priority, hostTableToHook, defaultNativeBehavior, hookedTableIsMetaTable)
    return Hook.add(key, callbackFn, priority, hostTableToHook, defaultNativeBehavior, hookedTableIsMetaTable, true)
end

---@deprecated
---@see Hook.add for args
function AddHook(...)
    local new = Hook.basic(...)
    return function(...)
        return new.next(...)
    end, new.remove
end

setmetatable(Hook, {
    __newindex = function(_, key, callback)
        Hook.add(key, callback)
    end
})

if Debug then Debug.endFile() end

Commits

Unit Tests
 
Last edited:

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
Looks good. However I'm not sure if it can solve the problem I encountered with Perfect PolledWait. For reference, this is the override it uses:
Lua:
local oldWait = PolledWait
   function PolledWait(duration)
      local thread = coroutine.running()
      if thread then
         NewTimer(function()
            coroutine.resume(thread)
         end, duration)
         coroutine.yield(thread)
      else
         oldWait(duration)
      end
   end
Since this override does not always use the oldWait, I assume it will be required to be used with doesItRunAfter=False and will return true after coroutine.yield(thread) so that the original wait is not executed. Now if I still wanted to have another hook attached to PolledWait that also works with Perfect PolledWait, I'd need to make sure that it is earlier in the hookBefore table. It also makes it impossible to have an after hook with Perfect PolledWait.

Maybe instead of returning true there could be multiple options:
1. Same as true currently: Prevents the original function from being called (as well as any other hooks)
2. Prevents the original function from being called, but still have other hooks

The second option would be more appropriate for Perfect PolledWait, because semantically it only changes HOW waits are implemented but does stop waits from being used so it makes no sense to stop after hooks attached to waits.


Another useful addition would be to be able to control where a hook is placed in the hookBefore table. In some cases placing a hook at the start of the table can be desired to avoid having the hook prevented by another hook. For example:
1. A debug message that does not stop other hooks. It won't do any harm and by having it at the start of the hookBefore table you ensure you will detect every call.
2. You want to completely disable the function call in certain situations. Then you want no other hooks to be executed, because you want it to be like the function was never called in the first place.
Maybe just have 2 tables for before. One high priority one for the above cases and then a normal one for cases where you only care that the hook is before the original call but not where it is in relation to other hooks.
 
Level 20
Joined
Jul 10, 2009
Messages
474
Can you explain, why you would prefer AddHook over the normal way of hooking?
Do you just consider it more convenient or is there any technical issue with normal hooks?
Lua:
do
    local oldCreateUnit = CreateUnit
    CreateUnit = function(player, unitCode, x, y, face)
        --hookbefore
        print("this happens before")
        local u =  oldCreateUnit(player,  unitCode, x, y, face)
        --hookafter
        print("this happens afterwards")
        return u
    end
end

Also, it looks like your replacement function does not return the original return values, so u = CreateUnit(...) would always leave u nil once CreateUnit has been hooked.

You might also consider to support multiple return values, as that is a rather common thing to do in Lua.
Granted, Wc3 natives never have multiple return values, but this would extend the scope of application to user-written functions.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Thanks both for your input on this.

@Eikonium true on that return value, I was thinking over this as I was falling asleep last night and realized I forgot to take into account the return value. I have now adjusted for that.

IF someone is overriding hooks with the traditional method, there is no way to safely "unhook" it*. If one lives in a vaccuum where they know they would not need to "unhook" something, then it is entirely safe (and more practical) to stick to the existing method. However, it's my understanding that we're trying to make this sort of thing be more extensible.

*It's not safe to "unhook" it in a scenario like the following:

Lua:
do --lib 1
    local oldCreate = CreateItem
    CreateItem = function(...) return oldCreate(...) end
end

do --lib 2
    local oldCreate = CreateItem
    CreateItem = function(...) return oldCreate(...) end
end
-- Then lib 1 says "I don't need this hook any more, let me set "CreateItem" back to oldCreate. This then overwrites the hook from lib 2.

I will try to think on how/if I can take that "multiple return values" idea into consideration. It probably wouldn't work without returning something that has a metatable assigned to it to spit out a valid argument if the caller needs a return value of a specific type or something. I currently have no idea how/if I could code that, though.

@Jampion, assigning a priority order to the hook is definitely doable, and something I'd like to do in a future update.

Edit: Putting the below here for later. I need to see if it compiles and run a number of tests on it.

Lua:
do --Hook v1.1.0.0 by Bribe
   
   local _LOW_PRIO   = -9001 -- a number to represent what should be the lowest priority for a trigger table.
   
   local hookBefore  = {} --stores a list of functions that are called prior to a hooked function.
   local priority    = {}
   local hookAfter   = {} --stores a list of functions that are called after a hooked function.
   local hook        = {} --stores a list of overriden functions
   
   function AddHook(oldFunc, userFunc, after)
      if type(oldFunc) == "string" and type(userFunc) == "function" then
         local tab, index, pt, weight
         
         if after and type(after) == "number" then
            weight = after
            after = nil
         end
         if after then
            tab = hookAfter[oldFunc]
         else
            weight = weight or _LOW_PRIO
            tab = hookBefore[oldFunc]
         end
         if tab then
            if weight then
               pt = priority[oldFunc]
               for i = 1, #tab do
                  if (not pt[i]) or pt[i] < weight then
                     table.insert(pt, i, weight) --insert at a specific point in the priority table in accordance with the weight
                     index = i
                     break
                  end
               end
               if not index then
                  index = #tab + 1
                  pt[index] = weight --this has the lowest priority. Insert at the end of the priority table.
               end
            else
               index = #tab + 1 --"hookAfter" doesn't require a specific order of execution.
            end
         else
            index = 1
            local old = _G[oldFunc] --extract function from _G table
            if type(old) ~= "function" then
               --print("AddHook Error: Tried to hook a function that doesn't exist: " .. oldFunc .. ".\nTry calling AddHook from a Global Initialization function.")
               return 0
            end
            tab = {}
            if after then
               hookAfter[oldFunc] = tab
            else
               hookBefore[oldFunc] = tab
               pt = {}
               priority[oldFunc] = pt
               pt[1] = weight
            end
            if not hook[oldFunc] then
               hook[oldFunc] = old
               _G[oldFunc] = function(...)
                  local val
                  local at, bt = hookAfter[oldFunc], hookBefore[oldFunc]
                  if bt then
                     for i = 1, #bt do
                        val = bt[i](...)
                        if val then break end--do not call any further "before" hooks nor the original function if a "before" hook returns true
                     end
                  end
                  val = val or old(...)
                  if at then
                     for i = 1, #at do
                        if val then at[i](val) else at[i]() end
                     end
                  end
                  return val
               end
            end
         end
         table.insert(tab, index, userFunc)
         return index --success
      end
      --print "AddHook Error: The first argument must be a string, the second must be a function."
      return 0
   end
   
   function RemoveHook(oldFunc, userFuncIndex, after)
      if type(oldFunc) == "string" and type(userFuncIndex) == "number" then
         local tab
         local at, bt = hookAfter[oldFunc], hookBefore[oldFunc]
         if after then
            tab = at
         else
            tab = bt
         end
         if tab then
            if tab[userFuncIndex] then
               local top = #tab
               if top == 1 then
                  tab[1] = nil
                  if tab == bt then
                     hookBefore[oldFunc] = nil
                     priority[oldFunc][1] = nil
                     priority[oldFunc] = nil
                     tab = at
                  else
                     hookAfter[oldFunc] = nil
                     tab = bt
                  end
                  if not tab then --no hooks of the other variety remain.
                     _G[oldFunc] = hook[oldFunc] --restore orignal function to _G table.
                     hook[oldFunc] = nil
                  end
               else
                  table.remove(tab, userFuncIndex)
                  if not after then
                     table.remove(priority[oldFunc], userFuncIndex)
                  end
               end
            --else
               --print("RemoveHook Error: Invalid index was passed as second parameter: " .. userFuncIndex)
            end
         --else
            --print("RemoveHook Error: Provided function isn't hooked: " .. oldFunc)
         end
      --else
         --print "RemoveHook Error: The first argument must be a string, the second must be the index returned by AddHook."
      end
   end
   
   function SkipHook(oldFunc, ...)
      if type(oldFunc) ~= "string" then
         --print("SkipHook Error: The first parameter passed must be the string representing the function name.")
         return
      end
      func = hook[oldFunc]
      if not func then
         func = _G[oldFunc] --it's not hooked. No need to throw an error. Prepare to call the function anyway.
      end
      if type(func) ~= "function" then
         --print("SkipHook Error: This is not a valid function: " .. oldFunc)
         return
      end
      return func(...)
   end
   
end
 
Last edited:
Level 20
Joined
Jul 10, 2009
Messages
474
IF someone is overriding hooks with the traditional method, there is no way to safely "unhook" it*. If one lives in a vaccuum where they know they would not need to "unhook" something, then it is entirely safe (and more practical) to stick to the existing method. However, it's my understanding that we're trying to make this sort of thing be more extensible.
That makes sense, thanks!
I just realized that I even hook and unhook the print-function in Ingame Console, so my first thought that unhooking is a rather rare case was instantly proven wrong, haha :D
true on that return value, I was thinking over this as I was falling asleep last night and realized I forgot to take into account the return value. I have now adjusted for that.
Happens to me all the time as well :D

I will try to think on how/if I can take that "multiple return values" idea into consideration. It probably wouldn't work without returning something that has a metatable assigned to it to spit out a valid argument if the caller needs a return value of a specific type or something. I currently have no idea how/if I could code that, though.
Not sure, if I understood your concern correctly, but just returning all values that the original function would have returned should not be much trouble. You can save all return values in a table for further use, see spoiler below.
Btw. you are using some if <var> then-statements that you don't need (sure thing, they don't hurt either). The actions inside would lead to the same result with the var being nil anyway, so that can maybe be simplified as well:

The following code is completely untested and only meant for example purpose.
Lua:
    function AddHook(oldFunc, userFunc, after)
        local container = after and hookAfter or hookBefore
        container[oldFunc] = container[oldFunc] or {} --putting in the empty table at this point prevents further case separations
        local index = #container[oldFunc] + 1
        container[oldFunc][index] = userFunc --might as well use table.insert instead of declaring an index-variable, but I see that the index shall be returned in the end?
        local old = _G[oldFunc] --extract function from _G table
        if not hook[oldFunc] then
            hook[oldFunc] = old
            _G[oldFunc] = function(...)
                local break_yn
                --Execute before-hooks. Using ipairs is beneficial, as it gives direct access to the function and also improves code readability.
                for _, hookFunc in ipairs(hookBefore[oldFunc]) do
                    break_yn = hookFunc(...)
                    if break_yn then break end--do not call any further "before" hooks nor the original function if a "before" hook returns true
                end
                --Save all return values in a table for further use.
                --I assume that you still want to execute the "after"-hooks.
                local returnVals = break_yn and {n=0} or table.pack(old(...)) --only execute "old", if break_yn is false. We still need a table-pack-like table in case its true to not mess with table.unpack.
                --Execute After-Hooks
                for _, hookFunc in ipairs(hookAfter[oldFunc]) do
                    hookFunc(table.unpack(returnVals, 1, returnVals.n))
                end
                --Return original Return-values
                return table.unpack(returnVals, 1, returnVals.n)
            end
        end
        return index --success
    end

There are also three things that bother me a bit:
  1. You currently save hooks based on function name, but function names are not unique in Lua, because functions are just first-class-values and can be saved in any variable. E.g. you can have a function f, set g = f and use AddHook for both "f" and "g". Your code would treat them as different functions, which might screw it up in some cases. Even worse, I might even choose to do f = NewFunction(...) at some point after having hooked "f", which would attach all hooks to the new function.
    The solution from my point of view would be to use the function itself as a key in hookAfter & hookBefore instead of the function name.
  2. Your hook-methods are currently restricted to functions in global scope, i.e. not applicable for those inside library-tables (such as math.sin). I don't use global scope much in my coding and even think it should be avoided at all costs to prevent name clashes (in fact, I only put library tables and classes to global scope). As such, I'd even recommend to pack AddHook, RemoveHook etc. into a table, but the point is that imo we should opt for supporting library functions as well.
    You could maybe consider taking the table containing the function to hook as an additional optional parameter in AddHook that defaults to _G. Again, using functionnames as keys are problematic, because the same function name can obviously be used in multiple scopes. Same solution as in point 1, using the function itself as a key instead.
  3. I see that returning true in one of the "before"-hooks prevents both the remaining before-hooks and the original function from running, but not the after-hooks. Is that intended? I feel like you can easily get into trouble, because the after-hooks take the original return values as parameters and might error out, if they don't get any, because the original function wasn't even executed.
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I really like your idea of having an optional parameter that defaults to _G for the table where the native is indexed. At that point, however, I think this resource will balloon into even more lines of code that I originally thought were to be quite short and straightforward.

The point you bring up about global functions being cached to locals and then having them called shouldn't be a problem IF the user is assigning those local pointers in real-time, rather than initializing them at the beginning like I see some people do "for the sake of efficiency". I saw a similar method where a function string is used that was being used in Garry's Mod (it came up when I was endlessly searching the internet for answers), however an actual function as a parameter would make so much sense and not limit it to the "_G" table.

Can the function itself be reliably mapped (as in, the handle index won't change dynamically, like it used to do when Lua was first introduced to WC3)?
 
Level 20
Joined
Jul 10, 2009
Messages
474
We need the functionname + its container to override the function, so passing just the function to AddHook would not work, because as you suspected, there is no way to reliably map the function back to any of its possible names.
My proposal rather was that AddHook still takes the functionname as string, but then uses the function as the index in the hookAfter-table instead of the string. That would solve many problems that might occur upon playing around with function variables.
Edit: I just thought that my proposed solution has its own complications, because the resource clearly overrides the original function with the one executing the hooks, so the function reference doesn't even stay the same. That could be solved by an additional refinement: Also save hookAfter[replacementFunc] = hookAfter[originalFunc]. But it's really getting nasty at this point.

The point you bring up about global functions being cached to locals and then having them called shouldn't be a problem IF the user is assigning those local pointers in real-time, rather than initializing them at the beginning
Yeah, functions cached before applying the first hook are really problematic. Exactly as you suggested, people commonly do that for performance reasons. I don't currently see a solution :(

as in, the handle index won't change dynamically, like it used to do when Lua was first introduced to WC3
The HandleId doesn't exist for pure Lua-objects like functions. It's specific for Warcraft handles. To my knowledge, it has never dynamically changed on a single handle, but it's just not synced across machines in a multiplayer game (so it's best practice not to use it).

I really like your idea of having an optional parameter that defaults to _G for the table where the native is indexed. At that point, however, I think this resource will balloon into even more lines of code that I originally thought were to be quite short and straightforward.
I think, it will only increase by one single line of code, but maybe I'm missing something. Can't you just add a parameter funcContainer, write funcContainer = funcContainer or _G as first line of code and replace all further occurances of _G by funcContainer?
 
Last edited:
This thread has piqued my interest, so after reading a bit, I went ahead and wrote the following snippet, based on both your and Elkonium's version/s. This also hasn't been tested yet, so I think there may be some nasty bugs lurking beneath it:

Lua:
-- This is partially based on Elkonium's own code for hooking.
do

local _LOW_PRIO     = -8
local _DEBUG_MODE   = false

local hookBefore    = {}
local hook          = {}
local hookAfter     = {}
local prioTable     = {}
local _call         = pcall
local _print        = print

-- 
if (not _DEBUG_MODE) then
    _print          =
    function(...)
    end
end

---@param oldFunc string
---@param userFunc function
---@param after boolean | integer (default = )
---@param srcTable table (default = _G)
---@param destTable table (default = srcTable)
---@return integer, boolean (index, success?)
-- ======================================================
---Usage: AddHook(oldFunc, userFunc[, after, srcTable, destTable])
-- ======================================================
---This hides the function stored in the string key (oldFunc) of
---the source table (srcTable), and replaces it with an identical
---function that processes other functions (userFunc) hooked to the
---target function.
function AddHook(oldFunc, userFunc, after, srcTable, destTable)
    srcTable    = ((type(srcTable) == "table") and srcTable) or _G
    destTable   = ((type(destTable) == "table") and destTable) or srcTable
    if (after == nil) then after = false; end
    if ((type(srcTable[oldFunc]) ~= "function")
     or (type(userFunc) ~= "function")) then
        return 0, false
    end

    --  Generate a placeholder function if hook[srcTable][oldFunc] does not exist
    local oldFuncPtr        = srcTable[oldFunc]
    hookBefore[oldFuncPtr]  = hookBefore[oldFuncPtr] or {} 
    hookAfter[oldFuncPtr]   = hookAfter[oldFuncPtr] or {}
    hook[srcTable]          = hook[srcTable] or {}
    if (not hook[srcTable][oldFunc]) then
        hook[srcTable][oldFunc] = oldFuncPtr
        destTable[oldFunc]      = 
        function(...)
            local hookyState, hookyErr, breakHook
            for _, hookFunc in ipairs(hookBefore[oldFuncPtr]) do
                hookyState, hookyErr = _call(hookFunc, ...)
                if (not hookyState) then
                    -- hookyErr is expected to be string
                    _print("AddHook >> {HOOK_BEFORE} An error occurred while calling a hooked function.\n"
                        .. "AddHook >> Function name -> " .. oldFunc .. "\n"
                        .. hookyState)
                    break
                end
                if hookyErr then
                    break
                end
            end
            local resultTable   = hookyErr and {n=0} or table.pack(oldFuncPtr(...))
            for _, hookFunc in ipairs(hookAfter[oldFuncPtr]) do
                hookyState, hookyErr = _call(hookFunc, table.unpack(resultTable, 1, resultTable.n))
                if (not hookyState) then
                    -- hookyErr is expected to be string
                    _print("AddHook >> {HOOK_AFTER} An error occurred while calling a hooked function.\n"
                        .. "AddHook >> Function name -> " .. oldFunc .. "\n"
                        .. hookyState)
                    break
                end
            end
            return table.unpack(resultTable)
        end
    end

    local prio
    local tab
    if (after and type(after) == 'number') then
        prio    = after or _LOW_PRIO
        after   = nil
    end
    if (after) then
        tab     = hookAfter[oldFuncPtr]
    else
        tab     = hookBefore[oldFuncPtr]
    end

    --  Attempt to sort by priority if the user function is
    --  to be hooked to execute before the target function.
    local index = #tab + 1
    if (not after) then
        local i             = 1
        prioTable[userFunc] = prio
        while (i <= #tab) do
            if (prioTable[userFunc] >= prioTable[tab[i]]) then
                index       = i + 1
                table.insert(tab, index, userFunc)
                break;
            end
            i   = i + 1
        end
    else
        tab[index]  = userFunc
    end
    return index, true
end

end
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Progress update for today: Version 1.2.0.0 compiles and runs perfectly from my tests. Added some extra API with added "InTable" which allows the user to manually specify a table to write variables into.

My goal for tomorrow is to update the API to use hook.whatever syntax so as to not clog up the _G table unnecessarily. I'll see if I can implement that multiple-return stuff somehow, as I study MyPad's script more.

Edit: I decided not to change the API. With 1.3, there are improvements to the SkipHook API. However, I’ve noticed a potential bug when removing hooks and shifting the list down-RemoveHook won’t point to the correct index. I’ll need to store the index separately. Due to this, I’ll probably end up using a unique index from a stack shared by all hooks, and the RemoveHook function would then only need one parameter.

Just as well, I’ve finally figured out what table.pack and unpack are doing, so I will implement the multiple-return values and fix the bug with version 1.4, likely to be ready before Saturday.
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Version 2.0.0.0 is finally here, quite well-tested, and it finally incorporates EVERY suggestion that I've received from @Eikonium and @MyPad and @Jampion .

This version breaks backwards-compatibility with the earlier iterations of RemoveHook, as it now requires a function as the second argument (the same function that was passed to the original AddHook). This prevents the Add/Remove functionality from being used with anonymous functions, but fixes the earlier glitch that could occur when multiple hooks from multiple sources would want to be removed from a well-hooked function.

Version 1.4, introduced an hour ago, had already implemented the multiple-return-value suggestions. There are a few more surprises hidden inside the code, such as "stop hook" being a return value from a hooked callback, hooks can now play much more nicely together with other hooks by accessing each others' return values and being able to call the original function without worrying about calling it more than once.
 
Last edited:

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
I think some of the documentation wasn't updated yet.
The priority of the before hook is controlled by the after parameter of AddHook, but the documentation only mentions false/nil and true as options.



The SkipHook is supposed to allow you to call the original function from within a hook, but looking at the code it seems to do a lot more:
Lua:
SkipHook = function(...)
    if called then
        return table.unpack(returned)
    else
        called = true
        return old(...)
    end
end
So it only calls the original function when used for the first time and returns stored return values at all additional calls?

Consider hooking KillUnit to code a spell "Life Link". Life Link links 4 units together and if one of them were to die, all the other 3 die instead:
Lua:
function LifeLinkHook(u)
    if u == LifeLinkTarget[1] then
        SkipHook(LifeLinkTarget[2])
        SkipHook(LifeLinkTarget[3])
        SkipHook(LifeLinkTarget[4])
    end
    if u == LifeLinkTarget[2] then
        SkipHook(LifeLinkTarget[1])
        SkipHook(LifeLinkTarget[3])
        SkipHook(LifeLinkTarget[4])
    end
    if u == LifeLinkTarget[3] then
        SkipHook(LifeLinkTarget[1])
        SkipHook(LifeLinkTarget[2])
        SkipHook(LifeLinkTarget[4])
    end
    if u == LifeLinkTarget[4] then
        SkipHook(LifeLinkTarget[1])
        SkipHook(LifeLinkTarget[2])
        SkipHook(LifeLinkTarget[3])
    end
    return _STOP_HOOK
end

I'd expect only one unit to die, because afterwards called is true and SkipHook will simply return table.unpack(returned) without calling the original function.

If SkipHook's only purpose is to call the original function without causing recursive hooks, why is it not simply SkipHook = old?

The part that runs the before hooks:
Lua:
for _, func in ipairs(hookBefore[old]) do
    val, stop = func(...)
    if val == _STOP_HOOK then
        val = nil
        stop = true
    elseif val ~= nil then
        table.insert(returned, val)
    end
    if stop then break end --do not call any further "before" hooks nor the original function if the user wants this to stop.
end
The func here is the userFunc in AddHook. Usually _STOP_HOOK is used to stop the hook, but if you want to return something, the first return value is returned and the second one will be used to stop the hook. This is something that should also be described in the documentation.

Regarding the return value:
The returned table will look like this: {hook1, hook2, ... hookN, originalReturnValue}.
Having the originalReturnValue at the start of the returned table would be good so that the after hook arguments start with the return value of the original function and return values from before hooks are optional parameters.

How exactly would communication between hooks look like? Let's say I have an after hook and would want to access the return value from a specific before hook. Since I don't know what other hooks there are, I don't know which index in the returned table belongs to the before hook I want. Are you supposed to add this information by yourself? For example by returning a table in the before hook. So instead of just returning a string "Arthas" that will be somewhere in the returned table, you return a table {"KilledHeroName": "Arthas"} and then you can search for a table with key "KilledHeroName".


_STOP_HOOK stops other before hooks and the original function.
How do you stop the after hooks as well? As I already mentioned, there can be cases where this is desired, though the behavior of _STOP_HOOK seems to be what is preferred in most situations.


Would be good to have access to the function arguments in the after hooks as well on top of the return value.
Maybe the entire arguments for the after hook should be:
hookedArguments, originalFunctionWasExecuted, originalFunctionReturnValue, beforeHooksReturnValues

If _STOP_HOOK is used, originalFunctionWasExecuted would be false to tell the after hook that the original function was not executed. Then after hooks can decide whether they still want to do something if the original function was skipped.
 
Level 20
Joined
Jul 10, 2009
Messages
474
To add to @Jampion's post, SkipHook currently doesn't do anything before the first hook was added (because it just starts out as the empty function). I agree that it should just call the original function without hooks instead of caching return values or anything else.

I have a few other points as well:
  1. It's looks really suspicious that SkipHook is a global function that dynamically changes based on the function that was last hooked.
    When I add a hook to my existing functions f and g in order, SkipHook will just always execute g. How now can I execute f without hooks?
    SkipHook clearly must take the function to execute as a parameter.

  2. Regarding the return value:
    The returned table will look like this: {hook1, hook2, ... hookN, originalReturnValue}.
    Having the originalReturnValue at the start of the returned table would be good so that the after hook arguments start with the return value of the original function and return values from before hooks are optional parameters.
    Jampion makes an important point: Having the original return values at last position would prevent you from even using the original function in the same manner as before. You simply can't write u = CreateUnit(...) after adding hooks, because u would be assigned the first hook's return value instead of the created unit.
    However, putting the original values to first place unfortunately doesn't work either, because it will also screw up, when the original function had more than one return value.
    Lua works in a way that the list {..., a,b,c} cuts ... down to a single value, if it is not the rightmost statement. If you want to keep all values, the multi-value-representative must stand last in the list.
    The same issue holds, when any hook returns more than one value or no value at all.

    Long story short, hook return values should not be returned at all. Let's just stick to the original return values.
  3. table.unpack(whichTable, startIndex, endIndex) should be called with both indices specified. If not, endIndex defaults to #whichTable, which can causes bugs on tables containing nil-values (and yes, nil can be a valid return value even in the middle of multiple ones). That's exactly why returned = table.pack(old(...)) is to be preferred over returned = {old(...)}: using table.pack provides the endIndex we need in returned.n.
  4. The system still has problems with multiple references to the same function that were created before adding the first hook.
    Lua:
    f = function() end
    g = f
    AddHook("f", function() print("hooked") end)
    f() --> will print "hooked"
    g() --> will not print "hooked", although it's the same function
    I admit that I don't currently see a solution and there probably is none :). So maybe it's just something that would need to be added to the documentation.
  5. I see some table.insert(returned, table.unpack(val)) in the code, but table.insert doesn't support more than one value (i.e. it will bug, if val contains more than one).
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I think some of the documentation wasn't updated yet.
The priority of the before hook is controlled by the after parameter of AddHook, but the documentation only mentions false/nil and true as options.



The SkipHook is supposed to allow you to call the original function from within a hook, but looking at the code it seems to do a lot more:

I have now completely changed the way that Hook.skip works. Now, it is just a boolean that the user can freely set or reference. Hook.add now returns the original function for those who need to be able to access it directly.

Regarding the return value:
The returned table will look like this: {hook1, hook2, ... hookN, originalReturnValue}.
Having the originalReturnValue at the start of the returned table would be good so that the after hook arguments start with the return value of the original function and return values from before hooks are optional parameters.
I have adjusted the priority of the returned functions so that the original returned value will appear at the start of the table.
How exactly would communication between hooks look like? Let's say I have an after hook and would want to access the return value from a specific before hook. Since I don't know what other hooks there are, I don't know which index in the returned table belongs to the before hook I want. Are you supposed to add this information by yourself? For example by returning a table in the before hook. So instead of just returning a string "Arthas" that will be somewhere in the returned table, you return a table {"KilledHeroName": "Arthas"} and then you can search for a table with key "KilledHeroName".
I've broken down the values into a single Hook.returned table. It's up to the user to determine how much of a distinction there is between various hooks.
_STOP_HOOK stops other before hooks and the original function.
How do you stop the after hooks as well? As I already mentioned, there can be cases where this is desired, though the behavior of _STOP_HOOK seems to be what is preferred in most situations.
Hook.skip can now be used from within those After functions to allow the user to determine if they should proceed.
Would be good to have access to the function arguments in the after hooks as well on top of the return value.
Maybe the entire arguments for the after hook should be:
hookedArguments, originalFunctionWasExecuted, originalFunctionReturnValue, beforeHooksReturnValues

If _STOP_HOOK is used, originalFunctionWasExecuted would be false to tell the after hook that the original function was not executed. Then after hooks can decide whether they still want to do something if the original function was skipped.

The "after hooks" will still get all the returned values as an unpacked table, but they can access Hook.skip and Hook.args if they need the data you mentioned.

To add to @Jampion's post, SkipHook currently doesn't do anything before the first hook was added (because it just starts out as the empty function). I agree that it should just call the original function without hooks instead of caching return values or anything else.
This is now fixed as the user has access to the original function via the return value of Hook.add.
Long story short, hook return values should not be returned at all. Let's just stick to the original return values.
This is now fixed and the final return just gets the first return value (if any were found). Someone now has to use an "after hook" if they want to be able to reference multiple return values.
table.unpack(whichTable, startIndex, endIndex) should be called with both indices specified. If not, endIndex defaults to #whichTable, which can causes bugs on tables containing nil-values (and yes, nil can be a valid return value even in the middle of multiple ones). That's exactly why returned = table.pack(old(...)) is to be preferred over returned = {old(...)}: using table.pack provides the endIndex we need in returned.n.
I have no idea what you're talking about here, to be honest. If you have another look at my code, do you think this could still be problematic? If so, under what circumstances would it still happen?

The system still has problems with multiple references to the same function that were created before adding the first hook.
Lua:
f = function() end
g = f
AddHook("f", function() print("hooked") end)
f() --> will print "hooked"
g() --> will not print "hooked", although it's the same function
I admit that I don't currently see a solution and there probably is none :). So maybe it's just something that would need to be added to the documentation.

I have added a note about this. The workaround is that the user who wishes to cache a function, but to also allow it to be hooked just in case, should use Global Initialization to inline those functions.
I see some table.insert(returned, table.unpack(val)) in the code, but table.insert doesn't support more than one value (i.e. it will bug, if val contains more than one).
That is now fixed.
 

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
Seems pretty good now. I tried this on my map which uses quite a lot of overridden functions.
If a function override does not use the original function, using Hook instead improves compatibility, because you are not discarding previous overrides.
However I had one case, where using Hook caused a new problem, that simple function overrides do not have. If you want to modify input parameters, you would do this with Hook by calling the original function with the modified parameters and set Hook.skip to true, so the function is not executed twice.

This is done in Perfect PolledWait for instance:
Lua:
local oldAction
oldAction = Hook.add("TriggerAddAction",
function(whichTrig, userAction)
    if not Hook.skip then
        Hook.skip = true
        return oldAction(whichTrig, function()
            coroutine.resume(coroutine.create(function()
                userAction()
            end))
        end)
    end
end)

If I wanted to have two Hooks like that, only one of them would be in use. Maybe if Hook.args is used as input for the original function instead of ..., before hooks can simply modify Hook.args instead of calling the old function. Then you could have multiple before hooks that modify input parameters. Generally, using the original function could break stuff because it skips all hooks, so it should only be used when necessary.
One thought I had was to be able to call the original function with hooks, but skip the first hooks up to the current hook. That would be closer to how normal function overrides work, but I'm not sure how feasible or necessary this is.


This is now fixed and the final return just gets the first return value (if any were found). Someone now has to use an "after hook" if they want to be able to reference multiple return values.
Does this mean any hooked function will be converted to a function with only 1 return value? What's the problem with just returning all the return values?
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Thank you for that feedback! That will actually allow some resources to be even shorter/more modular. I have updated to 3.1.

However, I'd like to get a clear consensus on those multiple return values. What exactly do people want to do? @MyPad @Eikonium you both seem to have a pretty clear idea of what you'd want to do with those multiple-return values. Should I be keeping track of all of multiple sets of return values, or allow the return values to be overriden by others? If I allow overriding it, then "Hook" won't fail in cases where the user wished to access multiple return values from the original function. As it is right now, I've primarily focused on ensuring this works with hooking stuff from the common/blizzard API.

Update to 3.2 - now allows Hook.flush("functionName") to clear all associated hooks. The main use I can think of for this is GlobalInitialization, to allow the garbage collector to dump those unneeded initialization hooks.
 
Last edited:
Level 20
Joined
Jul 10, 2009
Messages
474
@MyPad @Eikonium you both seem to have a pretty clear idea of what you'd want to do with those multiple-return values. Should I be keeping track of all of multiple sets of return values, or allow the return values to be overriden by others?
The most important point is not to break existing code.
Consider the following (probably non-sensible, but educative) example:
Lua:
--Calculates the center between two given locations
--Returns two coordinates instead of a location, because locations are insanely slow
function GetCenterPoint(x1,y1,x2,y2)
    return (x1+x2)/2, (y1+y2)/2
end

centerX, centerY = GetCenterPoint(0,0,1024,1024)
CreateUnit(Player(0), FourCC('hfoo'), centerX, centerY, 0)
This code would break after adding a hook to GetCenterPoint, because centerY would suddenly be nil.
I would also find limiting the resource on Wc3 natives unnecessary.

-> So from my point of view, the replacement function must return all original return values.

-> Regarding the hook/parameter behaviour, I would personally cut all overwriting of parameters from the resource.

Users who want to modify input-parameters to the original function would not use hooks for that anyway, but just write a wrapper function.
Lua:
--Wrapper function to modify input params of a given function f
function f_wrapped(x)
    return f(x+1)
end

The same holds for return values of hooks: I don't think they are useful enough to justify the tremendous performance impact they are currently producing. The resource currently packs all return values in a single big array by inserting every single value at front - which always shifts the whole existing array one place to the right. Plus you are using a loop to process values one by one.
That performance overhead can be huge - so cutting all hook return values would make the resource much quicker.

Another reason for why hook return values don't work out well is that you can't really know in the big returned-array, which value came from which hook. Adding more hooks might also add more values at any location and again screw up existing code.

Feel free to disagree with me of course - maybe there are scenarios I haven't thought about.

I also have a few other things in mind that might currently go wrong:
  • nil values can screw ipairs and table.unpack, if not done correctly
  • Hook.returned being global (local reference doesn't help), so recursive calls can still screw up
  • Resource also needs to use the new function as key in its data structure. Otherwise it will overwrite the function again with every new hook

But I can't currently write that down in detail. Will try by tomorrow.
 
Last edited:

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
Users who want to modify input-parameters to the original function would not use hooks for that anyway, but just write a wrapper function.
That only works if you can call f_wrapped instead of f. Sometimes you want to override how a function works that is used by other resources or in GUI triggers. Then you can't simply use a new function.

The function TriggerAddAction that is overridden by Perfect PolledWait is a good example, because it is implicitly used by every GUI trigger. Using conventional overrides, you have the issue that the order in which the function is overridden is relevant and the user will have to manually change the order in which the overrides occur. With this resource on the other hand, you can use the priority parameter to change the order. If two resources have correctly set up priorities, the user will not have to worry about ordering the resources in the correct order.

The same holds for return values of hooks: I don't think they are useful enough to justify the tremendous performance impact they are currently producing. The resource currently packs all return values in a single big array by inserting every single value at front - which always shifts the whole existing array one place to the right. Plus you are using a loop to process values one by one.
That performance overhead can be huge - so cutting all hook return values would make the resource much quicker.
The return value is very important for Item Indexers for example. You hook CreateItem and index the returned item. Considering all natives only have one return value, the performance impact would not be big, would it? If you need to hook a function with multiple return values, it will be a custom function anyway for which you can easily write a wrapper instead.

Another reason for why hook return values don't work out well is that you can't really know in the big returned-array, which value came from which hook. Adding more hooks might also add more values at any location and again screw up existing code.
Agreed. Hook returns should probably only be used if you set Hook.skip to true to replace the original return value.
Let's say you hook CreateTimer() to recycle timers instead of creating them. So you set Hook.skip to true and return a stored timer instead.
If there was already another before hook that added a return value, the returned timer will no longer be in the first place in the table and since Hook.skip is true, the original return value will not be added at the front and you end up with a return value of unexpected type.

@Bribe I noticed a major problem that is apparent when using Perfect PolledWait. Since waiting takes time, hooks can run in parallel and globals like Hook.skip can be messed up by another hook while the current hook is running.
This can be solved by having a local table like this:
Lua:
parent[oldFunc] =
function(...)
    local hookInstance = {
        returned = {},
        skip = false,
        args = {...}
    }
    ...
and adding hookInstance as first parameter to hook functions.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
What do you guys think of this prototype where the callback funcs only takes one argument (table) and directly access the arguments via its own indices?

Lua:
do --Hook v3.3.0.0 by Bribe, with very special thanks to Eikonium, Jampion and MyPad for shaping its developemnt.
    
    local _LOW_PRIO     = -9001 -- a number to represent what should be the lowest priority for a trigger table.
    
    local hookBefore    = {} --stores a list of functions that are called prior to a hooked function.
    local priority      = {}
    local hookAfter     = {} --stores a list of functions that are called after a hooked function.
    local hookedFunc    = {} --stores a list of overriden functions
    
    Hook                = {}
    
--[[--------------------------------------------------------------------------------------
    skip
    (boolean)
    Note: Set this to True from within a before-hook callback function to prevent the
          original function from being called. Users can check if this is set to True if
          they want to change the behavior of their own hooks accordingly.
          
    (table)
        Contains the original arguments passed during a hook. Useful for referencing in an
        "after hook". Can be modified by a "before hook".
        
    returned
    (table)
        Contains a table of all return values from "before" hooks and (if applicable) the
        original function. Useful for a "before hook" with a low priority that wants to
        check values returned by previous hooks.        
----------------------------------------------------------------------------------------]]
    
--[[--------------------------------------------------------------------------------------
    Internal functions
----------------------------------------------------------------------------------------]]
    
    local function parseArgs(oldFunc, parent)
        parent = parent or _G
        local hfp = hookedFunc[parent]
        local hf = hfp and hfp[oldFunc]
        return parent, hfp, hf, hf or parent[oldFunc]
    end
    
--[[--------------------------------------------------------------------------------------
    Hook.add
    Args: string oldFunc, function userFunc[, boolean after, table parent]
          @ oldFunc is a string that represents the name of a function (e.g. "CreateUnit")
          @ userFunc is the function you want to be called when the original function is
            called.
          @ after is an optional parameter that determines whether your function is called
            with the parameters of the original function before the original function is
            called (when false or nil), or whether it is called with the return value of
            that function (when true).
          @ parent is an optional parameter for where the oldFunc is hosted. By default,
            it assumes a global (_G table) such as "BJDebugMsg".
            
    Returns: The original function you are hooking (if successful) or nil (if failed).
----------------------------------------------------------------------------------------]]
    
    function Hook.add(oldFunc, userFunc, after, parent)
        local parent, hfp, hf, old = parseArgs(oldFunc, parent)
        
        if type(old) == "function" then
            if type(oldFunc) == "string" and type(userFunc) == "function" then
                
                if not hf then
                    if not hfp then
                        hfp         = {}
                        hookedFunc[parent] = hfp
                    end
                    hfp[oldFunc]    = old
                    hookBefore[old] = {}
                    hookAfter[old]  = {}
                    parent[oldFunc] =
                    function(...)
                        local this = {...}
                        this.returned = nil
                        this.skip = nil
                        this.call = old
                        
                        for _, func in ipairs(hookBefore[old]) do
                            func(this)
                        end
                        local r
                        if not this.skip then
                            r = table.pack(old(table.unpack(this.args)))
                            if #r > 0 then this.returned = r end
                        end
                        r = this.returned
                        --print("Hook report: returning " .. r .. " values.")
                        for _, func in ipairs(hookAfter[old]) do
                            func(this)
                        end
                        return r and table.unpack(r)
                    end
                end
                
                local tab, index, pt, weight
                
                if after and type(after) == "number" then
                    weight = after
                    after = nil
                else
                    weight = weight or not after and _LOW_PRIO
                end
                tab = after and hookAfter[old] or hookBefore[old]
                count = #tab
                if count > 0 then
                    if weight then
                        pt = priority[old]
                        for i = 1, count do
                            if (not pt[i]) or pt[i] < weight then
                                table.insert(pt, i, weight) --insert at a specific point in the priority table in accordance with the weight
                                index = i
                                break
                            end
                        end
                        if not index then
                            index = count + 1
                            pt[index] = weight --this has the lowest priority. Insert at the end of the priority table.
                        end
                    else
                        index = count + 1 --"hookAfter" doesn't require a specific order of execution.
                    end
                else
                    index = 1
                    if weight then
                        pt = {}
                        priority[old] = pt
                        pt[1] = weight
                    end
                end
                table.insert(tab, index, userFunc)
                --print("indexing " .. index)
                return old --success
            --else
                --print "Hook.add Error: The first argument must be a string, the second must be a function."
            end
        --else
            --print("Hook.add Error: Tried to hook a function that doesn't exist: " .. oldFunc .. ".\nTry calling Hook.add from a Global Initialization function.")
        end
    end
    
--[[--------------------------------------------------------------------------------------
    Hook.remove
    Args: string oldFunc, function userFunc[, boolean after, table parent]
    Note: The arguments should be equal to those that would have been passed to Hook.add.
----------------------------------------------------------------------------------------]]
    
    function Hook.remove(oldFunc, userFunc, after, parent)
        local parent, hfp, hf, old = parseArgs(oldFunc, parent)
        if hf then
            local at, bt = hookAfter[old], hookBefore[old]
            local tab = after and at or bt
            local top = #tab
            if top > 0 then
                local index
                for i = 1, top do
                    if tab[i] == userFunc then
                        index = i
                        break
                    end
                end
                if index then
                    table.remove(tab, index)
                    if not after then
                        table.remove(priority[old], index)
                        tab = at
                    else
                        tab = bt
                    end
                    if top == 1 and #tab == 0 then --no hooks of any kind remain on this function.
                        Hook.flush(oldFunc, parent)
                    end
                    return true --success
                end
            end
        end
        --print("Hook.remove Error: Provided function isn't hooked: " .. oldFunc)
    end
    
--[[--------------------------------------------------------------------------------------
    Hook.flush
    Args: string oldFunc[, table parent]
    Desc: Purges all hooks associated with the given function string and sets the original
          function back inside of the parent table.
----------------------------------------------------------------------------------------]]
    
    function Hook.flush(oldFunc, parent)
        local parent, hfp, hf, old = parseArgs(oldFunc, parent)
        if hf then
            parent[oldFunc] = old
            priority[old]   = nil
            hookBefore[old] = nil
            hookAfter[old]  = nil
            hfp[oldFunc]    = nil
        end
    end
    
end
 

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
I like it. Seems very intuitive with the this parameter.
Some remarks:
Lua:
local this = {...}
this.returned = nil
this.skip = nil
this.call = old
I guess you meant to do this = {} and this.args = {...}, since you refer to this.args later.
You can also initialize everything at one:
Lua:
local this = {
    args = {...},
    returned = nil,
    skip = nil,
    call = old
}

If you want to support multiple return values, you need to change the return a little bit:
Lua:
return r and table.unpack(r)
into
Lua:
if not r then
    return
end
return table.unpack(r)
The and only uses the first return value of table.unpack(r), so you lose the rest.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Ok, version 3.4 is now ready. This re-introduces the AddHook function which wraps the user's function into a more user-friendly syntax for those that don't want to screw around with the unintuitive way that Hook.add deals with parameters and return values. I think this offers the best of both worlds in terms of enabling power users and less experienced users alike.
 
Level 20
Joined
Jul 10, 2009
Messages
474
That only works if you can call f_wrapped instead of f. Sometimes you want to override how a function works that is used by other resources or in GUI triggers. Then you can't simply use a new function.
Yes, absolutely! But wouldn't changing input parameters on natives used by other resources potentially screw up that other resources? You can't even be sure if they get the changed version, because they might have cached the original native to local scope, before the first hook was added (which is a big problem anyway - as the same holds for normal hooks).
Changing input parameters on natives sounds like something that should be avoided to keep other resources functioning. There are valid applications of course like adding default values and error correction, but these wouldn't affect other resources anyway, if they have been correctly coded. So it should only ever affect your own code and thus wrapper-functions are always applicable?
Surely you have an applicaton in mind that I haven't thought about, though :)
To be clear, I'm talking about modifying input-parameters here. Modifying return values is a different story, like overwriting CreateTimer to add timer recycling is clearly considerable (although risky, as I will point out further below).
Anyway, everyone can of course operate at his own risk. The current solution @Bribe has implemented with this.args seems to work out well and allows you to modify input params without forcing it, so I'm absolutely fine with that right now.

The return value is very important for Item Indexers for example. You hook CreateItem and index the returned item. Considering all natives only have one return value, the performance impact would not be big, would it?
I actually just proposed cutting the return values of before-hooks and after-hooks. That from my point of view would not affect your example, because the example only requires passing the original return values to the after-hooks, which I find useful and didn't ever want to remove.
But your other example - letting CreateTimer return a recycled timer instead of a new one - is definitely on point.
Although [off-topic], I see a big risk in modifying CreateTimer towards recycling, because other resources might have implemented their own timer recycling. If so, timers can end up being registered to both recycling storages, which can easily lead to the same timer being fetched by different systems at once, leading to bugs that would take weeks to find (honestly).
But good news, you still convinced me that enabling after-hooks to modify original return values can be useful :)

Regarding the performance issue, I was specifically referring to the process of table-packing, looping, inserting and shifting all return values of all hooks one by one, which could have piled up to a lot of overhead after adding more hooks to the same native, even if that native only has one return value.
That issue is solved now, so all fine.



What do you guys think of this prototype where the callback funcs only takes one argument (table) and directly access the arguments via its own indices?
This is much better.
I see that the new way led to the removal of this pack-loop-insert-shift process, which I also appreciate :)

This re-introduces the AddHook function which wraps the user's function into a more user-friendly syntax
I also like this idea.
Still, the naming is too similar to Hook.add and it should be added to the Hook-library instead of using global scope. Maybe name it Hook.addSimple or something like that?



@Bribe I've got the following additional remarks for the current version of this resource:
  • You use table.unpack(table) a few times, but you should write table.unpack(table,1,table.n) to prevent bugs with nil-values. Just writing table.unpack(table) will unpack the table from index 1 to index #table, but #table has undefined behaviour for arrays with holes (i.e. with nil-values). Most of the time it works, but sometimes not.
    To support table.unpack(table,1,table.n), the table must have been created with table.pack(...) instead of {...}, as this function adds the .n parameter (maximum index in use).
  • The way you currently define your this-table produces the following table:
    Lua:
    local this = {
        args = {...},
        [1] = returned,
        [2] = skip,
        call = old
    }
    You would have to stick to the syntax @Jampion provided with returned = nil to use the index you desired, but even that wouldn't do anything, because nil-values are non-existent in a table. Just write this:
    Lua:
    local this = {
        args = {...},
        call = old
    }
  • You use old(table.unpack(this.args)), when you still have access to all args via ...-operator. You can instead write old(...).
    My bad, I forgot that we allow to change input args during before-hooks.
The above things can be accounted for like this.
Lua:
function(...)
    local this = {
        args = table.pack(...),
        call = old
    }
    for _, func in ipairs(hookBefore[old]) do
        func(this)
    end
    this.returned = this.skip and {n=0} or table.pack(old(table.unpack(this.args,1,this.args.n)))
    for _, func in ipairs(hookAfter[old]) do
        func(this)
    end
    return table.unpack(this.returned, 1, this.returned.n)
end
  • You use a count variable in global scope. Looks like you wanted it to be local count = #tab.
  • The system still saves funcs into hookedFunc[parent] based on functionname, so the following code will lead to unexpected behaviour:
    Lua:
    f = function() ... end
    AddHook("f", function() print("hooked f") end)
    g = f
    AddHook("g", function() print("hooked g") end)
    f() --> will print "hooked f"
    g() --> will both print "hooked f" and "hooked g".
    Solution would be to use the function as key again (although I haven't looked into further consequences on your code).
    This might require setting hfp[new] = hfp[old] to prevent additional overwrites.
  • The following code doesn't run at all:
    Lua:
    f = function() print("f") end
    AddHook("f", function() print("hooked") end) --> throws an "attempt to get length of a nil value (local 'r')" error
  • Why are weight and after the same parameter? How would I modify execution order within before-hooks?
 
Last edited:

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
You can't even be sure if they get the changed version, because they might have cached the original native to local scope, before the first hook was added (which is a big problem anyway - as the same holds for normal hooks).
What are reasons for caching natives to local scope? Performance? Shorter function name for convenience?

Changing input parameters on natives sounds like something that should be avoided to keep other resources functioning. There are valid applications of course like adding default values and error correction, but these wouldn't affect other resources anyway, if they have been correctly coded. So it should only ever affect your own code and thus wrapper-functions are always applicable?
Surely you have an applicaton in mind that I haven't thought about, though :)
To be clear, I'm talking about modifying input-parameters here. Modifying return values is a different story, like overwriting CreateTimer to add timer recycling is clearly considerable (although risky, as I will point out further below).
Sure, a resource should not hook a native in a way that it no longer works as expected.
Perfect PolledWait hooks TriggerSleepAction and TriggerAddAction to work its magic, but they still work as before just that waits are more accurate. It changes the function parameter of TriggerAddAction in order to wrap it in a coroutine. Right now, function parameters are the only example I can think of where you can reasonably change the parameter without messing with other resources that use the native.
In my map I also change the function parameter of TriggerAddAction by wrapping it in xpcall to get error messages.

Although [off-topic], I see a big risk in modifying CreateTimer towards recycling, because other resources might have implemented their own timer recycling. If so, timers can end up being registered to both recycling storages, which can easily lead to the same timer being fetched by different systems at once, leading to bugs that would take weeks to find (honestly).
The way it works in my map is that once you call DestroyTimer, the timer is instead recycled. If you have a resource that uses its own recycling, they will never destroy it and it will never be recycled by my system. I think it should be pretty safe, because a resource will only call DestroyTimer if it will not use the timer anymore.
The only danger I see is if you call DestroyTimer, but don't nil the variable and keep using it, but that's something you shouldn't do anyway.
 
Level 20
Joined
Jul 10, 2009
Messages
474
What are reasons for caching natives to local scope? Performance? Shorter function name for convenience?
Performance. Moving references into local scope of any function puts them into the function's closure and avoids cost-intensive table-lookups that would occur upon fetching from global scope.
It can be a good booster for functions that run very often (like maybe a periodic movement function using GetUnitX in an ice sliding map).
Right now, function parameters are the only example I can think of where you can reasonably change the parameter without messing with other resources that use the native.
I actually thought that the Hook library's intended use would be to use a before-hook casting the original function with changed function parameter and setting Hook.skip to true (you essentially want to overwrite the original function), making passing anything from the before-hooks to the original function unnecessary. But agreed, it's always good to provide multiple options.
Function parameters are a good point anyway, I hadn't thought too much about these. Thanks for pointing this out :)
The way it works in my map is that once you call DestroyTimer, the timer is instead recycled. [...] The only danger I see is if you call DestroyTimer, but don't nil the variable and keep using it, but that's something you shouldn't do anyway.
I agree, that sounds pretty safe. I guess, it doesn't really make sense for other resources to provide global timer recycling the same way as you do (would be problematic, if they did, but I don't see that happening).
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Still, the naming is too similar to Hook.add and it should be added to the Hook-library instead of using global scope. Maybe name it Hook.addSimple or something like that?
This change is now implemented. I had actually been considering "simple" as part of some syntax, because that's a word Blizzard uses for some of their wrappers. You've convinced me that it's better than adding to the Global table unnecessarily.
You use table.unpack(table) a few times, but you should write table.unpack(table,1,table.n) to prevent bugs with nil-values. Just writing table.unpack(table) will unpack the table from index 1 to index #table, but #table has undefined behaviour for arrays with holes (i.e. with nil-values). Most of the time it works, but sometimes not.
To support table.unpack(table,1,table.n), the table must have been created with table.pack(...) instead of {...}, as this function adds the .n parameter (maximum index in use).
I didn't know about that "n" property. I've made the changes now.
The way you currently define your this-table produces the following table:
Lua:
local this = {
    args = {...},
    [1] = returned,
    [2] = skip,
    call = old
}
You would have to stick to the syntax @Jampion provided with returned = nil to use the index you desired, but even that wouldn't do anything, because nil-values are non-existent in a table. Just write this:
Lua:
local this = {
    args = {...},
    call = old
}
Thank you, I hadn't thought of that! This is now reconciled.
You use a count variable in global scope. Looks like you wanted it to be local count = #tab.
Thanks again - this is now fixed.
The system still saves funcs into hookedFunc[parent] based on functionname, so the following code will lead to unexpected behaviour:
Lua:
f = function() ... end
AddHook("f", function() print("hooked f") end)
g = f
AddHook("g", function() print("hooked g") end)
f() --> will print "hooked f"
g() --> will both print "hooked f" and "hooked g".
Solution would be to use the function as key again (although I haven't looked into further consequences on your code).
This might require setting hfp[new] = hfp[old] to prevent additional overwrites.
I don't see this as being unexpected behavior. Who would someone hook a function by any name other than via the original name they were trying to hook? If you could provide a practical scenario for why someone would do this, then I can either propose an alternative or - if desirable - integrate it into this code.
The following code doesn't run at all:
Lua:
f = function() print("f") end
AddHook("f", function() print("hooked") end) --> throws an "attempt to get length of a nil value (local 'r')" error
Nice catch! This is now fixed.
Why are weight and after the same parameter? How would I modify execution order within before-hooks?

If "after" is "true", it is an after-hook. If it is instead a number, then that number will be treated as "weight". If you take a look at [Lua] Perfect PolledWait (GUI-friendly) or [Lua] Fast Triggers, you can see they are both using the "weight" functionality for TriggerAddAction.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Updated to version 3.6 - I'm actually pretty stoked about this update, and here's why:

Hook.add now takes an additional optional parameter: default. If the hooked function doesn't yet exist, the "default" parameter will play the part of the original function. This will allow things like hooking the __index or __newindex method on the _G table, without clashing with other libraries (provided they are also tailored to use Hook).

I will be using this technique in the Lua Damage Engine rewrite I've been working on to allow GUI variables like udg_DamageEventSource/Target/Amount to interact directly with the core script.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Thanks to version 3.6, the following library now exists:

 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Update to version 4.0. This brings some major new changes:

1) Hook.add syntax now uses the third argument strictly as "weight". Any negative number (or nil) will treat this as a "before" hook. Any number greater or equal to zero (or "true") will treat it as an "after" hook.

The lowest weight runs first. Thefore (exagerating here), -infinity down to -0.00000...1 would be the range for a "before" hook, and 0 up to infinity would be the range for an "after" hook.

2. Hook.add now returns two things, rather than just one. The first thing returned is still the original function. The second thing returned is a table, which represents the node that you added to the list with Hook.add. This brings us to point 3:

3. Hook.remove now only takes one argument: the user-created table found in the second returned value of Hook.add.

Again: After hooks can now be assigned with priority, just like Before Hooks, but now negative weight is treated as a "before" hook and positive weight is treated as an "after" hook. Hook.remove syntax has changed.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Alas, this thing has gotten so bloated. I've been able to shorten it, reduce its dependencies and complexity significantly, and it runs MUCH faster except on addition/removal (which doesn't need to be fast).


Lua:
--Hook 5.0 beta, a faster, more capable, lightweight version, with no requirements.
--Core API has been reduced from Hook.add, Hook.addSimple, Hook.remove, Hook.flush to just AddHook
--Secondary API has been reduced from hook.args, hook.returned, hook.old, hook.skip to just old(...)
--AddHook returns two functions: 1) old function* and 2) function to call to remove the hook.
--

--"Classic" way of hooking:
--[[
    local oldFunc = BJDebugMsg
    BJDebugMsg = print
]]--
--The below does the exact same thing:
--[[
    local oldFunc = AddHook("BJDebugMsg", print)
]]--
--More complicated approach involving temporary hooks (calling the old function and
--showing how to remove the hook when done):
--[[
    local oldFunc, removeFunc --remember to declare locals before any function that uses them
  
    oldFunc, removeFunc = AddHook("BJDebugMsg", function(s)
        if want_to_display_to_all_players then
            oldFunc(s)
        elseif want_to_remove_hook then
            removeFunc()
        elseif want_to_remove_all_hooks then
            removeFunc(true)
        else
            print(s)
        end
    end)
]]--

---@param oldFunc string
---@param userFunc function
---@param priority? number
---@param parent? table
---@param default? function
---@return function old_function
---@return function call_this_to_remove
function AddHook(oldFunc, userFunc, priority, parentTable, default, metatable)
    parentTable = parentTable or _G
    if default then
        metatable = getmetatable(parentTable)
        if not metatable then
            metatable = {}
            setmetatable(parentTable, metatable)
        end
        parentTable = metatable
    end
    priority        = priority or 0
  
    local index
    local hookStr   = "_hooked_"..oldFunc --You can change the prefix if you want.
    local hooks     = rawget(parentTable, hookStr)
    if hooks then
        local fin   = #hooks
        if priority > 0 then
            index = fin + 1
            repeat
                index = index - 1
                if hooks[index].priority < priority then break end
            until index == 2
        else
            index = 2
            repeat
                if hooks[index].priority > priority then break end
                index = index + 1
            until index > fin
        end
        hooks:insert(index, {})
        if index < fin then
            for i = index + 1, fin + 1 do
                hooks[i].updateIndex(i)
            end
        end
    else
        index = 2
        hooks = {{userFunc=rawget(parentTable, oldFunc) or default}, {}}
        rawset(parentTable, hookStr, hooks)
    end
    local hook       = hooks[index]
    hook.userFunc    = userFunc,
    hook.priority    = priority,
    hook.updateIndex = function(i) index = i end --no table/list API needed on the part of the user.
    if index == #hooks then
        rawset(parentTable, oldFunc, userFunc)
    end
    return function(...)
        return hooks[index-1].userFunc(...) --this is the "old" function.
    end, function(removeAll)
        local fin = #hooks
        if removeAll or fin == 2 then
            rawset(parentTable, hookStr, nil)
            rawset(parentTable, oldFunc, hooks[1].userFunc)
        elseif index < fin then
            hooks:remove(index)
            for i = index, fin do
                hooks[i].updateIndex(i)
            end
        end
    end
end
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Update to version 5.0 to bring the biggest performance improvement to date. I've needed to change all of the core API and some of the underlying mechanics in order to make this work, but it delivers on every front: shorter code, easier to work with, faster execution time, removed OOP syntax, removed dependency on LinkedList (at the cost of making add/removal O(n), which is fine as performance isn't important with those), and it's even more powerful than before thanks to optional metatable auto-creation.

All parameters in callback functions are identical to their native counterparts, the return value functions exactly the same way it always has. The one-trick pony in this show is the "old" function - the first of the two functions returned by AddHook - which you need to call in order to keep the hook-chain going. It is extremely fast compared to what it was like before, and will adapt as more hooks are added or removed.

@Jampion @Eikonium @MyPad thank you for your feedback in helping to shape the prior versions of this resource. @Wrda I am saddened to lose LinkedList for this resource, but this performs signifcantly better than it could have otherwise.
 

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
It's really cool how this resource went through so many major changes and being constantly improved. From the looks of it, the newest version seems be the best, even if I haven't really understood the code yet.

I want to use this resource for TriggerSleepAction together with PreciseWait. PreciseWait has priority -2 and does not call the old function. So if I set my Hook with priority -1, it should run before PreciseWait, but still be an after hook. I intend to get the following behavior:
Lua:
Wait = function(dur)
    PreciseWait(dur)
    MyHook()
end

While trying this out, I found an issue with the priorities.
Below the code of Hook, I added the following (I define my own TriggerSleepAction, so I can use it with the Lua debugger):

Lua:
function TriggerSleepAction(dur)
    print("sleep", dur)
end
local oldTriggerSleepAction, _removeFunc
oldTriggerSleepAction, _removeFunc = AddHook("TriggerSleepAction", function(dur)
    print("my wait 1", dur)
    oldTriggerSleepAction(dur)
    print("my wait 2")
end, -1)
AddHook("TriggerSleepAction", function (dur)
    print("precise wait")
end, -2)
TriggerSleepAction(17)

If I have it in that order, I get:
Code:
my wait 1       17
sleep   17
my wait 2
So precise wait is not used. Interestingly, if I switch the order and add the precise wait first it works:

Lua:
function TriggerSleepAction(dur)
    print("sleep", dur)
end
AddHook("TriggerSleepAction", function (dur)
    print("precise wait")
end, -2)
local oldTriggerSleepAction, _removeFunc
oldTriggerSleepAction, _removeFunc = AddHook("TriggerSleepAction", function(dur)
    print("my wait 1", dur)
    oldTriggerSleepAction(dur)
    print("my wait 2")
end, -1)
TriggerSleepAction(17)
Code:
my wait 1       17
precise wait
my wait 2

By having different priorities, the order in the code should not matter.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Thank you for taking the time to go over this crazy version! I have included a ton of additional comments in the code now to help decypher what the nameless functions/references are doing.

The issue you identified was caused by a missed "n = n + 1" line when inserting into the middle of the list (I had it in one of the beta versions, not sure why I had taken it out), and so now works as expected with the below test code producing the same print statements:

Lua:
function TriggerSleepAction(dur)
    print("sleep", dur)
end
local oldTriggerSleepAction, _removeFunc
oldTriggerSleepAction, _removeFunc = AddHook("TriggerSleepAction", function(dur)
    print("my wait 1", dur)
    oldTriggerSleepAction(dur)
    print("my wait 2")
end, -1)
AddHook("TriggerSleepAction", function (dur)
    print("precise wait")
end, -2)
TriggerSleepAction(17)

_removeFunc(true)

function TriggerSleepAction(dur)
    print("sleep", dur)
end
AddHook("TriggerSleepAction", function (dur)
    print("precise wait")
end, -2)
 
oldTriggerSleepAction, _removeFunc = AddHook("TriggerSleepAction", function(dur)
    print("my wait 1", dur)
    oldTriggerSleepAction(dur)
    print("my wait 2")
end, -1)
TriggerSleepAction(17)
 

Jampion

Code Reviewer
Level 15
Joined
Mar 25, 2016
Messages
1,327
The comments made it much easier to understand what's going on. The usage of local scopes is really smart and results in a very fast algorithm.
It works well for all use cases I can currently think of. Approved.

I would however recommend some changes to the documentation.

I don't think it's necessary to mention earlier versions anymore. This resource has seen many iterations and people that use it probably are already on 5.0. For new people, it can be confusing to mention functions like Hook.add, Hook.addSimple, Hook.remove, Hook.flush.

The default parameter is still hard to understand. From what I understand, it's supposed to be used in combination with storeIntoMetatable.
Is that because you cannot directly call the __index function and so you use the workaround with rawget as default function?
Is there ever a point to use the default parameter, but not storeIntoMetatable? If not, storeIntoMetatable could be removed.
Without storeIntoMetatable, the default parameter is only used, if the hooked function is not found. In that case hooking seems kind of pointless, because it's basically just an alternative way to define a new function.

You are missing a word or two here:
parentTable = mt --index the hook to this metatable instead of to the
 

Wrda

Spell Reviewer
Level 25
Joined
Nov 18, 2012
Messages
1,864
I think this would get greater and even god like if you presented several code examples using the other parameters, and even some other cases.
For example, I've been struggling to make 2 hooks on CreateUnit function for several weeks and I still haven't figured this one out.
Lua:
OnMapInit(function() 
    local oldfunc, remove
    oldfunc, remove = AddHook("CreateUnit", function(p, id, x, y, face)
    print("WORK BEFORE")
    oldfunc(p, id, x, y, face)
    print("WORK AFTER")
    end)
    oldfunc, remove = AddHook("CreateUnit", function(p, id, x, y, face)
        print("WORK BEFORE?")
        oldfunc(p, id, x, y, face)
        print("WORK AFTER?")
    end)
end)
I've tried several variants and even the same structure as you did above, no success.
The resource still can be intimidating even with good documentation you have :p
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I think this would get greater and even god like if you presented several code examples using the other parameters, and even some other cases.
For example, I've been struggling to make 2 hooks on CreateUnit function for several weeks and I still haven't figured this one out.
Lua:
OnMapInit(function()
    local oldfunc, remove
    oldfunc, remove = AddHook("CreateUnit", function(p, id, x, y, face)
    print("WORK BEFORE")
    oldfunc(p, id, x, y, face)
    print("WORK AFTER")
    end)
    oldfunc, remove = AddHook("CreateUnit", function(p, id, x, y, face)
        print("WORK BEFORE?")
        oldfunc(p, id, x, y, face)
        print("WORK AFTER?")
    end)
end)
I've tried several variants and even the same structure as you did above, no success.
The resource still can be intimidating even with good documentation you have :p
In your case, you need to have a variable by a different name (or in a different namespace) for the second hook, because oldFunc is now the same for both hooks (when it should be different).

Additionally, the hook functions need to be returning the return value of CreateUnit.

That being said, as you and Jampion have identified, the examples need to be tweaked. I'll draw a comparison to vJass hooks to show the difference. The best comparison, though, is to simple variable reassignment in Lua.
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I've just pushed out a new update that massively cleans up the inlined comments, changed the requirement of "storeIntoMetatable" to default to True if a default function is provided (as would be the case for most purposes) and fixed a critical bug with removal from the middle of a hook list. Also, added exception handling for when the hook function is not found in the table and no default function was provided.
 
Last edited:

Wrda

Spell Reviewer
Level 25
Joined
Nov 18, 2012
Messages
1,864
Lua:
---@return fun(args_should_match_the_original_function?):any    old_function_or_lower_priority_hooks
It's displaying an error in sumneko extension:
1665249659668.png

Putting the end brackets after "any" solves it :D
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Lua:
---@return fun(args_should_match_the_original_function?):any    old_function_or_lower_priority_hooks
It's displaying an error in sumneko extension: View attachment 410446
Putting the end brackets after "any" solves it :D
Sumneko's extension is glitchy as hell. I'm constantly fighting with it to get it to parse annotations correctly. It also hates when I set variables to nil, and it often has ghost documentation on variables using defunct information.

The only reason I use it is because the EmmyLua extension in JetBrains is somehow much much worse. Nothing beats a properly typed language in trying to represent proper types, let me tell you.
 
Level 20
Joined
Jul 10, 2009
Messages
474
In this case, it's actually the annotation that needs correction ;)
Param ---@return fun(args_should_match_the_original_function?):any should have a type defined within the brackets.
-> Change to ---@return fun(args_should_match_the_original_function?:boolean):any

Sumneko's extension is glitchy as hell.
Glitch behaviour changes from version to version, some are actually running smoothly. Let's let each other know, when we find a stable version after 3.X to stay on :)

Edit: Btw., I'm not currently experiencing sumneko issues as heavy as you described. I feel like properly clearing all warnings from the log (i.e. correcting dirty annotations as demanded by the warnings) greatly enhances extension speed.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Official update to Hook version 5.1; this was a complete rewrite and I am really pleased with the results. This materializes an idea that's been proposed by some others to be able to access raw natives easier, when the user knows they are hooked.

The code should also be MUCH easier to follow now. I didn't have proper indices for the internal tables, and had syntax like [1][2], which (while maybe it performed better) was extremely hard to understand as a reader.

This is still FULLY backwards-compatible with Hook 5 API.

It also introduces a very cool "nativeName.original()" syntax, which means that you can call the original native via any name you want. When inside of a callback, it functions identically to the first return value of AddHook. However, if called outside of a callback, it will always refer to the native function. Note that this API only works for functions which are hooked via AddHook.

This helps to clean up a bunch of code like this (just a rough example):

Lua:
local oldNative
oldNative = AddHook("blah", function() pcall(oldNative) end)

And now:
Lua:
AddHook("blah", function() pcall(blah.oldNative) end)

Note the property names are up to the user - you can use whatever name you want for the "old", "original", "native" or "whatever" function:
Lua:
AddHook("something", function() print "myHook" ; something.iJustMadeThisUpButItStillWorks() end)

This also has a very key benefit to cutting the number of objects stored into the host table in half, because previously I was storing a separate key in the user's table with the prefix "__hookHandler_". While this preserves the purity of the host table, it adds a slight overhead to the callbacks (because the hook first has to pass over the __call metamethod and now tracks the current callback position at each depth level).

You can now also remove all hooks on an object simply by setting the original native back to its host table, such as "TimerStart = TimerStart.nativeFunction".

Lastly, to remove a single hook instance, you'll still need to use the second return value from AddHook, just like before.
 
Last edited:

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Update to 5.1.1 - fixes an issue that was very difficult to isolate. I had broken the metatable functionality by using tables as faux metamethods:

Lua:
local faux = {__call = function() return "success" end}

setmetatable(_G, {__index=faux})

print(nonexistent_key) --prints 'nil'

Lua:
local faux = function() return "success" end

setmetatable(_G, {__index=faux})

print(nonexistent_key) --prints 'success'

Therefore, with this 5.1.1 update, I've needed to disable the HookName.originalNative type of syntax specifically for metatables. It still works as intended for regular tables, however metatables can't use it. It wouldn't have been practical for a user to use that kind of syntax for a metatable anyway, so this shouldn't come as a loss to anyone.
 
Level 20
Joined
Jul 10, 2009
Messages
474
Just took a look at the current version of Hook. The code looks much cleaner now, which I appreciate. I also ran a few tests and the results looked fine.

However, I read something in the resource description that I think must be reworked:

AddHook said:
As of version 5.1, the hooked function is turned into a table, which allows syntax like "CreateTimer.old"
I understand that the syntax is nice this way, but you are very likely to break existing code with this decision. My own map would certainly break.
Two reasons:
  1. People might have type(object)-checks in place, which now return 'table' instead of 'function' for hooked natives.
  2. People might still import resources that overwrite natives without AddHook, i.e. your tables might revert to functions at any random point in time. If that happens, any call of <native>.old will throw an error at runtime.

Bear in mind that so many people consider the missing type-safety to be one of Lua's major flaws, because there are so many scenarios that can screw up. Conducting major type-changes to everyone's development environment just for a bit syntactical convenience is a major no-go, which I kindly request to be reworked.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Good insights! I didn't think of either of these scenarios, so thank you for pointing them out.

I've just published a hotfix which deals directly with the problem proposed in scenario 1. I have set a configurable hook on the "type" method to assess type safety and return the string "function" if the object is a hook table.

The second scenario represents problems all on its own. There will always be a risk of natives with clashing hooks, whether they are using the AddHook resource or not. @Jampion had proposed quite a lot of feedback earlier in this resource's development that indicated a need to prioritize different hooks, and ensure that they interact with each other in a cooperative way.

My guidance is to only use raw hooks (ones that don't use AddHook) only during the Lua root (e.g. Lua-Infused GUI) and which intend to stay for the duration of the game, unless it is an obscure enough native that they are confident has no other competition. Anything else would be wiser to use AddHook, for dynamics and for cooperative prioritization.
 
Level 20
Joined
Jul 10, 2009
Messages
474
Conducting major type-changes to everyone's development environment just for a bit syntactical convenience is a major no-go
I have a strong opinion about this and must repeat this, the type change is a no-go. Please don't pseudo-solve it by adding workarounds. Your method is far too invasive given that there is absolutely no good reason for it (and overwriting type makes it even worse). I kindly ask you to rework that part of your resource to not using table-calls.

Also note that there might be any number of other issues with your function -> table replacement that we currently don't see. The two I mentioned were just examples that I came across quickly. Type-changes like this can have unpredictable consequences, so they should only be used as a last resort.

The second scenario represents problems all on its own. There will always be a risk of natives with clashing hooks, whether they are using the AddHook resource or not
My guidance is to only use raw hooks (ones that don't use AddHook) only during the Lua root (e.g. Lua-Infused GUI) and which intend to stay for the duration of the game
Your guidance doesn't solve the issue. The mixture of AddHook and normal hooks can already occur in Lua root.
Honestly, your resource must be compatible with normal hooks. Everything else would be terrible.
Sticking to functions easily solves this. Sure thing, priorities would only apply to those functions added via AddHook, but that's obvious and most importantly it doesn't crash.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I have a strong opinion about this and must repeat this, the type change is a no-go. Please don't pseudo-solve it by adding workarounds. Your method is far too invasive given that there is absolutely no good reason for it (and overwriting type makes it even worse). I kindly ask you to rework that part of your resource to not using table-calls.

Also note that there might be any number of other issues with your function -> table replacement that we currently don't see. The two I mentioned were just examples that I came across quickly. Type-changes like this can have unpredictable consequences, so they should only be used as a last resort.



Your guidance doesn't solve the issue. The mixture of AddHook and normal hooks can already occur in Lua root.
Honestly, your resource must be compatible with normal hooks. Everything else would be terrible.
Sticking to functions easily solves this. Sure thing, priorities would only apply to those functions added via AddHook, but that's obvious and most importantly it doesn't crash.
Yes, sorry it was a solution I typed up quickly and didn't give it much thought. In the back of my mind, I was not satisfied either, but just wanted to not leave an obvious problem unaddressed.

The solution I envision in the more stable long term is something like:

Hook.print (accesses the "old" print, or just "print" if there are no hooks on it).

Internally, further hooks would be stored by their table. A hooked non-G table would be accessed by that table as a key:

Hook[Event].OnUnitIndex

This would solve the "maybe there is still something unaccounted for" problem as well as not devolve the methodology back to the much worse solution that had been in-place in prior versions of Hook.

I'll definitely keep AddHook as a name around for legacy purposes, but this change would also enable something I've wanted to do for a while:

Hook("print", function(s) ... end)
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
Another major rewrite of Hook:

Version 6

While I had no choice but to break the functionality introduced in Hook version 5.1, I hope the quality of life and aesthetic/simplicity of implementing Hook v6 will make it worth everyone's while.

What functionality was lost?

Lua:
BJDebugMsg.old(s)
This will no longer work. Please use this instead:

Lua:
Hook.BJDebugMsg(s)

What functionality was retained?

Everything else.

What functionality is new?

Well, here's where it gets fun. You can now hook an object in the _G table simply by declaring it as a property of Hook:

Lua:
function Hook.RemoveUnit(u) recycleUnitInstead(u) end

Want to see what that would look like prior to version 6?

Lua:
AddHook("RemoveUnit", function(u) recycleUnitInstead(u) end)

The above still works, of course, but it does make it a bit less tedious to implement.

Removing hooks from _G can now be done via:

Lua:
Hook.TimerStart = nil

If the above is called from within a TimerStart hook, it will just remove that particular hook. If it's used from outside of the callback, it will remove all hooks on TimerStart.

Calling "Hook.print" will now successfully call the normal "print" if there are no hooks on "print".

Thanks @Eikonium for challenging me on the prior approach. In fact, I had previously run into issues with metatables in version 5.1 that needed to be patched because of that table-as-function approach, because metatable methods like __index do not cooperate when set to a table with a __call metamethod - you can only use actual functions.

While I hate to break backwards compatibility, this is definitely a safer (and more feature-rich) approach.

Anything else?

While rewriting everything, I had resolved to include a "default default" if a hook item did not exist in a non-_G table. I was thinking of what I had been coding in the Event library, and realized that I kept having to write out "DoNothing, false" as the last two parameters. This was annoying for me as its author, so I'd rather spare everyone the trouble of doing such madness themselves in the future.

Lastly, priorities of equal value will now append themselves chronologically, rather than prepend themselves. So if you add 3 hooks (A, B, C), they will now be called in the order of A, B, C, whereas they previously were called in the order of C, B, A.
 

Bribe

Code Moderator
Level 50
Joined
Sep 26, 2009
Messages
9,456
I've given it much thought, and found that there were far too many holes with the way I had tried the approach with Hook 5.1 and Hook 6. Trying to arbitrarily find whatever the "current hook" is adds a whole lot of overhead and unpredictability in how hooks talk to each other.

I'm now introducing Hook v7.

Hook v7 should be the final incarnation. I do not intend to do any further updates to this resource.

API:
  • AddHook -> works as it always has.
  • Hook(...) -> same args as AddHook, but the callback function takes the hook instance as a first parameter. Returns the hook instance, rather than the "next" and "remove" functions returned by AddHook. Also returns the table that stores all the hooks on that variable, but that's unlikely to be very popular.
  • function Hook:functionName(...) ... end -> Hooks functionName, but the defined function takes the hook instance as the first parameter (hence I recommend declaring "Hook:" instead of "Hook." so as to have cleaner syntax for the user).
The Hook instance has two key properties intended for the user: .old and :remove. Passing the hook instance as the first parameter to each hook is the easiest way to solve the problems I had been trying to deal with since version 5, and which had been glaring weaknesses of all prior versions of Hook by not having a clean solution.

Examples from the previous post would now look like this:

Lua:
function Hook:RemoveUnit(u) recycleUnitInstead(u) end

function Hook:TimerStart(...)
    print "a timer has been started!"
    self:remove()
    self.old(...)
end
 
Last edited:
Top