lucaspoffo / renet

Server/Client network library for multiplayer games with authentication and connection management made with Rust
Apache License 2.0
663 stars 68 forks source link

Use a dedicated type for client ID #41

Closed Shatur closed 1 year ago

Shatur commented 2 years ago

It would be great to wrap client ID (currently u64) into a newtype. This will improve code readability and eliminate possible mistakes by using it in wrong place. Something like Entity in Bevy.

Shatur commented 2 years ago

I would suggest to use NetworkId or NetId instead of ClientId. Because games usually refer to server with ID == 1. Renet currently don't use this magic number, but in games when you want to abstract networking (implement listen server, for example) users may use it like this. This is why I suggesting a more generic name.

roboteng commented 1 year ago

@lucaspoffo I started on something here. This is my implementation, so far. What do you think of it?


#[derive(Debug, Hash, PartialEq, Eq, PartialOrd, Ord, Clone, Copy)]
pub struct NetworkId(pub u64);

impl From<u64> for NetworkId {
    fn from(value: u64) -> Self {
        Self(value)
    }
}

impl Deref for NetworkId {
    type Target = u64;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl Display for NetworkId {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)
    }
}

Also, where do you think would be a good spot for this to live?

TrustNoOneElse commented 1 year ago

I wonder when you say NetworkId as a new wrapper for a ClientId. How do you name then internal mappings in your Game? Because i currently use NetworkId as a identifier over the network for a Entity. Because NetworkId sounds for me more like a Generic usecase for everything, but in this case it just recieve this naming, because there is a possibility for a listen server as example. That why i would suggest the name ConnectionID when you want a more general naming.

Shatur commented 1 year ago

How do you name then internal mappings in your Game? Because i currently use NetworkId as a identifier over the network for a Entity.

I map entities to entities directly using built-in MapEntities trait. See https://github.com/lifescapegame/bevy_replicon for more details.

That why i would suggest the name ConnectionID when you want a more general naming.

I'm not sure... I like the explicitness of the name, but it's a bit too long. I suggested NetworkId because that's how it's named in Godot.

TrustNoOneElse commented 1 year ago

I map entities to entities directly using built-in MapEntities trait. See https://github.com/lifescapegame/bevy_replicon for more details.

Pretty interesting, i will look into this, thank you!

I'm not sure... I like the explicitness of the name, but it's a bit too long. I suggested NetworkId because that's how it's named in Godot.

I mean you could like with NetworkId -> NetId name it like ConnectionId -> ConnId, but generally shouldn't the name length not really matter, as long as it make sense and the use is descriptive for everybody, before you shorten names. Ah okay i come from Unity, so thats why i was not aware of Godot naming here. In Unity you usally has ClientId even for the Server, ConnectionId or NetConnection from some other Networking Libraries. I just looked up Godot, i mean the method sounds like NetworkId but there RPCs says peer_id. I dunno, just wanted to share my thoughts, because it sounds for me to generic/generally, for the "simple" usecase of identify client/server.

Shatur commented 1 year ago

because it sounds for me to generic/generally, for the "simple" usecase of identify client/server.

I agree with you. Maybe go with ClientId then? Renet itself uses this alias inside renetcode.

TrustNoOneElse commented 1 year ago

I would support that name (ClientId)

Shatur commented 1 year ago

@roboteng what do you think about this name?

roboteng commented 1 year ago

I get that NetworkId is more vague. Would ConnId be any better? Otherwise, I like the explicitness of ConnectionId and then people could alias it to something shorter if they'd like

Shatur commented 1 year ago

@roboteng we thinking about ClientId