Skip to content

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions godot-core/src/obj/call_deferred.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright (c) godot-rust; Bromeon and contributors.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/
use crate::builtin::{Callable, Variant};
use crate::meta::UniformObjectDeref;
use crate::obj::bounds::Declarer;
use crate::obj::GodotClass;
#[cfg(since_api = "4.2")]
use crate::registry::signal::ToSignalObj;
use godot_ffi::is_main_thread;
use std::ops::DerefMut;

// Dummy traits to still allow bounds and imports.
#[cfg(before_api = "4.2")]
pub trait WithDeferredCall<T: GodotClass> {}

/// Trait that is automatically implemented for engine classes and user classes containing a `Base<T>` field.
///
/// This trait enables type safe deferred method calls.
///
/// # Usage
///
/// ```no_compile
Copy link
Author

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

Copy link
Member

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

Copy link
Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this? Prelude is just re-exporting other symbols, as long as they're in godot-core I don't see how that would matter...

What might matter is the "minimal set" I mentioned above. If you can find an example using classes in that list, that might fix it.

/// # use godot::prelude::*;
/// # use godot::classes::CollisionShape2D;
/// # use std::f32::consts::PI;
/// fn some_fn(mut shape: Gd<CollisionShape2D>)
/// {
/// shape.apply_deferred(|shape_mut| shape_mut.rotate(PI))
/// }
/// ```
#[cfg(since_api = "4.2")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole file is pointless before Godot 4.2, so this should rather be an inner attribute #![cfg(...)] at the start of the file (after license header).

pub trait WithDeferredCall<T: GodotClass> {
/// Runs the given Closure deferred.
///
/// 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.
fn apply_deferred<F>(&mut self, rust_function: F)
where
F: FnMut(&mut T) + 'static;
}

#[cfg(since_api = "4.2")]
impl<T, S, D: Declarer> WithDeferredCall<T> for S
where
T: UniformObjectDeref<D, Declarer = D>,
S: ToSignalObj<T>,
Comment on lines +45 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D bound should also be in the where list.

Suggested change
#[cfg(since_api = "4.2")]
impl<T, S, D: Declarer> WithDeferredCall<T> for S
where
T: UniformObjectDeref<D, Declarer = D>,
S: ToSignalObj<T>,
#[cfg(since_api = "4.2")]
impl<T, S, D> WithDeferredCall<T> for S
where
T: UniformObjectDeref<D, Declarer = D>,
S: ToSignalObj<T>,
D: Declarer,

{
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 the 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(&[]);
}
}
3 changes: 3 additions & 0 deletions godot-core/src/obj/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//! * [`Gd`], a smart pointer that manages instances of Godot classes.

mod base;
mod call_deferred;
mod dyn_gd;
mod gd;
mod guards;
Expand All @@ -24,6 +25,7 @@ mod traits;
pub(crate) mod rtti;

pub use base::*;
pub use call_deferred::WithDeferredCall;
pub use dyn_gd::DynGd;
pub use gd::*;
pub use guards::{BaseMut, BaseRef, DynGdMut, DynGdRef, GdMut, GdRef};
Expand All @@ -35,6 +37,7 @@ pub use traits::*;

pub mod bounds;
pub mod script;

pub use bounds::private::Bounds;

// Do not re-export rtti here.
Expand Down
1 change: 1 addition & 0 deletions godot-core/src/registry/signal/typed_signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use std::borrow::Cow;
use std::marker::PhantomData;
use std::ops::DerefMut;

// TODO(v0.4): find more general name for trait.
/// Object part of the signal receiver (handler).
///
/// Functionality overlaps partly with [`meta::AsObjectArg`] and [`meta::AsArg<ObjectArg>`]. Can however not directly be replaced
Expand Down
1 change: 0 additions & 1 deletion godot-core/src/task/async_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ pub fn spawn(future: impl Future<Output = ()> + 'static) -> TaskHandle {
// By limiting async tasks to the main thread we can redirect all signal callbacks back to the main thread via `call_deferred`.
//
// Once thread-safe futures are possible the restriction can be lifted.
#[cfg(not(wasm_nothreads))]
assert!(
crate::init::is_main_thread(),
"godot_task() can only be used on the main thread"
Expand Down
11 changes: 9 additions & 2 deletions godot-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -435,9 +435,16 @@ pub fn main_thread_id() -> std::thread::ThreadId {
///
/// # 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()
#[cfg(not(wasm_nothreads))]
{
std::thread::current().id() == main_thread_id()
}

#[cfg(wasm_nothreads)]
{
true
}
}

// ----------------------------------------------------------------------------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions godot/src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,6 @@ pub use super::obj::EngineEnum as _;
pub use super::obj::NewAlloc as _;
pub use super::obj::NewGd as _;
pub use super::obj::WithBaseField as _; // base(), base_mut(), to_gd()
pub use super::obj::WithDeferredCall as _;
pub use super::obj::WithSignals as _; // Gd::signals()
pub use super::obj::WithUserSignals as _; // self.signals()
113 changes: 113 additions & 0 deletions itest/rust/src/object_tests/call_deferred_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright (c) godot-rust; Bromeon and contributors.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/
use crate::framework::itest;
use godot::obj::WithBaseField;
use godot::prelude::*;
use godot::task::{SignalFuture, TaskHandle};
use std::ops::DerefMut;

const ACCEPTED_NAME: &str = "touched";

#[derive(GodotClass)]
#[class(init,base=Node2D)]
struct DeferredTestNode {
base: Base<Node2D>,
}

#[godot_api]
impl DeferredTestNode {
#[signal]
fn test_completed(name: StringName);

#[func]
fn gd_accept(&mut self) {
self.rust_accept();
}

fn rust_accept(&mut self) {
self.base_mut().set_name(ACCEPTED_NAME);
}
Comment on lines +26 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of having two functions is probably to make it explicit which one is invoked via reflection, and which one via Rust direct calls?

If yes, maybe add brief comment before the first one 🙂


fn as_expectation_task(&self) -> TaskHandle {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The as_ convention is used for references.

And I'm not sure if "expectation" is really clear in this context -- maybe to_testable_task?

assert_ne!(
self.base().get_name().to_string(),
ACCEPTED_NAME,
"accept evaluated synchronously"
);

let test_will_succeed: SignalFuture<(StringName,)> =
Signal::from_object_signal(&self.to_gd(), "test_completed").to_future();

godot::task::spawn(async move {
let (name,) = test_will_succeed.await;

assert_eq!(name.to_string(), ACCEPTED_NAME);
})
}
}

#[godot_api]
impl INode2D for DeferredTestNode {
fn process(&mut self, _delta: f64) {
let name = self.base().get_name();
self.signals().test_completed().emit(&name);
self.base_mut().queue_free();
}

fn ready(&mut self) {
self.base_mut().set_name("verify")
}
}

#[itest(async)]
fn call_deferred_untyped(ctx: &crate::framework::TestContext) -> TaskHandle {
let mut test_node = DeferredTestNode::new_alloc();
ctx.scene_tree.clone().add_child(&test_node);

test_node.call_deferred("gd_accept", &[]);

let handle = test_node.bind().as_expectation_task();
handle
}

#[itest(async)]
fn call_deferred_godot_class(ctx: &crate::framework::TestContext) -> TaskHandle {
let mut test_node = DeferredTestNode::new_alloc();
ctx.scene_tree.clone().add_child(&test_node);

let mut gd_mut = test_node.bind_mut();
// Explicitly check that this can be invoked on &mut T.
let godot_class_ref: &mut DeferredTestNode = gd_mut.deref_mut();
godot_class_ref.apply_deferred(DeferredTestNode::rust_accept);
drop(gd_mut);

let handle = test_node.bind().as_expectation_task();
handle
}

#[itest(async)]
fn call_deferred_gd_user_class(ctx: &crate::framework::TestContext) -> TaskHandle {
let mut test_node = DeferredTestNode::new_alloc();
ctx.scene_tree.clone().add_child(&test_node);

test_node.apply_deferred(DeferredTestNode::rust_accept);

let handle = test_node.bind().as_expectation_task();
handle
}

#[itest(async)]
fn call_deferred_gd_engine_class(ctx: &crate::framework::TestContext) -> TaskHandle {
let test_node = DeferredTestNode::new_alloc();
ctx.scene_tree.clone().add_child(&test_node);

let mut node = test_node.clone().upcast::<Node>();
node.apply_deferred(|that_node| that_node.set_name(ACCEPTED_NAME));

let handle = test_node.bind().as_expectation_task();
handle
}
3 changes: 3 additions & 0 deletions itest/rust/src/object_tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
mod base_test;
mod class_name_test;
mod class_rename_test;
// Test code depends on task API, Godot 4.2+.
#[cfg(since_api = "4.2")]
mod call_deferred_test;
mod dyn_gd_test;
mod dynamic_call_test;
mod enum_test;
Expand Down
Loading