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

Multiplayer networking renames/simplification #52480

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

mhilbrunner
Copy link
Member

@mhilbrunner mhilbrunner commented Sep 7, 2021

Removes the networking prefix from some methods and members, now that multiplayer has been largely moved out of Node and SceneTree and is seperated into its own set of classes. Previously, it was useful to mark these so people knew they were multiplayer related when looking at them on Node or SceneTree, but now they have moved to Multiplayer its self-evident and just makes them longer and less readable.

Part of #16863.


RENAMES

MultiplayerAPI
Functions and Properties
multiplayer.is_network_server() -> multiplayer.is_server()

multiplayer.network_peer -> multiplayer.multiplayer_peer
multiplayer.has_network_peer() ->multiplayer.has_multiplayer_peer()
multiplayer.get_network_peer() -> multiplayer.get_multiplayer_peer()
multiplayer.set_network_peer() -> multiplayer.set_multiplayer_peer()

multiplayer.get_network_connected_peers() -> multiplayer.get_peers()

multiplayer.get_network_unique_id -> multiplayer.get_unique_id()

multiplayer.refuse_new_network_connections -> multiplayer.refuse_new_connections
multiplayer.set_refuse_new_network_connections() -> multiplayer.set_refuse_new_connections()
multiplayer.is_refusing_new_network_connections() -> multiplayer.is_refusing_new_connections

Signals
network_peer_connected -> peer_connected
network_peer_disconnected -> peer_disconnected
network_peer_packet -> peer_packet

Node
Functions and Properties

Node.network_authority -> Node.multiplayer_authority
Node.is_network_authority() -> Node.is_multiplayer_authority()
Node.set_network_authority() -> Node.set_multiplayer_authority()
Node.get_network_authority() -> Node.get_multiplayer_authority()


Note this contains very minor changes to some log messages that are now cleaned up in RPCManager::_send_rpc(). I had to edit a few of them anyway for consistency with these changes.

@aleokdev
Copy link

aleokdev commented Sep 7, 2021

+1 for get_peers, connection is implied as you mentioned.

@Faless
Copy link
Collaborator

Faless commented Sep 7, 2021

Reaming of get/has_network_peer to has/get_peer could be a bit confusing (i.e. with has_peer(peer_id)),
And I think get_connected_peers is like that for the same reason (i.e. distinguishing between what we call a network_peer -> MultiplayerPeer instance, and a connected_peer or simply peer, meaning a remote peer ID).

So, I would leave get/has_network_peer untouched, but I'm okay with renaming get_connected_peers to just get_peers.
Alternatively, we could rename get/has_network_peer to get/has_multiplayer_peer (staying consistent with the idea that the result is a MultiplayerPeer instance or null),

EDIT: The other renames looks good.

@mhilbrunner
Copy link
Member Author

We discussed this briefly and decided to indeed rename multiplayer.get_network_connected_peers() to just multiplayer.get_peers().

get/has_network_peer will become get/has_multiplayer_peer, and we'll change network authority to multiplayer authority for consistency. Will do so tomorrow :)

@mhilbrunner mhilbrunner force-pushed the network-rename branch 2 times, most recently from 94ec64c to 9822efd Compare September 8, 2021 09:54
Removes _networking_ prefix from some methods and members, now that multiplayer has been largely moved out of Node and SceneTree and is seperated into its own set of classes.
@mhilbrunner
Copy link
Member Author

@Faless Updated as discussed, MultiplayerAPI.(set/has/get_)network_peer is now MultiplayerAPI.(set/has/get_)multiplayer_peer, Node.(set/get_)network_authority is now Node.(set/get_)multiplayer_authority and MultiplayerAPI.get_network_connected_peers() is now just MultiplayerAPI.get_peers().

Also updated the PR description to reflect these changes.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! 👍 . Thanks!

@Faless Faless merged commit fd0a2b6 into godotengine:master Sep 8, 2021
@mhilbrunner mhilbrunner deleted the network-rename branch September 8, 2021 12:33
@nathanfranke
Copy link
Contributor

var net := ENetMultiplayerPeer.new()
var err := net.create_server(12345)
assert(err == OK)
get_tree().multiplayer.multiplayer_peer = net

get_tree().multiplayer.multiplayer_peer = net is pretty verbose, what about get_tree().multiplayer.peer = net? Would that be confusing compared to get_peers()? Or am I just nitpicking?

@Faless
Copy link
Collaborator

Faless commented Sep 12, 2021

@nathanfranke you don't need to call get_tree() if you are not using the custom multiplayer override. You can just call multiplayer.
multiplayer.set_multiplayer_peer(peer) or multiplayer.multiplayer_peer = net.

Edit, and yes, I fear that will be confusing, especially the get_peer() function.

@mhilbrunner
Copy link
Member Author

Yeah, I had changed it to just peer first as you suggest, but it was indeed unclear because of the peer functions on multiplayer that aren't related, thus we changed it to multiplayer_peer. Its verbose, but usually you shouldn't need it too often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants