Signal API improvements #1152
Signal API improvements #1152
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1152 |
|
Nice! One question – could we use The implementation itself is easy (remove bounds The only issue is the abundance of methods that allow to do the very same thing ( |
Bromeon
added
quality-of-life
There was a problem hiding this comment.
Thanks a lot! Great observation with the type inference 👍
This PR fixes that by replacing the
SignalReceivertrait with direct usage ofFnMut(**).
It's really unfortunate that this means reverting the tuple trait back to macros 😕
Macros are also the approach we started with in the generated engine APIs, but over time we could generalize a lot using tuples, thus moving more logic to the Rust type system and make it a bit easier to reason/debug.
I understand why macros are needed, but this has some minor implications:
- The code is slightly harder to work with (IDEs don't always treat macro code first-class, e.g. regarding static analysis, formatting, syntax highlighting).
- It's no longer possible for users to do generic programming based on
SignalReceivertrait. I wonder if it's still possible to abstract overPs(e.g. viaParamTupletrait) if a user wishes so? - We now generate 10x the amount of code. It's probably minuscule but might have a tiny impact on parse times. On the other hand, there's less trait solving going on, so it might compensate.
What I wonder is: would it be possible to have just the "frontend" as macros, so the signatures can take FnMut directly -- but then they would delegate logic to actual generic code? Might cause even more complexity though 🤔
(Don't start changing anything here just yet, just wondering about the options...)
As a side bonus, I also included versions of
connect_**that accept native engine classes as receivers instead of user classes.
Like I wrote in Discord, I'd prefer this to be only in the builder for now and keep the top-level API minimal. But I'm still not fully convinced if this functionality is needed at all, it seems to provide only minor benefits over the already existing connect(), see my code comment.
If anything, we should consider adding this part in a separate PR (which can be done backwards-compatibly, unlike the other changes here). Or possibly discuss in an issue first, get overall community input and see which usage patterns are most burning. Background is that I really don't want to block 0.3 release further, and this change comes already at the last second 🙂
| let mut user = Emitter::new_alloc(); | ||
| let mut engine = Node::new_alloc(); | ||
|
|
||
| user.signals() | ||
| .child_entered_tree() | ||
| .connect_engine(&engine, |this, mut child| { | ||
| // Use methods that `Node` declares | ||
| let _ = this.get_path(); // ref | ||
| this.set_unique_name_in_owner(true); // mut | ||
|
|
||
| // `child` is also a node | ||
| let _ = child.get_path(); // ref | ||
| child.set_unique_name_in_owner(true); // mut | ||
| }); |
There was a problem hiding this comment.
Just for my understanding, where do you see the main advantage over connect() here?
Wouldn't the following work (if engine is no longer used -- otherwise with an extra clone)?
| let mut user = Emitter::new_alloc(); | |
| let mut engine = Node::new_alloc(); | |
| user.signals() | |
| .child_entered_tree() | |
| .connect_engine(&engine, |this, mut child| { | |
| // Use methods that `Node` declares | |
| let _ = this.get_path(); // ref | |
| this.set_unique_name_in_owner(true); // mut | |
| // `child` is also a node | |
| let _ = child.get_path(); // ref | |
| child.set_unique_name_in_owner(true); // mut | |
| }); | |
| let mut user = Emitter::new_alloc(); | |
| let mut engine = Node::new_alloc(); | |
| user.signals() | |
| .child_entered_tree() | |
| .connect(move |mut child| { | |
| // Use methods that `Node` declares | |
| let _ = engine.get_path(); // ref | |
| engine.set_unique_name_in_owner(true); // mut | |
| // `child` is also a node | |
| let _ = child.get_path(); // ref | |
| child.set_unique_name_in_owner(true); // mut | |
| }); |
There was a problem hiding this comment.
Also, this gives minor ergonomic benefits when a single object is used (no need to copy or have move closure), but you'd still need that pattern if more than 1 Gd needs to be accessed within the closure.
In comparison, connect_obj exists because it does the heavy lifting of rebinding the object so that the user can safely access it, especially when self.signals().xy().emit() is called. This isn't easily possible with other existing methods.
There was a problem hiding this comment.
It's a minor advantage, but Gd-Rust already has enough boilerplate, and each line removed counts towards reducing it.
Moving engine Gds into the closures is easy, but if you want to reuse said GD elsewhere, you need to add a line where you explicitly create a new variable, cloning the instance you want to reuse:
let user_class = Emitter::new_alloc();
let mut some_node = Node::new_alloc();
let node_clone = some_node.clone(); // extra line
user_class.signals().ready().connect(move || {
node_clone.do_something();
});
// keep using `some_node`Compare connecting to a signal in GDScript/C# to Rust:
var bound_obj = ...
emitter.signal_name.connect(func(): do_something(bound_obj))var bound_obj = ...;
emitter.signal_name += () => do_something(bound_obj);let source_obj = ...;
let bound_obj = source_obj.clone();
emitter.signals().signal_name().connect(move || { do_something(bound_obj) });So in my opinion, any boilerplate reduction is welcome.
The bellow is not directly related to the issue above, consider it as food for thought:
The most common use case in my experience is connecting to a signal that's on a exported field of your class, look at how many extra lines this takes:
#[derive(GodotClass)]
#[class(base = Control, init)]
pub struct SettingsUI {
base: Base<Control>,
#[init(node = "close_menu_button")]
close_menu_button: OnReady<Gd<BaseButton>>,
}
#[godot_api]
impl IControl for SettingsUI {
fn ready(&mut self) {
self.close_menu_button
.clone() // needed due to borrow checker, since `signals` takes `&mut self`, even though internally it doesn't really need a mutable reference
.signals() // extra call other languages don't have
.pressed()
.connect_builder() // extra call
.object_self(self) // extra call
.method_mut(|this| this.base_mut().hide())
.flags(ConnectFlags::DEFERRED) // connecting deferred is almost always preferable, so I just do it by default.
.done(); // extra call
}
}In my opinion, the API should be (and we can!) changed to allow the following:
#[godot_api]
impl IControl for SettingsUI {
fn ready(&mut self) {
self.close_menu_button
.signals() // we could get rid of this call, but for now, it's fine.
.pressed() // make this take `&self` instead of `&mut self`: no borrow checker issues, functionality remains the same.
.connect_other_deferred(self, |this| this.base_mut().hide());
}We get users having issues with double bind almost every day, which are easily solved by
connecting deferred. I believe that should be incentivized and facilitated.
There was a problem hiding this comment.
Thanks for the detailed elaboration!
Agree on unnecessary clone, and we can possibly collapse object_self + method_mut in the builder as well. The ToSignalObj trait abstracts over &mut WithUserSignals and Gd<T> and should (hopefully) be able to provide some groundwork here.
I don't agree with deferred, at least not right now.
- First, I think we should see if we can establish patterns that make borrow errors less frequent.
emit()for example is carefully crafted so it can be re-entrant on the same object. - Deferred isn't the solution to everything. It introduces asynchronous execution, and sometimes you want effects to be applied immediately, because later logic in the same frame depends on it.
- The approach of promoting everything to the top level still causes combinatoric explosion, even if used frequently. Some people use one-shot signals a lot. For some it's important to have cross-thread signals due to their architecture.
Not saying we can never do this, but these are very clearly QoL features that can possibly be added later, after more careful evaluation.
Rather than supporting every use case in the shortest possible syntax, we should make the builder modular, so that people can write their own abstractions on top. Imagine you could write extension traits like this:
#[godot_api]
impl IControl for SettingsUI {
fn ready(&mut self) {
self.close_menu_button
.signals()
.pressed() // ... extension trait on TypedSignal
.my_custom_connect(|this| this.base_mut().hide());
}but this might be harder with the macro-based approach now.
There was a problem hiding this comment.
Deferred isn't the solution to everything. It introduces asynchronous execution, and sometimes you want effects to be applied immediately, because later logic in the same frame depends on it.
I believe it is for 90% of cases, which has been (anecdotally) proven by the help requests we get on discord.
For the cases were a user wants to apply something immediately, they can just not connect with the Deferred flag and handle the consequences on their own.
When communication needs to happen immediately, there's a good chance that you're better off communicating directly in Rust rather than going through the Godot layer (again, in my experience).
Rather than supporting every use case in the shortest possible syntax
I don't think we should ever support every use case in the shortest possible syntax, my argument is that connect_deferred is more common than a regular connect, and thus it should be supported.
There was a problem hiding this comment.
I believe it is for 90% of cases, which has been (anecdotally) proven by the help requests we get on discord.
With the same argument, you could justify call_deferred everywhere because people constantly run into borrow issues with Gd::bind/bind_mut. And then it turns out that most of them can be solved by proper use of base/base_mut. And we can probably do better in conveying these concepts.
Deferred signals as a result of borrow errors are a workaround because of a Rust-specific problem, they are not the default in Godot itself. While I don't think we can 100% solve every case, I still believe more effort should be put into promoting safe usage of non-deferred signals, both with API design and documentation.
Yes -- we need to keep in mind that the typed signals API is a completely new addition for users. While it seems simple, there are still tons of little nuances (type inference, signal visibility, borrow semantics, thread safety, ...). I don't think we're doing users a favor by adding yet another slightly different method, there's just too much to ingest at once. I'm not opposed to extending the API in the future, but we should probably wait some time and see which usage patterns emerge. There are also things like disconnection which are still missing and more imporant, as they can't be emulated on user side. Once we have a crates.io release, we'll likely receive more user feedback as well. |
Fixed.
I provided a solution on discord (which I already tested and confirmed that works), which relies on using this trait for parameter resolving: #[allow(clippy::needless_lifetimes)] // False positive.
pub trait GodotDeref<Decl>: GodotClass {
type TargetRef<'a>;
type TargetMut<'a>;
fn get_ref<'a>(gd: &'a Gd<Self>) -> Self::TargetRef<'a>;
fn get_mut<'a>(gd: &'a mut Gd<Self>) -> Self::TargetMut<'a>;
}
#[allow(clippy::needless_lifetimes)] // False positive.
impl<T: GodotClass<Declarer = DeclEngine>> GodotDeref<DeclEngine> for T {
type TargetRef<'a> = &'a Gd<T>;
type TargetMut<'a> = &'a mut Gd<T>;
fn get_ref<'a>(gd: &'a Gd<Self>) -> Self::TargetRef<'a> { gd }
fn get_mut<'a>(gd: &'a mut Gd<Self>) -> Self::TargetMut<'a> { gd }
}
#[allow(clippy::needless_lifetimes)] // False positive.
impl<T: WithBaseField> GodotDeref<DeclUser> for T {
type TargetRef<'a> = GdRef<'a, T>;
type TargetMut<'a> = GdMut<'a, T>;
fn get_ref<'a>(gd: &'a Gd<Self>) -> Self::TargetRef<'a> { gd.bind() }
fn get_mut<'a>(gd: &'a mut Gd<Self>) -> Self::TargetMut<'a> { gd.bind_mut() }
}
It's not that simple, currently we have different behavior for User vs Engine classes, engine classes are provided as parameters "as is", while user classes need to go through
That would be fixed if my |
RustRover had no issues whatsoever, IDEs generally love declarative macros.
I don't think it's worth the hassle, do we have any report of users even wanting such a feature? In the worst case scenario they can just use untyped signals.
In my experience (and based on my journey through the internals of rustc), trait solving takes magnitudes of times longer than parsing. Also, the output of declarative macros is cached and re-used, trait-solving isn't.
I tried this, but we end up with the same problem, just internally.
As mentioned in my answer to yarvin, this issue can be fixed by the |
|
Thanks, that looks very interesting, will look into the Regarding macros vs. traits:
My main worry is that it becomes harder to extend the signal API because you can no longer use generic programming. See also my comment here. Is this not the case? |
I don't see how that makes things harder, it feels like planning for something we don't know that will actually be a problem. (And here I don't even understand what the problem is) Edit: I see what you mean now. #[godot_api]
impl IControl for SettingsUI {
fn ready(&mut self) {
self.close_menu_button
.signals()
.pressed() // ... extension trait on TypedSignal
.my_custom_connect(|this| this.base_mut().hide());
}
That might seen true, since previously you could just implement a generic trait over But you can still do it, just differently: make the extension return a self.close_menu_button
.signals()
.pressed()
.builder_deferred() // extension trait on TypedSignal => just does `connect_builder().flags(ConnectFlags::DEFERRED).object_self()`
.method_mut(|this| this.base_mut().hide()) // regular connect call on the builder
.done(); |
|
Speaking of the builder, I do think we can make it better, in my opinion it overly uses generics right now, and we can get rid of the Here's what I suggest: self.signals().pressed() // unchanged
.connect_builder() // unchanged
.flags(some_flags) // unchanged
.connect(some_static_fn_or_closure); // ends builder
self.signals().pressed() // unchanged
.connect_builder() // unchanged
.flags(some_flags) // unchanged
.connect_self(some_self_method_or_closure); // ends builder
self.signals().pressed() // unchanged
.connect_builder() // unchanged
.flags(some_flags) // unchanged
.connect_other(&other_gd, some_other_method_or_closure); // ends builderThe idea is that we always finish building when receiving the closure. This gets rid of:
The generics only reflect the TypedSignal's generics. (I also suspect we can get rid of the extra lifetime parameter. ) #[must_use]
pub struct ConnectBuilder<'ts, 'c, CSig: WithSignals, Ps> {
parent_sig: &'ts mut TypedSignal<'c, CSig, Ps>,
data: BuilderData,
}Then we only need two impls for the builder. Data impl: #[allow(clippy::needless_lifetimes)] // 'ts + 'c are used conditionally.
impl<'ts, 'c, CSig, Ps> ConnectBuilder<'ts, 'c, CSig, Ps>
where
CSig: WithSignals,
Ps: meta::ParamTuple,
{
pub fn name(mut self, name: impl meta::AsArg<GString>) -> Self {
assert!(
self.data.callable_name.is_none(),
"name() called twice on the same builder."
);
meta::arg_into_owned!(name);
self.data.callable_name = Some(name);
self
}
pub fn flags(mut self, flags: ConnectFlags) -> Self {
assert!(
self.data.connect_flags.is_none(),
"flags() called twice on the same builder."
);
self.data.connect_flags = Some(flags);
self
}
}Connect impls are still generated by the declarative macro, but we mimic TypedSignal's methods:
There is a world where we even merge pub struct TypedSignal<'c, C: WithSignals, Ps> {
object: C::__SignalObj<'c>,
name: Cow<'static, str>,
_signature: PhantomData<Ps>,
data: BuilderData, // gets filled with default values upon creation
}Which would allow you to: self.signals()
.pressed()
.flags(ConnectFlags::Deferred)
.connect_self(|this| foo(this)); |
|
I just implemented everything I suggested in my previous post, and it all works. (Check the tests for examples) |
|
Very cool ideas! 🔭
But that's not the same -- that limits you again to the self.close_menu_button
.signals()
.pressed()
.builder_deferred()
.connect_self(|this| this.base_mut().hide()); // ends builderit's still one more line than with direct extension self.close_menu_button
.signals()
.pressed() // ... extension trait on TypedSignal
.my_custom_connect(|this| this.base_mut().hide());and users cannot further customize it (such as wrap the function call, aspect-style). The user is also required to return a I just want to make sure we're aware of the trade-offs here. Is the only benefit of not having the trait currently that I guess there's always the escape of letting the user write macros -- which would also address the |
You're too fast 😅 I still need to read into all the latest changes, but this sounds already awesome. |
| @@ -446,7 +486,7 @@ mod emitter { | |||
| #[cfg(since_api = "4.2")] | |||
| pub fn connect_signals_internal(&mut self, tracker: Rc<Cell<i64>>) { | |||
| let mut sig = self.signals().signal_int(); | |||
| sig.connect_self(Self::self_receive); | |||
| sig.connect_self(|mut this, int| this.self_receive(int)); | |||
There was a problem hiding this comment.
Is the old syntax of
sig.connect_self(Self::self_receive);no longer possible?
Also, mut is now needed because it's GdMut<T> rather than &mut T?
There was a problem hiding this comment.
Thanks! I wonder if in that case, it wouldn't still be better to provide 2 separate methods connect_ref (for user methods and connect_gd (for any Gd<T>)... 🤔
There was a problem hiding this comment.
Another thing, if you have
fn another_method(emitter: &mut Emitter) {...}you have to write (with GdMut<T>):
sig.connect_self(|mut this, _int| another_method(&mut *this));versus (with &mut T):
sig.connect_self(|this, _int| another_method(this));There was a problem hiding this comment.
I don't think that's a problem at all, and being forbidden from dropping the bind from inside functions has caused many issues for users. The main issue being with IClass trait implementations where methods take self by & or &mut (for example fn ready(&mut self)).
I'd argue that even these trait implementations should use GdMut or GdRef instead of &mut T and &T.
That is outside the scope of this PR of course.
Do you want me to get rid of GdMut and revert to &mut usage?
You wanted to keep the API slim so maybe it doesn't make sense to add a version for both.
There was a problem hiding this comment.
Another thing, if you have
fn another_method(emitter: &mut Emitter) {...}you have to write (with
GdMut<T>):sig.connect_self(|mut this, _int| another_method(&mut *this));versus (with
&mut T):sig.connect_self(|this, _int| another_method(this));
I could not implement GodotDeref with the associated type being the user class, the main issue is that dereferencing into it requires calling bind_mut, but the bind is dropped upon exiting the method:
#[allow(clippy::needless_lifetimes)] // False positive.
impl<T: WithBaseField> GodotDeref<DeclUser> for T {
type GodotTarget = T;
// The bind is dropped upon exiting the method:
// Rustc Error: cannot return value referencing temporary variable.
fn get_ref(gd: &Gd<Self>) -> &Self::GodotTarget {
& *gd.bind()
}
fn get_mut(gd: &mut Gd<Self>) -> &mut Self::GodotTarget {
&mut *gd.bind_mut()
}
}There was a problem hiding this comment.
Do you want me to get rid of
GdMutand revert to &mut usage?
You wanted to keep the API slim so maybe it doesn't make sense to add a version for both.
I'd say yes, because accessing the user object for the full duration of the function is still very common. Having a direct &mut T is just the most ergonomic API in these cases, it also allows directly connecting to already existing methods via method-reference syntax.
I don't think that's a problem at all, and being forbidden from dropping the bind from inside functions has caused many issues for users.
Dropping guards early may be valid in some cases, but passing in GdMut/GdRef seems the wrong API to me: it only allows early release, but not late acquire. Having a function that passes in Gd instead of the guards would give users full flexibility. In the interest of reducing scope creep, I'd suggest we add this (alongside support for engine pointers) at a later point.
There was a problem hiding this comment.
Dropping guards early may be valid in some cases, but passing in GdMut/GdRef seems the wrong API to me: it only allows early release, but not late acquire.
I might have expressed myself poorly, this is not a user issue, it's a implementation one.
I can't make GodotDeref work (with safe code) while making get_mut return &mut T, the borrow checker won't allow. I could use unsafe and just deref the pointer returned by bind_mut.
Getting rid of GodotDeref means connect_self/connect_other methods won't accept both user and engine classes.
There was a problem hiding this comment.
My view here is:
-
✔️
&mut Tparameter is good -
✔️
Gd<T>parameter is good- necessary for engine classes
- also helpful for user classes, to give full flexibility about if/when to bind
-
❌
GdMut<T>parameter is bad- it's strictly less powerful than
Gd<T>in all cases, while still not giving the ergonomics of&mut T - there's also no prior art like this;
#[func(gd_self)]for example changes signature toGdas well
- it's strictly less powerful than
So I think it's better API design to focus on (1) and (2), with (2) being implemented later.
Getting rid of
GodotDerefmeansconnect_self/connect_othermethods won't accept both user and engine classes.
Just a random idea, instead of your get_ref/get_mut approach, would it be possible to use something like
fn with_ref<F, R>(user_fn: F) -> R
where
F: FnMut(&Self::GodotTarget) -> R,
{
user_fn(&*gd.bind())
}If not, that's a pity, but I believe supporting &mut T directly is worth it, even if it means users have to call different connect functions.
There was a problem hiding this comment.
I managed to fix it, already pushed the new commit.
| /// - [`connect()`][Self::connect] for global functions, associated functions or closures. | ||
| /// - [`connect_self()`][Self::connect_self] for methods with `&mut self` as the first parameter. | ||
| /// - [`connect_obj()`][Self::connect_obj] for methods with any `Gd<T>` (not `self`) as the first parameter. | ||
| /// - [`connect_builder()`][Self::connect_builder] for more complex setups. | ||
| /// - [`connect_self()`][Self::connect_self] for methods with `GdMut<Self>` (equivalent to `&mut self`) as the first parameter. | ||
| /// - [`connect_other()`][Self::connect_other] for methods with any `Gd<T>` (not `self`) as the first parameter. | ||
| /// - `connect_sync` for thread-safe functions (allows signal to be called from other threads). |
There was a problem hiding this comment.
This mixes two orthogonal dimensions (receiver, thread-safety) into one.
They're currently not fully orthogonal, since Gd<T> doesn't support cross-thread access at the moment, but once we have multi-threaded object pointers, this would again fall to combinatoric explosion here.
I know the builder isn't nice, but I found it's the only way to properly separate these dimensions and still allow flexibility. It enforced a Send bound which could then be checked against the function, no matter how that function came together.
There was a problem hiding this comment.
What would you like me to change?
There was a problem hiding this comment.
It's more a general question if you see a way forward without having dedicated ConnectBuilder, even for future extensibility? This then also forces every method to be on TypedSignal itself, so we can no longer have a "easy API" and "advanced API".
While I really don't like type-states, it seemed the most scalable/flexible to me. Especially your revamped version of having connect_* at the end (instead of done) would likely be a good middle ground, or how do you see it?
(No need to change anything just now 🙂 )
| /// If not provided, the Rust type name of the function/method is used. | ||
| /// | ||
| /// Only useful before using one of the `connect_**` methods. | ||
| pub fn name<'a: 'b, 'b>(&'a mut self, name: impl meta::AsArg<GString>) -> &'b mut Self { |
There was a problem hiding this comment.
Any particular reason why this doesn't take and return by value?
I don't think we should encourage both
signal.name("hello");
let signal = signal.name("hello");patterns; the functional approach is imo less confusing.
There was a problem hiding this comment.
I confused the signatures and thought that we never owned the signal type at any point, I'll fix it.
There was a problem hiding this comment.
I just checked this and actually, the issue is that signal types (such as SigTreeExiting) turn into TypedSignal by dereferencing, the borrow checker doesn't allow me to move out of the dereference.
There was a problem hiding this comment.
Ah, I had this very problem many times during signal implementation 🧐
Maybe it still makes sense to separate builder and signal. I agree there's some duplication, but we wouldn't need to force a builder pattern onto TypedSignal (and this is just a symptom of that, combined with the Deref hack of course...).
We could separate "basic" and "advanced" APIs and thus make people less overwhelmed when looking at the TypedSignal API. There's also no risk that we run into combinatoric explosion for anything that affects the type itself (like Send, and possibly other traits in the future), as we can just return a differently configured ConnectBuilder in each fluent method.
Houtamelo
force-pushed
the
signal_api_improvements
branch
2 times, most recently
from
6a08fc5 to
aa62e39
Compare
|
I implemented (most - the ones that were possible) changes requested, and squashed all previous commits together. There are now some random CI errors, none of which seem to be related to code that I changed? |
Indeed, I'm attempting to fix them in #1153. |
Bromeon
force-pushed
the
signal_api_improvements
branch
from
aa62e39 to
6ff4ed8
Compare
Houtamelo
force-pushed
the
signal_api_improvements
branch
from
6ff4ed8 to
a442b47
Compare
I think I just force-pushed over your fix |
There was a problem hiding this comment.
Thanks so much for the continuous updates, this starts to look really great 👍
Some comments:
-
Re change
Gd::signals(&mut self)->Gd::signals(&mut self):- I think it's OK, this is anyway not safety-relevant as you can always clone, but I wondered due to discrepancy now between
Object.connect/emit_signalwhich are both&mut self... - Using
&mut selfhere is problematic because it disallows passing in the sameGdas a capture into the closure, or was there another reason? - With
GodotDerefsupportingGdreceivers on engine classes, is this still burning? Maybe for user ones?
- I think it's OK, this is anyway not safety-relevant as you can always clone, but I wondered due to discrepancy now between
-
The problem that macros are much harder to extend than a tuple trait still remains. What do we recommend to users to write their own abstractions over recurring connect patterns? Not just to call multiple builder fns at once, but e.g. wrap the provided user function in their own 🤔
-
Also my previous concerns with
sync()+ possible combinatoric explosion if we add multithreaded support later. But I think your approach is more pragmatic for now, and also this can be extended if there's an actual need.
| #[allow(clippy::needless_lifetimes)] // False positive. | ||
| pub trait GodotDeref<Decl>: GodotClass { | ||
| type TargetRef<'a>: Deref<Target = Self>; | ||
| type TargetMut<'a>: DerefMut<Target = Self>; | ||
|
|
||
| fn get_ref<'a>(gd: &'a Gd<Self>) -> Self::TargetRef<'a>; | ||
| fn get_mut<'a>(gd: &'a mut Gd<Self>) -> Self::TargetMut<'a>; | ||
| } |
There was a problem hiding this comment.
Maybe brief docs what it is about.
I would also name it more specific to this context, e.g. SignalObjectDeref or so...
There was a problem hiding this comment.
⬆️ Could you rename the trait?
I would also name it more specific to this context, e.g.
SignalObjectDerefor so...
There was a problem hiding this comment.
I forgot to explain this, I didn't rename because the trait plays a very similar role to "AsObjectArg" and I was planning to merge them in the future. Renaming right now is fine though.
| /// - [`method_immut`][Self::method_immut]: Connect a `&self` method. | ||
| /// | ||
| /// ## Stage 3 | ||
| /// # Customization | ||
| /// All these methods are optional, and they can be combined. |
There was a problem hiding this comment.
It's very nice that there are no more strict stages, but I think there are still some dependencies on the order, that should maybe be mentioned? Also that the connect_* overloads terminate the builder?
Could probably be kept much shorter than before 🙂
There was a problem hiding this comment.
I did update those docs
There was a problem hiding this comment.
Ah sorry, normally GitHub shows "outdated" when the diff changes. Maybe you could confirm with a " 👍 " or short comment to acknowledge next time 🙂
| // Self-modifying method. | ||
| sig.connect_builder() | ||
| .object_self() | ||
| .method_mut(Emitter::self_receive) | ||
| .done(); | ||
| sig.connect_self(Emitter::self_receive); | ||
|
|
||
| // Connect to other object. | ||
| let receiver_mut = Receiver::new_alloc(); | ||
| sig.connect_builder() | ||
| .object(&receiver_mut) | ||
| .method_mut(Receiver::receive_int_mut) | ||
| .done(); | ||
|
|
||
| // Connect to yet another object, immutable receiver. | ||
| let receiver_immut = Receiver::new_alloc(); | ||
| sig.connect_builder() | ||
| .object(&receiver_immut) | ||
| .method_immut(Receiver::receive_int) | ||
| .done(); |
There was a problem hiding this comment.
These tests have existed to explicitly test the builder, could you add them back?
We should already cover the high-level methods on TypedSignal elsewhere.
There was a problem hiding this comment.
Added them except the immut version which we don't use anymore.
| sig.connect_builder() | ||
| .function(move |i| tracker.set(i)) | ||
| .done(); |
There was a problem hiding this comment.
Also here, this was a deliberate builder test with "associated" function.
| // Check that *other* instance method is invoked. | ||
| assert_eq!( | ||
| receiver_immut.bind().last_received(), | ||
| LastReceived::Int(552), | ||
| "Emit failed (other object, immut method)" | ||
| ); |
There was a problem hiding this comment.
This condition should still be verified.
There was a problem hiding this comment.
We no longer have immut versions of the connect methods.
There was a problem hiding this comment.
Ah, you didn't mention that you would remove them 🤔
As far as I see, ConnectBuilder::connect_other() is hardcoded to GodotDeref::get_mut():
pub fn connect_other<F, R, OtherC, Decl>(self, object: &impl ToSignalObj<OtherC>, mut method: F)
where
F: FnMut(&mut OtherC, $($Ps),*) -> R + 'static,
OtherC: GodotDeref<Decl>,
{
let mut gd = object.to_signal_obj();
let godot_fn = make_godot_fn(move |($($args,)*):($($Ps,)*)| {
let mut target = OtherC::get_mut(&mut gd);
let target_mut = target.deref_mut();
method(target_mut, $($args),*);
});
self.inner_connect_godot_fn::<F>(godot_fn);
}But you also added GodotDeref::get_ref(), which now seems unused?
While users can go the route of |this: &mut Self| { self.method_immut() }, this still requires an exclusive bind on the object and prevents interior mutability. So I think it would be good to offer that functionality, even if in an obscure API gated behind ConnectBuilder.
There was a problem hiding this comment.
But you also added GodotDeref::get_ref(), which now seems unused?
Yeah, I kept it for future usage.
While users can go the route of |this: &mut Self| { self.method_immut() }, this still requires an exclusive bind on the object and prevents interior mutability. So I think it would be good to offer that functionality, even if in an obscure API gated behind ConnectBuilder.
Understood
- `Class::signals()` now takes self by reference (`&self`) instead of `&mut self`, similar changes to the `connect_**` methods. This was introduced to avoid `clone()` boilerplate when a class wants to connect to a signal that belongs to one of their fields.
- Replace trait `SignalReceiver` with direct usage of `FnMut($(Ps,)*)`. This fixes type inference issues with Rustc.
- Standardized `connect` methods in the Signal API (both `TypedSignal` and `ConnectBuilder`), changes:
- `connect`: (unchanged).
- `connect_self`: now accepts both engine and user classes (by using the new trait `GodotDeref`).
- `connect_obj`: renamed to `connect_other`, now accepts both engine and user classes (by using the new trait `GodotDeref`).
- `ConnectBuilder::function`: renamed to `connect`.
- `ConnectBuilder::method_mut`: split into `connect_self` and `connect_other`.
- `ConnectBuilder::method_immut`: removed.
- `ConnectBuilder::done`: removed, building is finalized upon calling one of the `connect_**` methods.
- Removed type-state pattern on ConnectBuilder, and trimmed down generics.
Houtamelo
force-pushed
the
signal_api_improvements
branch
from
a442b47 to
74a11b0
Compare
#[derive(GodotClass)]
struct UIClass {
#[init(node = "Button")]
button: OnReady<Gd<BaseButton>>,
}
#[godot_api]
impl IControl for UIClass {
fn ready(&mut self) {
self.button
.signals() // borrows self by &mut
.pressed()
.connect_other(&self, Self::on_button_pressed); // borrows self by &
}
}If I believe I implemented all the requested changes, should be ready to merge now. |
Thanks for the update! I think you missed some of my comments, marked them again with ⬆️. What about this?
The problem is now also that the docs are very bloated:
All in all, we're enabling to write |
I would argue that this implementation makes it actually easier 🤔, albeit feels very hacky and clunky. One can change arguments into generic tuple and then forward them via decorator. Examples: fn decorate_with_logger<Ps>(mut receiver: impl FnMut(Ps) + 'static) -> impl FnMut(Ps) + 'static {
#[cfg(debug_assertions)]
{
move |tuple| {
godot_print!("Given signal has been called");
receiver(tuple);
}
}
#[cfg(not(debug_assertions))]
receiver
}
fn handle_cleanup_decorator<Ps>(
mut receiver: impl FnMut(Ps, Gd<Node>) + 'static,
) -> impl FnMut(Ps, Gd<Node>) + 'static {
move |tuple, entity| {
if entity.get_name() == StringName::from("oh noes!") {
EventBus::singleton().signals().end_turn();
} else {
receiver(tuple, entity);
}
}
}
// Sample usages
#[godot_api]
impl MyClass {
// A collection of two concrete wrappers.
fn connect_to_button(&self, mut func: impl FnMut() + 'static) {
let mut this_obj = self.to_gd();
self.button.signals().pressed().connect(move || {
godot_print!("A button has been pressed.");
this_obj.bind_mut().something += 1;
func();
});
}
fn connect_to_signal_b(&mut self, mut func: impl FnMut(i32) + 'static) {
self.signals().signal_b().connect(move |int, mut node| {
godot_print!("you shall not pass!");
node.queue_free();
func(int);
});
}
#[signal]
fn signal_b(a: i32, b: Gd<Node>);
#[signal]
fn signal_c(at: Vector2, who: Gd<Node>, idx: u32);
}
// Some complicated connection to signal with no arguments at all.
let this_obj = self.to_gd();
let func = move |_| {
this_obj
.clone()
.signals()
.selected()
.emit(&this_obj.clone())
};
let mut func = decorate_with_logger(func);
// Requires us to pass empty tuple as an argument. `(()))` looks awful
self.connect_to_button(move || func(()));
// Side effect – in this case our Ps is not generic tuple but… i32. Silly!
let func_2 = |i| {
godot_print!("a lone {i}");
};
self.connect_to_signal_b(decorate_with_logger(func_2));
// args to tuple. Again, hard to read
self.signals()
.signal_b()
.connect(|a, b| decorate_with_logger(|(a, b)| godot_print!("{a}, {b}")) ( (a, b) ) );
// Actual usecase in which such approach can be benefical – args to tuple & concrete type
self.signals().signal_c().connect(move |at, who, idx| {
handle_cleanup_decorator(|(at, idx), who| {
godot_print!("{who} has {idx} and is at {at}");
})((at, idx), who);
});
let mut another_generic = decorate_with_logger(|(a, b, c)| {
godot_print!("{a}, {b}, {c}");
});
self.signals()
.signal_c()
.connect(move |a, b, c| another_generic((a, b, c)));Albeit it is hard to say something concrete without solid examples and usecases. ah, and in real-world usage one would probably come out with some macro to improve the readability |
We are also getting:
|
This doesn't work, the closures passed to So you can't abstract over |
|
@Bromeon I won't be able to pick this up again for a indeterminate amount of time. |
Yeah, that's a bit the problem. Once you make your APIs macro-based, i.e. abstractions no longer operate at the type system level, then the user is forced to use macros as well. It's a bit as if a user would want to abstract over But thanks for experimenting with functions and showing possible approaches!
I'm not disputing the other improvements in this PR, the points you mention have very clear benefits 👍 But as far as I understand, they're orthogonal to the fact whether we have
Thanks for the heads-up, and huge thanks for the work! I definitely plan to integrate the PR -- I might experiment with some follow-up tweaks, or at least obtain a better understanding how we could use certain patterns in the future. I'll also try to stay true to the spirit of keeping common usage as non-verbose as possible. One last question maybe, did you consider having the macro-based approach on the |
This is what I meant –- it works in literal sense… // impl FnMut(Ps) -> Ps=(P0, P1, P2)
let mut another_generic = decorate_with_logger(|(a, b, c)| {
godot_print!("{a}, {b}, {c}");
});
self.signals()
.signal_c()
// takes impl FnMut(P0, P1, P2)
.connect(
// Our closure – impl FnMut(P0, P1, P2)
move |a, b, c| {
// Ps = (P0, P1, P2)
let our_param_tuple: (_, _, _) = (a, b, c);
another_generic(our_param_tuple);
},
);
(...)
// FnMut(Ps) – in our case PS = ()
let mut func = decorate_with_logger(func);
// impl FnMut calls FnMut(Ps) – empty tuple ()
self.connect_to_button(move || func(()));
self.signals()
.signal_b()
// takes something with impl FnMut(P0, P1)
.connect(
|a, b|
// takes something with impl FnMut(Ps), in our case Ps = tuple (P01, P1)
decorate_with_logger(
// good luck understanding it imminently – `closure(Ps)` where Ps = (P0, P1)…
// but in general this whole chain is mind-bending (signal function that calls decorated closure, but for some reason it provides all the arguments as a tuple…)
|(a, b)| godot_print!("{a}, {b}")) ( (a, b) )
);… but is hard to read and just feels hacky – in general such abstractions would be rather clunky (by literal sense I mean that this compiles and works) |
I didn't, because:
|
Rustchas a unfortunate limitation where it struggles with type inference when dealing with indirect implementations of theFnfamily traits.Currently, the Signal API uses such an indirection through the trait
SignalReceiver, and this causes compiler fails in case users don't specify the explicit types of receiver parameters on closures passed toconnect_**methods.This PR fixes that by replacing the
SignalReceivertrait with direct usage ofFnMut(**).Example of how this affects user code:
As a side bonus, I also included versions of
connect_**that accept native engine classes as receivers instead of user classes.