From 051ef479c93c0c830b60059e3dabed6fc381cdd6 Mon Sep 17 00:00:00 2001
From: TechnoPorg <jonah.janzen@gmail.com>
Date: Tue, 25 Jan 2022 08:37:41 -0700
Subject: [PATCH] Allow method binds to take Object subclasses as arguments

This commit adds a condition to VariantCaster that casts Variants of type OBJECT to any type T, if T is derived from Object.
This change enables a fair bit of code cleanup. First, the Variant implicit cast operators for Node and Control can be removed, which allows for some invalid includes to be removed. Second, helper methods in Tree whose sole purpose was to cast arguments to TreeItem * are no longer necessary.
A few small changes also had to be made to other files, due to the changes cascading down all the includes.
---
 core/io/resource.h                            |  2 +
 core/variant/binder_common.h                  | 50 +++++++++++--------
 core/variant/variant.cpp                      | 20 +-------
 core/variant/variant.h                        |  4 --
 doc/classes/Tree.xml                          |  8 +--
 editor/editor_properties.cpp                  |  2 +-
 editor/plugins/canvas_item_editor_plugin.cpp  |  2 +-
 editor/plugins/node_3d_editor_plugin.cpp      |  4 +-
 editor/plugins/script_editor_plugin.cpp       |  4 +-
 editor/rename_dialog.cpp                      |  2 +-
 .../language_server/gdscript_workspace.cpp    |  2 +-
 scene/debugger/scene_debugger.h               |  1 +
 scene/gui/tree.cpp                            |  8 +--
 scene/gui/tree.h                              | 17 -------
 tests/core/object/test_method_bind.h          | 16 ++++++
 15 files changed, 64 insertions(+), 78 deletions(-)

diff --git a/core/io/resource.h b/core/io/resource.h
index a0800c57cba..dea21606161 100644
--- a/core/io/resource.h
+++ b/core/io/resource.h
@@ -37,6 +37,8 @@
 #include "core/templates/safe_refcount.h"
 #include "core/templates/self_list.h"
 
+class Node;
+
 #define RES_BASE_EXTENSION(m_ext)                                                                                   \
 public:                                                                                                             \
 	static void register_custom_data_to_otdb() { ClassDB::add_resource_base_extension(m_ext, get_class_static()); } \
diff --git a/core/variant/binder_common.h b/core/variant/binder_common.h
index 14f49d530cf..b6fdb4d9020 100644
--- a/core/variant/binder_common.h
+++ b/core/variant/binder_common.h
@@ -44,24 +44,42 @@
 
 #include <stdio.h>
 
+// Variant cannot define an implicit cast operator for every Object subclass, so the
+// casting is done here, to allow binding methods with parameters more specific than Object *
+
 template <class T>
 struct VariantCaster {
 	static _FORCE_INLINE_ T cast(const Variant &p_variant) {
-		return p_variant;
+		using TStripped = std::remove_pointer_t<T>;
+		if constexpr (std::is_base_of<Object, TStripped>::value) {
+			return Object::cast_to<TStripped>(p_variant);
+		} else {
+			return p_variant;
+		}
 	}
 };
 
 template <class T>
 struct VariantCaster<T &> {
 	static _FORCE_INLINE_ T cast(const Variant &p_variant) {
-		return p_variant;
+		using TStripped = std::remove_pointer_t<T>;
+		if constexpr (std::is_base_of<Object, TStripped>::value) {
+			return Object::cast_to<TStripped>(p_variant);
+		} else {
+			return p_variant;
+		}
 	}
 };
 
 template <class T>
 struct VariantCaster<const T &> {
 	static _FORCE_INLINE_ T cast(const Variant &p_variant) {
-		return p_variant;
+		using TStripped = std::remove_pointer_t<T>;
+		if constexpr (std::is_base_of<Object, TStripped>::value) {
+			return Object::cast_to<TStripped>(p_variant);
+		} else {
+			return p_variant;
+		}
 	}
 };
 
@@ -135,7 +153,13 @@ struct PtrToArg<char32_t> {
 template <typename T>
 struct VariantObjectClassChecker {
 	static _FORCE_INLINE_ bool check(const Variant &p_variant) {
-		return true;
+		using TStripped = std::remove_pointer_t<T>;
+		if constexpr (std::is_base_of<Object, TStripped>::value) {
+			Object *obj = p_variant;
+			return Object::cast_to<TStripped>(p_variant) || !obj;
+		} else {
+			return true;
+		}
 	}
 };
 
@@ -151,24 +175,6 @@ struct VariantObjectClassChecker<const Ref<T> &> {
 	}
 };
 
-template <>
-struct VariantObjectClassChecker<Node *> {
-	static _FORCE_INLINE_ bool check(const Variant &p_variant) {
-		Object *obj = p_variant;
-		Node *node = p_variant;
-		return node || !obj;
-	}
-};
-
-template <>
-struct VariantObjectClassChecker<Control *> {
-	static _FORCE_INLINE_ bool check(const Variant &p_variant) {
-		Object *obj = p_variant;
-		Control *control = p_variant;
-		return control || !obj;
-	}
-};
-
 #ifdef DEBUG_METHODS_ENABLED
 
 template <class T>
diff --git a/core/variant/variant.cpp b/core/variant/variant.cpp
index db198da54a6..b85d7641f04 100644
--- a/core/variant/variant.cpp
+++ b/core/variant/variant.cpp
@@ -38,8 +38,6 @@
 #include "core/math/math_funcs.h"
 #include "core/string/print_string.h"
 #include "core/variant/variant_parser.h"
-#include "scene/gui/control.h"
-#include "scene/main/node.h"
 
 String Variant::get_type_name(Variant::Type p_type) {
 	switch (p_type) {
@@ -2006,22 +2004,6 @@ Object *Variant::get_validated_object() const {
 	}
 }
 
-Variant::operator Node *() const {
-	if (type == OBJECT) {
-		return Object::cast_to<Node>(_get_obj().obj);
-	} else {
-		return nullptr;
-	}
-}
-
-Variant::operator Control *() const {
-	if (type == OBJECT) {
-		return Object::cast_to<Control>(_get_obj().obj);
-	} else {
-		return nullptr;
-	}
-}
-
 Variant::operator Dictionary() const {
 	if (type == DICTIONARY) {
 		return *reinterpret_cast<const Dictionary *>(_data._mem);
@@ -3416,7 +3398,7 @@ String Variant::get_call_error_text(Object *p_base, const StringName &p_method,
 	}
 
 	String class_name = p_base->get_class();
-	Ref<Script> script = p_base->get_script();
+	Ref<Resource> script = p_base->get_script();
 	if (script.is_valid() && script->get_path().is_resource_file()) {
 		class_name += "(" + script->get_path().get_file() + ")";
 	}
diff --git a/core/variant/variant.h b/core/variant/variant.h
index 0860e7fdc37..ad7a3d0ef37 100644
--- a/core/variant/variant.h
+++ b/core/variant/variant.h
@@ -53,8 +53,6 @@
 #include "core/variant/dictionary.h"
 
 class Object;
-class Node; // helper
-class Control; // helper
 
 struct PropertyInfo;
 struct MethodInfo;
@@ -339,8 +337,6 @@ public:
 	operator ::RID() const;
 
 	operator Object *() const;
-	operator Node *() const;
-	operator Control *() const;
 
 	operator Callable() const;
 	operator Signal() const;
diff --git a/doc/classes/Tree.xml b/doc/classes/Tree.xml
index 10bbdc03018..2eda8990ca1 100644
--- a/doc/classes/Tree.xml
+++ b/doc/classes/Tree.xml
@@ -50,7 +50,7 @@
 		</method>
 		<method name="create_item">
 			<return type="TreeItem" />
-			<argument index="0" name="parent" type="Object" default="null" />
+			<argument index="0" name="parent" type="TreeItem" default="null" />
 			<argument index="1" name="idx" type="int" default="-1" />
 			<description>
 				Creates an item in the tree and adds it as a child of [code]parent[/code].
@@ -170,7 +170,7 @@
 		</method>
 		<method name="get_item_area_rect" qualifiers="const">
 			<return type="Rect2" />
-			<argument index="0" name="item" type="Object" />
+			<argument index="0" name="item" type="TreeItem" />
 			<argument index="1" name="column" type="int" default="-1" />
 			<description>
 				Returns the rectangle area for the specified item. If [code]column[/code] is specified, only get the position and size of that column, otherwise get the rectangle containing all columns.
@@ -185,7 +185,7 @@
 		</method>
 		<method name="get_next_selected">
 			<return type="TreeItem" />
-			<argument index="0" name="from" type="Object" />
+			<argument index="0" name="from" type="TreeItem" />
 			<description>
 				Returns the next selected item after the given one, or [code]null[/code] if the end is reached.
 				If [code]from[/code] is [code]null[/code], this returns the first selected item.
@@ -239,7 +239,7 @@
 		</method>
 		<method name="scroll_to_item">
 			<return type="void" />
-			<argument index="0" name="item" type="Object" />
+			<argument index="0" name="item" type="TreeItem" />
 			<description>
 				Causes the [Tree] to jump to the specified item.
 			</description>
diff --git a/editor/editor_properties.cpp b/editor/editor_properties.cpp
index 97a38b92004..e00dd698b66 100644
--- a/editor/editor_properties.cpp
+++ b/editor/editor_properties.cpp
@@ -2695,7 +2695,7 @@ void EditorPropertyNodePath::_node_selected(const NodePath &p_path) {
 	}
 
 	if (!base_node && get_edited_object()->has_method("get_root_path")) {
-		base_node = get_edited_object()->call("get_root_path");
+		base_node = Object::cast_to<Node>(get_edited_object()->call("get_root_path"));
 	}
 
 	if (!base_node && Object::cast_to<RefCounted>(get_edited_object())) {
diff --git a/editor/plugins/canvas_item_editor_plugin.cpp b/editor/plugins/canvas_item_editor_plugin.cpp
index cb84e7ea652..f38426fbfee 100644
--- a/editor/plugins/canvas_item_editor_plugin.cpp
+++ b/editor/plugins/canvas_item_editor_plugin.cpp
@@ -3977,7 +3977,7 @@ void CanvasItemEditor::_selection_changed() {
 
 void CanvasItemEditor::edit(CanvasItem *p_canvas_item) {
 	Array selection = editor_selection->get_selected_nodes();
-	if (selection.size() != 1 || (Node *)selection[0] != p_canvas_item) {
+	if (selection.size() != 1 || Object::cast_to<Node>(selection[0]) != p_canvas_item) {
 		drag_type = DRAG_NONE;
 
 		// Clear the selection
diff --git a/editor/plugins/node_3d_editor_plugin.cpp b/editor/plugins/node_3d_editor_plugin.cpp
index 6ea8fba9b5c..6073c8669db 100644
--- a/editor/plugins/node_3d_editor_plugin.cpp
+++ b/editor/plugins/node_3d_editor_plugin.cpp
@@ -6623,7 +6623,7 @@ void Node3DEditor::snap_selected_nodes_to_floor() {
 		// For snapping to be performed, there must be solid geometry under at least one of the selected nodes.
 		// We need to check this before snapping to register the undo/redo action only if needed.
 		for (int i = 0; i < keys.size(); i++) {
-			Node *node = keys[i];
+			Node *node = Object::cast_to<Node>(keys[i]);
 			Node3D *sp = Object::cast_to<Node3D>(node);
 			Dictionary d = snap_data[node];
 			Vector3 from = d["from"];
@@ -6645,7 +6645,7 @@ void Node3DEditor::snap_selected_nodes_to_floor() {
 
 			// Perform snapping if at least one node can be snapped
 			for (int i = 0; i < keys.size(); i++) {
-				Node *node = keys[i];
+				Node *node = Object::cast_to<Node>(keys[i]);
 				Node3D *sp = Object::cast_to<Node3D>(node);
 				Dictionary d = snap_data[node];
 				Vector3 from = d["from"];
diff --git a/editor/plugins/script_editor_plugin.cpp b/editor/plugins/script_editor_plugin.cpp
index 03ed0e0ef21..56ea072b512 100644
--- a/editor/plugins/script_editor_plugin.cpp
+++ b/editor/plugins/script_editor_plugin.cpp
@@ -2802,7 +2802,7 @@ bool ScriptEditor::can_drop_data_fw(const Point2 &p_point, const Variant &p_data
 	}
 
 	if (String(d["type"]) == "script_list_element") {
-		Node *node = d["script_list_element"];
+		Node *node = Object::cast_to<Node>(d["script_list_element"]);
 
 		ScriptEditorBase *se = Object::cast_to<ScriptEditorBase>(node);
 		if (se) {
@@ -2875,7 +2875,7 @@ void ScriptEditor::drop_data_fw(const Point2 &p_point, const Variant &p_data, Co
 	}
 
 	if (String(d["type"]) == "script_list_element") {
-		Node *node = d["script_list_element"];
+		Node *node = Object::cast_to<Node>(d["script_list_element"]);
 
 		ScriptEditorBase *se = Object::cast_to<ScriptEditorBase>(node);
 		EditorHelp *eh = Object::cast_to<EditorHelp>(node);
diff --git a/editor/rename_dialog.cpp b/editor/rename_dialog.cpp
index 0e34d200f26..288fff7d411 100644
--- a/editor/rename_dialog.cpp
+++ b/editor/rename_dialog.cpp
@@ -362,7 +362,7 @@ void RenameDialog::_post_popup() {
 	Array selected_node_list = editor_selection->get_selected_nodes();
 	ERR_FAIL_COND(selected_node_list.size() == 0);
 
-	preview_node = selected_node_list[0];
+	preview_node = Object::cast_to<Node>(selected_node_list[0]);
 
 	_update_preview();
 	_update_substitute();
diff --git a/modules/gdscript/language_server/gdscript_workspace.cpp b/modules/gdscript/language_server/gdscript_workspace.cpp
index a944844226b..a627eca45de 100644
--- a/modules/gdscript/language_server/gdscript_workspace.cpp
+++ b/modules/gdscript/language_server/gdscript_workspace.cpp
@@ -585,7 +585,7 @@ void GDScriptWorkspace::completion(const lsp::CompletionParams &p_params, List<S
 			stack.push_back(owner_scene_node);
 
 			while (!stack.is_empty()) {
-				current = stack.pop_back();
+				current = Object::cast_to<Node>(stack.pop_back());
 				Ref<GDScript> script = current->get_script();
 				if (script.is_valid() && script->get_path() == path) {
 					break;
diff --git a/scene/debugger/scene_debugger.h b/scene/debugger/scene_debugger.h
index 432317a4238..c8298391bb5 100644
--- a/scene/debugger/scene_debugger.h
+++ b/scene/debugger/scene_debugger.h
@@ -37,6 +37,7 @@
 #include "core/variant/array.h"
 
 class Script;
+class Node;
 
 class SceneDebugger {
 public:
diff --git a/scene/gui/tree.cpp b/scene/gui/tree.cpp
index e46de43f1e6..4f209db0f97 100644
--- a/scene/gui/tree.cpp
+++ b/scene/gui/tree.cpp
@@ -4752,7 +4752,7 @@ bool Tree::get_allow_reselect() const {
 
 void Tree::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("clear"), &Tree::clear);
-	ClassDB::bind_method(D_METHOD("create_item", "parent", "idx"), &Tree::_create_item, DEFVAL(Variant()), DEFVAL(-1));
+	ClassDB::bind_method(D_METHOD("create_item", "parent", "idx"), &Tree::create_item, DEFVAL(Variant()), DEFVAL(-1));
 
 	ClassDB::bind_method(D_METHOD("get_root"), &Tree::get_root);
 	ClassDB::bind_method(D_METHOD("set_column_custom_minimum_width", "column", "min_width"), &Tree::set_column_custom_minimum_width);
@@ -4767,7 +4767,7 @@ void Tree::_bind_methods() {
 
 	ClassDB::bind_method(D_METHOD("set_hide_root", "enable"), &Tree::set_hide_root);
 	ClassDB::bind_method(D_METHOD("is_root_hidden"), &Tree::is_root_hidden);
-	ClassDB::bind_method(D_METHOD("get_next_selected", "from"), &Tree::_get_next_selected);
+	ClassDB::bind_method(D_METHOD("get_next_selected", "from"), &Tree::get_next_selected);
 	ClassDB::bind_method(D_METHOD("get_selected"), &Tree::get_selected);
 	ClassDB::bind_method(D_METHOD("get_selected_column"), &Tree::get_selected_column);
 	ClassDB::bind_method(D_METHOD("get_pressed_button"), &Tree::get_pressed_button);
@@ -4781,7 +4781,7 @@ void Tree::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("get_edited_column"), &Tree::get_edited_column);
 	ClassDB::bind_method(D_METHOD("edit_selected"), &Tree::edit_selected);
 	ClassDB::bind_method(D_METHOD("get_custom_popup_rect"), &Tree::get_custom_popup_rect);
-	ClassDB::bind_method(D_METHOD("get_item_area_rect", "item", "column"), &Tree::_get_item_rect, DEFVAL(-1));
+	ClassDB::bind_method(D_METHOD("get_item_area_rect", "item", "column"), &Tree::get_item_rect, DEFVAL(-1));
 	ClassDB::bind_method(D_METHOD("get_item_at_position", "position"), &Tree::get_item_at_position);
 	ClassDB::bind_method(D_METHOD("get_column_at_position", "position"), &Tree::get_column_at_position);
 	ClassDB::bind_method(D_METHOD("get_drop_section_at_position", "position"), &Tree::get_drop_section_at_position);
@@ -4805,7 +4805,7 @@ void Tree::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("get_column_title_language", "column"), &Tree::get_column_title_language);
 
 	ClassDB::bind_method(D_METHOD("get_scroll"), &Tree::get_scroll);
-	ClassDB::bind_method(D_METHOD("scroll_to_item", "item"), &Tree::_scroll_to_item);
+	ClassDB::bind_method(D_METHOD("scroll_to_item", "item"), &Tree::scroll_to_item);
 
 	ClassDB::bind_method(D_METHOD("set_h_scroll_enabled", "h_scroll"), &Tree::set_h_scroll_enabled);
 	ClassDB::bind_method(D_METHOD("is_h_scroll_enabled"), &Tree::is_h_scroll_enabled);
diff --git a/scene/gui/tree.h b/scene/gui/tree.h
index c60c87564e2..1865bb45959 100644
--- a/scene/gui/tree.h
+++ b/scene/gui/tree.h
@@ -609,23 +609,6 @@ private:
 protected:
 	static void _bind_methods();
 
-	//bind helpers
-	TreeItem *_create_item(Object *p_parent, int p_idx = -1) {
-		return create_item(Object::cast_to<TreeItem>(p_parent), p_idx);
-	}
-
-	TreeItem *_get_next_selected(Object *p_item) {
-		return get_next_selected(Object::cast_to<TreeItem>(p_item));
-	}
-
-	Rect2 _get_item_rect(Object *p_item, int p_column) const {
-		return get_item_rect(Object::cast_to<TreeItem>(p_item), p_column);
-	}
-
-	void _scroll_to_item(Object *p_item) {
-		scroll_to_item(Object::cast_to<TreeItem>(p_item));
-	}
-
 public:
 	virtual void gui_input(const Ref<InputEvent> &p_event) override;
 
diff --git a/tests/core/object/test_method_bind.h b/tests/core/object/test_method_bind.h
index 0c7e47fc893..350a08b6e2c 100644
--- a/tests/core/object/test_method_bind.h
+++ b/tests/core/object/test_method_bind.h
@@ -51,9 +51,15 @@ public:
 		TEST_METHODRC,
 		TEST_METHODRC_ARGS,
 		TEST_METHOD_DEFARGS,
+		TEST_METHOD_OBJECT_CAST,
 		TEST_MAX
 	};
 
+	class ObjectSubclass : public Object {
+	public:
+		int value = 1;
+	};
+
 	int test_num = 0;
 
 	bool test_valid[TEST_MAX];
@@ -98,6 +104,10 @@ public:
 		test_valid[TEST_METHOD_DEFARGS] = p_arg1 == 1 && p_arg2 == 2 && p_arg3 == 3 && p_arg4 == 4 && p_arg5 == 5; //temporary
 	}
 
+	void test_method_object_cast(ObjectSubclass *p_object) {
+		test_valid[TEST_METHOD_OBJECT_CAST] = p_object->value == 1;
+	}
+
 	static void _bind_methods() {
 		ClassDB::bind_method(D_METHOD("test_method"), &MethodBindTester::test_method);
 		ClassDB::bind_method(D_METHOD("test_method_args"), &MethodBindTester::test_method_args);
@@ -108,6 +118,7 @@ public:
 		ClassDB::bind_method(D_METHOD("test_methodrc"), &MethodBindTester::test_methodrc);
 		ClassDB::bind_method(D_METHOD("test_methodrc_args"), &MethodBindTester::test_methodrc_args);
 		ClassDB::bind_method(D_METHOD("test_method_default_args"), &MethodBindTester::test_method_default_args, DEFVAL(9) /* wrong on purpose */, DEFVAL(4), DEFVAL(5));
+		ClassDB::bind_method(D_METHOD("test_method_object_cast", "object"), &MethodBindTester::test_method_object_cast);
 	}
 
 	virtual void run_tests() {
@@ -134,6 +145,10 @@ public:
 		test_valid[TEST_METHODRC_ARGS] = int(call("test_methodrc_args", test_num)) == test_num && test_valid[TEST_METHODRC_ARGS];
 
 		call("test_method_default_args", 1, 2, 3, 4);
+
+		ObjectSubclass *obj = memnew(ObjectSubclass);
+		call("test_method_object_cast", obj);
+		memdelete(obj);
 	}
 };
 
@@ -152,6 +167,7 @@ TEST_CASE("[MethodBind] check all method binds") {
 	CHECK(mbt->test_valid[MethodBindTester::TEST_METHODRC]);
 	CHECK(mbt->test_valid[MethodBindTester::TEST_METHODRC_ARGS]);
 	CHECK(mbt->test_valid[MethodBindTester::TEST_METHOD_DEFARGS]);
+	CHECK(mbt->test_valid[MethodBindTester::TEST_METHOD_OBJECT_CAST]);
 
 	memdelete(mbt);
 }