Some API Brevity/Clarity considerations

[Suor]

I should start from that I looked through most of the docs and some code, and I appreciate extra safety you minted in here. I also see that you care about clarity of your API a lot. Some of it felt somewhat bulky to me though. I might sometimes err on side of conciseness but reading your examples I foresee an internal resistance in migrating my mods to modern hooks.

We might come from different backgrounds and put our eyes onto different things. And I acknowledge such qualities of the library as safety, predictability and clarity. However, I also see brevity, slickness, even "coolness" as desirable. You can look at them as PR stuff, and I guess you want people to want to use your hooks over Adam's. For that to achieve making modern hooks fun to use might be invaluable.

Also I am not sure how open you are at this point to discussing Modern Hooks API, I guess it's better now than even later :) I thought of what looks not fun or otherwise might be improved in current API and sumarized it into several points:

1. Conciseness is part of clarity.
2. Too much nesting.
3. Artificial grouping.

Before expanding on that points I will try to apply them to your Migration example:

local mod = :Hooks.register(::MyCoolMod.ID, ::MyCoolMod.Name, ::MyCoolMod.Version);
mod.require("mod_i_need", "msu >= 1.2.6"); 
mod.failIf("mod_cool_mod_old", "mod_legends < 16");  // or proibit() or .incompatibleWith()
// Note a partial version, e.g. 16, might also be smth like 16.2

// varargs here, array might be fine too
mod.queue(">som_mod", "<other_mod", function () {
    // Using mod instead of passing id every time
    mod.addFields("scripts/some/file", {Value = 0});

    // Or automatically differentiate field vs method by type
    // Able to group sematically instead of by type
    mod.add("scripts/some/file", {
        Value = 0
        function getValue() {
            return this.m.Value;
        }
    })

    // NOTE: this one has an issue of worse integrating with IDEs I guess
    // 1 less level of nesting
    mod.wrap("scripts/items/item", "getName", function (_original) {
        // ...
    })

    // 2 less levels of nesting
    mod.wrap("scripts/items/item", "getName", function (_original, _param1, _param2 = "default") {
        // ...
    })

    // Not repeating Hook twice
    mod.hookRaw(...)
})

// No order requirements
mod.queue(function () { ... })

// other files will need
local mod = ::Hooks.getMod(::MyCoolMod.ID);

I will expand now.

1. You can see that I fell back into string version requirements, this is in my opinion a good DSL, which is understandable and concise, at least as long as you only have >=, >, <= and <. This is probably the reason why this is used in setting requirements in many ecosystems, e.g. Python and Node.js. One doesn't need ! if we separate incompatibilities from requirements, which makes sense.

Separating requirements from queue order is also a obvoiusly good decision, but may reuse >name and <name syntax here. The thing is in most cases it will be just:

mod.queue(">mod_other", function () { ... })

// which looks significatly clearer to me than

mod.queueFunction({After = "mod_other"}, function () { ... })

Note that I also removed Function/function dup here, same as ::Hooks.rawHook.

2. Once we write:

mod.queueFunction({...},function() {
    ::Hooks.wrapFunctions(::MyMod.ID, "scripts/ui/global/data_helper", {
        function addFlagsToUIData( _originalFunction )
        {
            return function( _entity, _activeEntity, _target ) {
                ::logInfo("addFlagsToUIData");
                return _originalFunction(_entity, _activeEntity, _target)
            }
        }
    });

We have 4 levels of nesting, which inhibits code readability. One can use queue(...{::include(...)}) trick to make it 3, but first, I don't think we should force people to split their files needlessly and second, 3 is still worse than 2 or 1.

Depending on where you come from it might not seem big, but I can assure you people are constantly confused by this, especially higher order pass function/return function thing. I wrote my fair share of decorators in Python and I saw enough of my teammates having trouble with them. So once I wrote a @decorator able to turn this:

def log_calls(prefix=""):
    def decorator(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            print(f"{prefix}{func.__name__}({args}, ...)")
            return func(*args, **kwargs)
        return wrapper
    return decorator

# Into this
@decorator
def log_calls(call, prefix=""):
    print(f"{prefix}{call._func.__name__}({call._args}, ...)")
    return call()

Everybody jumped ship pretty fast, even ones that were never confused by this and despite some performance loss.

3. The way addFields(), setFields(), addFunctions(), wrapFunctions(), etc go now they will encourage people to group their hooks by type, while I think this goes against readability, i.e.:

::Hooks.addFields(..., {
    Strength = 0
    Name = "<not set>"
})

::Hooks.addFunctions(..., {
    function getStrength() {
        return this.m.Strength
    }    
    function getName() {
        return this.m.Name
    }    
})

::Hooks.wrapFunctions(..., {
    function updateStrength(_original) {
        return function () { ... }
    }    
})

While it would be more readable to group Strength + getStrength() + updateStrength() and Name + getName().

I don't have a really good solution here though. Adam's hooks, however, don't discourage me from doing it now. Automatically detecting types in .add() and .set() goes half way only and add extra magic with it.

So what do you think about it? Are you open to this kind of discussion at this point? And if you are what's your take on this?

[Enduriel]

I'll repeat part of our dms here for context:
I 100% agree with your assessment of my personal priorities and the fact that I often err way too far on the side of being explicit and 'correct' to the detriment of actual usability. I generally require other people to point this out to me and wrangle me back to a reasonable place.

And again, this is also for anyone else reading, until 1.0.0 I am willing to make any changes to improve the API, including significant overhauls like what you described (for better or worse :/ sorry to those who may end up having to rewrite stuff).

I'll now go through your suggestions one by one and give comments as I go along.

Compatibility

mod.require("mod_i_need", "msu >= 1.2.6");
mod.failIf("mod_cool_mod_old", "mod_legends < 16");

On a fundamental level, I really like this approach, I think it's cleaner than mod_hook's single string, which I am not a fan of, while avoiding the verbosity in the current beta version.

I'm not personally a fan of partial semver versions, since they're never really necessary. Additionally, I unfortunately need to maintain backwards compatibility for non-semver versions from mod_hooks. This means versions with 0 or 1 decimals will be read as floats.

My other reason for switching from the single requirements string of mod_hooks was that some modders use very unreadable mod IDs, which makes it impossible to identify the mod they are referring to. This means users are confused as to what mod is actually required by a mod. I want to encourage the use of a readable version of the mod ID in mod requirements, to be displayed to users when that mod is missing.

I think I'd be happy with something like

mod.requires("mod_i_need (Needed Mod)", "msu >= 1.2.6 (Modding Standards & Utilities)");
mod.conflicts/*maybe With here*/("mod_cool_mod_old", "mod_legends < 16.0.0", "mod_using_hooks_versioning < 16.1");

Though I'm a little worried about extensibility in the future, then again we can still use [] and {} and I can't imagine that many more things might need to be added in there.

Queueing

mod.queue(">som_mod", "<other_mod", function () {});

As for this, honestly I think I'm happy with that as is, readable and easy enough to manage. Doc might be a bit strange to write but example should make it very simple. One downside is that there's no enforcement of grouping 'Before' and 'After' sections, but I think I'd rather not enforce it in this case.

Adding

mod.add("scripts/some/file", {
	Value = 0,
	function getValue() {
		return this.m.Value;
	}
});

Here, I like removing the mod ID parameter and shortening the function name.

As much as I think it's neat, I'm a bit concerned about merging the addition of functions and fields into a single table, because that doesn't represent how stuff is finally added at the end. (Fields go into an internal m table). Maybe this wouldn't be as confused for modders as I worry it might be? Not sure here, and would like to discuss that last point. (For my personal work though I agree it'd be really clean)

Wrapping

mod.wrap("scripts/items/item", "getName", function (_original) {
        // ...
    })

With this, I'm not a fan of the string function name, because, as you pointed out, it means IDEs will not identify this hook which will significantly worsen the searchability of modern hooks's mods.

mod.wrap("scripts/items/item", "getName", function (_original, _param1, _param2 = "default") {
    // ...
});

This approach may be worth investigating (specifically passing the original function as a parameter at the beginning). It would even be possible to pass the original function to the _original parameter no matter where it is placed (or not pass it if it is not present). I'd like to get specifically @TaroEld's and @LordMidas's opinion on this. Maybe even something like __original (which doesn't follow normal parameter naming conventions) would be better, this is similar to the way HarmonyLib handles harmony patch parameters, and I think taking inspiration from that is definitely a good direction.

Grouping

As for this point, I have to admit here I don't see things your way at all. In my experience, you will rarely want to group functions you add and those you wrap, and generally I've specifically wanted to separate functions I'm modifying and those I'm adding it also makes it much easier to read through the code someone writes, where you can check for conflicts only by reading the wrapper section.

The example you gave is obviously pretty unrealistic (though I acknowledge it was just a quick example), and I'd challenge you to think of a more realistic one. In contrast it is very easy to think of reasons why grouping by stuff you're modifying vs stuff you're adding would make logical:

• easier maintainability with game/mod updates
• easier maintainability of compatibility for other mod authors
• when you add content, you generally add multiple similar functions and fields, which you'd want to group together, but you'd rarely also wrap a function which is directly related
• generally nicer on the eyes and brain imo (grouping of changes and additions)

Final Note

Overall though, these are all really great points and I really appreciate you going through the doc and code and making them. I also invite others to make suggestions and give comments as well.

[Suor]

After giving it some extra thought I see that we can leverage Squirrel syntax here. It already has add/set distinction with <- vs = operators. So the example might be rewritten as:

// I renamed ::Hooks to ::Mods in this example as it makes more sense now
local mod = ::Mods.register(::MyCoolMod.ID, ::MyCoolMod.Name, ::MyCoolMod.Version);

// Verbose name is optional, everybody knows msu and legends anyway
mod.require("some_utility_mod (Proper Name)", "msu >= 1.2.6");
// Is able to run .requires() more than once
if (::Mods.have("mod_legends")) mod.require("some_utility_mod_legends");

mod.conflicts("...");

mod.queue(">som_mod", "<other_mod", function () {
    mod.hook("scripts/some/file", function (o) { // special object
        // add
        o.m.Value <- 0;
        o.getValue <- function() {
            return this.m.Value;
        }

        // set
        o.m.Name = "<not set>";
        o.getName = function () { ... }

        // wrap, less DRY but less nesting. Also very familar to people already
        local getName = o.getName;
        o.getName = function () {
            return getName() + " the First";
        }

        // or if we are wiling to use param introspection
        o.getName = function (_original) { // If there is no _original like above it won't be passed
            return _original() + " the First";
        }
    })

    mod.hookRaw("scripts/...", function (o) { // normal object
        ...
    })
})

// other files will need
local mod = ::Mods.get(::MyCoolMod.ID);

This should be achievable with something like:

// A demo code, never been even close to be ran, but metamethods do work as expected here
::Mods <- {impl = {}}
::Mods.impl <- {
    memberProxyProto = {
        function _newslot(idx, value) {
            // This obviously won't look like this, just a demonstration in terms of current API
            ::Hooks.addFields(this.__modId, this.__script, {idx: value});
        }
        function _set(idx, value) {
            ::Hooks.setFields(this.__modId, this.__script, {idx: value});
        }
        function _get(idx) {
            return this.__ref[idx];
        }
        // probably also at least _delslot
    }
    proxyProto = {
        function _newslot(idx, value) {
            ::Hooks.addFunctions(this.__modId, this.__script, {idx: value});
        }
        function _set(idx, value) {
            local infos = value.getinfos()
            local passOriginal = ...;
            ::Hooks.wrapFunctions(this.__modId, this.__script, {idx: function (_original) {
                // TODO: handle other args and context here
                return passOriginal ? value(_original) : value();
            }});
        }
        function _get(idx) {
            return this.__ref[idx];
        }
    }
}

Mod.hook <- function (script, callback) {
    local modId = this.Id;
    Mod.hookRaw(script, function (o) {
        local proxy = {
            __ref = o
            __script = script
            __modId = modId
            m = {}.setdelegate(::Mods.impl.memberProxyProto)
        }.setdelegate(::Mods.impl.proxyProto);
        callback(proxy);
    })
}

The advantages are:

• a familiar syntax
• 1 less nesting level
• natural IDE support
• extreme flexibility, so grouping question is left for author to decide

We can also join .queue() and .hook() into Mod.hook():

// Not sure should order params go before or after script
mod.hook("script/...", ">some_mode", "<other_mod", function (o) { // a proxy object
    // ... do stuff
})

This will remove another level of nesting and also ensure it's impossible to hook outside of queue. Might also be seen as more magic though. Might also be less DRY if you'll need to repeat order params several times.

I will write later more extensively on why I prefer to group by domain not by type and will try to come up with a better example. Even if this may become irrelevant for this particular story.

[Suor]

P.S. Mixing .queue() and .hook() params actually looks messy and also will need to be duplicated for .hookRaw() or whatever else. So instead we may employ chaining:

mod.queue(">some_mod").hook("scripts/player", function (patch) {  
    // whether call it proxy, patch or just o is a separate question
    patch.getDailyCost = function () { ... }
    ...    
})

// A queue might be reused to not repeat yourself
local queue = mod.queue(">some_mod");
queue.hook("scripts/player", function (o) {...})
queue.hook("scripts/item", function (o) {...})
queue.hookRaw("scripts/something", function (o) {...})

// Also may still pass a callback
queue.push(function () {
    ::logInfo(::MyCoolMod.ID + " done hooking");
    // Maybe even:
    mod.logInfo("done hooking"); // adds a prefix automatically    
})

// And still can do
mod.hook("scripts/player", function (p) {...});
// which will be a shorthand for
mod.queue().hook("scripts/player", function (p) {...});

It now looks quite fun even by my standarts :)

[Suor]

An example of how things are grouped semantically not by their type, i.e. members, sets, adds. This improves read and change locality, which translates as better code to me:

local mod = ::Mods.register("item_overhaul", "0.1.0", "Item Overhaul");

mod.hook("scipts/items/item", function (p) {
    // Weight system
    p.m.Weight <- 0; // To be set in real items
    p.m.BagWeightMult <- 0.5; // As opposed to hand
    p.getWeight <- function () {
        local mult = this.inBag() ? this.m.BagWeightMult : 1.0;
        return mult * this.m.Weight;
    }
    p.getTooltip = function (_original) {
        local result = _original();
        result.push({text = "Weighs " + this.getWeight() + " kg", ...});
        return result;
    }
    p.onPickedUp = function (_original) {
        _original();
        local user = this.getContainer().getActor();
        user.setFatigue(user.getFatigue() + Math.ceil(this.getWeight()));
    }

    // Entirely independent price modifiers system
    p.m.Used <- false;
    p.m.UsedDiscount <- 0.3;
    p.onCombatStarted = function (_original) {
        if (this.isEquipped()) this.m.Used = true;
        return _original();
    }
    p.getSellPriceMult = function (_original) {
        local mult = _original();
        if (this.m.Used) mult -= this.m.UsedDiscount;
        if (World.Retinue.hasFollower("follower.negotiator")) mult += 0.15;
        if (::MSU.isKindOf(this, "armor")
            && World.Retinue.hasFollower("follower.blacksmith")) mult += 0.1;
        return mult;
    }
    p.getBuyPrice = function () {
        local mult = _original();
        if (...) mult *= 0.9;
        // ...
        return mult;
    }
})

If these two aspects grow then each might even get its separate file, despite working with the same class/object.

[Enduriel]

I've been convinced by your suggestion that a hook should be a function just like mod hooks. I want to suggest an approach that looks something like this:

mod.hook("scripts/...", function(d){ // d for dummy? I want to make it clear that this isn't the prototype object unlike the raw hooks
	// also trying to _get from d should always fail
	// addFields equivalent
	d.m <- { 
		SomeNewValue = "Value"
	}
	d.m.SomeNewValue <- "Value" // I don't really want to allow this approach so there is only one way to achieve this but I suspect people will disagree and I will be convinced otherwise

	// addFunctions equivalent
	d.foo <- function() 
	{
		return "foo";
	}

	// wrapFunctions equivalent
	d.alreadyExistingFoo = function( __original ) // double underscore here to distinguish from normal parameters, __original is the function being replaced
	{
		// should this error if the function doesn't exist or should it just pass a dummy __original?
		// I am planning to error but want to hear suggestions on this
		// I could add a realtime check in a sort of debug mode perhaps to verify if a function is actually calling __original
		// but I don't want to add a realtime check for this in all players' games
		return __original();
	}

	// setFields equivalent
	d.m = {
		SomeAlreadyExistingValue = "NewValue"
	}
	d.m.SomeAlreadyExistingValue = "NewValue";
});
// then we have the same exact thing for hookLeaf
mod.hookLeaf("scripts/...", function(d){

});
// and for raw hooks we switch to 'p' since we are now modifying the prototype directly
mod.hookRaw("scripts/...", function(p){

})

mod.hookRawLeaf("scripts/...", function(p)
{

})

However, as we discussed I don't believe the changes to the queuing system are necessary. I think a single function the same way as it is handled in Adam's hooks is simple and functional enough for all use-cases. Your concerns as to indentation are irrelevant if people use the (much more readable) multi-file organizational structure, and for smaller mods this kind of thing is mostly irrelevant in my opinion compared to the added complexity both from a usability and backend complexity point of view.

[Suor]

We are getting close I guess. And since the most tricky "wrap nesting" is out of the way then it's already looks much nicer to me :)

However, I would say:

d.m <- {  // same for d.m = ...
    SomeNewValue = "Value"
}

Is way too misleading, it's like I am trying to overwrite the whole m key. I would much prefer

d.m.SomeNewValue <- "Value";

And I guess we either can't or even don't want to have both.

As for special object to patch things, I would call it patch or patcher, because that's what we are doing here anyway:

mod.hook("scripts/entity/tactical/actor", function (patch) {
    patch.onBlow = function (__original, _attacker) {
        __original(_attacker)
        // ... do something extra
    }
})

// Or may use "this"
mod.hook("scripts/entity/tactical/actor", function () {
    this.m.Name = "...";

    this.onBlow = function (__original, _attacker) {
        __original(_attacker)
        // ... do something extra
    }
})

// Or alternative use hook itself
local hook = mod.hook("scripts/entity/tactical/actor");
hook.onBlow = function (__original, _attacker) {
    __original(_attacker)
    // ... do something extra
}

Any single letter var people see in docs example would be misinterpreted way more reliably. And for raw can call it proto:

mod.hookRaw("scripts/entity/tactical/actor", function (proto) {
    proto.onBlow = function (_attacker) {
        // ... no __original here
    }
})

About restricting read access. It makes sense to limit things initially, it will be easy to extend them later but quite hard to limit back. However, one might need that, e.g.:

mod.hookLeafs("entity/world/settlement", function (patch) {
    if (patch.ClassName.find("_coast_") == null) return;

    // Spawn pirates in all coastal cities
    patch.create = function (__original) {
        create();
        this.m.DraftList.push("my_expansion/pirate_background");
    }
})

That's what I am actually doing now in one of my mods. Another thing connected to reading stuff is modifying mutable collections:

mod.hook("ai/tactical/behavior/ai_attack_swing", function (patch) {
    patch.m.PossibleSkills.push("actives.my_swing");    
})

BTW, the need to prefix everything with scripts/ would be a major source of confusion during migration. Even aware of that I made this mistake above. Aren't it impossible to inherit or new something outisde of scripts/? I.e. I remember that BB classes to be put there while some other code might go outside and be included manually. Anyway, unless you are planning to make people hook things outside scripts I would stick with not requiring it.

As for queue stuff, I guess you made your mind, and I don't have new arguments here. This is probably more of a preference. People using text editors, like me, generally prefer smaller number of bigger files and ones with IDEs bigger number of smaller files. Or maybe it came from language/platform used.

I still think queue in each separate file is better in terms of code locality than a central one, heh. :)

On double dash __original I understand your point. And as much as I would like to hide any double underscores deep into code that I won't see much we do share the namespace with arbitrary params here.

[Suor]

P.S. Even if we limit attribute access people might still do "attrName" in patch and be surprised since there is no metamethod in squirrel to control in operator. A good reason to call it something other than o :)

[Enduriel]

Yeah I've been convinced that the m table setter in this style is a bad idea so we can do it the old-fashioned way.

d.m <- {  // same for d.m = ...
    SomeNewValue = "Value"
}

I've also been convinced that we need an accessor for fields so will allow m table access, but other than being able to check for the existence of functions (using a functions we could add like exists()), I don't think we should be able to directly get a function in these processed hooks, worst case, as you say, we can re-evaluate later.

As to the scripts/ prefix, that has caused a lot of issues for me and other due to

• being unable to hook files outside of scripts/ (as you described), there is no reason this shouldn't be possible
• ::IO.enumerateFiles() returns full paths with scripts/
• when instantiating an object you pass the full path with scripts/

It is inconsistent and confusing that for this one scenario you do not need to pass the full path, and has caught out myself and others many times that I've seen. I understand there will be some growing pains with migration but this will be better in the long run.

[Enduriel]

As to the patch/proto full word I personally think having a single character is quite clean and despite some initial confusion when writing 1000s of lines of code it actually saves on a lot of characters and mental energy. I like having p for prototype for the rawHook, the problem is that neither I nor Midas nor Taro are able to think of a good (unreserved) character that can stand for a word for the patcher/dummy object

[Suor]

What are the blessed ways for pirate and swing examples then? Should I fall back to raw hooks for such things? Or you mean you intend to provide read access to m table too?

Currently the pirate example also far from pretty as ClassName is not yet set in ::mods_hookDescendants() so I am in fact using that + "inherit" hook together in a quite ugly code:

::mods_queue(::HackflowsExp.ID, "mod_hooks(>=20)", function() {
    ::mods_hookDescendants("entity/world/settlement", function (cls) {
        // .ClassName is not yet set here on cls, so we set a flag and parse script name in inherit
        cls.hackflows_isSettlement <- true;
    })
    ::mods_addHook("inherit", function (baseScript, script, cls) {
        if (!("hackflows_isSettlement" in cls)) return;

        local name = split(script, "/").top();
        local backgrounds = ...

        local create = cls.create;
        cls.create = function () {
            create();
            this.m.DraftList.extend(backgrounds);
        }
    })
})

I probably can simplify this by delaying backgrounds list calculation though. Will loose a bit in performance.

BTW, would modern hooks provide event hooks like that? These are quite rare to be used, but I saw people using campaignLoaded somewhere.

[Suor]

As for names, I personally prefer to not use single character vars for something spanning more than several lines. I.e. primarily use them in list comprehensions, lambdas and such. For patch/dummy I can also think about proxy and mock, but those are p and m again :) These are only repeated once per hook so probably less than usual param name though. Also less as we won't do while (!("setCondition" in obj)) obj = obj[obj.SuperName]; anymore.

Another thing is confusing me why do you use singular for leaf when we patch presumably more than one thing at a time this way, i.e. it should be .hookLeafs() over .hookLeaf().

And for "raw", I would put it either at the start (as you had originally) or in the end and never in the middle, i.e. rawHookLeafs() or hookLeafsRaw() not hookRawLeafs(), even the latter might sound more english.

[Enduriel]

For your first comment I ask for clarification because I don't understand the example you are giving. I did say I would provide read access to m table and do intend to do so.

As to your second comment, like I said, when writing big mods you need to write this object/prototype/patcher a lot and it is annoying. I do not see myself (or many others) doing so long term so a single character is almost necessary to start a convention in my opinion.

Maybe leafHook is better than hookLeaf or with your suggestion hookLeaves, the point is that the hook is a 'leaf style hook', because 'leaves' aren't really a precise way to describe exactly what gets hooked since abstract classes (and the target itself) also gets hooked, it just doesn't matter if they don't get instantiated.

This is also why rawLeafHook is best in my opinion.

[Suor]

Sorry for a messy example, the point is better made in the previous "pirate" sample code, i.e.:

mod.leafHook("scripts/entity/world/settlement", function (d) {
    // Only do the patching for coastal towns
    if (d.ClassName.find("_coast_") == null) return;

    // Spawn pirates in all coastal cities
    d.create = function (__original) {
        create();
        this.m.DraftList.push("my_expansion/pirate_background");
    }
})

Here I am examining .ClassName to decide whether to hook or not. So if we don't have read access on patch/dummy object but only on it's .m then I am wondering what would be the blessed way to do this kind of thing? I still want to hook many classes at once, so using a leaf hook feels natural.

About names, sorry for making it look like I am pressing you about this. I also didn't realize that "Leaf" may be taken as an adjective here, which added to my confusion. In the end I only share my experiences here and you will make a final call on any function names or doc example var names anyway. And I am grateful for all your work around this and around MSU.

I would also say the very fact that we are discussing names says that we have a solution for all the fundamentals.

[Enduriel]

As to your second point I don't mind I tend to be a bit of a perfectionist so feel free to critisize everything, just don't expect me to always agree 😝

As to your first point I understand now, I think you're right, and I will have to allow access to some of these special fields. I still don't think function access is necessary (and is in fact detrimental) in non-raw hooks so long as we have an exists() function to check for the presence of a function.

[Suor]

On an attempt to get a function we can even throw with a message "prohibited ..., probably should use wrap syntax instead like ... function (__original, ...) or if you absolutely must then can use raw hooks" and a link to docs.

BTW, when it's needed to check for presence of a function? I've only ever done this for a while (!("funcName" in obj)) obj = obj[obj.SuperName]; loop myself, but this should be covered now by leaf hooks as I understand.

[Enduriel]

I can imagine someone wanting to add a function to a bb class if they're used to it being there but it actually being added by another mod so they would add it conditionally (not saying this is a great pattern, just that it is something I could think of easily)

[Enduriel]

I've been working on implementing this and once I got to the _set metamethod (equivalent of overwriting.wrapping functions) I realized that with this approach we actually incur a runtime cost due to an additional function call required to pass the __original parameter. I'm not a huge fan of this since one of my main considerations was to (virtually) eliminate all possible runtime costs of the hooks framework. This means we're back to requiring an additional indentation level for wrapping, something like

o.foo = function(__original) {
	return function( _foo1, _foo2 ) {
		::logInfo("asdf");
		return __original();
	}
}

I know that's not ideal, but I don't think we're going to get anything better without runtime costs or a possibility for arbitrary temporary functions names (which I really want to avoid). I'll go forward implementing this approach, since I can tweak the syntax later without too many issues, but looking for comment on this.

[Suor]

There is a way to still achieve it with function.setroot():

function wrap3(original, wrapper) {
    // Cannot use local cause weakref won't hold to it.
    // Will need to put it to somewhere permament.
    ::closure <- {__original = orginal}; 
    return wrapper.setroot(closure);
}
Obj.foo = wrap3(Obj.foo, function (x) {
    return __original(x)
});

The overhead is the same as doing closure manually, either via local __original or passing param.

[Suor]

P.S. Unfortunately it looks like Battle Brothers Squirrel doesn't have .setroot() :(

[Suor]

Looks like short of some sort of eval we can't do that with minimal, i.e. same as raw wrapping overhead. However in my tests:

::Obj <- {
    Name = "Ulrich"
    function foo(x) {
        return this.Name + " " + x + "\n";
    }
}
local unwrapped = ::Obj.foo;

// 416 ns per iteration
for (local i = 0; i<10000000; i++) Obj.foo(i); 

// 462 ns
function wrap1(func) {
    local __original = func;
    return function (x) {
        return __original(x)
    }
}
Obj.foo = wrap1(unwrapped);
for (local i = 0; i<10000000; i++) Obj.foo(i); 

// 460 ns
function wrap2(__original) {
    return function (x) {
        return __original(x)
    }
}
Obj.foo = wrap2(unwrapped);
for (local i = 0; i<10000000; i++) Obj.foo(i); ;

// 515 ns
function wrap3(func, wrapper) {
    ::closure <- {__original = func}.setdelegate(Obj);  // cannot use local cause weakref won't hold to it
    return wrapper.setroot(closure);
}
Obj.foo = wrap3(Obj.foo, function (x) {
    return __original(x)
});
for (local i = 0; i<10000000; i++) Obj.foo(i); ;

// 879 ns
function wrap4(func, wrapper) {
    return function (...) {
        vargv.insert(0, func);
        vargv.insert(0, this);
        return wrapper.acall(vargv);
    }
}
Obj.foo = wrap4(unwrapped, function (__original, x) {
    return __original(x);
})
for (local i = 0; i<10000000; i++) Obj.foo(i);

// 56 ns
function noop(x) {}
for (local i = 0; i<10000000; i++) noop(i); return

The extra cost is around 400 nanoseconds, which is around 7 noop func calls. I would pay this for nicer interface.

[Suor]

Using compilestring() one can do almost the same as manually:

// 488 ns
function wrap5(func, wrapper) {
    return compilestring(
        "return function (__original, wrapper) {"
        + "    return function(x){ return wrapper(__original, x)"
        + "}}")()(func, wrapper);
}
Obj.foo = wrap5(unwrapped, function (__original, x) {
    return __original(x);
})

Also tried less wrapping in compilestring(), performance is same as above:

wrap5a <- compilestring("local o = vargv[0], w = vargv[1]; return function(x){ return w(o, x) }")

[Suor]

P.P.S. It is kind of possible even without compilestring():

// 480 ns
function wrap6(func, wrapper) {
    local infos = func.getinfos();
    // Fall back to slower version for varargs or too many params
    if (infos.varargs || infos.parameters.len() > 8) return wrap4(func, wrapper);

    switch (infos.parameters.len() - 1) { // excluding "this"
        case 0: return function () { return wrapper(func) };
        case 1: return function (a0) { return wrapper(func, a0) };
        case 2: return function (a0, a1) { return wrapper(func, a0, a1) };
        case 3: return function (a0, a1, a2) { return wrapper(func, a0, a1, a2) };
        case ...:  ...
    }
}

Although this does not handle defaults. With defaults it would be a quadratic number of cases :), still possible to handle the most common ones.

[Enduriel]

After more discussion on discord we ended up settling on

mod.foo = @(__original) function() {
	::logInfo("wrapper");
	return __original();
}

as an acceptable solution to minimize indentation while maintaining simplicity when wrapping functions.

[Enduriel]

via multiple commits including 2e114e3 and 4fd9cf4 I believe this has been resolved and will mark the issue as closed