feat: LuaCallable#224
Conversation
|
Builded and tested in a linux environment only I didn't use include guards like other scripts I've seen and may've broken some other styling rules, so pls tell me if I have to change it Also I've tried using a LuaCoroutine to solve a call freezing issue, but either I didn't understand how it's supposed to be used or did something wrong, anyways, it didn't work And I did a somewhat hacky thing with the lua definition, since I didn't really have time to delve into this part, if I should have done it differently pls tell/change |
| LuaCallable::~LuaCallable() { | ||
| _lua_func.unref(); | ||
| } |
There was a problem hiding this comment.
You don't really need to manually call unref, the Ref's destructor already does that, so a default destructor would be enough here.
| class LuaCallable : public CallableCustom { | ||
| Ref<LuaFunction> _lua_func; | ||
| public: | ||
| explicit LuaCallable(sol::function func) : _lua_func{memnew(LuaFunction(func))} {}; |
There was a problem hiding this comment.
Please use sol::protected_function here instead of sol::function, just like everywhere else in the code.
Also do not create a new LuaFunction here, use LuaObject::wrap_object<LuaFunction>(sol::protected_function f) instead.
| #include "Class.hpp" | ||
| #include "VariantArguments.hpp" | ||
| #include "convert_godot_lua.hpp" | ||
| #include "godot_cpp/variant/callable.hpp" |
There was a problem hiding this comment.
Includes for 3rd party headers live in the block below and use angled brackets #include <godot_cpp/...>. In any case, I don't think you need this include here, Godot's Variant types are reachable by the other headers already included. But feel free to keep it, since it's used directly, just put it in the other block.
There was a problem hiding this comment.
My bad, it's all auto-incudes from clangd. It's probably a good idea to add .clang-format for auto sorting and correct brackets, I can add it if you want
| #include <godot_cpp/classes/resource.hpp> | ||
| #include <godot_cpp/classes/script.hpp> | ||
| #include <godot_cpp/variant/utility_functions.hpp> | ||
| #include "LuaCallable.hpp" |
There was a problem hiding this comment.
Includes for Lua GDExtension's own headers live in the block above and are sorted alphabetically.
| return dictionary; | ||
| } | ||
| } else if (first_arg_type == sol::type::function) { | ||
| return LuaCallable::construct(args.get<sol::function>()); |
There was a problem hiding this comment.
Use sol::protected_function instead of sol::function to match the rest of the code.
| #include "sol/forward.hpp" | ||
| #include "sol/types.hpp" |
There was a problem hiding this comment.
You also don't really need these, VariantType.hpp already includes sol.hpp. It's ok to leave them here, but use angled brackets<> for 3rd party code instead of double quotes "" at least.
| virtual CompareLessFunc get_compare_less_func() const override; | ||
| uint32_t hash() const override; | ||
| void call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, GDExtensionCallError &r_call_error) const override; | ||
| static void register_lua(lua_State *L); |
There was a problem hiding this comment.
Why does it have this register_lua method? I don't really see this being called or even implemented in LuaCallable.cpp. Seems like it should be removed.
There was a problem hiding this comment.
Yeah, you're right, it's leftovers from when I tested, I'll remove it
| void call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, GDExtensionCallError &r_call_error) const override; | ||
| static void register_lua(lua_State *L); | ||
| int get_argument_count(bool &r_is_valid) const override; | ||
| static Variant construct(sol::function func); |
There was a problem hiding this comment.
Also please use sol::protected_function here instead of sol::function.
| int LuaCallable::get_argument_count(bool &r_is_valid) const { | ||
| r_is_valid = true; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
I don't think you need to override get_argument_count. CallableCustomBase::get_argument_count already does pretty much the same thing, although setting r_is_valid to false, which I think is the correct thing to do in this case (this argument count of 0 isn't really valid, the function may have more than 0 parameters, you could only know with a LuaDebug in Lua 5.2+).
|
Hi @illarn, thanks for the contribution! I only checked the code for now and added some comments. About these "known limitations", did you test if they only happen in Lua or if it happens in GDScript as well? I really don't think it's ok to have Callables that don't properly bind arguments or return values. Also, the freezing issue should most likely be fixed before we let people use this functionality. I'll take a look into it here and let you know if I find the cause in my tests. Just by reading the code everything seems fine. |
| local a = 1 | ||
| local custom_callable = Callable(function(val) | ||
| a = a + val | ||
| print(a == 6 and "Passed" or "Failed") | ||
| end) | ||
| custom_callable:call_deferred(5) |
There was a problem hiding this comment.
Automated tests should assert instead of only printing, the test runner only understands thrown errors and do not look into printed text.
There was a problem hiding this comment.
Yeah, this test is not very good, I'll change it. I completely missed the Makefile, so I've run tests wrong, I'll do it properly now
There was a problem hiding this comment.
There's no documentation of the test setup at all, so don't worry! Maybe I should have told you here as well, sorry about that. Just run make test and there you go (or run the command line from Makefile if you don't have make installed) 😄
|
I tested calling the LuaCallable here without |
|
Also tested bind and returning values and it's all working fine: local bound_callable = custom_callable:bind(5)
local result = bound_callable()
print(result) |
|
Thank you for the feedback, I'll go fix it now
Yeah, for me it also started working, I was sleepy and probably messed something up when testing it initially |
|
I've pushed fixes, fixed everything from the comments, fixed the test and added a few more cases. I've also added callable from function to the README. Calling via () doesn't freeze, returning and binding arguments works, so I think everything should be ready now |
|
I've added .clang-format, tried to make it as non-obstructive as possible, so it follows existing code style almost 1/1 |
|
Hey @illarn, thanks for the updates. Everything looks fine now, I'll just let the CI do its thing before merging. Just wanted to say that the .clang-format is one of those little organization things that all projects should have, and yet I never have setup in my projects 😅 Thanks for bringing that in! |
fixed, forgot to commit this and lost in autostash when switching branches, lol (sorry for the wasted CI run) |
|
Nice, now everything should work fine. Again, I'll just wait for the CI to do its thing and if everything is fine, merge afterwards. Thanks again for the contribution! |
Goal
Make it possible to create callables for godot's signals from lua
Changes
Known limitations
Binding arguments does nothingNo return valuesThese features do not contribute to this PR goal and are redundant since you can just use lua functions if you need thisTODO
Right now explicitly calling:call()on a LuaCallable freezes the game. While it's not intended to be used this way adding some error and/or a guard for this sounds reasonable. Or maybe a comment in the lua definition will be enough, not really sure