-
-
Notifications
You must be signed in to change notification settings - Fork 232
Introduce convenient, type-safe call_deferred alternative #1204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Introduce convenient, type-safe call_deferred alternative #1204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution!
Some first comments:
- You can mark the PR as draft without needing to change the title. This also prevents accidental merging.
- You can do a first attempt of running CI locally with
check.sh
. See book. - I don't see the point of
try_apply_deferred
🙂 any errors can't be returned to the user, so you might as well just haveapply_deferred
returning a generic return typeR
. - Regarding formatting of RustDoc comments, please check how other code does it (line length, header + body separation, etc.)
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1204 |
No reason, just unfamiliarty, so I mirrored Callable::from_local_fn closely at first. Great, than let's drop that and keep just the simple version.
I understand that godot-itest 4.1 compat could break, but I'm surprised that local |
67c8ea7
to
944805d
Compare
I need help with the doc-lint issue :) I don't understand why "it works on my machine" (🫡)... |
944805d
to
b20806a
Compare
That book page states:
In other words, But in your case it's not just notify-docs. The compatibility integration test for Godot 4.1 also fails, because |
3be4a2b
to
613b2df
Compare
Fixed the PR. I guess, to enable the pipeline on my fork is a bit late since I already created the PR. Sorry for triggering your builds so frequently. I'm still curious why I was a bit surprised that doc lint uses stricter pipeline rules tbh. I read the section that you linked, but I unconsiously assumed, that linting wouldn't be affected. My bad. |
I just checked, it's also wrong. The lint probably misses it because Thanks for bringing it up -- I can fix it, currently merging with some other doc issues that came up 🙂 |
c043bed
to
f67b834
Compare
What is the point of returning a generic type |
I meant the closure, not |
Why we would want to bind such function? Wouldn't it cause confusion instead of providing any benefit? |
I was thinking that some methods perform an action and only return an informational return value. But maybe you're right, let's start with |
godot-core/src/obj/gd.rs
Outdated
/// Runs the given Closure deferred. | ||
/// | ||
/// This can be a type-safe alternative to [`classes::Object::call_deferred`], but does not handle dynamic dispatch, unless explicitly used. | ||
/// This constructor only allows the callable to be invoked from the same thread as creating it.The closure receives a reference to this object back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constructor only allows the callable to be invoked from the same thread as creating it.
Note: might not be important in current scope, but call_deferred
always calls from the main thread and is recommended way to "sync" thread with a main thread in Godot (Godot itself even uses it few times, look for something along the lines of _thread_function
).
https://docs.godotengine.org/en/stable/tutorials/performance/thread_safe_apis.html#scene-tree
If you want to call functions from a thread, the call_deferred function may be used (…)
In other words following code:
godot_print!("Starting from the main thread… {:?}", std::thread::current().id());
std::thread::spawn(move || {
godot_print!("Hello from the secondary thread! {:?}", std::thread::current().id());
let c = Callable::from_sync_fn("Thread thread", |_| {
godot_print!("hello from the main thread!, {:?}", std::thread::current().id());
Ok(Variant::nil())
});
c.call_deferred(&[]);
}).join().unwrap();
Would print:
Starting from the main thread… ThreadId(1)
Hello from the secondary thread! ThreadId(3)
hello from the main thread!, ThreadId(1)
Now, since Gd<T>
is NOT thread safe it is debatable if we want to support such operations 🤷. It would open a whole can of worms for sure. (IMO current implementation with from_local_fn
is fine! Gd is not thread safe, thus all the hack/syncs can be done with from_sync_fn
/CustomCallable and should be responsibility of the user… and we have proper, better tools for syncing threads like for example the channels. Just wanted to note it).
The closure receives a reference to this object back.
I'm not sure what does it mean 🤔? Wouldn't it better to ensure that closure/callable keeps reference to a given object (i.e. if it is refcounted it won't be pruned before idle time/before said apply deferred is applied)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should maybe enforce this for now, using this function:
Lines 434 to 441 in 20cd346
/// Check if the current thread is the main thread. | |
/// | |
/// # Panics | |
/// - If it is called before the engine bindings have been initialized. | |
#[cfg(not(wasm_nothreads))] | |
pub fn is_main_thread() -> bool { | |
std::thread::current().id() == main_thread_id() | |
} |
Btw, is_main_thread
should have an implementation for #[cfg(wasm_nothreads)]
returning true
, otherwise clients constantly have to differentiate cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, enforcing it is good idea, since
let mut test_node = DeferredTestNode::new_alloc();
ctx.scene_tree.clone().add_child(&test_node);
let hack = test_node.instance_id();
std::thread::spawn(move || {
let mut obj: Gd<DeferredTestNode> = Gd::from_instance_id(hack);
obj.apply_deferred(|mut this| this.bind_mut().accept());
}).join().unwrap();
Will always fail anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closure receives a reference to this object back.
I'm not sure what does it mean 🤔? Wouldn't it better to ensure that closure/callable keeps reference to a given object (i.e. if it is refcounted it won't be pruned before idle time/before said apply deferred is applied)?
I drop that sentence. I wanted to document, that the callable is actually invoked with slef. Or now after signature
update, the inner self. Should be clear though.
Regarding multi threading
Okay, i was a bit too curious and came up with a new method bind_deferred that returns a closure which is a bit more restricted, but thread safe to call ... i think?
Godot recommends call_deferred for thread safe communication back to the main thread and it seems easier than channels for some cases.
If it is interesting enough, we can add it to this pr, but I guess we shouldn't get side tracked and follow up on this topic in a different one.
Check it out here goatfryed/gdext@feat/1199-type-safe-call-deferred...feat/1199-thread-safe-call-deferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is interesting enough, we can add it to this pr, but I guess we shouldn't get side tracked and follow up on this topic in a different one.
Yeah, I think we shouldn't support it for now 🤔; we should only restrict invocations of apply_deferred
to a main thread (and note that in the description to avoid any confusion).
Dealing with cross-thread access to user-defined GodotClass instances has been discussed here: #18. IMO until we deal with this problem we shouldn't encourage any workflows related to multi threading.
Godot recommends call_deferred for thread safe communication back to the main thread and it seems easier than channels for some cases.
Yep, thus the mention – it is something people would look for naturally, so noting down that it is available only on main thread would avoid any confusion (and we provide tools to use this pattern unsafely).
4503c5c
to
18f1602
Compare
00692f0
to
3e988eb
Compare
I've now introduced I saw three variations
Since the implementation for user and engine defined classes differ only in generic types, I had to introduce two distinct traits. Curious, if there is a better way. The pipeline fails atm for linux_release https://github.com/goatfryed/gdext/actions/runs/15757419567/job/44415859048 |
87c20ec
to
bf1c7a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Since the implementation for user and engine defined classes differ only in generic types, I had to introduce two distinct traits. Curious, if there is a better way.
Could you maybe reuse the ToSignalObj
trait here? It serves a very similar purpose. (I noticed this hasn't been officially made part of the public API, even though it's used in public bounds -- an oversight. Maybe a good opportunity to rename before making public...). Or possibly UniformObjectDeref
?
The pipeline fails atm for linux_release [...]
If I interpret it right, it's due to todays godot 4.5 release and some bc in another part of the code base?
Yes, I rebased and it passed ✔️
#[derive(GodotConvert, Var, Export, Clone, PartialEq, Debug)] | ||
#[godot(via = GString)] | ||
enum TestState { | ||
Initial, | ||
Accepted, | ||
VerifyEngineClass, | ||
FailedEngineClass, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are 4 states really necessary?
Can it not be a boolean that's toggled in a deferred call, and then the test verifies that it's indeed changed? That would also avoid current limitations of enums in typed signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already have the enum, i think it's simpler and cleaner to use it to tell the TestNode to execute case specifiy verifications and ensures proper test setup on the type system. Assuming this won't grow to a lot of cases that also require combinations of varifications.
But the values don't really reflect that. They could more general and more on point.
What do you think about Verify / VerifyWithName / Accepted / Failed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already have the enum, i think it's simpler and cleaner to use it to tell the TestNode to execute case specifiy verifications and ensures proper test setup on the type system.
I'm not following 🤔 I'm sure you have some thoughts about this specific design, but you need to elaborate why it's required for the test. I disagree with "simpler" 🙂
What do you mean with "since we already have the enum"? You add it in this PR. Tests should be as simple as possible, with minimal logic besides the property being tested. Here, we are just interested in two properties:
- the deferred function is not called immediately
- the deferred function is called eventually
I don't see why 4 states are needed for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how do you check whether some function was called?
For user defined classes this is easy. We define a function that modifies some state dedicated for the test setup and we are happy.
For Engine Defined classes we need to perform some side effect that is then evaluated on the next frame. I've decided to modify the name and now I need some logic in my TestNode to check this different side effect and a way to trigger this different assertion.
The forth tests needs different test logic. The assertion must be done on the next frame. Yes I introduce the enum in this PR, but if I introduce it, it seems fitting to use it rather than a boolean to switch assertion behavior.
That's why i need multiple test states. If you prefer,, I can replace the Enum with a two booleans or keep the boolean and add one boolean. Personally, this seemed most expressive to me and a good thing to warn you that there are actual multiple test states due to multiple test setups.
Note that the implementation is actually different for engine and user defined classes. Of course this might change with your suggestion to check out UniformObjectDeref. I'm still working on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the test and removed the enum. Should be easier now.
bf1c7a2
to
7f1e1d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
What do you think about reusing existing traits?
godot-core/src/obj/call_deferred.rs
Outdated
/// This can be a type-safe alternative to [`crate::classes::Object::call_deferred`], but does not handle dynamic dispatch, unless explicitly used. | ||
/// This must be used on the main thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with dynamic dispatch in this context? And how can the user "explicitly use" it? Please be specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What i wanted to express: I crate::classes::Object::call_deferred
allows that the deferred method can be overwritten by GDScript, because it's dynamically dispatched at runtime, while Gd::apply_deferred
method calls dispatch statically unless Gd::callv
is used.
I removed the line. This is covered in the rust godot book and should be clear with these kind of methods.
Hey thank you for the quick feedback. I'm still working on it. Maybe i should push a bit less or I should indicate, if it's reviewable again. But I don't mind the fast feedback. Just let me know what is best for you. I took a look at the existing traits and it seems promising, but I can't make it work yet. I seem to run into reoccuring issues. As I mentioned, I'm learning and this teaches me a lot about Rust type system. So it's exciting. ^^ It seems that the following implementation would work for both engine and user defined classes, but is obviously illegal rust. impl<T, S, Declarer> DeferredUserCallable<T> for S // Declarer E0207, not bound
where
T: GodotClass + UniformObjectDeref<Declarer>, // works for both concrete bounds:.DeclEngine, bounds::DeclUser
S: ToSignalObj<T>,
{
fn apply_deferred<'a,F>(&mut self, mut rust_function: F)
where
F: FnMut(&mut T) + 'static,
{
assert!(
is_main_thread(),
"apply_deferred must be called on main thread"
);
let mut this = self.to_signal_obj().clone();
let callable = Callable::from_local_fn("apply_deferred", move |_| {
rust_function(T::object_as_mut(&mut this).deref_mut());
Ok(Variant::nil())
});
callable.call_deferred(&[]);
}
} I'm currently looking into associated types to solve this, and don't feel blocked yet. But I wouldn't mind a tip, if i'm overlooking something obvious 😄 |
Yes, feel free to switch it to draft mode if you're not ready.
Maybe we can reduce the scope and provide only callbacks with Generally we need to be careful about our API complexity budget. Two new trait only to power this one function is too much in my opinion. |
@goatfryed to get the trait bound to work, you have to define it like this: impl<T, S, D: Declarer> DeferredUserCallable<T> for S
where
T: GodotClass<Declarer = D> + UniformObjectDeref<D>,
S: ToSignalObj<T> But impl<T, S, D: Declarer> DeferredUserCallable<T> for S
where
T: UniformObjectDeref<D, Declarer = D>,
S: ToSignalObj<T> |
c7e9af5
to
0b9f295
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, now the implementation looks nice and avoids multiple traits. Thank you for your patience and support 🙏 I learned a lot.
- added the trait to prelude
- cleaned up some last issues regarding pre 4.1 support stemming from that
Just some observation: For DynGd<C,T>
this calls the closure with &C
.
I did not rename ToSingnalObject in this, since i had some issues coming up with a good name that doesn't introduce confusion or overlap.
I see some overlap in both naming and signature with WithBaseField and the need for a clear separation from ToGodot
.
I'd propose to rename ToSingnalObject
to ToGd
, rename the method to to_gd()
and have WithBaseField extend this trait.
Sounds like an opportunity for another PR
/// | ||
/// # Usage | ||
/// | ||
/// ```no_compile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use a realisitic example here, but Engine classes are not available in core docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are, see examples like here: https://godot-rust.github.io/docs/gdext/master/godot/obj/struct.Gd.html#method.upcast_ref
Might work better if the types are among the "minimal set" of classes (Node
, Resource
, RefCounted
etc) 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for classes included in prelude it works, but import of godot::classes fails. Should I change the example, investigate the issue or is this fine for now?
godot-core/src/obj/call_deferred.rs
Outdated
{ | ||
assert!( | ||
is_main_thread(), | ||
"`apply_deferred` must be called on the main thread." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the same rule regarding comments apply to assert statements as well? I noticed different formats in the code base, which is expected from a growing project. Let me know what the preferred style is and if you'd like me to clean them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for asserts we follow mostly Rust style (lowercase, no period) -- except in case of multiple sentences.
But you're right it's not very consistent right now.
We can't rename it right now, as we keep compatibility with v0.3. Can you add the following comment? // TODO(v0.4): find more general name for trait. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI ran into a segfault on macOS before. I wonder if it's related to a change in this PR.
godot-core/src/obj/call_deferred.rs
Outdated
{ | ||
assert!( | ||
is_main_thread(), | ||
"`apply_deferred` must be called on the main thread." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for asserts we follow mostly Rust style (lowercase, no period) -- except in case of multiple sentences.
But you're right it's not very consistent right now.
/// | ||
/// # Usage | ||
/// | ||
/// ```no_compile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are, see examples like here: https://godot-rust.github.io/docs/gdext/master/godot/obj/struct.Gd.html#method.upcast_ref
Might work better if the types are among the "minimal set" of classes (Node
, Resource
, RefCounted
etc) 🙂
godot-core/src/obj/call_deferred.rs
Outdated
pub trait WithDeferredCall<T: GodotClass> { | ||
/// Runs the given Closure deferred. | ||
/// | ||
/// This can be a type-safe alternative to [`crate::classes::Object::call_deferred`]. This method must be used on the main thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// This can be a type-safe alternative to [`crate::classes::Object::call_deferred`]. This method must be used on the main thread. | |
/// This can be a type-safe alternative to [`Object::call_deferred()`][crate::classes::Object::call_deferred]. This method must be used on the main thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this would cause the doc test to fail unless i use Object in the module, which in turn would require me to ignore unused warning. Or am I mistaken? I fully qualified this due to failing doc test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to [text][ref]
syntax -- this one should work.
Hm... i ran a full ci on my repository yesterday evening and mac passed in all 5 architectures. Can you run a test on main to rule out some issue with the godot 4.5 beta, before i start digging deeper? |
I checked, it's unrelated and happens on other branches too. Looks like Godot broke something 😔 |
751770c
to
88428b1
Compare
apply_deferred
Implements #1199
I'm note entirely sold on my api design here, but I guess test and partial implementation are a good next step to explore the desired outcome. See comments in code
adds test for call_deferred
This seems like a cyclic test expectation, because async_test uses call_deferred internally and now call_deferred test uses async_test internally, but it was the first best idea i came up with, that wouldn't require changes to the test engine and allows me to await the idle time of the current frame.
todo
Depending on the discussion of the two loosely coupled topics here, feel free to cherry pick the test improvements or let just ask me to create a dedicated PR for that.