[Advanced] Modular Label Handling

[shadowart]

Most Xenobot scripts have label handlers that look something like this:

function onWalkerSelectLabel(labelName)
if (labelName == "Start") then
Walker.ConditionalGoto((Self.Position().z == 11), "BeginHunt", "ReachDepot")

elseif (labelName == "DepositGold") then
-- Deposit Gold, check balance.
Walker.Stop()
Self.SayToNpc({"hi", "deposit all", "yes"}, 100)

local withdrawManas = math.max(BuyMana - Self.ItemCount(ManaID), 0)*ManaCost
local withdrawHealths = math.max(BuyHealth - Self.ItemCount(HealthID), 0)*HealthCost
local withdrawAmmo = math.max(BuyAmmo - Self.ItemCount(AmmoID), 0)*AmmoCost
local total = math.abs(withdrawManas + withdrawHealths + withdrawAmmo)

if total >= 1 then
Self.SayToNpc({"withdraw " .. total, "yes", "balance"}, 100)
end
Walker.Start()

elseif (labelName == "DepositItems") then
etc...

Essentially, they're just a long series of if-statements that deal with every possible label. However, when you write large scripts (1000 lines plus), this design will get you into trouble.

Let's say you want to write a module that keeps you hasted while refilling. Then you have create a module somewhere in the code, and then in a totally different place you have to make sure the module is started when you leave the spawn and in yet another place you have to stop it when you enter the spawn.


elseif (labelName == "Leave") then
Module.Start("Haster")
elseif etc...

If you see the code above, how will you know if it works or even what it does? The definition of the Haster module might be hundreds of lines away and it might not even exist - maybe you actually named it "Refill Haster" or forgot to copy it from another script.

When you spread out code related to a single concept it makes your code hard to read and understand. This in turn makes it hard to debug, hard to change and hard to reuse in other scripts.

Now look at this:


do
local LabelHandlers = {}

function RegisterLabelHandler(Label, Name, Handler)
local Data = LabelHandlers[Label] or {}
Data[Name] = Handler
LabelHandlers[Label] = Data
end

local function ExecuteAll(FunctionTable, ...)
for _, f in pairs(FunctionTable) do
f(...)
end
end

registerEventListener(WALKER_SELECTLABEL, "HandleLabel")
function HandleLabel(LabelName)
local LabelParts = LabelName:split(";")
local LabelKey = LabelParts[1]
local HandlerTable = LabelHandlers[LabelKey]
if HandlerTable then
ExecuteAll(HandlerTable, unpack(LabelParts))
end
end
end

This is a framework that will allow you to associate actions with labels anywhere in the code. This is how our hasting module would look with my framework:


Module("Refill Haster", function(Haster)
if not Self.isHasted() and not Self.isInPz() then
Self.Cast("utani hur", 50)
end
end)

RegisterLabelHandler("Hunt", "Refill Haster", function()
if Module.IsActive("Refill Haster") then
Module.Stop("Refill Haster")
end
end)

RegisterLabelHandler("Leave", "Refill Haster", function()
if not Module.IsActive("Refill Haster") then
Module.Start("Refill Haster")
end
end)

Everything related to the haster is now contained in one place and the entire code may be displayed at once on a screen. This will, as I said earlier, make the code easier to debug, easier to change and easier to reuse.

But what if I want to do more than just stop the haster when I come to the "Hunt" label? That's totally fine. Maybe you want to run a gold dropper inside the spawn. That could be done like this:


do
local DropGoldBelowThisCap = 500

Module("Gold Dropper", function(Drop)
if Self.Cap() < DropGoldBelowThisCap then
local pos = Self.Position()
Self.DropItem(pos.x, pos.y, pos.z, "gold coin", 100)
end
end, false)

RegisterLabelHandler("Hunt", "Gold Dropper", function()
if not Module.IsActive("Gold Dropper") then
Module.Start("Gold Dropper")
end
end)

RegisterLabelHandler("Leave", "Gold Dropper", function()
if Module.IsActive("Gold Dropper") then
Module.Stop("Gold Dropper")
end
end)
end

Now your script will both start the gold dropper, and stop the haster when you enter the spawn. However, if you just look at the code related to the gold dropper you will have no idea that stuff related to the haster also happens at the "Hunt" label. Nor should you have to. The two concepts are totally unrelated and should not be intermixed within the code.

Of course you sometimes want to write code that executes at many different labels and does slightly different things depending on the label name. For example, let's say you want to write a randomized script and you want to write a label handler that goes to a random label depending on the label it's executed at. This is how you could do it with my framework:


RegisterLabelHandler("RandomGoto", "RandomGoto", function(_, Tag, Limit)
Walker.Goto(Tag..math.random(1, Limit))
end)

What does this code do? If you encounter a label called "RandomGoto;BeforeBank;3" it will randomly go to one of the labels "BeforeBank1", "BeforeBank2" or "BeforeBank3".

This last example has nothing to do with modularity, nor is it something that couldn't be done using a simple series of if-statements. I just wanted to demonstrate that using my framework doesn't prevent you from writing this kind of code.

[eldera]

Niceee ;D

[alpha90]

Thank you, very useful :o

[DarkstaR]

You're over-thinking it.

function onWalkerSelectLabel(labelName)
_G[labelName]()
end


function myLabel1()


end
function myLabel2()


end

[shadowart]

You're over-thinking it.

function onWalkerSelectLabel(labelName)
_G[labelName]()
end


function myLabel1()


end
function myLabel2()


end
That only allows one function per label which either forces you to combine functions (and breaking the modularity), or adding extra labels which slows down the walker. It also pollutes the global name space.

[DarkstaR]

1. You can't really pollute the global namespace in Lua. It was designed to work that way, and OOP was an afterthought that had to be implemented as an unintended abstraction on tables. The language wasn't designed for OOP, and it definitely wasn't designed to be a functional mess of nested crap. Also, if your script is so complex that you're worrying about polluting global namespace, you might be doing something wrong.

2. Your code is pretty messy. You're nesting a handler for a proxy inside a handler for a label, and then you're doing a bunch of nested looping. It may feel fancy to use closures literally everywhere, but it doesn't make for fast or maintainable code. If you simply had one function for your label, or if you at least created the proxy callback OUTSIDE of the label handler, it would be cleaner (which is your goal, right?).

3. You can easily do multiple functions per label, and you can even embed parameters in the labels in the same way that you did:


-- label should be randomChoice|huntBranch|5 or similar
function onWalkerSelectLabel(labelName)
local stuff = split(labelName, "|")
_G[stuff[1]](unpack(stuff))
end


function randomChoice(labelName, tag, limit)
Walker.Goto(Tag..math.random(1, Limit))
end



4. I'm going to re-visit what I said about nested closures. In your code, it is MESSY, but wont have a huge negative impact. Xeno allows one callback per proxy name, so even if you're calling the code to create the proxy and supplying the closure function multiple times, it will handle it well. You can even do the same from modules. It will handle it well. Your code isn't the same, though, and that's confusing. Here's what I mean.

This will work perfectly fine, because Xeno has been designed to handle it. It's not clean IMHO, but it will work:


RegisterLabelHandler("Hunt", function()
LocalSpeechProxy.OnReceive("Chat Replier", function(_, msgType, speaker, level, text)
--some code
end)
end)


However, if it were inverted, you'd end up with the same closure being called hundreds or thousands of times (it gets re-added every time the LocalSpeechProxy is called):




LocalSpeechProxy.OnReceive("Chat Replier", function(_, msgType, speaker, level, text)
RegisterLabelHandler("Hunt", function()
--some code
end)
end)


I'd call this terrible design, because you're expecting to remember these details and, by releasing it on the forums, you're expecting the community to just figure that out (which they won't without reading the code). And your examples are promoting the use of closures-in-closures for handlers, which will probably make people say "oh, hey, I can do it that way!" and then get banned because XenoBot is saying "hi deposit all yes" 2500 times every time the "Deposit" label is hit.


In conclusion, you're over-thinking it (or under thinking it, depending on your perspective). I say you're overthinking it because, especially when releasing something like this on the forums, the best bet is to keep the framework, examples, and idea very simple. But you could also be under-thinking it, because it would be a good idea if designed right, but it's honestly not. It may work for you, but it's messy.

[shadowart]

Mostly fair points, I can especially see why #4 would be important to you. However, I think you're wrong about #3. What you have is multiple labels tied to one function, not multiple functions tied to one label. You could accomplish that too in a similar manner but it would lead to increased coupling between the waypoints and the script (if you add more modules you need to change the labels), or by combining the functions which I mentioned earlier.

[DarkstaR]

However, I think you're wrong about #3. What you have is multiple labels tied to one function, not multiple functions tied to one label..




function onWalkerSelectLabel(labelName)
local stuff = split(labelName, "|")
local f = _G[stuff[1]]
if (type(f) == "table") then
for _, vf in pairs(f) do vf(unpack(stuff)) end
else
f(unpack(stuff))
end
end


local randomChoice = {}
randomChoice.handler1 = function(labelName, tag, limit)


end
randomChoice.handler2 = function(labelName, tag, limit)


end

[shadowart]

function onWalkerSelectLabel(labelName)
local stuff = split(labelName, "|")
local f = _G[stuff[1]]
if (type(f) == "table") then
for _, vf in pairs(f) do vf(unpack(stuff)) end
else
f(unpack(stuff))
end
end


local randomChoice = {}
randomChoice.handler1 = function(labelName, tag, limit)


end
randomChoice.handler2 = function(labelName, tag, limit)


end
Keeping track of whether there's already another function for a label or not (whether you should just name the function after the label or whether you have to add it to a table - that might or might not exist already) is tedious. You'll have to add a function to "register" the handler for you to avoid keeping track of such stuff manually. And suddenly your solution looks very similar to mine. You've managed to pack my two cases (dispatch on whole label or just prefix) into one but otherwise the differences are superficial.

[DarkstaR]

Keeping track of whether there's already another function for a label or not (whether you should just name the function after the label or whether you have to add it to a table - that might or might not exist already) is tedious. You'll have to add a function to "register" the handler for you to avoid keeping track of such stuff manually. And suddenly your solution looks very similar to mine. You've managed to pack my two cases (dispatch on whole label or just prefix) into one but otherwise the differences are superficial.

They're not "superficial", though. If you simplified yours to handle the edge cases properly and cleaned up the example code, I'd say it's good. But right now, it's not, it has some major issues. Mine may not be as "pretty" (for lack of a better word), but it is easy to use, it is functional, and if you make a mistake, lua's error handler will tell you. And you don't really need to track anything unless you're adding multiple functions per label, which is an edge case that I'm sure most people won't encounter.

[shadowart]

They're not "superficial", though. If you simplified yours to handle the edge cases properly and cleaned up the example code, I'd say it's good. But right now, it's not, it has some major issues. Mine may not be as "pretty" (for lack of a better word), but it is easy to use, it is functional, and if you make a mistake, lua's error handler will tell you. And you don't really need to track anything unless you're adding multiple functions per label, which is an edge case that I'm sure most people won't encounter.
Going back and re-reading your initial criticism I realized that you're right, but this should fix the issues with the dispatcher, right? Nestling closures in the wrong order no longer fucks you up.


do
local LabelHandlers = {}
local LabelMatchers = {}

local function InsertKVAtKey(HandlerTable, Key, Name, Value)
local Data = HandlerTable[Key] or {}
Data[Name] = Value
HandlerTable[Key] = Data
end

local function ExecuteAll(FunctionTable, ...)
for _, f in pairs(FunctionTable) do
f(...)
end
end

registerEventListener(WALKER_SELECTLABEL, "HandleLabel")
function HandleLabel(LabelName)
local HandlerTable = LabelHandlers[LabelName]
if HandlerTable then
ExecuteAll(HandlerTable)
else
local Prefix = LabelName:match("^(.-);(.*)")
HandlerTable = LabelMatchers[Prefix]
if HandlerTable then
ExecuteAll(HandlerTable, LabelName)
end
end
end

function RegisterLabelHandler(Label, Name, Handler)
InsertKVAtKey(LabelHandlers, Label, Name, Handler)
end

function RegisterLabelMatcher(Prefix, Name, Handler)
InsertKVAtKey(LabelMatchers, Prefix, Name, Handler)
end
end


I'll also change the second example to something simpler, like a gold dropper.

[DarkstaR]

Yeah, that fixes it so you can keep nesting your closures (plx god nao). I still don't like the matcher, but that's more of a preference thing. I would get rid of it and instead use the | or ; delimiting of params like in my example (that's how JX did it afaik, so credit to them)

[shadowart]

Yeah, that fixes it so you can keep nesting your closures (plx god nao). I still don't like the matcher, but that's more of a preference thing. I would get rid of it and instead use the | or ; delimiting of params like in my example (that's how JX did it afaik, so credit to them)
Yeah I agree that feels better; I wanted to keep it close to my original version but I can change that too. I'll update the op tomorrow as it is sleepy time for me now.

[shadowart]

Updated op.