-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to bind an unbind arguments to Callable. #42683
Conversation
} | ||
Callable Callable::unbind(int p_argcount) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
Callable Callable::unbind(int p_argcount) const { | |
} | |
Callable Callable::unbind(int p_argcount) const { |
@@ -0,0 +1,163 @@ | |||
#include "callable_bind.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And review spacing between functions (yes, I'll go pester clang-format devs about it again ;))
@@ -0,0 +1,55 @@ | |||
#ifndef CALLABLE_BIND_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header.
df6ef0d
to
351a122
Compare
@@ -1315,9 +1315,10 @@ Error Object::connect(const StringName &p_signal, const Callable &p_callable, co | |||
|
|||
Callable target = p_callable; | |||
|
|||
if (s->slot_map.has(target)) { | |||
//compare with the base callable, so binds can be ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to get used to this comment style that the team agreed upon:
//compare with the base callable, so binds can be ignored | |
// Compare with the base callable, so binds can be ignored. |
We can't rely on formatting tools for that, we have to be consistent in the code we write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this comment style is everywhere in the codebase, even in that file. I honestly just forget. We should try to ask the clang format guys to add support for this and pester them about it.
@@ -1141,7 +1141,7 @@ void SceneTreeDock::_notification(int p_what) { | |||
node_shortcuts->add_child(button_custom); | |||
button_custom->set_text(TTR("Other Node")); | |||
button_custom->set_icon(get_theme_icon("Add", "EditorIcons")); | |||
button_custom->connect("pressed", callable_mp(this, &SceneTreeDock::_tool_selected), make_binds(TOOL_NEW, false)); | |||
button_custom->connect("pressed", callable_bind(callable_mp(this, &SceneTreeDock::_tool_selected), TOOL_NEW, false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth it (and possible) to add some sugar so we can define a Callable
with binds directly? E.g. callable_mp_bind(this, &SceneTreeDock::_tool_selected, TOOL_NEW, false)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a bit difficult because callable_mp is a macro, I wonder if vararg macros are finally supported in C++17..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe someone with more C++17 fu can help me figure it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreaCatania's fork has an ecs
branch which has some examples of variadic templates, iirc. Example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already implememted them in a couple of places, but it won't help here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also callable_bind is ok, because there are other places where regular callables (not mp) are used, so a generic function needs to exist, so given this is not used super often, it may be ok to leave as is.
Would be useful if the PR included also documentation. It's just two methods. |
@KoBeWi can try after the next PR, since I will have to change the doc system due to the changes to builtin methods. |
I usually use the following command when there's no documentation written. 🙂
Commits can then be followed by URLs specified by commits, leading to PRs with actual descriptions. |
Where is the documentation for this feature? |
Could |
The old way of binding arguments to a callable via array is still shown in the documentation, on the gdscript basics page to be specific. CTRL+F "battlelog" and you'll see the old example. |
After the today's contributor meeting we decided to change the way arguments are bound and unbound to signals and other structures by doing this directly in Callable.
As such, its now possible to bind and unbind arguments directly from the Callable:
from C++, the callable_bind() functions were added, can be used like this:
The ability to pass an array in the connect() version that takes a callable, in both C++ and script languages will be removed, so these will have to be used from now on in Godot 4.0 (the connect method for Object will remain unchanged in case flexibility is desired)). This change will be done project wide in a subsequent pull request.