Godot-proposals: Make networking easier to use

[Shadowblitz16]

Describe the project you are working on:
Networking test

Describe the problem or limitation you are having in your project:
networking is hard to setup and understand

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
I suggest removing the existing networking signals and implementation the following..

network_player_banned(host:String, port:int, id:int, reason:String)->void
network_player_kicked(host:String, port:int, id:int, reason:String)->void
network_player_failed(host:String, port:int)->void
network_player_enter(host:String, port:int, id:int)->void
network_player_leave(host:String, port:int, id:int)->void

along with the following for trees..
is_network_self(id:int) #id is optional
is_network_peer(id:int) #id is optional
is_network_client(id:int) #id is optional
is_network_server(id:int) #id is optional
start_enet_server(port, max_players, ...)
start_enet_client(port, host, ...)
start_peer_server(port, max_players, ...)
start_peer_client(port, host, ...)

I'm not actually sure about peer versions I haven't used them

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
it would work something like this..

var players = {}
func _ready():
   get_tree().connect("network_player_enter", self, "_player_enter")
   get_tree().connect("network_player_leave", self, "_player_leave")


func host(port):
  #returns NetworkedMultiplayerENet
  get_tree().start_enet_server(port)

func join(port, host):
  #returns NetworkedMultiplayerENet
  get_tree().start_enet_client(port, host)

func leave():
  get_tree().close_connections()

func _player_enter(host, port, id):
    #check to see if server
    if get_tree().is_network_server(id): 
        player[id] = Dictionary()
    print(("server" if get_tree().is_network_server(id) else "client" + " has joined with id "+str(id))

func _player_leave(host, port, id):
    print(("server" if get_tree().is_network_server(id) else "client" + " has left with id "+str(id))

#rpc'd on server for all clients and server
remotesync func start_game(scene, parent, players):

    if has_node(parent): 
        for i in range(players.size()):
            if get_tree().is_network_client(players[i]):
                var inst = scene.instance()
                get_node(parent).add_child(inst)
                #is_local is a new function to check to see if a id is ours
                if is_network_self(id): inst.set_network_master(player[i])

    emit_signal("game_ready")

If this enhancement will not be used often, can it be worked around with a few lines of script?:
I would definitely use it. Its much clearer to understand what is going on and it actually makes networking take fewer lines of script.

Is there a reason why this should be core and not an add-on in the asset library?:
It would make setting up networking easier and is more consistent

[Calinou]

network_player_banned(host:String, port:int, id:int, reason:String)->void
network_player_kicked(host:String, port:int, id:int, reason:String)->void

I think managing a ban list is outside the scope of a game engine's core functionality. To me, this is clearly game logic territory.

[Shadowblitz16]

Thats fine but I still think it would be easier to use

[jonbonazza]

A couple things:

1. The network api does not operate on players. It operates on connections. A connection does not need to necessarily be from a player, so the suggested naming doesn't make sense. I also don't see how renaming signals makes it easier to use.
2. Multiplayer functions actually used to be directly part of SceneTree but were moved to a multiplayer member because people wanted a way to start a server abd a client (and/or multiple servers or multiple clients) in a single game. This is useful for local multiplayer or local testing. It's also useful for things like ai bots and such. And again, I don't see how this would be any easier to use. It's just a single layer of indirection rewuiring you to use get_tree().multiplayer instead of just get_tree().
3. Multiplayer is hard no matter what you do. It's a very advanced topic in gamedev, so you're never going to get something that's super easy withput sacrificing _a lot_ of flexibility (if it's even truly possible at all). This is why I suggest devs don't even bother with multiplayer unless they really know what they are doing. Entertainingly, it's actually very easy to do multiplager badly and bad multiplayer is worse than not having multiplayer support at all.

[Shadowblitz16]

1. 
   The player naming was a bit wrong it would be more of connection. maybe the following would be a better term?
2. network_failed(host, port)
3. network_connected(host, port, id)
4. network_disconnected(host, port, id)

Right now there isn't a good way to check if a server connect or disconnect on itself because it doesn't actually connect to itself
That's why I was using the keyword player. edit: socket could be a better term too

Another issue is I think handling connections in one place is better then having them in a bunch of different places for each use case.
if you need a specific check then you can do the following...

func some_consistently_named_disconnect_signal(id):
  if is_server(id):
     #server
     #could be me or a peer

  if is_self(id):
     #could be server or client
     #me i'm no longer online

  if is_self(id) and is_server(id):
     #server
     #me i'm not longer online

   if is_peer(id) and is_client(id):
      #client
      #i'm still online

2.
I don't see why they need to be moved out of it when they can just be a alternate solution.

3.
I think there is always a way to cover all use cases and make it easier to use you just have to find them.
Also the following is based on the current system

[jonbonazza]

Right now there isn't a good way to check if a server connect or disconnect on itself because it doesn't actually connect to itself

If a server fails to start, an error is returned directly, so a signal is unnecessary. As for disconnection, this probably should be added. I don't know that there is a way to do this currently and would be a welcome addition. Should probably make a separate proposal for this.

Also, you already have the network_peer_disconnected which should be called everywhere that can be used for a single place for your code.

1. I don't see why they need to be moved out of it when they can just be a alternate solution

Moving them out is the best solution. There's no way to do this cleanly with leaving everything in SceneTree directly. I don't see how doing multiplayer.foo() is any more difficult than get_tree().foo().

Note that with the abstraction, you don'y need to use the scene tree at all. Every Node has a multiplayer member that is guaranteed to be set correctly once it's in the scene tree. There is literally no difference here.

I think there is always a way to cover all use cases and make it easier to use you just have to find them.
Also the following is based on the current system

There really isn't because most things multiplayer are very game specific. A lot of things can be implemented somewhat generically if you sacrifice a lot of optimizations in regards to bandwidth and latency, but for actual games you never want these sacrifices cause they cost money. This is why the godot multiplayer api (and every other engine's multiplayer api) is fairly low level, so that these game-specific optimizations can be made.

If you want to create addons that are a bit higher level but a bit more opinionated tgat's great. They would be welcomed, but they don't belong in core.

[Shadowblitz16]

@jonbonazza
The thing is the peer signals don't trigger when the server connects or disconnects to itself
The server can be a client too so it should be treated as one

Having a single place where i am not only notified of peers but also myself is also a nice thing to have because I can put all my code in one place.

The is_* functions where purely for checking ids in those methods and any method that supplies and network id

The start and stop (I forgot stop) methods were there to simplify wrap 3 lines of code that are needed to do one thing.

as for them being in the tree it would probably be better if they were in node as they would be easier to access

[jonbonazza]

@jonbonazza
The thing is the peer signals don't trigger when the server connects or disconnects to itself
The server can be a client too so it should be treated as one

Having a single place where i am not only notified of peers but also myself is also a nice thing to have because I can put all my code in one place.

The is_* functions where purely for checking ids in those methods and any method that supplies and network id

The start and stop (I forgot stop) methods were there to simplify wrap 3 lines of code that are needed to do one thing.

as for them being in the tree it would probably be better if they were in node as they would be easier to access

Right, i agree adding a signal for server disconnect would be useful, but in practice you never want the same code to run when a peer disconnects from another peer (serber or otherwise) and when a server disconnects itself, so having two separate signals makes thebmost sense here.

To clarify, the methods alread _are_ in node.

You just need to access them via the multiplayer member of Node.

[Shadowblitz16]

what about network_host_connected and network_host_disconnected?

The naming would be consistent and its would even line up!

[jonbonazza]

It's not my call alone, but i'd be fine with that namine for disvonnect, but again, you don't need the connect function because when you start the server, it blocks until it's either connected or it failed. You can just check the returned Error

I suggest making a dedicated proposal for this.

[Shadowblitz16]

the issue is although the connect function isn't needed it would still be nice to have because you can connect to it.
even if it was documented it was for convenience only.

[jonbonazza]

It's not a covenience though. It's actually _less_ convenient than just doing whatever you need immediately after starting the server

[Shadowblitz16]

I disagree you can connect to the signal outside of the of the networking logic which makes it convenient

[jonbonazza]

You are missing the point entirely. The first thing you do is call create_server. As soon as that function returns, you are either connect successfully or you've failed to connect. There is no value or reason to use a signal ever.

There is no world where

func _ready():
    var peer = NetworkMultiPlayerENet.new()
    peer.connect("server_connected", self, "_connected)
    peer.create_server(port, 100)

func _connected():
    # we are connected . Do something now
    pass

is more convenient than

func _ready():
    var peer = NetworkMultiPlayerENet.new()
    var err = peer.create_server(port, 100)
    if err != OK:
        # we are connected. do something now

[Shadowblitz16]

Thats not true!
I need to do something in another script when that happens!

[jonbonazza]

Id argue that's a poor code organization problem.

Anyway, i've presented my argument against this enough

I would say that it would be better if each of the suggestions you propose here where a separate proposal. "Collection" proposoals are not ideal because parts of them could be fixed or addressed while others not, leaving the proposal in a state of limbo.

[Shadowblitz16]

ok
https://github.com/godotengine/godot-proposals/issues/1892