Type-safe call_deferred alternative

[goatfryed]

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

• Finalize signature
• Expose the same signature on GodotClass. I'm getting familiar with the code base
• Improve docs. Thankful for any pointers what I should update, once this is finalized

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.

[Bromeon]

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 have apply_deferred returning a generic return type R.
• Regarding formatting of RustDoc comments, please check how other code does it (line length, header + body separation, etc.)

[GodotRust]

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

[goatfryed]

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 have apply_deferred returning a generic return type R.

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.

You can do a first attempt of running CI locally with check.sh. See book.

I understand that godot-itest 4.1 compat could break, but I'm surprised that local ./check.sh doesn't catch doc-lint issues. I assume I just need to use the linked type symbol to fix this?

[goatfryed]

I need help with the doc-lint issue :) I don't understand why "it works on my machine" (🫡)...
I see that L289 references Object as well and there it works.

[Bromeon]

but I'm surprised that local ./check.sh doesn't catch doc-lint issues.
[...]
I need help with the doc-lint issue :) I don't understand why "it works on my machine" (🫡)...

That book page states:

The script check.sh in the project root can be used to mimic a minimal version of CI locally.

[...]

If you want to have the full CI experience, you can experiment as much as you like on your own gdext fork, before submitting a pull request.

In other words, check.sh is just a first sanity check. The authority on correctness is the GitHub Actions pipeline. And you can enable GitHub Actions on your repo fork to see the CI status, it's all described on that page 😉

But in your case it's not just notify-docs. The compatibility integration test for Godot 4.1 also fails, because signals() isn't available there. You need to #[cfg(since_api = "4.2")] your test/module out if you don't want it to run under 4.1, see other tests.

[goatfryed]

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 Object::get_class() is fine for L289, but well, I guess i can just qualify my link. Should i qualify L289 as well while I'm at it?

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.

[Bromeon]

I'm still curious why Object::get_class() is fine for L289, but well, I guess i can just qualify my link. Should i qualify L289 as well while I'm at it?

I just checked, it's also wrong. The lint probably misses it because Gd::dynamic_class_string() is a private method.

Thanks for bringing it up -- I can fix it, currently merging with some other doc issues that came up 🙂

[Houtamelo]

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](https://godot-rust.github.io/book/contribute/dev-tools.html).

* 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 have `apply_deferred` returning a generic return type `R`.

* Regarding formatting of RustDoc comments, please check how other code does it (line length, header + body separation, etc.)

What is the point of returning a generic type R when the return value of the function is always discarded? Why not just return () (nothing) ?

[Bromeon]

I meant the closure, not apply_deferred itself. It would allow binding functions that have a return value (which is discarded).

[Houtamelo]

I meant the closure, not apply_deferred itself. It would allow binding functions that have a return value (which is discarded).

Why we would want to bind such function? Wouldn't it cause confusion instead of providing any benefit?

[Bromeon]

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 () only -- after all, it's a non-breaking change to extend this in the future 👍

[Yarwin]

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)?

[Bromeon]

We should maybe enforce this for now, using this function:

gdext/godot-ffi/src/lib.rs

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.

[Yarwin]

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.

[goatfryed]

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

[Yarwin]

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).

[goatfryed]

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

[Bromeon]

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.

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.

[Bromeon]

CI ran into a segfault on macOS before. I wonder if it's related to a change in this PR.

[goatfryed]

CI ran into a segfault on macOS before. I wonder if it's related to a change in this PR.

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?

[Bromeon]

I checked, it's unrelated and happens on other branches too. Looks like Godot broke something 😔

[TitanNano]

Could you clearly document the panic conditions like it has been done in other places?

gdext/godot-core/src/obj/on_ready.rs

Lines 198 to 203 in 4902b34

	/// Runs manual initialization.
	///
	/// # Panics
	/// - If `init()` was called before.
	/// - If this object was already provided with a closure during construction, in [`Self::new()`] or any other automatic constructor.

[goatfryed]

This is just about main thread condition, right? If I'm not missing any nested conditions, any potential internal panic conditions of callable should be safe and hidden from the user, I think.

Fixed the format

[TitanNano]

yes I didn't notice anything else.

[goatfryed]

Hm.. i've been experimenting with this feature a bit in my own project and I think F: FnOnce() + 'static would be more ergonomic than the current F: FnMut(&mut T) + 'static signature, because it avoids unnecessary clones of moved gd references.

What do you think? Should we update the signature?

Callable can't guarantee FnOnce, but here we can do something like

        let mut rust_fn_once = Some(Box::new(rust_fn_once));
        let mut this = self.to_signal_obj().clone();
        let callable = Callable::from_local_fn("apply_deferred", move |_| {
            // safe because we control that callable is invoked only once
            rust_fn_once.take().unwrap()(T::object_as_mut(&mut this).deref_mut());
            Ok(Variant::nil())
        });
        callable.call_deferred(&[]);

unless i'm missing a potential issue here.

[Bromeon]

FnOnce sounds generally like a good idea. Less restrictive bound, if we can indeed guarantee that Object::call_deferred() only runs once (which it should, according to docs).

The one issue it does have is that we can no longer run call_deferred(Self::deferred_method) 🤔

Regarding your code snippet, do you need the extra box? Also, please use expect() instead of unwrap() + comment, and use variables for the function and its arguments (for clarity) 🙂

[goatfryed]

The one issue it does have is that we can no longer run call_deferred(Self::deferred_method)

Do you mean apply_deferred or something else? Unless i misunderstand you, this is still possible. Test call_deferred_gd_user_class does it and doesn't require modification.