Merge pull request #25721 from neikeq/ww

Use script instance binding for objects constructed from C#

5 files changed, 70 insertions, 16 deletions

diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 5da1344595..9c127b837d 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -1113,19 +1113,23 @@ bool CSharpLanguage::setup_csharp_script_binding(CSharpScriptBinding &r_script_b
 
 void *CSharpLanguage::alloc_instance_binding_data(Object *p_object) {
 
+	SCOPED_MUTEX_LOCK(language_bind_mutex);
+
+	Map<Object *, CSharpScriptBinding>::Element *match = script_bindings.find(p_object);
+	if (match)
+		return (void *)match;
+
 	CSharpScriptBinding script_binding;
 
 	if (!setup_csharp_script_binding(script_binding, p_object))
 		return NULL;
 
-	void *data;
+	return (void *)insert_script_binding(p_object, script_binding);
+}
 
-	{
-		SCOPED_MUTEX_LOCK(language_bind_mutex);
-		data = (void *)script_bindings.insert(p_object, script_binding);
-	}
+Map<Object *, CSharpScriptBinding>::Element *CSharpLanguage::insert_script_binding(Object *p_object, const CSharpScriptBinding &p_script_binding) {
 
-	return data;
+	return script_bindings.insert(p_object, p_script_binding);
 }
 
 void CSharpLanguage::free_instance_binding_data(void *p_data) {
@@ -2279,17 +2283,18 @@ void CSharpScript::_bind_methods() {
 	ClassDB::bind_vararg_method(METHOD_FLAGS_DEFAULT, "new", &CSharpScript::_new, MethodInfo(Variant::OBJECT, "new"));
 }
 
-Ref<CSharpScript> CSharpScript::create_for_managed_type(GDMonoClass *p_class) {
+Ref<CSharpScript> CSharpScript::create_for_managed_type(GDMonoClass *p_class, GDMonoClass *p_native) {
 
 	// This method should not fail
 
 	CRASH_COND(!p_class);
 
+	// TODO: Cache the 'CSharpScript' associated with this 'p_class' instead of allocating a new one every time
 	Ref<CSharpScript> script = memnew(CSharpScript);
 
 	script->name = p_class->get_name();
 	script->script_class = p_class;
-	script->native = GDMonoUtils::get_class_native_base(script->script_class);
+	script->native = p_native;
 
 	CRASH_COND(script->native == NULL);
 
diff --git a/modules/mono/csharp_script.h b/modules/mono/csharp_script.h
index db9d6a73a2..8b1a4b5f7e 100644
--- a/modules/mono/csharp_script.h
+++ b/modules/mono/csharp_script.h
@@ -135,7 +135,7 @@ class CSharpScript : public Script {
 
 	// Do not use unless you know what you are doing
 	friend void GDMonoInternals::tie_managed_to_unmanaged(MonoObject *, Object *);
-	static Ref<CSharpScript> create_for_managed_type(GDMonoClass *p_class);
+	static Ref<CSharpScript> create_for_managed_type(GDMonoClass *p_class, GDMonoClass *p_native);
 
 protected:
 	static void _bind_methods();
@@ -312,6 +312,8 @@ class CSharpLanguage : public ScriptLanguage {
 public:
 	StringNameCache string_names;
 
+	Mutex *get_language_bind_mutex() { return language_bind_mutex; }
+
 	_FORCE_INLINE_ int get_language_index() { return lang_idx; }
 	void set_language_index(int p_idx);
 
@@ -406,6 +408,7 @@ public:
 	virtual void refcount_incremented_instance_binding(Object *p_object);
 	virtual bool refcount_decremented_instance_binding(Object *p_object);
 
+	Map<Object *, CSharpScriptBinding>::Element *insert_script_binding(Object *p_object, const CSharpScriptBinding &p_script_binding);
 	bool setup_csharp_script_binding(CSharpScriptBinding &r_script_binding, Object *p_object);
 
 #ifdef DEBUG_ENABLED
diff --git a/modules/mono/editor/bindings_generator.cpp b/modules/mono/editor/bindings_generator.cpp
index 0b6a6a327c..77e9b1f1f4 100644
--- a/modules/mono/editor/bindings_generator.cpp
+++ b/modules/mono/editor/bindings_generator.cpp
@@ -849,6 +849,8 @@ Error BindingsGenerator::_generate_cs_type(const TypeInterface &itype, const Str
 		}
 	}
 
+	// TODO: BINDINGS_NATIVE_NAME_FIELD should be StringName, once we support it in C#
+
 	if (itype.is_singleton) {
 		// Add the type name and the singleton pointer as static fields
 
diff --git a/modules/mono/mono_gd/gd_mono_internals.cpp b/modules/mono/mono_gd/gd_mono_internals.cpp
index 0daa394e63..14ec24466b 100644
--- a/modules/mono/mono_gd/gd_mono_internals.cpp
+++ b/modules/mono/mono_gd/gd_mono_internals.cpp
@@ -34,6 +34,8 @@
 #include "../mono_gc_handle.h"
 #include "../utils/macros.h"
 #include "../utils/thread_local.h"
+#include "gd_mono_class.h"
+#include "gd_mono_marshal.h"
 #include "gd_mono_utils.h"
 
 #include <mono/metadata/exception.h>
@@ -55,10 +57,49 @@ void tie_managed_to_unmanaged(MonoObject *managed, Object *unmanaged) {
 
 	CRASH_COND(!klass);
 
+	GDMonoClass *native = GDMonoUtils::get_class_native_base(klass);
+
+	CRASH_COND(native == NULL);
+
+	if (native == klass) {
+		// If it's just a wrapper Godot class and not a custom inheriting class, then attach a
+		// script binding instead. One of the advantages of this is that if a script is attached
+		// later and it's not a C# script, then the managed object won't have to be disposed.
+		// Another reason for doing this is that this instance could outlive CSharpLanguage, which would
+		// be problematic when using a script. See: https://github.com/godotengine/godot/issues/25621
+
+		CSharpScriptBinding script_binding;
+
+		script_binding.inited = true;
+		script_binding.type_name = NATIVE_GDMONOCLASS_NAME(klass);
+		script_binding.wrapper_class = klass;
+		script_binding.gchandle = MonoGCHandle::create_strong(managed);
+
+		Reference *ref = Object::cast_to<Reference>(unmanaged);
+		if (ref) {
+			// Unsafe refcount increment. The managed instance also counts as a reference.
+			// This way if the unmanaged world has no references to our owner
+			// but the managed instance is alive, the refcount will be 1 instead of 0.
+			// See: godot_icall_Reference_Dtor(MonoObject *p_obj, Object *p_ptr)
+
+			ref->reference();
+		}
+
+		// The object was just created, no script instance binding should have been attached
+		CRASH_COND(unmanaged->has_script_instance_binding(CSharpLanguage::get_singleton()->get_language_index()));
+
+		void *data = (void *)CSharpLanguage::get_singleton()->insert_script_binding(unmanaged, script_binding);
+
+		// Should be thread safe because the object was just created and nothing else should be referencing it
+		unmanaged->set_script_instance_binding(CSharpLanguage::get_singleton()->get_language_index(), data);
+
+		return;
+	}
+
 	Ref<MonoGCHandle> gchandle = ref ? MonoGCHandle::create_weak(managed) :
 									   MonoGCHandle::create_strong(managed);
 
-	Ref<CSharpScript> script = CSharpScript::create_for_managed_type(klass);
+	Ref<CSharpScript> script = CSharpScript::create_for_managed_type(klass, native);
 
 	CRASH_COND(script.is_null());
 
diff --git a/modules/mono/mono_gd/gd_mono_utils.cpp b/modules/mono/mono_gd/gd_mono_utils.cpp
index ac6ccac3a7..3b97339fea 100644
--- a/modules/mono/mono_gd/gd_mono_utils.cpp
+++ b/modules/mono/mono_gd/gd_mono_utils.cpp
@@ -39,6 +39,7 @@
 
 #include "../csharp_script.h"
 #include "../utils/macros.h"
+#include "../utils/mutex_utils.h"
 #include "gd_mono.h"
 #include "gd_mono_class.h"
 #include "gd_mono_marshal.h"
@@ -281,17 +282,19 @@ MonoObject *unmanaged_get_managed(Object *unmanaged) {
 
 	void *data = unmanaged->get_script_instance_binding(CSharpLanguage::get_singleton()->get_language_index());
 
-	if (!data)
-		return NULL;
+	ERR_FAIL_NULL_V(data, NULL);
 
 	CSharpScriptBinding &script_binding = ((Map<Object *, CSharpScriptBinding>::Element *)data)->value();
 
 	if (!script_binding.inited) {
-		// Already had a binding that needs to be setup
-		CSharpLanguage::get_singleton()->setup_csharp_script_binding(script_binding, unmanaged);
+		SCOPED_MUTEX_LOCK(CSharpLanguage::get_singleton()->get_language_bind_mutex());
+
+		if (!script_binding.inited) { // Other thread may have set it up
+			// Already had a binding that needs to be setup
+			CSharpLanguage::get_singleton()->setup_csharp_script_binding(script_binding, unmanaged);
 
-		if (!script_binding.inited)
-			return NULL;
+			ERR_FAIL_COND_V(!script_binding.inited, NULL);
+		}
 	}
 
 	Ref<MonoGCHandle> &gchandle = script_binding.gchandle;