Signals: Port connect calls to use callable_mp #36426
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akien-mga
force-pushed
the
calling-all-stations
branch
from
3230ac5
to
41f3b8b
Compare
|
Fixed my script which missed a lot of bindings to remove. There might be false positives in the lot though. |
akien-mga
changed the title
akien-mga
force-pushed
the
calling-all-stations
branch
2 times, most recently
from
5968edd
to
385ac1d
Compare
|
I ported most uses of For example:
So if we want to convert those, we need to either make Another quirk of the new system is shown with e.g.: To call a public method from a parent class, we're forced to cast the object. Finally, while everything compiles fine, opening a new project triggers a segfault: Over to you @reduz :) You can checkout this branch locally with: |
|
The next time, if you want someone to do repeated work let me know. I'm your human script haha. I want to help. |
@akien-mga I analyzed this. Comparing the behavior before and after the new signal system. I think this is a side effect of the new signal system. This particular case is because this is using 'search_box' before it is instantiated. Here is the code in: And now, with the new signal system, we have the binding immediately when it connect to the method unlike before it was binding when it execute "_bind_methods" (what is called in "initialize_class()" by the GDCLASS define). So my proposals for this are:
|
akien-mga
force-pushed
the
calling-all-stations
branch
2 times, most recently
from
b17c70b
to
590b1a4
Compare
akien-mga
force-pushed
the
calling-all-stations
branch
from
b204c15
to
0aa2b62
Compare
|
Thanks to @kuruk-mm this is no longer crashing :) There are still issues to find and fix, but it can get some more broader testing if anyone is interested. |
I'm on it. I'm clicking every button in Godot 😃 |
akien-mga
force-pushed
the
calling-all-stations
branch
2 times, most recently
from
f8db1c5
to
c777daa
Compare
akien-mga
force-pushed
the
calling-all-stations
branch
from
c777daa
to
dcb6c7d
Compare
|
I reviewed the list of bindings removed in the first commit that don't start with a Also fixed start errors which pointed to other wrongly removed bindings: |
akien-mga
force-pushed
the
calling-all-stations
branch
from
dcb6c7d
to
ad9e23b
Compare
Using Commenting those out then led me to find more errors when adding nodes to the scene tree. The diff below seems to silence all of them, though it obviously breaks the editor. diff --git a/core/callable.cpp b/core/callable.cpp
index 34b79cea10..a25830bf17 100644
--- a/core/callable.cpp
+++ b/core/callable.cpp
@@ -73,7 +73,8 @@ ObjectID Callable::get_object_id() const {
}
}
StringName Callable::get_method() const {
- ERR_FAIL_COND_V(is_custom(), StringName());
+ ERR_FAIL_COND_V_MSG(is_custom(), StringName(),
+ vformat("Can't get method on CallableCustom \"%s\".", operator String()));
return method;
}
uint32_t Callable::hash() const {
diff --git a/editor/editor_data.cpp b/editor/editor_data.cpp
index 26d132665c..01a4cca5ed 100644
--- a/editor/editor_data.cpp
+++ b/editor/editor_data.cpp
@@ -1024,7 +1024,9 @@ void EditorSelection::add_node(Node *p_node) {
}
selection[p_node] = meta;
+ /*
p_node->connect("tree_exiting", callable_mp(this, &EditorSelection::_node_removed), varray(p_node), CONNECT_ONESHOT);
+ */
//emit_signal("selection_changed");
}
@@ -1042,7 +1044,9 @@ void EditorSelection::remove_node(Node *p_node) {
if (meta)
memdelete(meta);
selection.erase(p_node);
+ /*
p_node->disconnect("tree_exiting", callable_mp(this, &EditorSelection::_node_removed));
+ */
//emit_signal("selection_changed");
}
bool EditorSelection::is_selected(Node *p_node) const {
diff --git a/editor/scene_tree_editor.cpp b/editor/scene_tree_editor.cpp
index e4e642e368..fb5d9cecd9 100644
--- a/editor/scene_tree_editor.cpp
+++ b/editor/scene_tree_editor.cpp
@@ -323,8 +323,10 @@ bool SceneTreeEditor::_add_nodes(Node *p_node, TreeItem *p_parent) {
if (can_open_instance && undo_redo) { //Show buttons only when necessary(SceneTreeDock) to avoid crashes
+ /*
if (!p_node->is_connected("script_changed", callable_mp(this, &SceneTreeEditor::_node_script_changed)))
p_node->connect("script_changed", callable_mp(this, &SceneTreeEditor::_node_script_changed), varray(p_node));
+ */
Ref<Script> script = p_node->get_script();
if (!script.is_null()) {
@@ -350,8 +352,10 @@ bool SceneTreeEditor::_add_nodes(Node *p_node, TreeItem *p_parent) {
else
item->add_button(0, get_icon("GuiVisibilityHidden", "EditorIcons"), BUTTON_VISIBILITY, false, TTR("Toggle Visibility"));
+ /*
if (!p_node->is_connected("visibility_changed", callable_mp(this, &SceneTreeEditor::_node_visibility_changed)))
p_node->connect("visibility_changed", callable_mp(this, &SceneTreeEditor::_node_visibility_changed), varray(p_node));
+ */
_update_visibility_color(p_node, item);
} else if (p_node->is_class("Spatial")) {
@@ -370,8 +374,10 @@ bool SceneTreeEditor::_add_nodes(Node *p_node, TreeItem *p_parent) {
else
item->add_button(0, get_icon("GuiVisibilityHidden", "EditorIcons"), BUTTON_VISIBILITY, false, TTR("Toggle Visibility"));
+ /*
if (!p_node->is_connected("visibility_changed", callable_mp(this, &SceneTreeEditor::_node_visibility_changed)))
p_node->connect("visibility_changed", callable_mp(this, &SceneTreeEditor::_node_visibility_changed), varray(p_node));
+ */
_update_visibility_color(p_node, item);
} else if (p_node->is_class("AnimationPlayer")) {
diff --git a/scene/gui/control.cpp b/scene/gui/control.cpp
index 1a231e368b..de6737ec4f 100644
--- a/scene/gui/control.cpp
+++ b/scene/gui/control.cpp
@@ -542,7 +542,9 @@ void Control::_notification(int p_notification) {
if (data.parent_canvas_item) {
+ /*
data.parent_canvas_item->connect("item_rect_changed", callable_mp(this, &Control::_size_changed));
+ */
} else {
//connect viewport
get_viewport()->connect("size_changed", callable_mp(this, &Control::_size_changed));
@@ -561,7 +563,9 @@ void Control::_notification(int p_notification) {
if (data.parent_canvas_item) {
+ /*
data.parent_canvas_item->disconnect("item_rect_changed", callable_mp(this, &Control::_size_changed));
+ */
data.parent_canvas_item = NULL;
} else if (!is_set_as_toplevel()) {
//disconnect viewport |
akien-mga
force-pushed
the
calling-all-stations
branch
from
ad9e23b
to
c64f210
Compare
Remove now unnecessary bindings of signal callbacks in the public API. There might be some false positives that need rebinding if they were meant to be public. No regular expressions were harmed in the making of this commit. (Nah, just kidding.)
It's tedious work... Some can't be ported as they depend on private or protected methods of different classes, which is not supported by callable_mp (even if it's a class inherited by the current one).
Those were problematic as they call a method of their parent class, but callable_mp does not allow that unless it's public. To solve it, we declare a local class that calls the parent class' method, which now needs to be protected to be accessible in the derived class.
akien-mga
force-pushed
the
calling-all-stations
branch
from
c64f210
to
32ccf30
Compare
|
Alright with help from @reduz I could fix those errors: #36426 (comment) From some quick testing everything seems to behave OK. I'm pretty sure there might still be situations where a removed binding would still be needed by MessageQueue or UndoRedo, but we can assess those as errors pop up. |
akien-mga
changed the title
|
I made a simple 2D game and works fine. So, I think this is working fine |
|
Thanks a lot for the help @kuruk-mm! |
nathanfranke
reviewed
Mar 1, 2020
akien-mga
added a commit
to akien-mga/godot
that referenced
this pull request
Mar 3, 2020
- Fix `callable_mp` bindings to methods which used to have default arguments passed to `bind_method`. We now have to re-specify them manually when connecting. - Re-add `GroupsEditor::update_tree` binding. - Misc code quality changes along the way.
akien-mga
added a commit
that referenced
this pull request
Mar 3, 2020
Signals: Fix some regressions from #36426
giarve
added a commit
to giarve/godot
that referenced
this pull request
Mar 3, 2020
Same behavior as godotengine#36684. Removed by mistake in godotengine#36426. Fixes godotengine#36757.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #36393 (and fixes a couple issues missed before merging it).
Remove now unnecessary bindings of signal callbacks in the public API.
No regular expressions were harmed in the making of this commit.
(Nah, just kidding.)
This is the output of the wonderfully hacky bash script:
There are still a few cases which I now need to review manually (namely signals connected to something else than
this, and signals connected to a method bound in a parent class).