Add `#[rpc]` attribute to user-defined functions #902

Houtamelo · 2024-09-19T13:23:34Z

The style is similar to GDScript's @rpc annotation, the macro can be used as follows:

1 - Separate arguments:

#[rpc(any_peer, reliable)]
fn some_rpc(&mut self) {
    //..
}

Providing overlapping arguments generates a compile error.

Any omitted arguments are set to their default values.

2 - Provide an expression:

const CONFIG: RpcArgs = RpcArgs {
    mode: RpcMode::Authority,
    ..RpcArgs::default()
};

#[rpc(config = CONFIG)]
fn some_rpc(&mut self) {
    //..
}

Number 2 is useful in case you want to reuse the configuration on multiple functions.
Number 2 is mutually exclusive with number 1.

You can use the #[func] attribute in conjunction with #[rpc]. If #[func] is not present (but #[rpc] is), then #[func] is implied with default arguments.

The generated macro code works as follows:

Caches the configuration in a ClassPlugin.
On __before_ready(), searches for the configuration in the plugin, registering them with Node::rpc_config().

Edit: the enums RpcMode and TransferMode are only available if the feature __codegen_full is enabled, so I had to feature-gate everything related to the implementation behind that feature.

GodotRust · 2024-09-19T13:31:57Z

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

Bromeon

Thanks a lot, this looks great! 😊

I would maybe rename RpcInfo to RpcConfig, just so it's in line with #[rpc(config = ...)] and Node::rpc_config().

Bromeon · 2024-09-19T13:28:58Z

godot-core/src/meta/rpc_args.rs

+use crate::meta::ToGodot;
+
+/// See [Godot documentation](https://docs.godotengine.org/en/stable/tutorials/networking/high_level_multiplayer.html#remote-procedure-calls)
+#[derive(Debug, Clone, Copy)]


Suggested change

#[derive(Debug, Clone, Copy)]

#[derive(Copy, Clone, Debug)]

Just canonical order 🙂

Bromeon · 2024-09-19T13:40:25Z

godot-macros/src/class/data_models/rpc.rs

+pub fn make_rpc_registrations_fn(
+    class_name: &Ident,
+    funcs: &mut [FuncDefinition],
+) -> Option<TokenStream> {
+    let rpc_registrations = funcs
+        .iter_mut()
+        .filter_map(make_rpc_registration)
+        .collect::<Vec<TokenStream>>();


It's a bit unusual that filter_map modifies the element, even though not forbidden.

Is the only write operation you need this, or also something else?

let rpc_info = func_def.rpc_info.take()?;

Does this have to be a take()? What if you just read the element?

Keeping things immutable is nice because it allows multiple borrows (in future code refactors) and makes immediately obvious that the argument isn't changed. With the above mutable parameter funcs: &mut [FuncDefinition], the first impression is "this changes the function signature".

Bromeon · 2024-09-19T13:41:20Z

godot-macros/src/class/data_models/rpc.rs

+
+            let mut gd = object
+                .downcast_mut::<#class_name>()
+                .expect("bad type erasure")


Context is helpful:

Suggested change

.expect("bad type erasure")

.expect("bad type erasure when registering RPCs")

Bromeon · 2024-09-19T13:42:50Z

godot-macros/src/class/data_models/rpc.rs

+pub enum RpcInfo {
+    // Individual keys in the `rpc` attribute.
+    // Example: `#[rpc(any_peer, reliable, call_remote, channel = 3)]`
+    SeparatedArgs(RpcSeparatedArgs),


Unless I'm missing something, it doesn't look like you benefit from having an actual struct here, so could you inline the 4 fields into the enum variant?

I'm not worried about the future, it's a 1-click IDE operation to extract to a struct, should we ever need the flexibility.

Bromeon · 2024-09-19T13:43:47Z

godot-macros/src/class/data_models/inherent_impl.rs

+		            // Ok: Only `args = [expr]` is present.
+		            (Some(expr), (None, None, None, None)) => RpcInfo::Expression(expr),
+		            // Err: `args = [expr]` is present along other parameters, which is not allowed.
+		            (Some(_), _) => return bail!(&error_scope, "`#[rpc(config = [expr])]` is mutually exclusive with any other parameters(`any_peer`, `reliable`, `call_local`, `channel = 0`)"),


Suggested change

(Some(_), _) => return bail!(&error_scope, "`#[rpc(config = [expr])]` is mutually exclusive with any other parameters(`any_peer`, `reliable`, `call_local`, `channel = 0`)"),

(Some(_), _) => return bail!(&error_scope, "`#[rpc(config = ...)]` is mutually exclusive with any other parameters(`any_peer`, `reliable`, `call_local`, `channel = 0`)"),

Bromeon · 2024-09-19T13:55:53Z

godot-macros/src/class/data_models/inherent_impl.rs

+                ty: ItemAttrType::Func {
+                    rename: None,
+                    is_virtual: false,
+                    has_gd_self: false,
+                    rpc_info: Some(rpc_info),
+                },


Would probably be good to implement default() here, which represents the #[func] attribute without keys.

That would require extracting the enum variant into a struct, would you like me to do that?

Bromeon · 2024-09-19T13:58:09Z

godot-macros/src/class/data_models/rpc.rs

+                let token = if call_local {
+                    quote! { true }
+                } else {
+                    quote! { false }
+                };
+
+                quote! { let args = RpcArgs { call_local: #token, ..args }; }


Suggested change

let token = if call_local {

quote! { true }

} else {

quote! { false }

};

quote! { let args = RpcArgs { call_local: #token, ..args }; }

quote! { let args = RpcArgs { call_local: #call_local, ..args }; }

facepalm moment 😆

Bromeon · 2024-09-19T14:11:07Z

godot-core/src/registry/class.rs

+    // We do this manually instead of using `iterate_plugins()` because we want to break as soon as we find a match.
+    let plugins = crate::private::__godot_rust_plugin___GODOT_PLUGIN_REGISTRY
+        .lock()
+        .unwrap();


This should be kept private.

Maybe it's better to add an additional function find_plugin(class_name).

Bromeon · 2024-09-19T14:13:26Z

godot-macros/src/class/data_models/inherent_impl.rs

+    #[cfg(not(feature = "codegen-full"))]
+    let rpc_registrations = Option::<TokenStream>::None;


Typically we use TokenStream::new() for "no tokens", unless there's an explicit is_some() / if let Some(..) check on it needed.

Bromeon · 2024-09-19T14:17:35Z

godot-macros/src/class/data_models/rpc.rs

+                    RpcMode::AnyPeer => quote! { RpcMode::ANY_PEER },
+                };
+
+                quote! { let args = RpcArgs { mode: #token, ..args }; }


Not that it matters much, but why did you choose this over args.mode = #token;?
To avoid the "unused mut" warning when no field is changed?

One option is also to provide directly the field inits, as in:

mode: #token

and then have

let args = RpcConfig { #( #field_inits, )* ..Default::default() }

But possibly the ..Default::default() must be disabled if all are set, not 100% sure right now 🤔

I did it that way because of your comment on the other issue:

Generated code by the macro -- one call for each key:

Makes sense, but the "Generated code by the macro" would be per-field init, no? In Rust, you can have a trailing ..Default::default() even if all fields have already been specified.

So this would be even simpler to understand, and also lead to less generated code:

let args = RpcConfig { #( #field_inits, )* ..Default::default() }

Houtamelo · 2024-09-19T15:36:19Z

@Bromeon Should be all good now

Bromeon

Thanks a lot for the update! 👍

Bromeon · 2024-09-20T06:38:20Z

godot-macros/Cargo.toml

@@ -31,7 +32,7 @@ godot-bindings = { path = "../godot-bindings", version = "=0.1.3" } # emit_godot

 # Reverse dev dependencies so doctests can use `godot::` prefix.
 [dev-dependencies]
-godot = { path = "../godot", default-features = false }
+godot = { path = "../godot", default-features = false, features = ["__codegen-full"] }


A regular cargo test (or similar) command may now pull in the entire codegen, which increases local and CI cycle duration.

Why is it necessary? I'd argue for doc-links alone, it's not worth it. Also because godot-macros's internal docs are only useful for developers, not users.

The CI fails without the change I made, because for some reason cargo test is compiling godot-macros with the feature, but godot without, so it fails to compile because godot-macros then generates code that's feature-gated in godot.

But it worked before? So what changed that makes it necessary now? Doctests?

It's not like the macros access any library functionality from the other crates -- they just generate code.

But it worked before? So what changed that makes it necessary now? Doctests?

Not with my additions, no, they require enums that are only present when code-gen_full is enabled

Bromeon · 2024-09-20T06:44:12Z

godot-macros/src/class/data_models/rpc.rs

+                    RpcMode::AnyPeer => quote! { RpcMode::ANY_PEER },
+                };
+
+                quote! { let args = RpcArgs { mode: #token, ..args }; }


Makes sense, but the "Generated code by the macro" would be per-field init, no? In Rust, you can have a trailing ..Default::default() even if all fields have already been specified.

So this would be even simpler to understand, and also lead to less generated code:

let args = RpcConfig { #( #field_inits, )* ..Default::default() }

Bromeon · 2024-09-20T07:01:04Z

godot-core/src/meta/mod.rs

+#[cfg(feature = "codegen-full")]
+mod rpc_config;


Maybe add a brief comment that MultiplayerPeer::TransferMode and MultiplayerApi::RpcMode aren't part of the basic set of classes (also below).

Bromeon · 2024-09-20T07:04:40Z

godot-core/src/meta/rpc_config.rs

+use crate::meta::ToGodot;
+
+/// See [Godot documentation](https://docs.godotengine.org/en/stable/tutorials/networking/high_level_multiplayer.html#remote-procedure-calls)
+#[derive(Clone, Copy, Debug)]


Please apply my suggestion verbatim (unless I make a mistake, of course). You can just click "Commit" on the web UI and squash later into your own commit.

The order should be Copy, Clone, Debug. I know it's bikeshedding, but it's documented, and with my readily committable suggestion, the effort on your side to change should be minimal 😉

(Yes, I know I should automate this at some point...)

Bromeon · 2024-09-20T07:07:20Z

godot-core/src/meta/rpc_config.rs

+    /// Returns a [`Dictionary`] populated with the values required for a call to [`Node::rpc_config`].
+    pub fn into_dictionary(self) -> Dictionary {
+        dict! {
+            "mode": self.mode,
+            "transfer_mode": self.transfer_mode,
+            "call_local": self.call_local,
+            "transfer_channel": self.transfer_channel,
+        }
+    }


According to Godot docs, the key names here are incorrect.

Could you then also rename the struct fields to match the dict keys?

itest/rust/src/register_tests/rpc_test.rs

Bromeon · 2024-09-20T07:17:16Z

itest/rust/src/register_tests/rpc_test.rs

+#[godot_api]
+impl RpcTest {
+    #[rpc]
+    pub fn default_args(&mut self) {}
+
+    #[rpc(any_peer)]
+    pub fn arg_any_peer(&mut self) {}
+
+    #[rpc(authority)]
+    pub fn arg_authority(&mut self) {}
+
+    #[rpc(reliable)]
+    pub fn arg_reliable(&mut self) {}
+
+    #[rpc(unreliable)]
+    pub fn arg_unreliable(&mut self) {}
+
+    #[rpc(unreliable_ordered)]
+    pub fn arg_unreliable_ordered(&mut self) {}
+
+    #[rpc(call_local)]
+    pub fn arg_call_local(&mut self) {}
+
+    #[rpc(call_remote)]
+    pub fn arg_call_remote(&mut self) {}
+
+    #[rpc(channel = 2)]
+    pub fn arg_channel(&mut self) {}
+
+    #[rpc(config = CACHED_CFG)]
+    pub fn arg_config(&mut self) {}
+}


Maybe add 1 case where you combine it with #[func].

godot-macros/src/class/data_models/rpc.rs

godot-macros/src/class/data_models/inherent_impl.rs

godot-macros/src/lib.rs

@rpc

The style is similar to GDScript's @rpc annotation, the macro can be used as follows: godot-rust#1 - Separate arguments: ```rust #[rpc(any_peer, reliable)] fn some_rpc(&mut self) { //.. } ``` Providing overlapping arguments generates a compile error. Any omitted arguments are set to their default values. godot-rust#2 - Provide an expression: ```rust const CONFIG: RpcArgs = RpcArgs { mode: RpcMode::Authority, ..RpcArgs::default() }; #[rpc(config = CONFIG_EXPR)] fn some_rpc(&mut self) { //.. } ``` Number godot-rust#2 is useful in case you want to reuse the configuration on multiple functions. Number godot-rust#2 is mutually exclusive with number godot-rust#1. --- The generated macro code works as follows: - Caches the configuration in a `ClassPlugin`. - On `__before_ready()`, searches for the configuration in the plugin, registering them with Node::rpc_config().

Houtamelo · 2024-09-20T21:23:57Z

@Bromeon ready for another round

Edit: Forgot to mention: I made a bunch of changes to the module inherent_impl, those were made with the goal of fixing a bug where the attributes #[func] and #[rpc] caused a compilation error when used together.
The bug was because, after parsing, the macro would only remove the first of the arguments found.

Bromeon · 2024-09-22T08:58:49Z

Made a few minor changes:

Reverted feature dependency in godot-macro/Cargo.toml, since that meant ./check.sh test would have pulled in full codegen.
Renamed RpcConfig methods register -> configure_node and into_dictionary -> to_dictionary (the latter can take by-ref, there's no advantage in consuming the argument).
Added a #[cfg], without which there was a local Clippy error.

Thanks a lot for this great addition! 💪

Bromeon linked an issue Sep 19, 2024 that may be closed by this pull request

@rpc support and examples #784

Closed

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Sep 19, 2024

Houtamelo force-pushed the rpc_annotation branch from 8ae84df to 45779d0 Compare September 19, 2024 13:28

Houtamelo force-pushed the rpc_annotation branch from 45779d0 to 33fc9f0 Compare September 19, 2024 14:05

Bromeon reviewed Sep 19, 2024

View reviewed changes

Houtamelo force-pushed the rpc_annotation branch from 33fc9f0 to 7aabeaf Compare September 19, 2024 15:30

Bromeon reviewed Sep 20, 2024

View reviewed changes

godot-macros/src/lib.rs Show resolved Hide resolved

Bromeon changed the title ~~#[rpc] annotation for functions in #[godot_api] inherent impl blocks.~~ Add #[rpc] attribute to user-defined functions Sep 20, 2024

Houtamelo force-pushed the rpc_annotation branch from 0669831 to c4dd6b7 Compare September 20, 2024 21:20

Follow-up changes to RpcConfig + #[cfg] + clippy

2ae2995

Bromeon force-pushed the rpc_annotation branch from 6ad6756 to 2ae2995 Compare September 22, 2024 08:55

Bromeon added this pull request to the merge queue Sep 22, 2024

Merged via the queue into godot-rust:master with commit b26e114 Sep 22, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `#[rpc]` attribute to user-defined functions #902

Add `#[rpc]` attribute to user-defined functions #902

Houtamelo commented Sep 19, 2024 •

edited

Loading

GodotRust commented Sep 19, 2024

Bromeon left a comment

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Houtamelo Sep 19, 2024

Bromeon Sep 19, 2024

Houtamelo Sep 19, 2024

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Bromeon Sep 19, 2024

Houtamelo Sep 19, 2024

Bromeon Sep 20, 2024

Houtamelo commented Sep 19, 2024

Bromeon left a comment

Bromeon Sep 20, 2024

Houtamelo Sep 20, 2024

Bromeon Sep 20, 2024

Houtamelo Sep 20, 2024 •

edited

Loading

Bromeon Sep 20, 2024

Bromeon Sep 20, 2024

Bromeon Sep 20, 2024

Bromeon Sep 20, 2024

Bromeon Sep 20, 2024

Houtamelo commented Sep 20, 2024 •

edited

Loading

Bromeon commented Sep 22, 2024

	.expect("bad type erasure")
	.expect("bad type erasure when registering RPCs")

	(Some(_), _) => return bail!(&error_scope, "`#[rpc(config = [expr])]` is mutually exclusive with any other parameters(`any_peer`, `reliable`, `call_local`, `channel = 0`)"),
	(Some(_), _) => return bail!(&error_scope, "`#[rpc(config = ...)]` is mutually exclusive with any other parameters(`any_peer`, `reliable`, `call_local`, `channel = 0`)"),

		#[cfg(not(feature = "codegen-full"))]
		let rpc_registrations = Option::<TokenStream>::None;

		#[cfg(feature = "codegen-full")]
		mod rpc_config;

Add #[rpc] attribute to user-defined functions #902

Add #[rpc] attribute to user-defined functions #902

Conversation

Houtamelo commented Sep 19, 2024 • edited Loading

1 - Separate arguments:

2 - Provide an expression:

GodotRust commented Sep 19, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Houtamelo commented Sep 19, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Houtamelo Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Houtamelo commented Sep 20, 2024 • edited Loading

Bromeon commented Sep 22, 2024

Add `#[rpc]` attribute to user-defined functions #902

Add `#[rpc]` attribute to user-defined functions #902

Houtamelo commented Sep 19, 2024 •

edited

Loading

Houtamelo Sep 20, 2024 •

edited

Loading

Houtamelo commented Sep 20, 2024 •

edited

Loading