Skip to content

Commit

Permalink
C#: Add WeakEvent to store subscriptions of events
Browse files Browse the repository at this point in the history
- Add `GodotWeakEvent` type that stores the subscriptions of events.
  - Keeps only a weak reference to the target and the method info of the handler, so the target can be garbage collected.
  - If the target is collected, it will eventually get rid of the subscription data and avoids raising the event for collected targets.
  - Used in user script events and native class events, resulting in consistent behavior.
- Fix generation of signal events documentation that was adding the documentation comments in the wrong place.
  • Loading branch information
raulsntos committed Sep 1, 2023
1 parent 549fcce commit 0254260
Show file tree
Hide file tree
Showing 12 changed files with 597 additions and 253 deletions.
65 changes: 50 additions & 15 deletions modules/mono/csharp_script.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1299,6 +1299,17 @@ bool CSharpLanguage::setup_csharp_script_binding(CSharpScriptBinding &r_script_b
CSharpLanguage::get_singleton()->post_unsafe_reference(rc);
}

// connect_event_signals
{
List<MethodInfo> signals;
p_object->get_signal_list(&signals);
Vector<StringName> signal_names;
for (const MethodInfo &signal : signals) {
signal_names.push_back(signal.name);
}
connect_event_signals(signal_names, r_script_binding.owner, r_script_binding.connected_event_signals);
}

return true;
}

Expand Down Expand Up @@ -1344,6 +1355,8 @@ void CSharpLanguage::_instance_binding_free_callback(void *, void *, void *p_bin
CSharpScriptBinding &script_binding = data->value();

if (script_binding.inited) {
disconnect_event_signals(script_binding.owner, script_binding.connected_event_signals);

// Set the native instance field to IntPtr.Zero, if not yet garbage collected.
// This is done to avoid trying to dispose the native instance from Dispose(bool).
GDMonoCache::managed_callbacks.ScriptManagerBridge_SetGodotObjectPtr(
Expand Down Expand Up @@ -1430,6 +1443,26 @@ GDExtensionBool CSharpLanguage::_instance_binding_reference_callback(void *p_tok
}
}

void CSharpLanguage::connect_event_signals(Vector<StringName> p_signals, Object *p_owner, List<Callable> &r_connected_event_signals) {
for (const StringName &signal_name : p_signals) {
// TODO: Use pooling for ManagedCallable instances.
EventSignalCallable *event_signal_callable = memnew(EventSignalCallable(p_owner, signal_name));

Callable callable(event_signal_callable);
r_connected_event_signals.push_back(callable);
p_owner->connect(signal_name, callable);
}
}

void CSharpLanguage::disconnect_event_signals(Object *p_owner, List<Callable> &p_connected_event_signals) {
for (const Callable &callable : p_connected_event_signals) {
const EventSignalCallable *event_signal_callable = static_cast<const EventSignalCallable *>(callable.get_custom());
p_owner->disconnect(event_signal_callable->get_signal(), callable);
}

p_connected_event_signals.clear();
}

void *CSharpLanguage::get_instance_binding(Object *p_object) {
void *binding = p_object->get_instance_binding(get_singleton(), &_instance_binding_callbacks);

Expand Down Expand Up @@ -1466,6 +1499,7 @@ void CSharpLanguage::set_instance_binding(Object *p_object, void *p_binding) {
bool CSharpLanguage::has_instance_binding(Object *p_object) {
return p_object->has_instance_binding(get_singleton());
}

void CSharpLanguage::tie_native_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, const StringName *p_native_name, bool p_ref_counted) {
// This method should not fail

Expand Down Expand Up @@ -1517,6 +1551,17 @@ void CSharpLanguage::tie_native_managed_to_unmanaged(GCHandleIntPtr p_gchandle_i

// Should be thread safe because the object was just created and nothing else should be referencing it
CSharpLanguage::set_instance_binding(p_unmanaged, data);

// connect_event_signals
{
List<MethodInfo> signals;
p_unmanaged->get_signal_list(&signals);
Vector<StringName> signal_names;
for (const MethodInfo &signal : signals) {
signal_names.push_back(signal.name);
}
connect_event_signals(signal_names, script_binding.owner, script_binding.connected_event_signals);
}
}

void CSharpLanguage::tie_user_managed_to_unmanaged(GCHandleIntPtr p_gchandle_intptr, Object *p_unmanaged, Ref<CSharpScript> *p_script, bool p_ref_counted) {
Expand Down Expand Up @@ -1882,25 +1927,15 @@ void CSharpInstance::mono_object_disposed_baseref(GCHandleIntPtr p_gchandle_to_f

void CSharpInstance::connect_event_signals() {
// The script signals list includes the signals declared in base scripts.
for (CSharpScript::EventSignalInfo &signal : script->get_script_event_signals()) {
String signal_name = signal.name;

// TODO: Use pooling for ManagedCallable instances.
EventSignalCallable *event_signal_callable = memnew(EventSignalCallable(owner, signal_name));

Callable callable(event_signal_callable);
connected_event_signals.push_back(callable);
owner->connect(signal_name, callable);
Vector<StringName> signal_names;
for (const CSharpScript::EventSignalInfo &signal : script->get_script_event_signals()) {
signal_names.push_back(signal.name);
}
CSharpLanguage::connect_event_signals(signal_names, owner, connected_event_signals);
}

void CSharpInstance::disconnect_event_signals() {
for (const Callable &callable : connected_event_signals) {
const EventSignalCallable *event_signal_callable = static_cast<const EventSignalCallable *>(callable.get_custom());
owner->disconnect(event_signal_callable->get_signal(), callable);
}

connected_event_signals.clear();
CSharpLanguage::disconnect_event_signals(owner, connected_event_signals);
}

void CSharpInstance::refcount_incremented() {
Expand Down
5 changes: 5 additions & 0 deletions modules/mono/csharp_script.h
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ struct CSharpScriptBinding {
MonoGCHandleData gchandle;
Object *owner = nullptr;

List<Callable> connected_event_signals;

CSharpScriptBinding() {}
};

Expand Down Expand Up @@ -356,6 +358,9 @@ class CSharpLanguage : public ScriptLanguage {

static GDExtensionInstanceBindingCallbacks _instance_binding_callbacks;

static void connect_event_signals(Vector<StringName> p_signals, Object *p_owner, List<Callable> &r_connected_event_signals);
static void disconnect_event_signals(Object *p_owner, List<Callable> &p_connected_event_signals);

public:
static void *get_instance_binding(Object *p_object);
static void *get_existing_instance_binding(Object *p_object);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ INamedTypeSymbol symbol
{
string signalName = signalDelegate.Name;

source.Append(" info.AddSignalEventDelegate(SignalName.")
source.Append(" info.AddSignalGodotWeakEvent(SignalName.")
.Append(signalName)
.Append(", this.backing_")
.Append(signalName)
Expand Down Expand Up @@ -249,7 +249,7 @@ INamedTypeSymbol symbol
string signalName = signalDelegate.Name;
string signalDelegateQualifiedName = signalDelegate.DelegateSymbol.FullQualifiedNameIncludeGlobal();

source.Append(" if (info.TryGetSignalEventDelegate<")
source.Append(" if (info.TryGetSignalGodotWeakEvent<")
.Append(signalDelegateQualifiedName)
.Append(">(SignalName.")
.Append(signalName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,32 +245,40 @@ INamedTypeSymbol symbol
foreach (var signalDelegate in godotSignalDelegates)
{
string signalName = signalDelegate.Name;
var invokeMethodData = signalDelegate.InvokeMethodData;

// TODO: Hide backing event from code-completion and debugger
// The reason we have a backing field is to hide the invoke method from the event,
// as it doesn't emit the signal, only the event delegates. This can confuse users.
// Maybe we should directly connect the delegates, as we do with native signals?
source.Append(" private ")
source.Append(" [global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]\n")
.Append(" [global::System.Diagnostics.DebuggerBrowsable(global::System.Diagnostics.DebuggerBrowsableState.Never)]\n")
.Append(" private global::Godot.GodotWeakEvent<")
.Append(signalDelegate.DelegateSymbol.FullQualifiedNameIncludeGlobal())
.Append(" backing_")
.Append("> backing_")
.Append(signalName)
.Append(";\n");
.Append(";\n\n");

source.Append(
$" /// <inheritdoc cref=\"{signalDelegate.DelegateSymbol.FullQualifiedNameIncludeGlobal()}\"/>\n");

source.Append(" public event ")
.Append(signalDelegate.DelegateSymbol.FullQualifiedNameIncludeGlobal())
.Append(" ")
.Append(' ')
.Append(signalName)
.Append(" {\n")
.Append(" add => backing_")
.Append('\n')
.Append(" {\n");

source.Append(" add\n")
.Append(" {\n")
.Append(" backing_")
.Append(signalName)
.Append(" ??= new();\n")
.Append(" backing_")
.Append(signalName)
.Append(" += value;\n")
.Append(" remove => backing_")
.Append(".AddEventHandler(value);\n")
.Append(" }\n");

source.Append(" remove => backing_")
.Append(signalName)
.Append(" -= value;\n")
.Append("}\n");
.Append("?.RemoveEventHandler(value);\n")
.Append(" }\n\n");
}

// Generate RaiseGodotClassSignalCallbacks
Expand All @@ -290,7 +298,7 @@ INamedTypeSymbol symbol

source.Append(" base.RaiseGodotClassSignalCallbacks(signal, args);\n");

source.Append(" }\n");
source.Append(" }\n\n");
}

// Generate HasGodotClassSignal
Expand Down Expand Up @@ -483,7 +491,7 @@ StringBuilder source
source.Append(") {\n");
source.Append(" backing_");
source.Append(signalName);
source.Append("?.Invoke(");
source.Append("?.RaiseEvent(");

for (int i = 0; i < invokeMethodData.ParamTypes.Length; i++)
{
Expand Down
Loading

0 comments on commit 0254260

Please sign in to comment.