Skip to content
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

Single- and multi-threading support for Callable::from_fn #588

Open
MatrixDev opened this issue Jan 31, 2024 · 3 comments
Open

Single- and multi-threading support for Callable::from_fn #588

MatrixDev opened this issue Jan 31, 2024 · 3 comments
Labels
c: core Core components feature Adds functionality to the library

Comments

@MatrixDev
Copy link

Callable::from_fn requires Send + Sync from closures but Gd<T> is not. I saw in few bugs Callable::from_fn is mentioned as an alternative to using function name Base::callable / Callable::from_object_method (which are not type safe because function name can be misspelled) but it doesn't really work.

Basically this limitation limits usefulness of Callable::from_fn quite a bit.

Possibly related to #18 but usecase is different. Maybe Callable can be adjusted instead of Gd<T>.

Code

#[derive(GodotClass)]
#[class(init, base = Node3D)]
pub struct TestNode {
    base: Base<Node3D>,
    #[export]
    button: Option<Gd<Button>>,
}

#[godot_api]
impl TestNode {
    fn on_clicked(&mut self) {}
}

#[godot_api]
impl INode3D for TestNode {
    fn ready(&mut self) {
        if let Some(mut button) = &self.button {
            let mut this = self.to_gd();
            let callback = Callable::from_fn("callback", move |args| {
                this.bind_mut().on_clicked();
                Ok(Variant::nil())
            });
            button.connect("pressed".into(), callback);
        }
    }
}

Error

error[E0277]: `*mut TestNode` cannot be sent between threads safely
   --> src/nodes/screens/ship_edit.rs:101:56
    |
101 |               let callback = Callable::from_fn("sensor", move |args| {
    |                              -----------------           ^----------
    |                              |                           |
    |  ____________________________|___________________________within this `{closure@src/nodes/screens/ship_edit.rs:101:56: 101:67}`
    | |                            |
    | |                            required by a bound introduced by this call
102 | |                 this.bind_mut().on_clicked();
103 | |                 Ok(Variant::nil())
104 | |             });
    | |_____________^ `*mut TestNode` cannot be sent between threads safely
    |
    = help: within `{closure@src/nodes/screens/ship_edit.rs:101:56: 101:67}`, the trait `Send` is not implemented for `*mut TestNode`
note: required because it appears within the type `RawGd<TestNode>`
   --> /Users/matrixdev/.cargo/git/checkouts/gdext-76630c89719e160c/df302ea/godot-core/src/obj/raw.rs:30:12
    |
30  | pub struct RawGd<T: GodotClass> {
    |            ^^^^^
note: required because it appears within the type `Gd<TestNode>`
   --> /Users/matrixdev/.cargo/git/checkouts/gdext-76630c89719e160c/df302ea/godot-core/src/obj/gd.rs:87:12
    |
87  | pub struct Gd<T: GodotClass> {
    |            ^^
note: required because it's used within this closure
   --> src/nodes/screens/ship_edit.rs:101:56
    |
101 |             let callback = Callable::from_fn("sensor", move |args| {
    |                                                        ^^^^^^^^^^^
note: required by a bound in `godot::prelude::Callable::from_fn`
   --> /Users/matrixdev/.cargo/git/checkouts/gdext-76630c89719e160c/df302ea/godot-core/src/builtin/callable.rs:95:22
    |
93  |     pub fn from_fn<F, S>(name: S, rust_function: F) -> Self
    |            ------- required by a bound in this associated function
94  |     where
95  |         F: 'static + Send + Sync + FnMut(&[&Variant]) -> Result<Variant, ()>,
    |                      ^^^^ required by this bound in `Callable::from_fn`
@Bromeon
Copy link
Member

Bromeon commented Jan 31, 2024

Callable::from_fn requires Send + Sync from closures but Gd<T> is not.

This is mostly a direct consequence of #18 not yet being implemented.

Mid-term, I see the option to have two functions:

  1. Callable::from_unsync_fn()

    • This would record the thread ID on creation and only allow execution on the same thread.
    • Even if called from GDScript, it could simply skip the user code and print an error if invoked from a different thread.
    • It would not require Send/Sync.
  2. Callable::from_sync_fn()

    • Requires Send/Sync and can be called from any thread.
    • To be actually useful, it needs threading support of objects.

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Jan 31, 2024
@Bromeon Bromeon changed the title Callable::from_fn is not usable for connecting signals Single- and multi-threading support for Callable::from_fn Jan 31, 2024
@Houtamelo
Copy link
Contributor

@Bromeon I'm working on the Signal support PR, and while testing my design I remembered that callable requiring Send + Sync essentially means you can't interact with Godot from inside them, which extremely limits their usefulness.

In my experience, 95% of the time callables were used to connect to signals, and so I had to make this workaround that bypasses the Send + Sync: https://github.com/Houtamelo/houtamelo_utils_gdext/blob/main/src/connect_with_deferred.rs. I use it every single time I want to connect to a signal because otherwise there isn't much I can do with the Send + Sync bounds in place.

I think it would be reasonable if we at least had a from_fn_unchecked that's marked with unsafe.

@Bromeon
Copy link
Member

Bromeon commented Oct 9, 2024

I think it would be reasonable if we at least had a from_fn_unchecked that's marked with unsafe.

Fair point, maybe it's worth providing this as an interim solution until we have a solid design. We should probably explicitly state in the docs that such an API will be removed once there is a proper solution.

@Houtamelo Houtamelo mentioned this issue Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

No branches or pull requests

3 participants