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

Use a dedicated type for client ID #41

Closed
Shatur opened this issue Aug 28, 2022 · 10 comments · Fixed by #103
Closed

Use a dedicated type for client ID #41

Shatur opened this issue Aug 28, 2022 · 10 comments · Fixed by #103
Labels
enhancement New feature or request

Comments

@Shatur
Copy link
Contributor

Shatur commented Aug 28, 2022

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.

@lucaspoffo lucaspoffo added the enhancement New feature or request label Oct 1, 2022
@Shatur
Copy link
Contributor Author

Shatur commented Oct 4, 2022

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
Copy link
Contributor

roboteng commented Jul 25, 2023

@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
Copy link
Contributor

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
Copy link
Contributor Author

Shatur commented Jul 29, 2023

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
Copy link
Contributor

TrustNoOneElse commented Jul 29, 2023

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
Copy link
Contributor Author

Shatur commented Jul 29, 2023

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
Copy link
Contributor

TrustNoOneElse commented Jul 29, 2023

I would support that name (ClientId)

@Shatur
Copy link
Contributor Author

Shatur commented Jul 29, 2023

@roboteng what do you think about this name?

@roboteng
Copy link
Contributor

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
Copy link
Contributor Author

Shatur commented Aug 12, 2023

@roboteng we thinking about ClientId

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants