godot-rust / gdext

Rust bindings for Godot 4
https://mastodon.gamedev.place/@GodotRust
Mozilla Public License 2.0
3.03k stars 189 forks source link

`@rpc` support and examples #784

Closed StepanTheGreat closed 2 weeks ago

StepanTheGreat commented 3 months ago

Hello, I've been trying to find any way to use godot's RPCs in gdext, but couldn't find any example of it (searched for it in the issues and features under this repo as well). It would be great to see a working gdext example like in gdnative, or at least some code examples to see how to declare RPC functions? So far I was only able to find the Base rpc method, but that's about it.

Bromeon commented 3 months ago

We don't have such examples because there's no dedicated RPC support in gdext yet.

Would you be interested in participating in a design discussion about @rpc & Co. in Rust? We could probably use this issue -- and as you stated, there's some prior work in gdnative :slightly_smiling_face:

StepanTheGreat commented 2 months ago

Sure. I personally think this could be indeed almost the same macro as in gdnative's RPC, though with required (debatable) fields like packet reliability, remote/local code execution and channel ID. I'm new to rust's macros in general, though I think something like this could be possible:

#[func(rpc=("authority", "reliable", "remote", 1))]
fn ping(&mut self);

(Might not be convenient to paste default values ("authority" for example) each time though)

toolness commented 2 months ago

For what it's worth, if you really need RPC right now, it is technically possible to do, it's just not type-checked very well. I think I found a thread in the Discord discussing how to do it, but the TLDR is that you can use code like this in your INode::ready() implementation:

self.base_mut().rpc_config(
            "proxy_request_to_server_internal".into(),
            dict! {
                "rpc_mode": RpcMode::ANY_PEER,
                "transfer_mode": TransferMode::RELIABLE,
                "call_local": false,
            }
            .to_variant(),
        );

In this case proxy_request_to_server_internal is a method you've defined on your class.

You can also call RPC methods on your class like this:

self.base_mut().rpc_id(
                            1, // Send to server only, its ID is always 1.
                            "proxy_request_to_server_internal".into(),
                            &[
                                request_id.to_variant(),
                                serialized_request_body.into_godot().to_variant(),
                            ],
                        );

Again, it's all terribly un-ergonomic and not well-typed, but it gets the job done if you need it right this minute.

ambeeeeee commented 2 months ago

I did some initial work for implementing an #[rpc] attribute in the macros, I wanted to discuss the syntax and finer implementation details before I did anything else with it. The code's in this branch but it's not really important to the discussion. I need to investigate how gdext is already preserving type safety in existing gdext macros.

It's a parsing implementation detail and easily changed, but I've implemented and would propose this syntax:

#[godot_api]
impl MyRefCounted {
    #[rpc(mode = RpcMode::AUTHORITY, transfer_mode = TransferMode::RELIABLE, call_local = true, transfer_channel = 10)]
    pub fn test() {}
}

(it should be noted that call_local is not implemented in my branch)

In this case #[rpc] would imply #[func], since I'd expect you need to expose your function properly to Godot for it to be callable by name. You can configure #[rpc] with the same values you can use for #[func] on top of the extra rpc-specific configuration.

I'd be glad to carry the PR further into implementation and out of "here's how it could work and a half implemented base" but the syntax to target is an open question.

Also I could use some clarification on how I should inject the logic that calls rpc_config. Right now I added a method to godot::obj::cap::ImplementsGodotApi but its not injected into any initialization process for the node, so it doesn't do anything on top of potentially being a wildly incorrect solution.

Bromeon commented 2 months ago

Thanks a lot, cool to see someone is tackling this! 💪

In this case #[rpc] would imply #[func], since I'd expect you need to expose your function properly to Godot for it to be callable by name. You can configure #[rpc] with the same values you can use for #[func] on top of the extra rpc-specific configuration.

So, the arguments that you can provide to #[func] and #[rpc] should be non-overlapping, so people definitely need to write #[func] as soon as they use one of its keys.

On the topic of #[rpc] implying #[func] (without keys): it may be a bit more convenient, but it does make it harder to discover all registered functions (e.g. using ripgrep or IDE search). The only place where we do this is with #[var] and #[export], and in this case both are well-known, whereas#[rpc]is something that many users do not encounter. So maybe we should keep the two independent from the start (always requiring#[func]`) and then consider ergonomic improvements later, or what do you think?


#[godot_api]
impl MyRefCounted {
    #[rpc(
        mode = RpcMode::AUTHORITY, 
        transfer_mode = TransferMode::RELIABLE, 
        call_local = true, 
        transfer_channel = 10
    )]
    pub fn test() {}
}

This looks great! You should be able to reuse most of KvParser's functionality. The enums should be treated as regular expressions without "magic" (don't try to parse them, just forward).

I assume you have default values for those, which match Godot?


Also I could use some clarification on how I should inject the logic that calls rpc_config. Right now I added a method to godot::obj::cap::ImplementsGodotApi but its not injected into any initialization process for the node, so it doesn't do anything on top of potentially being a wildly incorrect solution.

I'm currently not in front of my PC, can have a look towards the evening or so.

ambeeeeee commented 2 months ago

On the topic of #[rpc] implying #[func] (without keys): it may be a bit more convenient, but it does make it harder to discover all registered functions (e.g. using ripgrep or IDE search).

The main reason I had #[rpc] imply #[func] was because that would mean every single use of #[rpc] would have an associated #[func] attribute as well. There's nothing inherently wrong with this, and I agree that there are benefits around having both expressed individually. It might be a small headache for users when they try to use #[rpc] without #[func] and are greeted with an error telling them to also add #[func].

Making the decision to require both isn't permanent. #[rpc] could be made to imply #[func] down the line, likely without any backwards compatibility issues. It would incur a permanent maintenance overhead though. The semver implications are typically up for debate as well.

This looks great! You should be able to reuse most of KvParser's functionality. The enums should be treated as regular expressions without "magic" (don't try to parse them, just forward).

Right now I have it forwarding the TokenStream from each attribute key's value directly into the codegen, My thinking is that I'm trying to preserve the span but I might be making the wrong assumptions here.

The main problem with my implementation right now is that the way it's implemented directly generates a Dictionary with the dict!{} macro which means that you can pass in any type you want as a value, because its not checked for its type. You'd probably end up with a runtime error which we can likely avoid at compile-time.

Example of the above ```rs #[godot_api] impl MyRefCounted { #[rpc( mode = String::from("completely wrong value"), transfer_mode = 0.1f32, call_local = true, transfer_channel = 10 )] pub fn test() {} } ```

I've done some work with type safety in macros outside of this crate so I'm familiar with the general strategies but I'd like to know how you think I should approach this so it fits with the rest of the code.

I assume you have default values for those, which match Godot?

I haven't tracked down the defaults or determined whether we can omit entries and have Godot provide its defaults. Honestly, it sounds like the path forward here would be determining the defaults and I'll do that.

Bromeon commented 2 months ago

I've done some work with type safety in macros outside of this crate so I'm familiar with the general strategies but I'd like to know how you think I should approach this so it fits with the rest of the code.

[Edit] I was originally suggesting a builder, but it's not even needed. Might be nice once we have a builder API, but we can always add that once it's actually necessary.

Probably a small struct would be appropriate:

// Private API
pub struct RpcArgs {
    mode: RpcMode,
    transfer_mode: TransferMode,
    call_local: bool,
    transfer_channel: u32,
}

According to these docs, the defaults are:

impl Default for RpcArgs {
    fn default() -> Self {
        Self {
            mode: RpcMode::AUTHORITY,
            transfer_mode: TransferMode::UNRELIABLE,
            call_local: false,
            transfer_channel: 0,
        }
    }
}

Usage:

// Generated code by the macro -- one call for each key:
let rpc = RpcArgs {
    mode: RpcMode::AUTHORITY,
    call_local: true,
    ..Default::default()
};

// The API that actually registers this with Godot then accesses the builder directly:
let transfer_mode = rpc.transfer_mode;
...
Bromeon commented 2 months ago

Making the decision to require both isn't permanent. #[rpc] could be made to imply #[func] down the line, likely without any backwards compatibility issues. It would incur a permanent maintenance overhead though. The semver implications are typically up for debate as well.

The syntax with both must anyway be accepted:

#[func]
#[rpc]
fn method() { ... }

Simply for consistency, if people experiment with keys to func.

But yeah, I think it's OK to assume empty #[func] if only #[rpc] is specified. We should however definitely not allow any #[func] arguments inside #[rpc].

ambeeeeee commented 2 months ago

I implemented the type safety using the struct as you suggested, feedback to the user on type mismatch looks great so no more concerns there. call_local is also properly implemented now.

I'll get #[func] and #[rpc] split up properly and following the requirements we've converged on. Then I'll integrated rpc configuration into the lifecycle of GodotClass derived types and open a PR so we can iterate on the implementation details. I plan to PR the book to add a section on RPC and work on the macro documentation after opening the feature PR.

Planned syntax Just to concretely describe the syntax I'm implementing right now in a concrete way, and as I said before its easy to change: The `#[rpc]` attribute implies a `#[func]` attribute, so the following are equivalent: ```rs #[rpc] pub fn test() {} ``` ```rs #[rpc] #[func] pub fn test() {} ``` To use features of the `#[func]` attribute, you must specify it explicitly: ```rs #[rpc] #[func(rename = "foo")] pub fn test() {} ``` (a specific thing to call out here, the rpc macro will be aware of the renamed function and use the correct name) Unlike the gdscript and C# attributes for RPC, the Rust attribute *only* supports named parameters rather than additionally allowing positional parameters: This is supported ```rs #[rpc( mode = RpcMode::AUTHORITY, call_local = true, transfer_mode = TransferMode::RELIABLE, transfer_channel = 10 )] pub fn test() {} ``` This is **not** supported: ```rs #[rpc(RpcMode::Authority, true, TransferMode::RELIABLE, 10)] pub fn test() {} ``` I'd say it's better to use key-value for explicitness, and I worry about the potential to pick up gdscript syntax for the sake of picking up gdscript syntax. It should be noted C# supports both positional attribute parameters and named attribute parameters, so the C# API doesn't really offer any defense for not adding it. I'd definitely rather see one single way to use the `rpc` attribute, but there's definitely a conversation to be had about whether there's an actual benefit to slimming down the RPC boilerplate. It might also be interesting to see a feature where a user could derive a macro on a type to function as an "RPC Channel" or "RPC Configuration" and the type could be re-used across multiple invocations of `#[rpc]` to share a share a set of compile-time rpc configuration values between them. Like `#[rpc(RpcChannelType)]`. This type of expression could be incompatible with positional typed arguments due to the fact we can't determine whether the first positional parameter is `RpcMode` or an `RpcChannel` type. If we're adding multiple ways to express the `#[rpc]` macro, then there's this which is basically just a mapping of how the RPC attribute can be used in gdscript code: ```rs #[rpc(authority, call_local, reliable, 10)] pub fn test() {} ``` I'd call it a mistake to support this, it negates the benefits of using actual types in a macro's invocation and then, as a result, requires us to decide the type in the macro code. Then we have to generate our own error when its not one of the correct values. Rust analyzer would provide 0 feedback while writing the code.
Generated code after the latest changes ```rs #[godot_api] impl MyRefCounted { #[rpc( mode = RpcMode::AUTHORITY, transfer_mode = TransferMode::RELIABLE, transfer_channel = 10, call_local = true )] pub fn test() {} #[rpc( mode = RpcMode::ANY_PEER, transfer_mode = TransferMode::UNRELIABLE_ORDERED, transfer_channel = 15, call_local = false )] pub fn foo() {} } ``` ```rs fn __register_rpc(base: &mut ::godot::obj::Gd<::godot::classes::Node>) { let rpc_configuration = ::godot::obj::RpcArgs { mode: RpcMode::AUTHORITY, transfer_mode: TransferMode::RELIABLE, call_local: true, transfer_channel: 10, ..Default::default() } .into_dictionary(); base.rpc_config("test".into(), rpc_configuration.to_variant()); let rpc_configuration = ::godot::obj::RpcArgs { mode: RpcMode::ANY_PEER, transfer_mode: TransferMode::UNRELIABLE_ORDERED, call_local: false, transfer_channel: 15, ..Default::default() } .into_dictionary(); base.rpc_config("foo".into(), rpc_configuration.to_variant()); } ``` `RpcArgs::into_dictionary` is a function that just creates the dictionary form of Rpc arguments.
Bromeon commented 2 months ago

I'd say it's better to use key-value for explicitness, and I worry about the potential to pick up gdscript syntax for the sake of picking up gdscript syntax.

I was thinking about this, too; but I think key-value is better for two reasons:

I know it's a bit more wordy and there's some RTFM involved when translating GDScript for the first time, but I'm not sure if it's that bad. We should probably see how users work with this.


Another thing I was briefly thinking about:

   #[rpc(
        mode = AUTHORITY, // omit RpcMode::
        transfer_mode = RELIABLE, // omit TransferMode::
        transfer_channel = 10,
        call_local = true
    )]

However I'm not sure if it's a good idea. While we can easily inject the enum prefixes, this prevents use cases like:

const MY_MODE: RpcMode = RpcMode::AUTHORITY;

 #[rpc(mode = MY_MODE)]

which would btw partially address the "reuse config" point you mentioned. But I agree, if people find themselves repeating a lot, we could still later add a config = key and make RpcConfig public.


Could you also add tests where only some RPC config keys are specified?

Is there a way to test this at all? There seems to be no getter counterpart to Node::rpc_config, so how to verify that the keys are properly applied?

ambeeeeee commented 2 months ago

However I'm not sure if it's a good idea. While we can easily inject the enum prefixes, this prevents use cases like: ... which would btw partially address the "reuse config" point you mentioned. But I agree, if people find themselves repeating a lot, we could still later add a config = key and make RpcConfig public.

I didn't even think about that, we could make RpcConfig public ahead of time to allow them to use this pattern if they desire the ability to have one definition shared across multiple rpcs, and this obviously wouldn't work with the omission of the enum prefixes.

const MY_RPC_CONFIG: RpcConfig = RpcConfig {
    mode: RpcMode::AUTHORITY,
    transfer_mode: TransferMode::RELIABLE,
    transfer_channel: 10,
    call_local: true
}

#[rpc(mode = MY_RPC_CONFIG.mode, transfer_mode = MY_RPC_CONFIG.transfer_mode, transfer_channel = MY_RPC_CONFIG.transfer_channel, call_local = MY_RPC_CONFIG.call_local]

There's... definitely a function in the Godot source code to get rpc configuration but it doesn't seem like it'll help.

If we can write tests that actually use Godot's RPC (by being the host, so we don't need any networking), we can at the very least test calls where call_local = true, but we can't test mode's value since we'd be the authority, and transfer_channel would have no effect. Integration tests for this are definitely going to be a challenge.

Bromeon commented 2 months ago

I didn't even think about that, we could make RpcConfig public ahead of time to allow them to use this pattern if they desire the ability to have one definition shared across multiple rpcs

You're right, maybe we should just make it public then? Should we then also provide config = directly?

I think the tools module may be a candidate for the RpcConfig struct.

(The config = key could btw be a separate PR, so we can finish this faster :slightly_smiling_face: )


If we can write tests that actually use Godot's RPC (by being the host, so we don't need any networking), we can at the very least test calls where call_local = true, but we can't test mode's value since we'd be the authority, and transfer_channel would have no effect. Integration tests for this are definitely going to be a challenge.

Yes, this is going to be both development- and maintenance-heavy; I'd rather avoid it. Maybe we can open a PR to Godot to expose this functionality. I can try to find out if there's a reason why it's not exposed :thinking:

ambeeeeee commented 2 months ago

Added config = RpcConfigValue named attribute parameter and moved RpcConfig over to the tools module. Here's where the syntax is at, collapsed since the number of examples is growing quite large.

Click to view syntax ```rs const BARE_CONST_CONFIG: RpcConfig = RpcConfig { mode: RpcMode::AUTHORITY, transfer_mode: TransferMode::UNRELIABLE, call_local: false, transfer_channel: 1, }; #[godot_api] impl MyRefCounted { #[rpc( mode = RpcMode::AUTHORITY, transfer_mode = TransferMode::RELIABLE, transfer_channel = 10, call_local = true )] pub fn test() {} #[rpc( mode = RpcMode::ANY_PEER, transfer_mode = TransferMode::UNRELIABLE_ORDERED, transfer_channel = 15, call_local = false )] pub fn foo() {} // Rpc + func not supported yet #[rpc(call_local = true)] #[func(rename = "PascalCaseEndpoint")] pub fn snake_case_endpoint() { } const ASSOCIATED_CONFIG: RpcConfig = RpcConfig { mode: RpcMode::AUTHORITY, transfer_mode: TransferMode::UNRELIABLE, call_local: false, transfer_channel: 1, }; #[rpc(config = Self::ASSOCIATED_CONFIG)] pub fn new_config_endpoint() {} #[rpc(config = BARE_CONST_CONFIG)] pub fn other_config_endpoint() {} } ```

This is where we're at syntax wise. #[rpc] and #[func] aren't able to be defined at the same time yet, since I need to adjust how I am processing the rpc attribute to associate the invocation with a #[func] attribute, especially for rename support.

I also need to add a check to prevent the #[rpc] functions from having a return type, as that is not supported by the RPC mechanism Godot exposes. That's a pretty simple problem to solve, but its worth mentioning.

I'll open the PR following the codegen changes for #[rpc] as that concludes the code side of things and we can get eyes on it while I work on some documentation and an example.

Bromeon commented 2 months ago

This looks great, thanks a lot! 👍

One small possibility is that Godot would extend the RpcConfig in the future, and then we'd have a breaking change. This could be technically addressed by a builder and #[non_exhaustive], but I think we don't need it for now. If such a change happens, we could also introduce it across a minor version boundary -- maybe even good to notify people that there's now a new option.

Bromeon commented 1 month ago

@ambeeeeee Have you had a chance to follow up on this yet? 🙂

Houtamelo commented 2 weeks ago

I propose allowing the following, which aligns with GDScript's style:

#[rpc("any_peer", "reliable", "call_local", channel = 0)]
fn my_rpc_call(&mut self) {
    //..
}

const SHARED_RPC_ARGS: RpcArgs = RpcArgs { .. };

#[rpc(args = SHARED_RPC_ARGS)]
fn my_rpc_call(&mut self) {
    //..
}

#[rpc(args = SHARED_RPC_ARGS, "any_peer")] // compiler error, can't have both args constant and individual
fn my_rpc_call(&mut self) {
    //..
}

#[func(gd_self)] // #[func] can be specified to provide additional arguments, otherwise it's implied with default ones
#[rpc("any_peer")]
fn my_rpc_call(&mut self) {
    //..
}

I prefer having a separate attribute because:

If there's no #[func] attribute, then it's implied with default arguments.

The RpcArgs struct would be the one agreeded on the previous answers.

I can contribute this PR if a design is agreed upon.

Bromeon commented 2 weeks ago

@ambeeeeee's previous suggestion included this syntax:

#[godot_api]
impl MyRefCounted {
    #[rpc(mode = RpcMode::AUTHORITY, transfer_mode = TransferMode::RELIABLE, call_local = true, transfer_channel = 10)]
    pub fn test() {}
}

What's your view on that vs. yours, @Houtamelo?

If we keep it shorter, then we should not quote keys as strings; no other proc-macro API of gdext does this currently. Syntax cohesion with GDScript is less important than internal consistency within the library. That is, instead of:

#[rpc("any_peer", "reliable", "call_local", channel = 0)]
fn my_rpc_call(&mut self)

it would be

#[rpc(any_peer, reliable, call_local, channel = 0)]
fn my_rpc_call(&mut self)

Note that you essentially have 3 enum parameters with 2-3 variants each:

rpc parameters

And 2 of those are encoded in RpcMode and TransferMode.

But maybe the shorter syntax is worth it for the sake of clarity and GDScript familiarity. In that case, people who want explicit enum orthogonality could still use #[rpc(args = ...)].

Houtamelo commented 2 weeks ago

@ambeeeeee's previous suggestion included this syntax:

#[godot_api]
impl MyRefCounted {
    #[rpc(mode = RpcMode::AUTHORITY, transfer_mode = TransferMode::RELIABLE, call_local = true, transfer_channel = 10)]
    pub fn test() {}
}

What's your view on that vs. yours, @Houtamelo? Imo it's too long with no real benefit.

If we keep it shorter, then we should not quote keys as strings; no other proc-macro API of gdext does this currently. Syntax cohesion with GDScript is less important than internal consistency within the library. That is, instead of:

#[rpc("any_peer", "reliable", "call_local", channel = 0)]
fn my_rpc_call(&mut self)

it would be

#[rpc(any_peer, reliable, call_local, channel = 0)]
fn my_rpc_call(&mut self)

Looks good, I'm in favor of that.

Bromeon commented 2 weeks ago

Sounds good to me, you can gladly go ahead with a PR :blush:

Houtamelo commented 2 weeks ago

Note that you essentially have 3 enum parameters with 2-3 variants each:

And 2 of those are encoded in RpcMode and TransferMode.

What about sync, should we add an enum for that too?

pub enum SyncMode {
        CallLocal,
        CallRemote,
}
Bromeon commented 2 weeks ago

I think the idea was to follow the dictionary used in Node::rpc_config(), which has enums for rpc_mode and transfer_mode, but bool for call_local.

That also reminds me why the original key was named config and not args: #[rpc(config = ...)] then becomes a very direct wrapper on top of Node::rpc_config().