Merge pull request #1223 from Yarwin/add-bound-callable

Callables linked to objects; let Godot auto-disconnect signals

Author: Bromeon

godot-core/src/builtin/callable.rs

@@ -152,6 +152,32 @@ impl Callable {
             rust_function,
             name,
             thread_id: Some(std::thread::current().id()),
+            linked_obj_id: None,
+        })
+    }
+
+    /// Creates a new callable linked to the given object from **single-threaded** Rust function or closure.
+    ///
+    /// `name` is used for the string representation of the closure, which helps with debugging.
+    ///
+    /// Such a callable will be automatically invalidated by Godot when a linked object is freed.
+    /// Prefer using [`Gd::linked_callable()`] instead.
+    ///
+    /// If you need a callable which can live indefinitely use [`Callable::from_local_fn()`].
+    #[cfg(since_api = "4.2")]
+    pub fn from_linked_fn<F, T, S>(name: S, linked_object: &Gd<T>, rust_function: F) -> Self
+    where
+        T: GodotClass,
+        F: 'static + FnMut(&[&Variant]) -> Result<Variant, ()>,
+        S: meta::AsArg<GString>,
+    {
+        meta::arg_into_owned!(name);
+
+        Self::from_fn_wrapper(FnWrapper {
+            rust_function,
+            name,
+            thread_id: Some(std::thread::current().id()),
+            linked_obj_id: Some(linked_object.instance_id()),
         })
     }
 
@@ -168,6 +194,7 @@ impl Callable {
             rust_function,
             name,
             thread_id: Some(std::thread::current().id()),
+            linked_obj_id: None,
         });
 
         callable_usage(&callable)
@@ -205,6 +232,7 @@ impl Callable {
             rust_function,
             name,
             thread_id: None,
+            linked_obj_id: None,
         })
     }
 
@@ -238,9 +266,12 @@ impl Callable {
     where
         F: FnMut(&[&Variant]) -> Result<Variant, ()>,
     {
+        let object_id = inner.linked_object_id();
+
         let userdata = CallableUserdata { inner };
 
         let info = CallableCustomInfo {
+            object_id,
             callable_userdata: Box::into_raw(Box::new(userdata)) as *mut std::ffi::c_void,
             call_func: Some(rust_callable_call_fn::<F>),
             free_func: Some(rust_callable_destroy::<FnWrapper<F>>),
@@ -493,6 +524,7 @@ pub use custom_callable::RustCallable;
 mod custom_callable {
     use super::*;
     use crate::builtin::GString;
+    use godot_ffi::GDObjectInstanceID;
     use std::hash::Hash;
     use std::thread::ThreadId;
 
@@ -515,6 +547,14 @@ mod custom_callable {
 
         /// `None` if the callable is multi-threaded ([`Callable::from_sync_fn`]).
         pub(super) thread_id: Option<ThreadId>,
+        /// `None` if callable is not linked with any object.
+        pub(super) linked_obj_id: Option<InstanceId>,
+    }
+
+    impl<F> FnWrapper<F> {
+        pub(crate) fn linked_object_id(&self) -> GDObjectInstanceID {
+            self.linked_obj_id.map(InstanceId::to_u64).unwrap_or(0)
+        }
     }
 
     /// Represents a custom callable object defined in Rust.

godot-core/src/obj/gd.rs

@@ -11,7 +11,7 @@ use std::ops::{Deref, DerefMut};
 use godot_ffi as sys;
 use sys::{static_assert_eq_size_align, SysPtr as _};
 
-use crate::builtin::{Callable, NodePath, StringName, Variant};
+use crate::builtin::{Callable, GString, NodePath, StringName, Variant};
 use crate::meta::error::{ConvertError, FromFfiError};
 use crate::meta::{
     ArrayElement, AsArg, ByRef, CallContext, ClassName, CowArg, FromGodot, GodotConvert, GodotType,
@@ -494,6 +494,21 @@ impl<T: GodotClass> Gd<T> {
         Callable::from_object_method(self, method_name)
     }
 
+    /// Creates a new callable linked to the given object from **single-threaded** Rust function or closure.
+    /// This is shorter syntax for [`Callable::from_linked_fn()`].
+    ///
+    /// `name` is used for the string representation of the closure, which helps with debugging.
+    ///
+    /// Such a callable will be automatically invalidated by Godot when a linked Object is freed.
+    /// If you need a Callable which can live indefinitely use [`Callable::from_local_fn()`].
+    #[cfg(since_api = "4.2")]
+    pub fn linked_callable<F>(&self, method_name: impl AsArg<GString>, rust_function: F) -> Callable
+    where
+        F: 'static + FnMut(&[&Variant]) -> Result<Variant, ()>,
+    {
+        Callable::from_linked_fn(method_name, self, rust_function)
+    }
+
     pub(crate) unsafe fn from_obj_sys_or_none(
         ptr: sys::GDExtensionObjectPtr,
     ) -> Result<Self, ConvertError> {

godot-core/src/registry/signal/connect_builder.rs

@@ -6,7 +6,9 @@
  */
 
 use super::{make_callable_name, make_godot_fn};
-use crate::builtin::{Callable, GString, Variant};
+#[cfg(feature = "experimental-threads")]
+use crate::builtin::Callable;
+use crate::builtin::{GString, Variant};
 use crate::classes::object::ConnectFlags;
 use crate::meta;
 use crate::meta::InParamTuple;
@@ -125,13 +127,14 @@ where
     fn inner_connect_godot_fn<F>(
         self,
         godot_fn: impl FnMut(&[&Variant]) -> Result<Variant, ()> + 'static,
+        bound: &Gd<impl GodotClass>,
     ) -> ConnectHandle {
         let callable_name = match &self.data.callable_name {
             Some(user_provided_name) => user_provided_name,
             None => &make_callable_name::<F>(),
         };
 
-        let callable = Callable::from_local_fn(callable_name, godot_fn);
+        let callable = bound.linked_callable(callable_name, godot_fn);
         self.parent_sig
             .inner_connect_untyped(callable, self.data.connect_flags)
     }
@@ -165,7 +168,8 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> ConnectBuilder<'_, '_, C, Ps> {
                 .call((), args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        let bound = self.parent_sig.receiver_object();
+        self.inner_connect_godot_fn::<F>(godot_fn, &bound)
     }
 
     /// Connect a method with `&mut self` as the first parameter (user classes only).
@@ -191,7 +195,8 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> ConnectBuilder<'_, '_, C, Ps> {
                 .call(&mut *guard, args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        let bound = self.parent_sig.receiver_object();
+        self.inner_connect_godot_fn::<F>(godot_fn, &bound)
     }
 
     /// Connect a method with `&mut Gd<Self>` as the first parameter (user + engine classes).
@@ -208,14 +213,15 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> ConnectBuilder<'_, '_, C, Ps> {
         for<'c_rcv> IndirectSignalReceiver<'c_rcv, Gd<C>, Ps, F>: From<&'c_rcv mut F>,
     {
         let gd = self.parent_sig.receiver_object();
+        let bound = gd.clone();
 
         let godot_fn = make_godot_fn(move |args| {
             IndirectSignalReceiver::from(&mut function)
                 .function()
                 .call(gd.clone(), args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        self.inner_connect_godot_fn::<F>(godot_fn, &bound)
     }
 
     /// Connect a method with any `&mut OtherC` as the first parameter (user classes only).
@@ -250,7 +256,7 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> ConnectBuilder<'_, '_, C, Ps> {
                 .call(&mut *guard, args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        self.inner_connect_godot_fn::<F>(godot_fn, &object.to_signal_obj())
     }
 
     /// Connect a method with any `&mut Gd<OtherC>` as the first parameter (user + engine classes).
@@ -282,7 +288,7 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> ConnectBuilder<'_, '_, C, Ps> {
                 .call(gd.clone(), args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        self.inner_connect_godot_fn::<F>(godot_fn, &object.to_signal_obj())
     }
 
     /// Connect to this signal using a thread-safe function, allows the signal to be called across threads.

godot-core/src/registry/signal/typed_signal.rs

@@ -132,16 +132,19 @@ impl<'c, C: WithSignals, Ps: meta::ParamTuple> TypedSignal<'c, C, Ps> {
         });
     }
 
-    /// Directly connect a Rust callable `godot_fn`, with a name based on `F`.
+    /// Directly connect a Rust callable `godot_fn`, with a name based on `F` bound to given object.
+    ///
+    /// Signal will be automatically disconnected by Godot after bound object will be freed.
     ///
     /// This exists as a shorthand for the connect methods on [`TypedSignal`] and avoids the generic instantiation of the full-blown
     /// type state builder for simple + common connections, thus hopefully being a tiny bit lighter on compile times.
     fn inner_connect_godot_fn<F>(
         &self,
         godot_fn: impl FnMut(&[&Variant]) -> Result<Variant, ()> + 'static,
+        bound: &Gd<impl GodotClass>,
     ) -> ConnectHandle {
         let callable_name = make_callable_name::<F>();
-        let callable = Callable::from_local_fn(&callable_name, godot_fn);
+        let callable = bound.linked_callable(&callable_name, godot_fn);
         self.inner_connect_untyped(callable, None)
     }
 
@@ -197,7 +200,7 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> TypedSignal<'_, C, Ps> {
                 .call((), args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        self.inner_connect_godot_fn::<F>(godot_fn, &self.receiver_object())
     }
 
     /// Connect a method (member function) with `&mut self` as the first parameter.
@@ -219,7 +222,7 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> TypedSignal<'_, C, Ps> {
                 .call(target_mut, args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        self.inner_connect_godot_fn::<F>(godot_fn, &self.receiver_object())
     }
 
     /// Connect a method (member function) with any `&mut OtherC` as the first parameter, where
@@ -254,6 +257,6 @@ impl<C: WithSignals, Ps: InParamTuple + 'static> TypedSignal<'_, C, Ps> {
                 .call(target_mut, args);
         });
 
-        self.inner_connect_godot_fn::<F>(godot_fn)
+        self.inner_connect_godot_fn::<F>(godot_fn, &object.to_signal_obj())
     }
 }

itest/rust/src/builtin_tests/containers/signal_disconnect_test.rs

@@ -154,12 +154,12 @@ fn handle_recognizes_direct_object_disconnect() {
 
 #[itest]
 fn test_handle_after_freeing_broadcaster() {
-    test_freed_nodes(true);
+    test_freed_nodes_handles(true);
 }
 
 #[itest]
 fn test_handle_after_freeing_receiver() {
-    test_freed_nodes(false);
+    test_freed_nodes_handles(false);
 }
 
 // Helper functions:
@@ -233,7 +233,7 @@ fn test_handle_recognizes_non_valid_state(disconnect_function: impl FnOnce(&mut
     obj.free();
 }
 
-fn test_freed_nodes(free_broadcaster_first: bool) {
+fn test_freed_nodes_handles(free_broadcaster_first: bool) {
     let broadcaster = SignalDisc::new_alloc();
     let receiver = SignalDisc::new_alloc();
 
@@ -251,21 +251,13 @@ fn test_freed_nodes(free_broadcaster_first: bool) {
     };
 
     // Free one of the nodes, and check if the handle thinks the objects are connected.
-    // If the broadcaster is freed, its signals to other objects is implicitly freed as well. Thus, the handle should not be connected.
-    // If the receiver is freed, the connection between the valid broadcaster and the non-valid freed object still exists.
-    // In the latter case, the connection can - and probably should - be freed with disconnect().
+    // In both cases godot runtime should handle disconnecting the signals.
     to_free.free();
-    let is_connected = handle.is_connected();
-    assert_ne!(
-        free_broadcaster_first, is_connected,
-        "Handle should only return false if broadcasting is freed!"
-    );
-
-    // It should be possible to disconnect a connection between a valid broadcaster and a non-valid receiver.
-    // If the connection is not valid (e.g. because of freed broadcaster), calling disconnect() should panic.
-    if is_connected {
-        handle.disconnect();
-    } else {
+    assert!(!handle.is_connected());
+
+    // Calling disconnect() on already disconnected handle should panic in the Debug mode.
+    // Otherwise, in release mode, the error will happen in Godot runtime.
+    if cfg!(debug_assertions) {
         expect_panic("Disconnected invalid handle!", || {
             handle.disconnect();
         });

itest/rust/src/builtin_tests/containers/signal_test.rs

@@ -55,6 +55,9 @@ fn signal_symbols_internal() {
     internal.connect_signals_internal(tracker.clone());
     drop(internal);
 
+    // Make sure that connection has been properly registered by Godot.
+    assert!(!emitter.get_incoming_connections().is_empty());
+
     emitter.bind_mut().emit_signals_internal();
 
     // Check that closure is invoked.
@@ -146,6 +149,29 @@ fn signal_symbols_complex_emit() {
     emitter.free();
 }
 
+#[cfg(since_api = "4.2")]
+#[itest]
+fn signal_receiver_auto_disconnect() {
+    let emitter = Emitter::new_alloc();
+    let sig = emitter.signals().signal_int();
+
+    let receiver = Receiver::new_alloc();
+    sig.connect_other(&receiver, Receiver::receive_int_mut);
+
+    let outgoing_connections = emitter.get_signal_connection_list("signal_int");
+    let incoming_connections = receiver.get_incoming_connections();
+
+    assert_eq!(incoming_connections.len(), 1);
+    assert_eq!(incoming_connections, outgoing_connections);
+
+    receiver.free();
+
+    // Should be auto-disconnected by Godot.
+    let outgoing_connections = emitter.get_signal_connection_list("signal_int");
+    assert!(outgoing_connections.is_empty());
+    emitter.free();
+}
+
 // "External" means connect/emit happens from outside the class, via Gd::signals().
 #[cfg(since_api = "4.2")]
 #[itest]