better client id structure(s).

Author: SubAt0m1c

Cargo.lock

@@ -16,3 +16,4 @@ fstr = { path = "../crates/fstr"}
 replays = { path = "../replays" }
 glam = "0.30.5"
 enumset = "1.1.10"
+slotmap = "1.0.7"

server/Cargo.toml

@@ -3,11 +3,16 @@ use std::collections::HashMap;
 use anyhow::bail;
 use bytes::{Buf, Bytes, BytesMut};
 use fstr::FString;
+use slotmap::new_key_type;
 use tokio::{io::{self, AsyncReadExt, AsyncWriteExt}, net::{TcpStream, tcp::{OwnedReadHalf, OwnedWriteHalf}}, sync::mpsc::UnboundedSender, task::JoinHandle};
 use uuid::Uuid;
 
 use crate::{ClientId, GameProfile, GameProfileProperty, network::{binary::var_int::{VarInt, peek_var_int}, connection_state::ConnectionState, internal_packets::{MainThreadMessage, NetworkThreadMessage}, packets::{packet_buffer::PacketBuffer, packet_deserialize::PacketDeserializable}, protocol::{handshake::serverbound::Handshake, login::{clientbound::LoginSuccess, serverbound::LoginStart}, play::serverbound::Play, status::{clientbound::{StatusPong, StatusResponse}, serverbound::StatusPing}}}, types::status::StatusBytes};
 
+new_key_type! {
+    pub struct ClientKey;
+}
+
 #[derive(Debug, Clone)]
 pub struct Client {
     pub id: ClientId,

server/src/network/client.rs

@@ -1,18 +1,16 @@
-use crate::network::client::{ClientHandler};
+use crate::network::client::{ClientHandler, ClientKey};
 use crate::network::connection_state::ConnectionState;
 use crate::types::status::Status;
 use core::panic;
-use std::collections::HashMap;
+use slotmap::SlotMap;
 use tokio::net::TcpListener;
 use tokio::sync::mpsc::{unbounded_channel, UnboundedReceiver, UnboundedSender};
 
 use crate::network::internal_packets::{MainThreadMessage, NetworkThreadMessage};
-use crate::player::player::ClientId;
 
 type Sender<T> = UnboundedSender<T>;
 type Receiver<T> = UnboundedReceiver<T>;
 
-type ClientMap = HashMap<ClientId, ClientHandler>;
 
 pub fn start_network(
     ip: &'static str,
@@ -34,21 +32,20 @@ async fn run_network_thread(
     let listener = TcpListener::bind(ip).await.unwrap();
     println!("Network thread listening on {ip}");
 
-    let mut clients: ClientMap = HashMap::new();
-    let mut client_id_counter: ClientId = 1;
-
+    // slotmap is faster than a hashmap and works just as well for us here
+    let mut clients: SlotMap<ClientKey, ClientHandler> = SlotMap::with_key();
+    
     loop {
         tokio::select! {
             // a client failing to connect here is recoverable and doesnt really do anything, so we can just ignore it.
             // we do need to continue on a failed connection though, otherwise it would need to wait for network_rx to receive
             // before attempting to get a new connection.
             result = listener.accept() => {
                 let Ok((socket, _)) = result else { continue };
-                let client_id: ClientId = client_id_counter;
-                client_id_counter += 1;
 
-                let handler = ClientHandler::spawn(client_id, socket, network_tx.clone(), main_tx.clone(), status.get());
-                clients.insert(client_id, handler);
+                clients.insert_with_key(|key| {
+                    ClientHandler::spawn(key, socket, network_tx.clone(), main_tx.clone(), status.get())
+                });
             }
 
             // this can never be none since this function owns a network_tx.
@@ -58,27 +55,27 @@ async fn run_network_thread(
                 match msg { 
                     NetworkThreadMessage::UpdateStatus(update) => status.set(update),
                     NetworkThreadMessage::SendPackets { client_id, buffer } => {
-                        if let Some(handler) = clients.get_mut(&client_id) {
+                        if let Some(handler) = clients.get_mut(client_id) {
                             if let Err(e) = handler.send(&buffer).await {
-                                eprintln!("Client {} handler failed to send: {}", client_id, e);
-                                clients.remove(&client_id);
+                                eprintln!("Client {:?} handler failed to send: {}", client_id, e);
+                                clients.remove(client_id);
                                 main_tx.send(MainThreadMessage::ClientDisconnected { client_id }).expect("Main thread should never drop its network reciever.");
                             }
                         }
                     }
 
                     NetworkThreadMessage::DisconnectClient { client_id } => {
-                        if let Some(handler) = clients.remove(&client_id) {
+                        if let Some(handler) = clients.remove(client_id) {
                             if let Err(e) = handler.disconnect().await {
-                                eprintln!("Client {} writer failed to shutdown: {}", client_id, e);
+                                eprintln!("Client {:?} writer failed to shutdown: {}", client_id, e);
                             }
                             main_tx.send(MainThreadMessage::ClientDisconnected { client_id }).expect("Main thread should never drop its network reciever.");
                         }
                     }
 
                     NetworkThreadMessage::ConnectionClosed { client_id, connection_state } => {
                         // we probably shouldnt tell the main thread a client it never added got disconnected? 
-                        if clients.remove(&client_id).is_some() && connection_state == ConnectionState::Play {
+                        if clients.remove(client_id).is_some() && connection_state == ConnectionState::Play {
                             main_tx.send(MainThreadMessage::ClientDisconnected { client_id }).expect("Main thread should never drop its network reciever.");
                         }
                     }

server/src/network/network.rs

@@ -5,6 +5,7 @@ use crate::inventory::item_stack::ItemStack;
 use crate::inventory::menu::OpenContainer;
 use crate::inventory::Inventory;
 use crate::network::binary::var_int::VarInt;
+use crate::network::client::ClientKey;
 use crate::network::packets::packet::IdentifiedPacket;
 use crate::network::packets::packet_buffer::PacketBuffer;
 use crate::network::packets::packet_serialize::PacketSerializable;
@@ -24,7 +25,7 @@ use std::f32::consts::PI;
 use std::ptr::NonNull;
 use uuid::Uuid;
 
-pub type ClientId = usize;
+pub type ClientId = ClientKey;
 
 #[derive(Debug, Clone)]
 pub struct GameProfileProperty {

server/src/player/player.rs

@@ -4,6 +4,7 @@ use crate::network::packets::packet_buffer::PacketBuffer;
 use crate::network::protocol::play::clientbound::ChunkData;
 use crate::player::player::ClientId;
 use glam::DVec3;
+use slotmap::SparseSecondaryMap;
 use std::collections::HashSet;
 
 pub struct ChunkSection {
@@ -15,6 +16,8 @@ pub struct Chunk {
     pub packet_buffer: PacketBuffer,
 
     pub players: HashSet<ClientId>,
+    // this was the same behavior as before though so maybe it doesnt matter.
+    pub players: SparseSecondaryMap<ClientId, ()>,
     pub entities: HashSet<EntityId>,
 
     // contains the chunk data packet,
@@ -27,11 +30,10 @@ pub struct Chunk {
 
 impl Chunk {
 
-    pub fn new() -> Self {
-        Self {
-            chunk_sections: [const { None }; 16],
+    pub fn             players: HashSet::new(),
+      chunk_sections: [const { None }; 16],
             packet_buffer: PacketBuffer::new(),
-            players: HashSet::new(),
+            players: SparseSecondaryMap::new(),
             entities: HashSet::new(),
 
             cached_chunk_data: PacketBuffer::new(),
@@ -121,23 +123,19 @@ impl Chunk {
                 bitmask,
                 data,
             });
-            self.dirty = false;
-        }
-        into.copy_from(&self.cached_chunk_data);
-    }
-    pub fn insert_player(&mut self, client_id: ClientId) {
-        debug_assert!(!self.players.contains(&client_id), "player already in chunk");
+            self        debug_assert!(!self.players.contains(&client_id), "player already in chunk");
         self.players.insert(client_id);
+: ClientId) {
+        debug_assert!(!self.players.contains_key(client_id), "player already in chunk");
+        self.players.insert(client_id, ());
     }
 
     pub fn insert_entity(&mut self, entity_id: EntityId) {
-        debug_assert!(!self.entities.contains(&entity_id), "entity already in chunk");
-        self.entities.insert(entity_id);
-    }
-
-    pub fn remove_player(&mut self, client_id: ClientId) {
-        debug_assert!(self.players.contains(&client_id), "player was never in this chunk");
+        debug_assert!(!self.entities.contains(&        debug_assert!(self.players.contains(&client_id), "player was never in this chunk");
         self.players.remove(&client_id);
+ClientId) {
+        debug_assert!(self.players.contains_key(client_id), "player was never in this chunk");
+        self.players.remove(client_id);
     }
 
     pub fn remove_entity(&mut self, entity_id: EntityId) {

server/src/world/chunk/chunk.rs

@@ -12,6 +12,7 @@ use crate::world::chunk::chunk_grid::ChunkGrid;
 use crate::world::chunk::get_chunk_position;
 use enumset::EnumSet;
 use glam::{DVec3, Vec3};
+use slotmap::SecondaryMap;
 use std::collections::HashMap;
 use std::ops::{Deref, DerefMut};
 use tokio::sync::mpsc::UnboundedSender;
@@ -34,9 +35,9 @@ pub trait WorldExtension : Sized {
 
 pub struct World<E : WorldExtension> {
     pub network_tx: UnboundedSender<NetworkThreadMessage>,
-
+    
     pub players: Vec<Player<E::Player>>,
-    pub player_map: HashMap<ClientId, usize>,
+    pub player_map: SecondaryMap<ClientId, usize>,
     pub npc_profiles: HashMap<&'static str, GameProfile>,
 
     entity_id: i32,
@@ -58,7 +59,7 @@ impl<E : WorldExtension> World<E> {
         Self {
             network_tx,
             players: Vec::new(),
-            player_map: HashMap::new(),
+            player_map: SecondaryMap::new(),
             npc_profiles,
             entity_id: 0,
             entities: Vec::new(),
@@ -214,7 +215,7 @@ impl<E : WorldExtension> World<E> {
         // order isn't preserved doing this. 
         // however with old implementation, it did use std hashmap to iterate, 
         // and order wasn't preserved there so it should be fine. 
-        if let Some(index) = self.player_map.remove(&client_id) {
+        if let Some(index) = self.player_map.remove(client_id) {
             let last_index = self.players.len() - 1;
             let player = self.players.swap_remove(index);
 
@@ -289,7 +290,7 @@ impl<E : WorldExtension> World<E> {
                 let _ = self.network_tx.send(NetworkThreadMessage::UpdateStatus(StatusUpdate::Players(self.players.len() as u32)));
             }
             MainThreadMessage::PacketReceived { client_id, packet } => {
-                if let Some(index) = self.player_map.get(&client_id) {
+                if let Some(index) = self.player_map.get(client_id) {
                     let player = &mut self.players[*index];
                     packet.process(player)
                 }