-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Fix node duplication in update after external changes. #89992
Fix node duplication in update after external changes. #89992
Conversation
Here: diff --git a/scene/main/node.compat.inc b/scene/main/node.compat.inc
new file mode 100644
index 0000000000..f88b6f1d59
--- /dev/null
+++ b/scene/main/node.compat.inc
@@ -0,0 +1,41 @@
+/**************************************************************************/
+/* node.compat.inc */
+/**************************************************************************/
+/* This file is part of: */
+/* GODOT ENGINE */
+/* https://godotengine.org */
+/**************************************************************************/
+/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */
+/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */
+/* */
+/* Permission is hereby granted, free of charge, to any person obtaining */
+/* a copy of this software and associated documentation files (the */
+/* "Software"), to deal in the Software without restriction, including */
+/* without limitation the rights to use, copy, modify, merge, publish, */
+/* distribute, sublicense, and/or sell copies of the Software, and to */
+/* permit persons to whom the Software is furnished to do so, subject to */
+/* the following conditions: */
+/* */
+/* The above copyright notice and this permission notice shall be */
+/* included in all copies or substantial portions of the Software. */
+/* */
+/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */
+/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */
+/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */
+/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */
+/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */
+/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
+/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
+/**************************************************************************/
+
+#ifndef DISABLE_DEPRECATED
+
+void Node::_replace_by_bind_compat_89992(Node *p_node, bool p_keep_data = false) {
+ replace_by(p_node, p_keep_data, true);
+}
+
+void Node::_bind_compatibility_methods() {
+ ClassDB::bind_compatibility_method(D_METHOD("replace_by", "node", "keep_groups"), &Node::_replace_by_bind_compat_89992, DEFVAL(false));
+}
+
+#endif
diff --git a/scene/main/node.cpp b/scene/main/node.cpp
index 0e09b3b2b9..a1adba9a51 100644
--- a/scene/main/node.cpp
+++ b/scene/main/node.cpp
@@ -29,6 +29,7 @@
/**************************************************************************/
#include "node.h"
+#include "node.compat.inc"
#include "core/config/project_settings.h"
#include "core/core_string_names.h"
diff --git a/scene/main/node.h b/scene/main/node.h
index 455f726349..bf43c0d64a 100644
--- a/scene/main/node.h
+++ b/scene/main/node.h
@@ -309,6 +309,11 @@ private:
Variant _call_deferred_thread_group_bind(const Variant **p_args, int p_argcount, Callable::CallError &r_error);
Variant _call_thread_safe_bind(const Variant **p_args, int p_argcount, Callable::CallError &r_error);
+#ifndef DISABLE_DEPRECATED
+ void _replace_by_bind_compat_89992(Node *p_node, bool p_keep_data = false);
+ static void _bind_compatibility_methods();
+#endif // DISABLE_DEPRECATED
+
protected:
void _block() { data.blocked++; }
void _unblock() { data.blocked--; } And of course add it to the |
Edit: Tested and this PR does seem to fix the issue. I want to reiterate my concerns about replace_by, since from a previous debugging session, I found that it does not properly handle internal nodes, possibly leading to off-by-n errors or other misbehavior if any such nodes are present (such as editor gizmos or other things).
It would be good to verify that replace_by is handling internal nodes correctly. Seeing calls such as |
3aeebc9
to
885c215
Compare
885c215
to
7d78ab6
Compare
4aefc05
to
8f9c781
Compare
This comment was marked as resolved.
This comment was marked as resolved.
8f9c781
to
ae47286
Compare
Thanks! |
Fixes #89877
The fix ws implemented aaccording to this idea : #89877 (comment)
One thing about the implementation which is not related to the fix itself. replace_by with the new argument set to true will not copy children but will reconnect any connections. Unfortunately this means also connections to child that were not reparented. In case you just delete the old child (like it is done here for the fix) this is no problem since nodes are deleted and connections are then automatically removed. I feel like just warning about it in the documentation should be enough but I could also try to remove connection to children of the replaced node.