Refactor skin selection to command

HKMP/Api/Client/IClientManager.cs

@@ -59,6 +59,7 @@ public interface IClientManager {
     /// Changes the skin of the local player.
     /// </summary>
     /// <param name="skinId">The ID of the skin.</param>
+    [Obsolete("ChangeSkin is deprecated. Skin changes are handled by the IServerManager.")]
     void ChangeSkin(byte skinId);
 
     /// <summary>

HKMP/Game/Client/ClientManager.cs

@@ -248,9 +248,6 @@ ModSettings modSettings
         };
         uiManager.RequestClientDisconnectEvent += Disconnect;
 
-        // uiManager.SettingsInterface.OnTeamRadioButtonChange += InternalChangeTeam;
-        // uiManager.SettingsInterface.OnSkinIdChange += InternalChangeSkin;
-
         UiManager.InternalChatBox.ChatInputEvent += OnChatInput;
 
         netClient.ConnectEvent += _ => uiManager.OnSuccessfulConnect();
@@ -414,27 +411,6 @@ private void OnChatInput(string message) {
         _netClient.UpdateManager.SetChatMessage(message);
     }
 
-    /// <summary>
-    /// Internal method for changing the local player skin.
-    /// </summary>
-    /// <param name="skinId">The ID of the new skin.</param>
-    private void InternalChangeSkin(byte skinId) {
-        if (!_netClient.IsConnected) {
-            return;
-        }
-
-        if (!_serverSettings.AllowSkins) {
-            Logger.Debug("User changed skin ID, but skins are not allowed by server");
-            return;
-        }
-
-        Logger.Debug($"Changed local player skin to ID: {skinId}");
-
-        // Let the player manager handle the skin updating and send the change to the server
-        _playerManager.UpdateLocalPlayerSkin(skinId);
-        _netClient.UpdateManager.SetSkinUpdate(skinId);
-    }
-
     /// <summary>
     /// Callback method for when the net client establishes a connection with a server.
     /// </summary>
@@ -1041,11 +1017,6 @@ public void ChangeTeam(Team team) {
 
     /// <inheritdoc />
     public void ChangeSkin(byte skinId) {
-        if (!_netClient.IsConnected) {
-            throw new InvalidOperationException("Client is not connected, cannot change skin");
-        }
-
-        InternalChangeSkin(skinId);
     }
 
     #endregion

HKMP/Game/Client/PlayerManager.cs

@@ -597,29 +597,35 @@ public Team GetPlayerTeam(ushort id) {
         return playerData.Team;
     }
 
-    /// <summary>
-    /// Update the skin of the local player.
-    /// </summary>
-    /// <param name="skinId">The ID of the skin to update to.</param>
-    public void UpdateLocalPlayerSkin(byte skinId) {
-        _skinManager.UpdateLocalPlayerSkin(skinId);
-    }
-
     /// <summary>
     /// Callback method for when a player updates their skin.
     /// </summary>
     /// <param name="playerSkinUpdate">The ClientPlayerSkinUpdate packet data.</param>
     private void OnPlayerSkinUpdate(ClientPlayerSkinUpdate playerSkinUpdate) {
-        var id = playerSkinUpdate.Id;
         var skinId = playerSkinUpdate.SkinId;
+
+        if (playerSkinUpdate.Self) {
+            Logger.Debug($"Received PlayerSkinUpdate for local player: {skinId}");
+
+            _skinManager.UpdateLocalPlayerSkin(skinId);
+            return;
+        }
+
+        var id = playerSkinUpdate.Id;
+        Logger.Debug($"Received PlayerSkinUpdate for ID: {id}, skin ID: {skinId}");
 
         if (!_playerData.TryGetValue(id, out var playerData)) {
-            Logger.Debug($"Received PlayerSkinUpdate for ID: {id}, skinId: {skinId}");
+            Logger.Debug("  Could not find player");
             return;
         }
 
         playerData.SkinId = skinId;
 
+        // If the player is not in the local scene, we don't have to apply the skin update to the player object
+        if (!playerData.IsInLocalScene) {
+            return;
+        }
+
         _skinManager.UpdatePlayerSkin(playerData.PlayerObject, skinId);
     }
 

HKMP/Game/Command/Server/SkinCommand.cs

@@ -0,0 +1,55 @@
+using System;
+using Hkmp.Api.Command.Server;
+using Hkmp.Game.Server;
+
+namespace Hkmp.Game.Command.Server;
+
+/// <summary>
+/// Command for changing the skin of the player.
+/// </summary>
+internal class SkinCommand : IServerCommand {
+    /// <inheritdoc />
+    public string Trigger => "/skin";
+
+    /// <inheritdoc />
+    public string[] Aliases => Array.Empty<string>();
+
+    /// <inheritdoc />
+    public bool AuthorizedOnly => false;
+
+    /// <summary>
+    /// The server manager instance.
+    /// </summary>
+    private readonly ServerManager _serverManager;
+
+    public SkinCommand(ServerManager serverManager) {
+        _serverManager = serverManager;
+    }
+
+    /// <inheritdoc />
+    public void Execute(ICommandSender commandSender, string[] arguments) {
+        if (commandSender.Type == CommandSenderType.Console) {
+            commandSender.SendMessage("Console cannot change skins.");
+            return;
+        }
+
+        var sender = (PlayerCommandSender) commandSender;
+
+        if (arguments.Length != 2) {
+            sender.SendMessage($"Usage: {Trigger} <skin ID>");
+            return;
+        }
+
+        var skinIdArg = arguments[1];
+        if (!byte.TryParse(skinIdArg, out var skinId)) {
+            sender.SendMessage($"Unknown skin ID '{skinIdArg}', please provide a value between 0-255");
+            return;
+        }
+
+        if (_serverManager.TryUpdatePlayerSkin(sender.Id, skinId, out var reason)) {
+            sender.SendMessage($"Skin ID changed to '{skinId}'");
+        } else {
+            sender.SendMessage(reason);
+        }
+    }
+}

HKMP/Game/Server/ServerManager.cs

@@ -147,8 +147,6 @@ PacketManager packetManager
             OnReliableEntityUpdate);
         packetManager.RegisterServerPacketHandler(ServerPacketId.PlayerDisconnect, OnPlayerDisconnect);
         packetManager.RegisterServerPacketHandler(ServerPacketId.PlayerDeath, OnPlayerDeath);
-        packetManager.RegisterServerPacketHandler<ServerPlayerSkinUpdate>(ServerPacketId.PlayerSkinUpdate,
-            OnPlayerSkinUpdate);
         packetManager.RegisterServerPacketHandler<ChatMessage>(ServerPacketId.ChatMessage, OnChatMessage);
         packetManager.RegisterServerPacketHandler<SaveUpdate>(ServerPacketId.SaveUpdate, OnSaveUpdate);
 
@@ -183,6 +181,7 @@ protected virtual void RegisterCommands() {
         CommandManager.RegisterCommand(new BanCommand(_banList, this));
         CommandManager.RegisterCommand(new KickCommand(this));
         CommandManager.RegisterCommand(new TeamCommand(this));
+        CommandManager.RegisterCommand(new SkinCommand(this));
     }
 
     /// <summary>
@@ -994,7 +993,7 @@ public bool TryUpdatePlayerTeam(ushort id, Team team, out string reason) {
         if (!_playerData.TryGetValue(id, out var playerData)) {
             Logger.Warn($"Received PlayerTeamUpdate data, but player with ID {id} is not in mapping");
 
-            reason = "Could not find player.";
+            reason = "Could not find player";
             return false;
         }
 
@@ -1003,7 +1002,7 @@ public bool TryUpdatePlayerTeam(ushort id, Team team, out string reason) {
         if (!ServerSettings.TeamsEnabled) {
             Logger.Info("  Teams are not enabled, won't update team");
 
-            reason = "Unable to change team.";
+            reason = "Unable to change team";
             return false;
         }
 
@@ -1028,33 +1027,59 @@ public bool TryUpdatePlayerTeam(ushort id, Team team, out string reason) {
     }
 
     /// <summary>
-    /// Callback method for when a player updates their skin.
+    /// Try to update the skin for the player with the given ID.
     /// </summary>
     /// <param name="id">The ID of the player.</param>
-    /// <param name="skinUpdate">The ServerPlayerSkinUpdate packet data.</param>
-    private void OnPlayerSkinUpdate(ushort id, ServerPlayerSkinUpdate skinUpdate) {
+    /// <param name="skinId">The ID of the skin to change the player to.</param>
+    /// <param name="reason">The reason if the skin could not be updated, otherwise null.</param>
+    /// <returns>True if the player's team was updated, false otherwise.</returns>
+    public bool TryUpdatePlayerSkin(ushort id, byte skinId, out string reason) {
         if (!_playerData.TryGetValue(id, out var playerData)) {
             Logger.Warn($"Received PlayerSkinUpdate data, but player with ID {id} is not in mapping");
-            return;
+
+            reason = "Could not find player";
+            return false;
         }
-
-        if (playerData.SkinId == skinUpdate.SkinId) {
-            Logger.Debug($"Received PlayerSkinUpdate data from ({id}, {playerData.Username}), but skin was the same");
-            return;
+
+        Logger.Info($"Received PlayerSkinUpdate data from ({id}, {playerData.Username}) for skin ID: {skinId}");
+
+        if (!ServerSettings.AllowSkins) {
+            Logger.Info("  Skins are not allowed, won't update skin");
+
+            reason = "Unable to change skin";
+            return false;
         }
 
-        Logger.Debug($"Received PlayerSkinUpdate data from ({id}, {playerData.Username}), new skin ID: {skinUpdate.SkinId}");
+        if (playerData.SkinId == skinId) {
+            Logger.Info("  Skins is the same as current, won't update skin");
+
+            reason = "Skin is already in use";
+            return false;
+        }
 
         // Update the skin ID in the player data
-        playerData.SkinId = skinUpdate.SkinId;
-
-        SendDataInSameScene(
-            id,
-            playerData.CurrentScene,
-            otherId => {
-                _netServer.GetUpdateManagerForClient(otherId)?.AddPlayerSkinUpdateData(id, playerData.SkinId);
+        playerData.SkinId = skinId;
+
+        foreach (var idPlayerDataPair in _playerData) {
+            var otherId = idPlayerDataPair.Key;
+
+            if (otherId == id) {
+                _netServer.GetUpdateManagerForClient(id)?.AddPlayerSkinUpdateData(skinId);
+                continue;
             }
-        );
+
+            var otherPd = idPlayerDataPair.Value;
+
+            // Skip sending skin to players not in the same scene
+            if (!string.Equals(otherPd.CurrentScene, playerData.CurrentScene)) {
+                continue;
+            }
+
+            _netServer.GetUpdateManagerForClient(otherId)?.AddOtherPlayerSkinUpdateData(id, skinId);
+        }
+
+        reason = null;
+        return true;
     }
 
     /// <summary>

HKMP/Networking/Client/ClientUpdateManager.cs

@@ -328,19 +328,6 @@ public void SetPlayerDisconnect() {
         }
     }
 
-    /// <summary>
-    /// Set a skin update in the current packet.
-    /// </summary>
-    /// <param name="skinId">The ID of the skin of the player.</param>
-    public void SetSkinUpdate(byte skinId) {
-        lock (Lock) {
-            CurrentUpdatePacket.SetSendingPacketData(
-                ServerPacketId.PlayerSkinUpdate,
-                new ServerPlayerSkinUpdate { SkinId = skinId }
-            );
-        }
-    }
-
     /// <summary>
     /// Set hello server data in the current packet.
     /// </summary>

HKMP/Networking/Packet/Data/PlayerSkinUpdate.cs

@@ -4,6 +4,11 @@ namespace Hkmp.Networking.Packet.Data;
 /// Packet data for client-bound player skin update.
 /// </summary>
 internal class ClientPlayerSkinUpdate : GenericClientData {
+    /// <summary>
+    /// Whether the skin update is for the player receiving this packet.
+    /// </summary>
+    public bool Self { get; set; }
+
     /// <summary>
     /// The ID of the skin.
     /// </summary>
@@ -14,44 +19,28 @@ internal class ClientPlayerSkinUpdate : GenericClientData {
     /// </summary>
     public ClientPlayerSkinUpdate() {
         IsReliable = true;
-        DropReliableDataIfNewerExists = true;
+        DropReliableDataIfNewerExists = false;
     }
 
     /// <inheritdoc />
     public override void WriteData(IPacket packet) {
-        packet.Write(Id);
+        packet.Write(Self);
+
+        if (!Self) {
+            packet.Write(Id);
+        }
+
         packet.Write(SkinId);
     }
 
     /// <inheritdoc />
     public override void ReadData(IPacket packet) {
-        Id = packet.ReadUShort();
-        SkinId = packet.ReadByte();
-    }
-}
+        Self = packet.ReadBool();
 
-/// <summary>
-/// Packet data for the server-bound player skin update.
-/// </summary>
-internal class ServerPlayerSkinUpdate : IPacketData {
-    /// <inheritdoc />
-    public bool IsReliable => true;
+        if (!Self) {
+            Id = packet.ReadUShort();
+        }
 
-    /// <inheritdoc />
-    public bool DropReliableDataIfNewerExists => true;
-
-    /// <summary>
-    /// The ID of the skin.
-    /// </summary>
-    public byte SkinId { get; set; }
-
-    /// <inheritdoc />
-    public void WriteData(IPacket packet) {
-        packet.Write(SkinId);
-    }
-
-    /// <inheritdoc />
-    public void ReadData(IPacket packet) {
         SkinId = packet.ReadByte();
     }
 }

HKMP/Networking/Packet/PacketId.cs

@@ -164,18 +164,13 @@ public enum ServerPacketId {
     /// </summary>
     PlayerDeath = 10,
 
-    /// <summary>
-    /// Notify that a player has changed skins.
-    /// </summary>
-    PlayerSkinUpdate = 11,
-
     /// <summary>
     /// Player sent chat message.
     /// </summary>
-    ChatMessage = 12,
+    ChatMessage = 11,
 
     /// <summary>
     /// Value in the save file has updated.
     /// </summary>
-    SaveUpdate = 13,
+    SaveUpdate = 12,
 }

HKMP/Networking/Packet/UpdatePacket.cs

@@ -873,8 +873,6 @@ protected override IPacketData InstantiatePacketDataFromId(ServerPacketId packet
                 return new PacketDataCollection<ReliableEntityUpdate>();
             case ServerPacketId.PlayerEnterScene:
                 return new ServerPlayerEnterScene();
-            case ServerPacketId.PlayerSkinUpdate:
-                return new ServerPlayerSkinUpdate();
             case ServerPacketId.ChatMessage:
                 return new ChatMessage();
             case ServerPacketId.SaveUpdate: