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

Networking API changes for 4.0 #1939

Closed
reduz opened this issue Dec 4, 2020 · 45 comments
Closed

Networking API changes for 4.0 #1939

reduz opened this issue Dec 4, 2020 · 45 comments
Labels
breaks compat Proposal will inevitably break compatibility topic:network
Milestone

Comments

@reduz
Copy link
Member

reduz commented Dec 4, 2020

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

It's time to take the chance to improve networking a bit

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Networking is nice in Godot, and its beautifully integrated to the scene and to GDScript, but we can do better.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The plan is to do as follows:

Call mode is in function annotations

With the new annotation system in Godot 4.0 GDScript, we can make the RPC call mode a hint of the function instead of arguments to RPC call, example:

@puppet @unreliable_ordered @channel=2 func enemy_moved(pos):
      enemy.position = pos

RPC annotations are as follows:

  • @reliable (packets always arrive, in order)
  • @unreliable (packets may or not arrive, and out of order)
  • @unreliable_ordered (packets may or not arrive, but older packets get discarded)
    You have to pick one of those three. Then @channel=N can also add a channel index if using channels.

Likewise, we can use a special syntax for RPC calling, since it no longer requires arguments.

    somenode.enemy_moved&(Vector2(4,4)) #special RPC call syntax

This syntax (not sure if this is the best way to do it, suggestions welcome), would ensure you are calling the function properly, no longer needing to pass strings.

If this enhancement will not be used often, can it be worked around with a few lines of script?

no

Is there a reason why this should be core and not an add-on in the asset library?

its core

@Faless
Copy link

Faless commented Dec 4, 2020

I would rename @unreliable_ordered to just @ordered

@jordo
Copy link

jordo commented Dec 4, 2020

I think unreliable_ordered will be more obvious to people not in the know

@reduz
Copy link
Member Author

reduz commented Dec 4, 2020

@Faless I kinda tend to prefer explicitness with these things, dont mind the longer name

@Jummit
Copy link

Jummit commented Dec 4, 2020

The rpc call syntactic sugar is tempting, but it is much easier to read netcode when the rpc method is used explicitly.
I am all for autocompletion and static typing for rpc calls though.
Just thinking of possible alternatives, what about this:

@rpc(id)
send_player_packet(player, status, info)

Note that the same would apply to rset:

@rpc_puppet @reliable
player_info = player_status

@dsnopek
Copy link

dsnopek commented Dec 4, 2020

Likewise, we can use a special syntax for RPC calling, since it no longer requires arguments.

somenode.enemy_moved&(Vector2(4,4)) #special RPC call syntax

I tried Google up any other languages that have an RPC operator, to see if there was a more sensible or recognizable one that we could steal -- I didn't find anything.

However, I do think that whatever syntax gets used it should be more explicit than a single character. Ideally, if someone saw the syntax, they'd be able to easily Google and answer the question "what's that?"

Also, there will still need to be a way to do something like rpc_id().

Could we maybe treat functions as if they are objects (similar to how signals are handled in Godot 4) and do something like:

somenode.enemy_moved.rpc(Vector2(4,4))
somenode.enemy_moved.rpc_id(123, Vector2(4, 4))

That way it's explicit (a user can Google "rpc" or "rpc_id" and figure out what that is), and we have the ability to do both rpc() and rpc_id()?

@Faless
Copy link

Faless commented Dec 4, 2020

Just thinking of possible alternatives, what about this:

@Jummit
This has potentials, but also drawbacks.
If you want to reconfigure your animation_update functions to use a different channel, or being unreliable, you will have to find all the places where you are calling it. Which is suboptimal, and more error prone (you might send it in reliably in some places unreliably in some others without noticing).

Could we maybe treat functions as if they are objects (similar to how signals are handled in Godot 4) and do something like:

@dsnopek
I think they are already actually, so we might indeed bind:

somenode.enemy_moved.rpc(PARAMS)
somenode.enemy_moved.rpc_id(ID, PARAMS)
somenode.enemy_moved.rpc_raw(MODE, CHANNEL, ID, PARAMS) # optional

I like this quite much.

@antocorr
Copy link

antocorr commented Dec 4, 2020

TL;DR; Maybe totally unrelated to the main topic

I really love the flexibility this implementations gives. Fales and the team made an excellent work with this Enet implementation.
But having done some work with Unet and Forge networking I think what's missing it's a bit of ease of use for new users.
I'm not a big fan of synchronizing the state of the game by propagating every input (like someone does) and I normally call rpcs based on the action performed by a node. But I would love to do something different.
I think it may be a limitation of GDscript itself but it would be nice to be notified for every change that affect a node. Even something like a signal would be nice.
This way I can have easy, drag and drop networking setup.
What I immagine is having a network manager in the scene and an array of nodePaths. Every nodePaths/Node in the list notifies the network manager about every property change or method call and the changes are propagated in the network.
I've done something similar during lockdown but only for translation and rotation.
Thank you guys for the great work you are doing!

@reduz
Copy link
Member Author

reduz commented Dec 4, 2020

@Jummit the idea of annotations is that they describe metadata for the engine, but not have effect in the actual interpreter, so annotations should not be used for this. Its either a symbol or a keyword.

@Calinou Calinou added this to the 4.0 milestone Dec 4, 2020
@reduz
Copy link
Member Author

reduz commented Dec 4, 2020

@antocorr there are some plans in discussion for creating nodes that do automatic synchronization of nodes and properties, or even higher level sync, the problem is in general that..

  • When programming authoritative, you really need to validate every single thing, for which you still need code. It's a hassle but it's the only way.
  • It will often not warrant it to make it core engine. Still a NetworkSync node where you select other nodes and properties and how to synchronize them may be good enough. Then again, this can be dangerous because you can't validate, but maybe it's not too bad.

In any case, this is likely material for a more higher level proposal.

@vnen
Copy link
Member

vnen commented Dec 5, 2020

I just want to point out that annotation syntax is similar to functions, with arguments inside parentheses. So this would be @channel(2) instead.

@nathanfranke
Copy link
Contributor

nathanfranke commented Dec 5, 2020

If we put all the needed information in the method signature, why should we need an operator in the first place?

@remote func input(v):
    ...

func _ready():
    input(Vector2.ZERO)

Otherwise, keyword similar to await to signify rpc

func _ready():
    rpc input(Vector2.ZERO)

Edit: This is now implemented in godot 4 as input.rpc(Vector2.ZERO)

@reduz
Copy link
Member Author

reduz commented Dec 5, 2020

@nathanfranke You've got a point there, but if you are not using typing and you get an object you don't know the type of, it may not have enough info to do the rpc.

@aaronfranke
Copy link
Member

aaronfranke commented Dec 6, 2020

@reduz: Likewise, we can use a special syntax for RPC calling, since it no longer requires arguments.

somenode.enemy_moved&(Vector2(4,4)) #special RPC call syntax

I don't like this operator idea at all. I don't know of any other language that does this, and it looks confusing. Users might look at this and think it's a "C++-ism" brought into GDScript. & doesn't really give the impression of "this has to do with networking".

I think we should just allow somenode.enemy_moved to be defined as an RPC with the annotations, and then it's called like a normal method. Or, alternatively, a keyword like rpc. So basically, exactly what @nathanfranke suggested :)

@vnen: I just want to point out that annotation syntax is similar to functions, with arguments inside parentheses. So this would be @channel(2) instead.

What if we had something like @unreliable(ordered: bool = false) instead of @unreliable and @unreliable_ordered? So you could write @unreliable(true) for unreliable ordered, or @reliable(false) for reliable unordered.

@Faless
Copy link

Faless commented Dec 6, 2020

So you could write @unreliable(true) for unreliable ordered, or @reliable(false) for reliable unordered.

There is no reliable unordered, and this is why I suggested dropping the unreliable part in unreliable_ordered.

@JotaFaD
Copy link

JotaFaD commented Dec 7, 2020

I like the idea of using the dot to call an rpc on a function. This also extends nicely to setting variables.

#Normal:
somenode.somefunction(123)
somenode.somevariable = 123
#RPC:
somenode.somefunction.rpc(123)
somenode.somevariable.rset(123)

Also, to keep things short, maybe @reliable @unreliable and @ordered could be different annotations that you combine to get the behaviour you want (ofc @reliable @ordered would be redundant and maybe generate a warning)

@jcmonkey
Copy link

jcmonkey commented Dec 7, 2020

annotations are great and all, but after playing a bit with rust and java, what about something like this.

var did_enemy_move = enemy_moved(pos).puppet().unreliable_ordered().channel(2)

@WraithWinterly
Copy link

@reduz: Likewise, we can use a special syntax for RPC calling, since it no longer requires arguments.

somenode.enemy_moved&(Vector2(4,4)) #special RPC call syntax

I don't like this operator idea at all. I don't know of any other language that does this, and it looks confusing. Users might look at this and think it's a "C++-ism" brought into GDScript. & doesn't really give the impression of "this has to do with networking".

I think we should just allow somenode.enemy_moved to be defined as an RPC with the annotations, and then it's called like a normal method. Or, alternatively, a keyword like rpc. So basically, exactly what @nathanfranke suggested :)

@vnen: I just want to point out that annotation syntax is similar to functions, with arguments inside parentheses. So this would be @channel(2) instead.

What if we had something like @unreliable(ordered: bool = false) instead of @unreliable and @unreliable_ordered? So you could write @unreliable(true) for unreliable ordered, or @reliable(false) for reliable unordered.

I agree with this, that & symbol will make it very damn confusing, something like .rpc or @rpc would make it a lot better and more understandable. Also, how is this going to be planned to also work in c#, which isn't a custom language?

@AndreaCatania
Copy link

Configure the rpc directly on the method definition reduces drastically the versatility that we currently have. When you define a function you can mark it as networked but you can still decide how to call it.

In other words this mean that you can call the same function unreliably and when needed reliably:

func _process(delta):
    rpc_unreliable("enemy_moved", Vector3())

func teleport(pos):
    rpc("enemy_moved", pos)

@remote
func enemy_moved(pos):
      enemy.position = pos

Have the possibility to choose the channel is great, but if we can still decide at function call level is level of magnitude more versatile:

var unreliable_position_channel

func _ready():
    unreliable_position_channel = create_network_channel(UNRELIABLE)

func _process(delta):
    rpc_channel(unreliable_position_channel, "enemy_moved", Vector3())

func teleport(pos):
    rpc("enemy_moved", pos)

@remote
func enemy_moved(pos):
      enemy.position = pos

@remram44
Copy link

remram44 commented Dec 13, 2020

For completeness sake, what are the arguments for requiring special syntax at all when calling functions that have been explicitly annotated RPC?

I would assume functions declared RPC are almost always called as RPC. It is also easy to factor out the internal machinery as a separate function if we want to call such machinery without an RPC. As far as I remember, this is the way it worked in UnrealScript.

I understand this might be a bigger breaking change than is acceptable though, even for 4.0.

@AndreaCatania
Copy link

See this example, it's a lot useful be able to call the same method in different ways: locally / remote / remote unreliable.

The system, as is now, is versatile and make it less explicit doesn't sound a good idea.

@remram44
Copy link

remram44 commented Dec 13, 2020

This example can be fixed by simply making teleport() into an RPC. I don't find it very compelling.

@AndreaCatania
Copy link

AndreaCatania commented Dec 14, 2020

Well, the point is to not have more functions than you need. You will end create more functions that does the same thing with a different setting and this would just make the code look bad.

Just imagine an already big script where you have to duplicate or triplicate the functions..

func enemy_moved_local(pos):
    # Do something with pos

@remote
func enemy_moved_reliable(pos):
    # Do something with pos

@remote @unreliable_ordered
func enemy_moved_unreliable(pos):
    # Do something with pos

# ----

func _process(delta):
    for peer in active_peers:
        enemy_moved_unreliable(peer, Vector3())

func teleport(pos):
    # Called on server
    enemy_moved_local(Vector3())
    for peer in active_peers:
        enemy_moved_reliable(peer, Vector3())

Also, it's not even clear how to send an rpc to a specific peer...

@remram44
Copy link

remram44 commented Dec 14, 2020

I don't want to pile on this issue with all those comments, but you are missing my point. You would not create functions like enemy_moved_unreliable(), but go one level higher in abstraction and actually name it into what the function is for. In what cases do you need to do this unreliably, rather than reliably? enemy_teleport() and enemy_moved() is an example of a higher level of abstraction, compared to enemy_set_position_reliable() and enemy_set_position_unreliable() which make it harder for collaborators or yourself to pick the right one. Same with rpc_reliable("enemy_set_position") and rpc_unreliable("enemy_set_position").

This will make the uses of that function much clearer, e.g. you can now use enemy_teleport() everywhere on your code an entity teleports and encapsulate what that means in terms of netcode in enemy_teleport(), instead of having to remember which arguments to use when doing enemy_moved() RPC in which situations. You no longer have to document what settings to use with enemy_moved() for which types of movement, that is encoded in the code of your functions, which is what encapsulation is.

If you are inlining the correct RPC settings all over the place, you now have to go over your whole code to update those settings if, say, you want to change the channel used to communicate character movements. It is much more beneficial to encapsulate this in functions such as enemy_teleport(), so you can change which RPC settings are used for all instantaneous movement in one place.

@AndreaCatania
Copy link

AndreaCatania commented Dec 14, 2020

This could work with a simple game coded by just 1 person that knows the entire code base. In a small medium sized game (2/3 members) most of the time the dev who is using the API is not the one who written that API. Be so implicit may make that API to be used in the wrong way.
Also, usually the RPC calls are not exposed to the API users, so following your design we would have normal functions that just call the rpc functions:

func teleport(pos):
    if is_server:
        __teleport(pos)

@remote
func __teleport(pos):
    change_position(pos)

func change_position(pos)
    # Do something with pos

Compared to:

func teleport(pos):
    if is_server:
        rpc("change_position", pos)

@remote
func change_position(pos)
    # Do something with pos

However, can you please give me an example (in code) that illustrates how can I implement this?

I've a function set_platform_colour(colour, platform) that the server uses to change the colour of a platform for a specific peer and sometimes itself.

@jonbonazza
Copy link

jonbonazza commented Dec 25, 2020

Late to the party here, but i think a keyword actually makes the most sense here since keywords are used for operations and declarations. Rpc is an operation on a function.

@bfelbo
Copy link

bfelbo commented Dec 27, 2020

It seems to me that having just a plain rpc keyword is problematic in that one often needs to specify the ID as currently done with rpc_id(). This is not something that can be added to the function signature as the same function is used across connections and since the IDs are not known at compile time.

If we want to use a keyword, then perhaps a rpc keyword with optional arguments (id, channel) could fulfill those wishes?

rpc somenode.enemy_moved(Vector2(4,4))
rpc(123) somenode.enemy_moved(Vector2(4, 4))
rpc(123, 2) somenode.enemy_moved(Vector2(4, 4))

If channel is provided, that channel would be used even if it uses a different mode than the function annotation (e.g. reliable instead of unreliable), thereby allowing for the flexibility that @AndreaCatania argued for.

A keyword with arguments is a bit exotic though so I'm not sure if this would be the best approach.

EDIT: I got a bit too excited and posted a wall-of-text 😄 I've trimmed it down to make it more readable.

@Ansraer
Copy link

Ansraer commented Feb 3, 2021

I personally prefer the keyword with optional argumets approach. Especially with code highlighting enabled this will be easier to read (and notice that it is in fact a remote call) than chained functions.

I would also suggest to rename '@reliable' to '@reliable_ordered' to make it more explicit.

@bfelbo
Copy link

bfelbo commented Feb 3, 2021

Currently the RPC calls use strings, which make it clear that the client can emit an RPC for a function on the server, where there's no corresponding function on the client. It's unclear to me how this common use case would be handled with the approaches discussed in this proposal, especially with the function style.

How would the code completion, static type checking, compilation, etc. handle if there's no somenode.enemy_moved function in the client codebase, but only in the server codebase?

somenode.enemy_moved.rpc(PARAMS)

@tx350z
Copy link

tx350z commented Feb 21, 2021

Just came across this proposal yesterday. I am deep into network coding on my current project and have encountered a number of challenges & limitations. So I'll chime in. First, I'd like to add that "equal but different is not better". Don't change RPC just for the sake of change. If the change does not add functionality, don't do it.

Here are my biggest wishes for networking:
Return value from RPC & HTTP calls: There are many times when game logic must request something from a server and cannot continue until a response is received. GDScript currently does not have a way to do this reliably and predictably. I've tried all the patterns which work well enough in simple examples but become unreliable in complex game communications. The best solution I've found thus far is a finite state machine approach which adds a large amount of code just because an RPC call has no ability to return a value.

Networking security features: My project is being built on the assumption that there will be bad actors trying various attacks. One concern is a bad actor creating a simple Godot app to launch DDoS attacks on the server. I'd like to have a way to set a maximum message size for RPC calls. In my case any connection that exceeds the maximum would be closed and the remote IP added to a block/ban list (would be nice if block/ban could be included in Godot's lowest level network layer).

@jonbonazza
Copy link

jonbonazza commented Feb 21, 2021

I see a lot of people request the "rpc return values" feature and I think it mostly stems from a misunderstanding of how game servers should be architected. It sounds like people are using godot to interact directly with their databases or using godot instances as api servers. This is the only reason i can think of as to why people believe they need this feature. The problem is this is a really bad design. Your game server (i.e. the godot instances running your game logic) should not be communicating directly with the db, and you should not be using godot instances to host an api.

There are many reasons why both of these are true, but the ideal multiplayer game architecture would consist of a domain specific database api server whose sole responsibility is to fetch data from the database and present it to game servers (and sometimes clients) in domain specific requests (i.e give me inventory for player xxxx) using a protocol like http or websockets that is designed for request/response communications like this. This db api server should not be a godot instance, but a scalable server written in a common server pogramming language like Go or Java.

The Godot server instances running your game's simulation will then access the db through these db api servers via a load balancer (for scale) using http or websockets (secured with tls of course).

In the end, there is no reason this feature should be needed in a properly designed multiplayer game architecture. If you think you need it, it just means your server architecture is flawed.

@dsnopek
Copy link

dsnopek commented Feb 21, 2021

Here's the earlier proposal where "rpc return values" was discussed in depth: #2032

@tx350z
Copy link

tx350z commented Feb 21, 2021

Here's the earlier proposal where "rpc return values" was discussed in depth: #2032

If you notice, I also created that proposal.

@tx350z
Copy link

tx350z commented Feb 21, 2021

In the end, there is no reason this feature should be needed in a properly designed multiplayer game architecture. If you think you need it, it just means your server architecture is flawed.

I see this kind of "you're doing it wrong" response frequently. It's the same group-think that killed Java. Regardless, change for the sake of change is never justification for change.

@AndreaCatania
Copy link

AndreaCatania commented Feb 21, 2021

In the end, there is no reason this feature should be needed in a properly designed multiplayer game architecture.

I agree with this.
Make the rpc methods to return a value has a lot of grey spots that you need a different behaviour depending your game needs. For example: what happens while you are waiting the server answer? Do you want to stop the game? Or do you want to use an yield like? Do you want to be notified? etc... so it's likely that you need to customize it depending on the game needs.

On the other side, there is nothing that stops you to wrap the current RPC methods and so make it return a value, or create a mechanism that notify you, etc... . Right now Godot (differently from other engines) doesn't enforce any behaviour, and keep the developer free to implement anything it needs.

As side note, I think that this type of communication should not be encouraged. I can already imagine that someone will writes code like this:

func _ready():
    var color = rpc_get_color()
    var state = rpc_get_state()
    var time = rpc_get_time()

This code looks cool, but it's extremely non optimal and should be avoided. Encourage it doesn't seems great.

@tx350z
Copy link

tx350z commented Feb 21, 2021

For example: what happens while you are waiting the server answer? Do you want to stop the game?

This is exactly what I need. In my case, the game logic cannot proceed without the response from the server. I've tried different approaches. The best solution thus far is a state machine that waits for signals emitted in server called RPC call-back methods. In effect, the game logic is "stopped" until the server responds. However, code bloat is a huge problem with this approach and the logic is scattered across multiple scripts.

As side note, I think that this type of communication should not be encouraged. I can already imagine that someone will writes code like this:

func _ready():
    var color = rpc_get_color()
    var state = rpc_get_state()
    var time = rpc_get_time()

This code looks cool, but it's extremely non optimal and should be avoided. Encourage it doesn't seems great.

I agree your example is bad. But there are a vastly large number of ways to write bad code. For example, take your example and try to recode it the "right way" as GDScript exists now.

Don't misunderstand, I love the GDScript network support. It's great with only the limitation that is doesn't support blocking request/response protocols. What I've been promoting is providing a way to not force writing bad code (complex finite state machines) with a simple concept: provide support for blocking request/response network communications when it is needed.

For my project, I may resort to writing a custom networking layer if the FSM approach becomes too large and fragile.

@AndreaCatania
Copy link

If you see your use case from a neutral point of view, your use case is already really specific. The majority of the games doesn't want to stop the execution, so we can't provide exactly what you need. If we would implement it, we would already cut out some other cases. On top of that, for a game "stop the execution" could mean freeze the process; while for other game it could mean don't process some specific nodes; while for yet another game it could mean pause the game, and so on...

There is not a right way to do it, and anything we would provide is an half good solution that most likely you can't use anyway, because it doesn't perfectly fit what you need. Rather, Godot provides the tools you need to implement the feature you need, so a vast majority of games can be implemented. Usually, the code you write specifically for your game is much much better than otherwise provided by Godot.

However, if you don't have a tool to complete what you need, most likely we are missing some feature elsewhere and in that case we can implement it I guess.

@tx350z
Copy link

tx350z commented Feb 21, 2021

However, if you don't have a tool to complete what you need, most likely we are missing some feature elsewhere and in that case we can implement it I guess.

If there is blocking request/response networking support in GDScript I haven't found it. I admit I could be missing something. If I had the time to re-learn C++ (it's been 25+ years), I'd step-up and build something like rpc_id_with_response().

@AndreaCatania
Copy link

AndreaCatania commented Feb 21, 2021

I don't know the details of you game, but a state machine with just two states should already fix the issue:

class_name MyObjectBase

func rpc(method):
    # This is client
    RpcEngine.is_waiting_server_answer = true
    rpc("send_data", method)

remote func send_data(method):
    # This is server
    var data = call(method)
    rpc("receive", method, data)

remote func receive(method, data):
    # This is client
    emit_signal("return_" + method, data)
    RpcEngine.is_waiting_server_answer = false
# Caller example script
extends MyObjectBase

func _ready():
    rpc("do_stuff")
    # Here you can use await or signal registration to take back the data.

func do_stuff() -> int:
    return 123

func _process(delta):
    if RpcEngine.is_waiting_server_answer:
        return
    # ... 
# Other random  script in the scene:
func _process(delta):
    if RpcEngine.is_waiting_server_answer:
        return
    # ... 

However, you call also play with the pause mechanism to completely stop the execution and probably with the await you can also improve the syntax and return the value.

This is just a sketch, I hope it's clear enough :)

@nathanfranke
Copy link
Contributor

I don't understand why the rpc return values proposal is so disliked. It seems there are two reasons

  • The use case of authentication is bad because Godot shouldn't be used for authentication (ok?)
  • The function shouldn't block because most people don't want blocking functions

The latter is the only real reason, but it can simply be fixed by making the rpc return a GDScriptFunctionState. OP of that proposal should then seek to block that function state.

Lastly I will show one of possibly many use cases for this feature.

remote func get_chunk_data(chunk: Vector3) -> ChunkData:
    return chunk_data[chunk]
print(await get_chunk_data.rpc_id(1, [Vector3(1, 2, 3)]))

@dsnopek
Copy link

dsnopek commented Feb 21, 2021

The latter is the only real reason, but it can simply be fixed by making the rpc return a GDScriptFunctionState

This was already discussed in-depth on the other proposal (#2032), but I don't think this is quite so simple.

If it returns a GDScriptFunctionState, that means the engine is keeping a big list of RPC's that have been sent, but that haven't returned yet. What happens if they never return? Probably there should be some kind of timeout, where if a RPC doesn't return after 30 seconds (or some configurable amount), the function state will be resumed anyway with some kind of error value (to prevent that list just growing forever, and those functions never getting resumed). How to return that error value? GDScript doesn't have exceptions, and returning null may be a valid return from your RPC. Also, what if 30 seconds go by, we timeout assuming the response is never going to come, and then at the 40th second, the response does come? If you've already retried due to the failure, your server (which seems to be overloaded if it's taking so long) seems likely to enter a death spiral.

There's just a lot of edge cases and extra accounting that would need to go into the engine to support this. And, that's for a feature that most people don't seem to want or need. Everytime I try to imagine a use-case for this, it seems like that use-case would be better implemented with a game-specific protocol to handle all the error and edge cases.

@tx350z
Copy link

tx350z commented Feb 22, 2021

The latter is the only real reason, but it can simply be fixed by making the rpc return a GDScriptFunctionState

This was already discussed in-depth on the other proposal (#2032), but I don't think this is quite so simple.

If it returns a GDScriptFunctionState, that means the engine is keeping a big list of RPC's that have been sent, but that haven't returned yet. What happens if they never return? Probably there should be some kind of timeout, where if a RPC doesn't return after 30 seconds (or some configurable amount), the function state will be resumed anyway with some kind of error value (to prevent that list just growing forever, and those functions never getting resumed). How to return that error value? GDScript doesn't have exceptions, and returning null may be a valid return from your RPC. Also, what if 30 seconds go by, we timeout assuming the response is never going to come, and then at the 40th second, the response does come? If you've already retried due to the failure, your server (which seems to be overloaded if it's taking so long) seems likely to enter a death spiral.

There's just a lot of edge cases and extra accounting that would need to go into the engine to support this. And, that's for a feature that most people don't seem to want or need. Everytime I try to imagine a use-case for this, it seems like that use-case would be better implemented with a game-specific protocol to handle all the error and edge cases.

You are correct. However, none of the issues you describe occur if a form of RPC supported returning a response. It could, for example, be similar to JSON.parse(). Example:

func login(_name:String, _password:String)->bool:
	# RPC call blocks until server returns a result, the times out, or a connection error occurs
	var login_result:RPCResult = rpc_id_with_return(1, "login", _name, _password);
	if login_result.error != OK:
		# Indicates some type of RPC call failure (timeout, etc)
		print("Login failed -- %s" % [login_result.error_string]);
		return false;
	elif (login_result.result as Dictionary)["login_error"] != OK:
		# Indicates a failure in the login (wrong name, password, etc)
		print("Login failed -- %s" % [(login_result.result as Dictionary)["login_error_string"]]);
		return false;
	# Indicates successful login
	return true;

@jonbonazza
Copy link

jonbonazza commented Feb 22, 2021

You are correct. However, none of the issues you describe occur if a form of RPC supported returning a response. It could, for example, be similar to JSON.parse(). Example:

No it can't, because rpcs must be async calls.

The only way this could be supported reasonably is with a Future, however it would not be easy to implement in a generic way, and the cost of such an implementaion would just not be worth it since, as mentioned alread, there is never a case where this makes sense in practice. If there was, I would have encountered it in my almist 10 years in the AAA game industry, almost exclusively working on networked games and game services.

@tx350z
Copy link

tx350z commented Feb 22, 2021

Did a search for "RPC calls must be asynchronous" and found there are many platforms that support synchronous and asynchronous RPC calls.

Regardless, I'm going silent on this discussion since it has diverted from the purpose of the OP.

I'm in favor of enhancing and expanding networking support in Godot but hope that the changes are not just cosmetic. "Different but equal is not better".

@jonbonazza
Copy link

I mean in the context of godot, they must be asynchronous. The architecture of godot reqyires this (and honestly, the architecture of any game engine requires this).

@Calinou Calinou added the breaks compat Proposal will inevitably break compatibility label May 15, 2022
@YuriSizov YuriSizov moved this to In Discussion in Godot Proposal Metaverse Jul 14, 2022
@YuriSizov YuriSizov moved this from In Discussion to Ready for Review in Godot Proposal Metaverse Jul 14, 2022
@YuriSizov YuriSizov moved this from Ready for Review to Implemented in Godot Proposal Metaverse Jul 14, 2022
@YuriSizov
Copy link
Contributor

This should be implemented now, but in a slightly different way (one @rpc annotation to rule them all).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat Proposal will inevitably break compatibility topic:network
Projects
Status: Implemented
Development

No branches or pull requests