Signals #8
Comments
Bromeon
added
feature
|
It is probably possible to make signals into futures. allowing us to do |
# This is the 1st commit message: Parse gdextension_interface.h declarations using regex # This is the commit message #2: AsUninit trait to convert FFI pointers to their uninitialized versions # This is the commit message godot-rust#3: GodotFfi::from_sys_init() now uses uninitialized pointer types # This is the commit message godot-rust#4: Introduce GDExtensionUninitialized*Ptr, without changing semantics # This is the commit message godot-rust#5: Adjust init code to new get_proc_address mechanism # This is the commit message godot-rust#6: Make `trace` feature available in godot-ffi, fix interface access before initialization # This is the commit message godot-rust#7: Compatibility layer between Godot 4.0 and 4.1 (different GDExtension APIs) # This is the commit message godot-rust#8: Add GdextBuild to access build/runtime metadata # This is the commit message godot-rust#9: Detect 4.0 <-> 4.1 mismatches in both directions + missing `compatibility_minimum = 4.1` # This is the commit message godot-rust#10: Detect legacy/modern version of C header (also without `custom-godot` feature) # This is the commit message godot-rust#11: CI: add jobs that use patched 4.0.x versions # This is the commit message godot-rust#12: Remove several memory leaks by constructing into uninitialized pointers # This is the commit message godot-rust#13: CI: memcheck jobs for both 4.0.3 and nightly # This is the commit message godot-rust#14: Remove ToVariant, FromVariant, and VariantMetadata impls for pointers This commit splits SignatureTuple into two separate traits: PtrcallSignatureTuple and VarcallSignatureTuple. The latter is a child of the former. PtrcallSignatureTuple is used for ptrcall and only demands GodotFuncMarshall of its arguments. VarcallSignatureTuple is used for varcall and additionally demands ToVariant, FromVariant, and VariantMetadata of its arguments, so pointers cannot benefit from the optimizations provided by varcall over ptrcall. # This is the commit message godot-rust#15: Adds FromVariant and ToVariant proc macros # This is the commit message godot-rust#16: godot-core: builtin: reimplement Plane functions/methods # This is the commit message godot-rust#17: impl GodotFfi for Option<T> when T is pointer sized and nullable godot-rust#240 Additionally FromVariant and ToVariant are also implemented for Option<Gd<T>> to satisfy all the requirements for ffi and godot_api. # This is the commit message godot-rust#18: Fix UB in virtual method calls that take objects Fix incorrect incrementing of refcount when calling in to godot Fix refcount not being incremented when we receive a refcounted object in virtual methods # This is the commit message godot-rust#19: fix UB caused by preload weirdness # This is the commit message godot-rust#20: Implements swizzle and converts from/to tuples
|
Is their an appetite for a shorter term solution? I'd like the #[signal]
pub fn hit()to expand to pub fn hit() {
self.base.emit_signal("hit".into(), &[])
}In the case of: #[signal]
pub fn message(some_message: String) {}It could expand to pub fn message(some_message: String) {
self.base.emit_signal("message".into(), &[some_message])
}It wouldn't currently allow for await semantics, but it would make the There's two gotchas that I can see. First being "what if there isn't a base?" I'm imagining that would be solved by trait bounds. The tougher issue is "what if the base isn't the If you are open to this I'm interested in giving implementing it a go. Just don't want to do stuff up if it isn't a direction you're looking for or something that doesn't line up with your prioritization scheme. |
|
I kinda need signal (it's the only major blocker in my project), so you have my go ahead. My only suggestion is that the signal method should return a struct. Then more methods can be added like With regards to |
|
I don't think inherent methods are the right abstraction for signals; there is no meaningful method body the user can provide. They should be fields, possibly ZST. This would also allow Ideally this is done in a way that extends naturally to a later builder API; i.e. any sort of magic added by proc-macros should also be possible to implement in a procedural, programmatic way (see #4). Just as a preparation, I would not want to start the builder API yet. For example, one approach would be to include the signature as a generic argument. #[signal]
message: Signal<Fn(String) -> ()> |
|
It's been almost a year and I don't know if any discussion has happened in the discord but I could imagine signals continuing to look like this: #[derive(GodotClass)]
#[class(init)]
pub struct MyRefCounted {
base: Base<RefCounted>,
}
#[godot_api]
impl MyRefCounted {
#[signal]
fn foo_signal();
#[signal]
fn bar_signal(parameter: Gd<Node>) -> String;
}So from a magic macro DSL standpoint, nothing has changed. #[godot_api]
impl MyRefCounted {
// ZST definitions for FooSignal and BarSignal generated by macro
fn foo_signal() -> FooSignal {
// ...
}
fn bar_signal() -> BarSignal {
// ...
}
}The generated types would expose an API as you suggest for the member-based solution: struct FooSignal;
// or trait impl style macro magic, same difference
impl FooSignal {
pub fn connect(/* signature is implementation detail, can't remember precise callable logic impl */) { }
pub fn emit(source: &mut MyRefCounted) {}
}So that'd allow you to interact with signals as follows: #[godot_api]
impl MyRefCounted {
#[signal]
fn foo_signal();
#[func]
fn callback() {
// do things here
}
}
#[godot_api]
impl INode for MyRefCounted {
fn on_ready(&mut self) {
self.foo_signal().emit(self);
self.foo_signal().connect(Callable::from_object_method(correct_value, "callback"));
}
}I think this better models the relationship between signals and Godot classes. The way they work is so detached from the struct definition of a godot class in Rust that I'd push for them to continue using the function representation. That said, this is firmly in the territory of macro magic. We're rewriting a function signature entirely differently than how it exists in the source code, and adding magic syntax to signals. I'd definitely argue that this is justified (although that's a given because I am currently suggesting it). And as you said, you would like to see things be able to be implemented in both builder patterns and macro magic. This could be imagined as follows, but it may not fall in line with your vision for a builder API in the future: #[signal]
fn bar_signal(parameter: Gd<Node>) -> String;pub fn bar_signal() -> SignalBuilder<Fn(Gd<Node>) -> String> {
// Instead of a specialized type, we instead provided a pre-initialized builder
// that you could have just used without the macro magic
SignalBuilder::new("bar_signal")
} So we'd be able to just re-use the procedural builder and unify the usage of the pattern with this solution, but the |
|
Very interesting, thanks a lot for the detailed thoughts! 👍 I agree that the function syntax of |
|
I would argue for implementing signals as members of the associated object, e.g. #[derive(GodotClass)]
#[class(base = Object)]
struct GrassController {
#[export]
highlight: Color,
#[var]
animation_frame: u32,
grass_cut: Signal<(/* grass_id */ u32, /* how_cut */ CutKind)>,
base: Base<Object>,
}
The benefits of this approach:
The drawbacks of this approach:
|
|
Great list! Just playing devil's advocate:
But you have a point that a
Not sure I agree: function-based declaration syntax is much closer to GDScript, and can use named parameters. These could be used in a generated Place of declaration isn't a strong argument -- we already have
In Cargo-SemVer, 0.x minor versions behave like major versions. So we can make breaking changes in 0.2, 0.3 etc. This particular change has also been communicated for eternities:
There is already codegen that generates its own
There is, via custom trait -- it's what we do in |
|
There's a lot of discussion here, but I'd like to point out that the motivation behind my specific suggestions were to come up with a way to have both
This in general brings up the idea that whatever we put into place here should also go into place for engine types as they have plenty of signals associated with their types. edit after the fact: are there any obvious challenges that would come with injecting signals as struct members in code generated from the engine types? What about serialization codegen, how do we know that the magic signal structs are not needed to be serialized?
Well damn that's an oversight and a half on my end. Some discussion on Signal<(String,)>Single value tuples are a bit of a weird concept for Rust devs who haven't been familiarized with why we reach for them, and can sometimes be something they gloss over. This can be a problem, but it would be fine with documentation. I think whatever we do there is a clear pathway to do something with macros that complements procedural builders, without feeling like we're just duplicating the effort involved. I personally feel like initializing a user-defined
I can't believe that a derive macro is picking up on a type in the type system. Isn't that a bad idea? Looking into how it works, it doesn't really break anything when you trick the macros gdext has into accepting the wrong type. by just naming a random type So it wouldn't "break" anything to have the macro pick up on another special type in the type system, but I guess I underestimated how much macro magic is already in gdext. |
|
I am more inclined towards including signals in Nonetheless, it adds additional special, potentially useless field if the signal ends up not being used (eg. if class is extended in gdscript and signal won't get emitted). On @fpdotmonkey suggestion, the special field problem could potentially be worked around in such circumstances. If the overhead of creating some |
This is actually a very good point. We don't have access to the struct definition inside the As a functionWhat we could do however is add new methods, and extra state that's dynamically registered in the // All generated by #[godot_api]:
impl MyClass {
fn signals(&'a self) -> MyClassSignals<'a>
}
struct MyClassSignals<'a> {}
impl<'a> MyClassSignals<'a> {
// These would look up some dynamic property behind self.base...
fn on_created() -> Signal<...>;
fn on_destroyed() -> Signal<...>;
}Usage: self.signals().on_created().emit(...);As a fieldBut then again, this would also be possible with fields. struct MyClass {
on_created: Signal<...>,
on_destroyed: Signal<...>,
}Which, as was mentioned, has the advantage that it's less magic and closer to a procedural approach. self.on_created.emit(...);Proc-macro inference
It's not great as in "most idiomatic" but since the guy who wanted to introduce actual reflection was un-invited from RustConf last year, we likely won't have anything better than proc-macros this decade, so we need to be creative. In practice, detecting types like Note that removing an explicit attribute I see these things very pragmatic and gladly deviate from best practices™ when it helps ergonomics. Abusing |
|
Re: de/serialization of |
This to me seems a very neat compromise, perhaps even without the gen by macros. I could imagine something like this. #[derive(GodotClass)]
#[class(signals=GrassControllerSignals, ...)]
struct GrassController {
// no Signal members
}
#[derive(GodotSignals)] // perhaps a compile error if not all members are the appropriate type
struct GrassControllerSignals<'a> {
fn grass_cut() -> Signal<...>,
}
my_grass_controller.signals().grass_cut().emit(...); |
|
Interesting approach, as well! You declared a In practice, this would however require one more block for users (or even two if you declare actual |
I copy-pasted what you had written. I understood it as a shorthand for a |
|
Anyone working on this? |
|
Would be appreciated! Also, don't hesitate to further discuss the design before spending a lot of time on implementation (or whichever approach works best for you 🙂 ). I won't have that much time to review in the next 2 weeks, so no need to rush. |
|
I've been trying a few designs now that I finally got time to work on this. Based on what's discussed above, these are my goals:
After some hours of trying different things, I ended up with 2 solutions, and neither meets all the points above. The 1st solution satisfies: 1, 2 and 3. SharedI'll start with what's shared between both solutions: the definition of the signal struct and most of the methods provided in GDScript: https://docs.godotengine.org/en/stable/classes/class_signal.html #[derive(Debug, Clone)]
pub struct Signal<T> {
obj: Gd<Object>,
name: StringName,
#[doc(hidden)]
_pd: PhantomData<T>,
}
impl<S> Signal<S> {
pub fn new<Class>(obj: &Gd<Class>, name: impl Into<StringName>) -> Self
where Class: Inherits<Object>
{
Self {
obj: obj.clone().upcast(),
name: name.into(),
_pd: PhantomData,
}
}
pub fn connect(&mut self, func: impl Into<Callable>, flags: ConnectFlags) {
self.obj
.connect_ex(self.name.clone(), func.into())
.flags(flags.ord() as u32)
.done();
}
pub fn connect_deferred(&mut self, func: impl Into<Callable>) {
self.obj
.connect_ex(self.name.clone(), func.into())
.flags(ConnectFlags::DEFERRED.ord() as u32)
.done();
}
pub fn get_connections(&self) -> Array<Dictionary> {
self.obj.get_signal_connection_list(self.name.clone())
}
pub fn get_name(&self) -> StringName {
self.name.clone()
}
pub fn get_object(&self) -> Gd<Object> {
self.obj.clone()
}
pub fn get_object_id(&self) -> InstanceId {
self.obj.instance_id()
}
pub fn is_connected(&self, func: impl Into<Callable>) -> bool {
self.obj.is_connected(self.name.clone(), func.into())
}
pub fn is_null(&self) -> bool {
!self.obj.is_instance_valid() || !self.obj.has_signal(self.name.clone())
}
}
impl<T> PartialEq for Signal<T> {
fn eq(&self, other: &Self) -> bool {
if !self.obj.is_instance_valid() || !other.obj.is_instance_valid() {
return false;
}
self.obj.instance_id() == other.obj.instance_id() && self.name == other.name
}
}Note that Both solutions would share the same proc-macro input, which is exactly the same as the current implementation: #[derive(GodotClass)]
#[class(no_init, base = Object)]
struct MyClass {
base: Base<Object>,
}
#[godot_api]
impl MyClass {
#[signal]
fn child_entered(child: Gd<Node>) {}
}1st solution - Use declarative macros to provide method impls that convert generic tuples into separate argumentsimpl<S> Signal<S> {
pub fn connect(&mut self, func: impl Into<Callable>, flags: ConnectFlags) {
self.obj.connect_ex(self.name.clone(), func.into())
.flags(flags.ord() as u32)
.done();
}
pub fn connect_deferred(&mut self, func: impl Into<Callable>) {
self.obj.connect_ex(self.name.clone(), func.into())
.flags(ConnectFlags::DEFERRED.ord() as u32)
.done();
}
}
macro_rules! impl_variadic_args_methods {
($($arg: ident: $T: ident),*) => {
#[allow(clippy::too_many_arguments)]
#[allow(unused)]
impl<$($T: FromGodot + ToGodot),*> Signal<($($T,)*)> {
pub fn connect_fn<F>(&mut self, mut func: F, flags: ConnectFlags)
where F: FnMut($($T),*) + 'static + Send + Sync
{
let callable = Callable::from_fn("lambda", move |args| {
let mut idx = 0;
$(
let $arg = args[idx].to::<$T>();
idx += 1;
)*
func($($arg,)*);
Ok(Variant::nil())
});
self.connect(callable, flags);
}
pub fn connect_fn_deferred<F>(&mut self, func: F)
where F: FnMut($($T),*) + 'static
{
let mut func = AssertThreadSafe(func);
let callable = Callable::from_fn("lambda", move |args: &[&Variant]| {
let mut idx = 0;
$(
let $arg = args[idx].to::<$T>();
idx += 1;
)*
func($($arg,)*);
Ok(Variant::nil())
});
self.connect_deferred(callable);
}
pub fn emit(&mut self, $($arg: $T),*) {
self.obj.emit_signal(self.name.clone(), &[$($arg.to_variant()),*]);
}
}
};
}
impl_variadic_args_methods!();
impl_variadic_args_methods!(arg: T);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2, arg_3: T3);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2, arg_3: T3, arg_4: T4);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2, arg_3: T3, arg_4: T4, arg_5: T5);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2, arg_3: T3, arg_4: T4, arg_5: T5, arg_6: T6);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2, arg_3: T3, arg_4: T4, arg_5: T5, arg_6: T6, arg_7: T7);
impl_variadic_args_methods!(arg_1: T1, arg_2: T2, arg_3: T3, arg_4: T4, arg_5: T5, arg_6: T6, arg_7: T7, arg_8: T8);What happens here is that for each combination of tuple sizes, we create method impls that "unwrap" those tuples into separate arguments. We can't properly name arguments because those are inherent impls that are written in Godot-Rust. Code generated by proc macro: pub trait MyClass_child_entered {
#[must_use]
fn child_entered(&mut self) -> Signal<(Gd<Node>,)>;
}
impl MyClass_child_entered for MyClass {
fn child_entered(&mut self) -> Signal<(Gd<Node>,)> {
Signal::new(&self.to_gd(), "child_entered")
}
}
impl MyClass_child_entered for Gd<MyClass> {
fn child_entered(&mut self) -> Signal<(Gd<Node>,)> {
Signal::new(self, "child_entered")
}
}
// Which can be used like this:
fn use_signal(my_class: &mut Gd<MyClass>) {
my_class.child_entered()
.connect_fn_deferred(|node| {
godot_print!("Child entered: {:?}", node.get_name());
});
}I used a trait because I wanted to allow fetching the signal from both If fetching the signal from 2nd solution - Proc-macros + zero-sized typesAll code specific to this solution would be proc-macro generated: pub struct child_entered;
#[allow(unused)]
pub trait Signal_child_entered {
fn connect_fn<F>(&mut self, f: F, flags: ConnectFlags) where F: FnMut(Gd<Node>) + 'static + Send + Sync;
fn connect_fn_deferred<F>(&mut self, f: F) where F: FnMut(Gd<Node>) + 'static;
fn emit(&mut self, child: Gd<Node>);
}
#[allow(unused)]
impl Signal_child_entered for Signal<child_entered> {
fn connect_fn<F>(&mut self, mut f: F, flags: ConnectFlags)
where F: FnMut(Gd<Node>) + 'static + Send + Sync
{
self.connect(Callable::from_fn("lambda", move |args: &[&Variant]| {
let mut idx = 0;
let child = args[idx].to::<Gd<Node>>();
idx += 1;
f(child);
Ok(Variant::nil())
}), flags);
}
fn connect_fn_deferred<F>(&mut self, f: F)
where F: FnMut(Gd<Node>) + 'static
{
let mut f = AssertThreadSafe(f);
let callable = Callable::from_fn("lambda", move |args: &[&Variant]| {
let mut idx = 0;
let child = args[idx].to::<Gd<Node>>();
idx += 1;
f(child);
Ok(Variant::nil())
});
self.connect(callable, ConnectFlags::DEFERRED);
}
fn emit(&mut self, child: Gd<Node>) {
self.obj.emit_signal(self.name.clone(), &[child.to_variant()]);
}
}
pub trait MyClass_child_entered {
#[must_use]
fn child_entered(&mut self) -> Signal<child_entered>;
}
impl MyClass_child_entered for MyClass {
fn child_entered(&mut self) -> Signal<child_entered> {
Signal::new(&self.to_gd(), "child_entered")
}
}
impl MyClass_child_entered for Gd<MyClass> {
fn child_entered(&mut self) -> Signal<child_entered> {
Signal::new(self, "child_entered")
}
}Both solutions can be used like this, except the 2nd shows the proper parameter name: fn use_signal(my_class: &mut Gd<MyClass>) {
my_class.child_entered().connect_fn_deferred(|node| {
godot_print!("Child entered: {:?}", node.get_name());
});
let node = Node::new_alloc();
my_class.child_entered().emit(node);
}Personally, I'm okay with both, but I prefer the first solution, I don't think parameter names are worth sacrificing proc-macro "independence". I can start the actual implementation when a design is agreed upon. |
|
Did some tinkering again today, and I realized something: we can combine both solutions presented in my post above. We can keep the tuple implementations from the 1st, but use the 2nd to generate code with the proc macro. The result:
|
To me, it's not obvious that
Is it not allowed to require
Is there be a reason to not make it Does the |
I don't think this is an issue.
You can't bundle all signals in the same trait because they could have different signatures, you can't have two
Yes, it allows me to put a generic Edit: fixing formatting. |
|
Thanks a lot for the great design, @Houtamelo! 👍
I think these are great premises. If we want to get close to GDScript usability, signals should be named and type-safe wherever possible. And the more natural they feel in Rust, the more ergonomic the experience. Regarding
Only 1 of these 4 points are visible at the usage site, and only once the code is already written. A dedicated
I also don't know if an extra
I'm also a bit worried about the number of introduced symbols and the genericity, which both have an impact on compile times and doc complexity. It may be a bit hard to discover, debug and read related compile errors. But about @fpdotmonkey's suggestions, would something like this not be possible? I tried to use more concrete types where applicable, and reduce the per-signal symbols from
to
It's well possible that I missed something though. pub struct MyClassSignals {
pub child_entered: MyClassSignalChildEntered,
... // other signals
}
pub struct MyClassSignalAreaEntered {
signal: Signal, // actual reference to object + method
}
impl MyClassSignalAreaEntered {
pub fn connect<F>(&mut self, function: F)
where F: FnMut(Gd<Area2D>) { ... }
pub fn emit(&mut self, area: Gd<Area2D>) { ... }
}
// Maybe this API could be hidden, as it's only needed for type erasure/dispatch, or maybe
// it can be useful for users later, too... Name TBD
impl SignalDispatch for MyClassSignalAreaEntered {
type Params = (...); // some tuple
fn generic_connect(...) { ... }
fn generic_emit(...) { ... }
}
trait SignalProvider {
type SignalGroup;
fn signals(&self) -> SignalGroup;
}
impl SignalProvider for MyClass {
type SignalGroup = MyClassSignals;
pub fn signals(&self) -> MyClassSignals { ... }
}
// Do NOT impl SignalProvider for Gd<MyClass>
// Inside gdext library -- could this solve the orphan issue?
impl<T> Gd<T>
where T: SignalProvider {
pub fn signals(&self) -> T::SignalGroup {
// Get direct access to instance without bind().
// Must be unsafe, but works as long as SignalProvider doesn't access Rust-side state of the object.
let obj: &T = unsafe { self.bind_unchecked() }; // private API
// Accepting Gd<Self> instead of &self would make the access more elegant,
// but at the cost of more indirections/symbols.
obj.signals()
}
} |
|
I likely won't be able to pick this up again this year, so anyone else is welcome to move forward with the implementation if they'd like. |
See how we can support signal registration, in the builder API and/or proc-macros.
The text was updated successfully, but these errors were encountered: