Experiment with splitting up signature differently by lilizoey · Pull Request #1042 · godot-rust/gdext

lilizoey · 2025-02-09T18:45:42Z

VarcallSignatureTuple and PtrcallSignatureTuple are now a struct Signature<Params, Ret>. The behavior of the params are in a new set of traits: ParamList, InParamList and OutParamList.

This split allows input parameters and output parameters to be treated differently, for instance this makes it so that only FromGodot is needed for user-defined functions.

Should probably be coordinated with #1000 if we actually want to add it, as both this PR and that PR adds a generic type for parameter tuples.

Currently I've just added the new signature stuff to a separate file, rather than overwriting signature.rs. Mostly to make it easier to read here in the diff and such. This would be changed if we go with this. This needs a lot of cleanup in general lol.

But i wanna at least run the minimal CI on it here and get feedback on if this is the strategy we want to go for before i like polish it up. Since there are other strategies we could go with. The main concrete issue this really solves is splitting up parameters based on if they're in an in vs out call.

I'm not entirely sure if this is a breaking change, i think it isnt? It should just make code that used to not compile, compile. And it doesn't change any public symbols.

GodotRust · 2025-02-09T18:55:33Z

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

lilizoey · 2025-03-23T22:14:06Z

Im gonna do another pass at cleaning up some minor things later, but all the functionality is in place so im marking it ready for review

Bromeon

Thanks a lot! 👍

This change now leaks a new type to the public API:

fn param_info(
    index: usize,
    param_name: &str
) -> Option<MethodParamOrReturnInfo>

However MethodParamOrReturnInfo is not public.

What's the general idea of APIs, in terms of what's public?

Bromeon · 2025-03-24T17:50:10Z

godot-core/src/task/futures.rs

 }

-impl<C: WithBaseField, R: ParamTuple + Sync + Send> IntoFuture for &TypedSignal<'_, C, R> {
+impl<C: WithBaseField, R: InParamTuple + Sync + Send + 'static> IntoFuture


Any reason why this adds 'static bound?
Also in some other places...

InParamTuple can maybe not be 'static itself, if RefArg or so is used?

It adds a 'static bound because ParamTuple had a 'static bound before and TypedSignal relied on that.

And yeah, i don't remember exactly where but there are cases where ParamTuple can't be 'static.

Bromeon · 2025-03-24T17:52:40Z

godot-core/src/meta/param_tuple/impls.rs

+unsafe_impl_param_tuple!(13; (p0, 0): P0, (p1, 1): P1, (p2, 2): P2, (p3, 3): P3, (p4, 4): P4, (p5, 5): P5, (p6, 6): P6, (p7, 7): P7, (p8, 8): P8, (p9, 9): P9, (p10, 10): P10, (p11, 11): P11, (p12, 12): P12);
+unsafe_impl_param_tuple!(14; (p0, 0): P0, (p1, 1): P1, (p2, 2): P2, (p3, 3): P3, (p4, 4): P4, (p5, 5): P5, (p6, 6): P6, (p7, 7): P7, (p8, 8): P8, (p9, 9): P9, (p10, 10): P10, (p11, 11): P11, (p12, 12): P12, (p13, 13): P13);
+
+// Manually implement for () so we dont have to add a bunch of #[allow(..)] above for the 0-length case.


I'd rather have such #[allow]s than manually duplicating code that can be generated by the macro.

They can also cover an entire impls module or so.

i mainly dont like it because it can be useful to have those lints for the code, though i think it may be possible to generate them only when necessary?

Yes, the question is how much indirection this adds if it's handled inside the macro. Having complex de-linting logic may be overkill.

Otherwise, would this potentially work?

#[allow(bla)] mod impl0 { // macro invocation for 0 } mod impls { // others }

Bromeon · 2025-03-24T17:53:19Z

godot-core/src/meta/param_tuple/impls.rs

+use crate::meta::{signature, CallContext};
+use crate::meta::{
+    FromGodot, GodotConvert, GodotFfiVariant, GodotType, InParamTuple, OutParamTuple, ParamTuple,
+    ToGodot,
+};


These 2 can be combined 🙂

Bromeon · 2025-03-24T17:58:52Z

godot-core/src/meta/param_tuple/impls.rs

+                format!(
+                    // This repeat expression is basically just `"{$p:?}"`, the rest is only needed so that
+                    // the repetition separator can be `", "` instead of `,`.
+                    concat!("" $(, "{", stringify!($p), ":?}" ,)", "*),
+                    $($p=$p),*
+                )


Would it not be easier to use positional format args?

Then you don't need stringify!($p) and the expression is already much simpler.

oh yeah true, i didnt think of that actually

Bromeon · 2025-03-24T17:59:17Z

godot-core/src/meta/param_tuple.rs

+/// As an example, this would be used to call Godot functions through ffi, however this is _not_ used when Godot calls a user-defined
+/// function.


Suggested change

/// As an example, this would be used to call Godot functions through ffi, however this is _not_ used when Godot calls a user-defined

/// function.

/// As an example, this would be used to call Godot functions through FFI, however this is _not_ used when Godot calls a user-defined

/// function.

Bromeon · 2025-03-24T18:04:42Z

godot-core/src/registry/method.rs

 /// Info relating to an argument or return type in a method.
 pub struct MethodParamOrReturnInfo {
-    info: PropertyInfo,
+    pub info: PropertyInfo,


Making this pub is intended?

See also main comment about MethodParamOrReturnInfo, which is currently not public. If it becomes public, we should probably think about the encapsulation.

Bromeon · 2025-03-24T18:08:42Z

godot-core/src/registry/signal/typed_signal.rs

+    /// Fully customizable connection setup.
+    ///
+    /// The returned builder provides several methods to configure how to connect the signal. It needs to be finalized with a call to
+    /// [`ConnectBuilder::done()`].
+    pub fn connect_builder(&mut self) -> ConnectBuilder<'_, 'c, C, (), Ps, ()> {
+        ConnectBuilder::new(self)
+    }
+
+    /// Directly connect a Rust callable `godot_fn`, with a name based on `F`.
+    ///
+    /// This exists as a short-hand for the connect methods on [`TypedSignal`] and avoids the generic instantiation of the full-blown
+    /// type state builder for simple + common connections, thus hopefully being a tiny bit lighter on compile times.
+    fn inner_connect_godot_fn<F>(
+        &mut self,
+        godot_fn: impl FnMut(&[&Variant]) -> Result<Variant, ()> + 'static,
+    ) {
+        let callable_name = make_callable_name::<F>();
+        let callable = Callable::from_local_fn(&callable_name, godot_fn);
+
+        let signal_name = self.name.as_ref();
+        self.owner.with_object_mut(|obj| {
+            obj.connect(signal_name, &callable);
+        });
+    }
+
+    /// Connect an untyped callable, with optional flags.
+    ///
+    /// Used by [`ConnectBuilder::done()`]. Any other type-state (such as thread-local/sync, callable debug name, etc.) are baked into
+    /// `callable` and thus type-erased into runtime logic.
+    pub(super) fn inner_connect_untyped(
+        &mut self,
+        callable: &Callable,
+        flags: Option<ConnectFlags>,
+    ) {
+        use crate::obj::EngineBitfield;
+
+        let signal_name = self.name.as_ref();
+
+        self.owner.with_object_mut(|obj| {
+            let mut c = obj.connect_ex(signal_name, callable);
+            if let Some(flags) = flags {
+                c = c.flags(flags.ord() as u32);
+            }
+            c.done();
+        });
+    }
+
+    pub(crate) fn to_untyped(&self) -> crate::builtin::Signal {
+        crate::builtin::Signal::from_object_signal(&self.receiver_object(), &*self.name)
+    }
+}


Moving those up changes the order of declaration, which no longer documents the symbols in order of importance.

See

https://godot-rust.github.io/docs/gdext/pr-1042/godot/register/struct.TypedSignal.html#implementations

https://godot-rust.github.io/docs/gdext/master/godot/register/struct.TypedSignal.html#implementations

Could you restore the order?

Bromeon · 2025-03-24T18:10:15Z

godot-codegen/src/util.rs

+        use crate::meta::{AsArg, AsObjectArg, ClassName, CowArg, ObjectArg, ObjectCow, RefArg};
+        use crate::meta::{ParamTuple, InParamTuple, OutParamTuple, Signature};


Also here can be combined 🙂

Bromeon · 2025-03-24T18:11:02Z

godot-core/src/meta/signature.rs

-
-use godot_ffi as sys;
-use sys::{BuiltinMethodBind, ClassMethodBind, GodotFfi, UtilityFunctionBind};
+use std::{borrow::Cow, fmt, marker::PhantomData};


Please flatten

Bromeon · 2025-03-24T18:13:28Z

godot-core/src/meta/signature.rs

+    /// Performs a ptrcall and processes the return value to give nice error output.
+    ///
+    /// # Safety
+    ///
+    /// This calls [`GodotFfi::new_with_init`] and passes the ptr as the second argument to `f`, see that function for safety docs.
+    unsafe fn perform_ptrcall(


The brief description of the method is good, but "perform" doesn't carry any of that information. Is it maybe possible to make the name a bit more descriptive, even if longer?

i can't really think of anything that's of similar length, but maybe just perform_ptrcall_and_process_return works?

"process" isn't much better... I'd like to avoid generic filler words; if it's not easy to add more context, let's keep it short.

Maybe something highlighting that this operates on the raw FFI args/ret: raw_ptrcall(...)
That would at least differentiate it from the other functions like out_builtin_ptrcall and so on.

lilizoey · 2025-03-25T21:01:58Z

Thanks a lot! 👍

This change now leaks a new type to the public API:
fn param_info(
    index: usize,
    param_name: &str
) -> Option<MethodParamOrReturnInfo>
However MethodParamOrReturnInfo is not public.

What's the general idea of APIs, in terms of what's public?

Hm yeah, ParamTuple should probably be public so people can see what determines a legal signature. However i am not sure if that particular method should be. The way it was before with one method for the property info and one for the property info + metadata is a bit redundant? maybe i can just make this #[doc(hidden)] and make a public method that just returns PropertyInfo? so like what it was before but they share implementation by property_info calling param_info. That would make it impossible for the user to implement their own tuples, but also we dont really want people to be doing that anyway.

TitanNano · 2025-03-26T22:47:09Z

godot-core/src/meta/param_tuple/impls.rs

+macro_rules! unsafe_impl_param_tuple {
+    ($Len:literal; $(($p:ident, $n:tt): $P:ident),+) => {
+        impl<$($P),+> ParamTuple for ($($P,)+) where $($P: GodotConvert + fmt::Debug),+ {
+            const LEN: usize = $Len;


you can count the number of $Ps with something like this and don't have to manually maintain the count:

(count_items; $PP:ident, $($P:ident),+) => { 1 + unsafe_impl_param_tuple!(count_items; $($P),+) }; (count_items; $P:ident) => { 1 };

and:

const LEN: usize = unsafe_impl_param_tuple!(count_items; $($P),+);

Bromeon · 2025-03-30T21:59:44Z

Since I caused merge conflicts with #1111, I added a commit to resolve them. I hope I added the new traits in the right places. Feel free to do whatever with it (including revert/change/...), don't need to keep authorship either 🙂

Split parameter behavior into a set of new traits: `ParamList`, `InParamList` and `OutParamList`. Allows treating input/output parameters separately.

Bromeon · 2025-04-27T17:39:20Z

Requeuing after #1143...

lilizoey added feature Adds functionality to the library quality-of-life No new functionality, but improves ergonomics/internals c: ffi Low-level components and interaction with GDExtension API labels Feb 9, 2025

lilizoey force-pushed the refactor/signature branch from a0f9079 to f2411b0 Compare March 16, 2025 18:26

lilizoey force-pushed the refactor/signature branch 2 times, most recently from ae505cf to e29f2f8 Compare March 23, 2025 22:01

lilizoey marked this pull request as ready for review March 23, 2025 22:12

Bromeon reviewed Mar 24, 2025

View reviewed changes

Bromeon mentioned this pull request Mar 26, 2025

Allow Gd<T> to be passed as a parameter in async signals #1091

Merged

TitanNano reviewed Mar 26, 2025

View reviewed changes

lilizoey force-pushed the refactor/signature branch 3 times, most recently from dcd467b to 4d5e63a Compare March 30, 2025 18:48

lilizoey force-pushed the refactor/signature branch from f5dc50c to 7578d86 Compare April 4, 2025 16:20

Bromeon force-pushed the refactor/signature branch from 7578d86 to 3aac094 Compare April 17, 2025 11:54

Bromeon added this to the 0.3 milestone Apr 17, 2025

Merge {Varcall/Ptrcall}SignatureTuple -> Signature<Params, Ret>

8975297

Split parameter behavior into a set of new traits: `ParamList`, `InParamList` and `OutParamList`. Allows treating input/output parameters separately.

Bromeon force-pushed the refactor/signature branch from 3aac094 to 8975297 Compare April 27, 2025 11:22

Bromeon removed the feature Adds functionality to the library label Apr 27, 2025

Bromeon added this pull request to the merge queue Apr 27, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 27, 2025

Bromeon added this pull request to the merge queue Apr 27, 2025

Merged via the queue into godot-rust:master with commit 007b6b8 Apr 27, 2025
16 checks passed

		/// As an example, this would be used to call Godot functions through ffi, however this is _not_ used when Godot calls a user-defined
		/// function.

		use crate::meta::{AsArg, AsObjectArg, ClassName, CowArg, ObjectArg, ObjectCow, RefArg};
		use crate::meta::{ParamTuple, InParamTuple, OutParamTuple, Signature};

Uh oh!

Conversation

lilizoey commented Feb 9, 2025

Uh oh!

GodotRust commented Feb 9, 2025

Uh oh!

lilizoey commented Mar 23, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lilizoey commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Bromeon commented Apr 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lilizoey commented Mar 25, 2025 •

edited

Loading

Bromeon commented Mar 30, 2025 •

edited

Loading