Allow `Gd<T>` to be passed as a parameter in async signals by TitanNano · Pull Request #1091 · godot-rust/gdext

TitanNano · 2025-03-20T21:16:26Z

Introduce a runtime check if the passed signal argument was actually sent between threads while being !Send.

This allows Gd<T> to be used when awaiting signals, as long as the signal is not emitted on a different thread than the main-thread. Should the user await a signal with a non !Send argument from a different thread, a panic will occur.

Resolves #1074.

Depends on #1090.

GodotRust · 2025-03-20T21:27:10Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1091

Bromeon

Thanks a lot! 👍

Resolves #1074.

How would the syntax of passing Gd look now?

Could you maybe add an itest that supports this use case?

godot-core/src/task/futures.rs

Bromeon · 2025-03-21T15:33:18Z

godot-core/src/task/futures.rs

+impl_maybe_send!(
+    Send;
+    bool, u8, u16, u32, u64, i8, i16, i32, i64, f32, f64, StringName, Transform2D, Transform3D, Vector2, Vector2i, Vector2Axis,
+    Vector3, Vector3i, Vector3Axis, Vector4, Vector4i, Rect2, Rect2i, Plane, Quaternion, Aabb, Basis, Projection, Color, Rid
+);
+
+impl_maybe_send!(
+    !Send;
+    Variant, GString, Dictionary, VariantArray, Callable, NodePath, PackedByteArray, PackedInt32Array, PackedInt64Array, PackedFloat32Array,
+    PackedFloat64Array, PackedStringArray, PackedVector2Array, PackedVector3Array, PackedColorArray, PackedVector4Array, Signal
+);


There are already several lists that set some compile-time attributes for each variant type:

for Var/Export/BuiltinExport

for FFI traits

GodotConvert/ToGodot/FromGodot which are registered in distributed code

I wonder if we should just keep adding those lists decentrally, or rather provide more parameters to existing lists that influence the Send property here.

Having fewer lists also makes it exceedingly unlikely to forget an implementation (as types simply unusable without basic trait support). So that makes the special machinery to "check if all are covered" redundant.

And overall, it means of course supporting new types needs changes in fewer places, reducing maintenance overhead.

We could define the macros in the respective modules, but have a central module that calls all of them for each type.

That sounds great, then we'd still have the logic local to the files, but central registration 👍

Maybe you could integrate the checks here into one of the existing macros.
Possibly this is the best candidate?

Try to keep changes there as minimal. We can then at a later point refactor the other lists...

We should also not forget this point later 🙂

I tried to keep the changed minimal. Do you want to merge impl_ffi_variant and impl_dynamic_send?

Bromeon · 2025-03-21T15:35:08Z

godot-core/src/task/futures.rs

+// This should be kept in sync with crate::registry::signal::variadic.
+impl_maybe_send!(tuple; );
+impl_maybe_send!(tuple; A1);
+impl_maybe_send!(tuple; A1, A2);
+impl_maybe_send!(tuple; A1, A2, A3);
+impl_maybe_send!(tuple; A1, A2, A3, A4);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6, A7);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6, A7, A8);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6, A7, A8, A9);


This should be kept in sync with crate::registry::signal::variadic.

Then maybe we could add it directly to those macros.

Of course it would mean that the responsibility of Send is now mixed with other responsibilities, but this coupling exists either way -- it's just that here it's implicit and only with a comment.

And it would play into my previous comment regarding centralization of static type properties.

You resolved this but it's still there -- I assume, we would try to centralize it later, maybe with #1042...

Ah I see, to me, this fell under:

Try to keep changes there as minimal. We can then at a later point refactor the other lists...

I.e. we will merge crate::registry::signal::variadic into crate::bulitin::variant::impls in a later PR.

Sounds good! Maybe wait for #1042, otherwise there will likely be merge conflicts.

godot-core/src/task/futures.rs

TitanNano · 2025-03-22T09:28:36Z

How would the syntax of passing Gd look now?

The syntax does not differ from any other argument type.

signal.to_future::<(Gd<Object>,)>().await

Or

signal.to_future::<(bool, Gd<Object>, u32)>().await

Could you maybe add an itest that supports this use case?

I have now added it to the existing I test, but I will also add a negative test to assert that it panics correctly.

Bromeon

Thanks for the updates!

godot-core/src/builtin/variant/impls.rs

itest/rust/Cargo.toml

godot-core/src/task/futures.rs

Bromeon

There are some resolved conversations that haven't been addressed yet. To me, it's fine if we do the refactoring later (separate from the feature PR here), just so we don't forget:

Do not list all builtin impls separately, but integrate them into existing macros.
That also makes the "check if all cases" match obsolete.
- see comment
Tuple impls could be combined with #1042 variadic traits.
- see comment

Anything else from your side? Or is this ready?

godot-core/src/builtin/variant/impls.rs

Bromeon · 2025-03-26T21:13:54Z

godot-core/src/task/futures.rs

+// This should be kept in sync with crate::registry::signal::variadic.
+impl_maybe_send!(tuple; );
+impl_maybe_send!(tuple; A1);
+impl_maybe_send!(tuple; A1, A2);
+impl_maybe_send!(tuple; A1, A2, A3);
+impl_maybe_send!(tuple; A1, A2, A3, A4);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6, A7);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6, A7, A8);
+impl_maybe_send!(tuple; A1, A2, A3, A4, A5, A6, A7, A8, A9);


You resolved this but it's still there -- I assume, we would try to centralize it later, maybe with #1042...

Bromeon · 2025-03-26T21:17:10Z

godot-core/src/task/futures.rs

+impl_maybe_send!(
+    Send;
+    bool, u8, u16, u32, u64, i8, i16, i32, i64, f32, f64, StringName, Transform2D, Transform3D, Vector2, Vector2i, Vector2Axis,
+    Vector3, Vector3i, Vector3Axis, Vector4, Vector4i, Rect2, Rect2i, Plane, Quaternion, Aabb, Basis, Projection, Color, Rid
+);
+
+impl_maybe_send!(
+    !Send;
+    Variant, GString, Dictionary, VariantArray, Callable, NodePath, PackedByteArray, PackedInt32Array, PackedInt64Array, PackedFloat32Array,
+    PackedFloat64Array, PackedStringArray, PackedVector2Array, PackedVector3Array, PackedColorArray, PackedVector4Array, Signal
+);


We should also not forget this point later 🙂

TitanNano · 2025-03-26T22:18:39Z

Anything else from your side? Or is this ready?

Nothing else I think.

Bromeon reviewed Mar 21, 2025

View reviewed changes

Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Mar 21, 2025

TitanNano force-pushed the jovan/signal_future_non_send_args branch 2 times, most recently from 60a907a to 56367f0 Compare March 21, 2025 22:31

TitanNano force-pushed the jovan/signal_future_non_send_args branch 6 times, most recently from c9c5952 to 0977874 Compare March 23, 2025 09:49

Bromeon reviewed Mar 23, 2025

View reviewed changes

TitanNano force-pushed the jovan/signal_future_non_send_args branch 3 times, most recently from 6ae6547 to 0910aa9 Compare March 23, 2025 14:50

Bromeon reviewed Mar 26, 2025

View reviewed changes

Allow non Sync + Send signal arguments inside SignalFuture

8334785

TitanNano force-pushed the jovan/signal_future_non_send_args branch from 0910aa9 to 8334785 Compare March 26, 2025 22:15

Bromeon enabled auto-merge March 26, 2025 22:19

Bromeon added this pull request to the merge queue Mar 26, 2025

Merged via the queue into godot-rust:master with commit 6e21024 Mar 26, 2025
16 checks passed

TitanNano deleted the jovan/signal_future_non_send_args branch March 26, 2025 23:02

Bromeon added the c: async Everything related to async tasks/signals label Apr 9, 2025

Uh oh!

Conversation

TitanNano commented Mar 20, 2025

Uh oh!

GodotRust commented Mar 20, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TitanNano Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TitanNano commented Mar 22, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bromeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TitanNano commented Mar 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TitanNano Mar 21, 2025 •

edited

Loading

Bromeon left a comment •

edited

Loading