fix: latent issues in lobby management + added tests

OpenPolytopia.Common/LobbyData.cs

@@ -55,6 +55,11 @@ public class LobbyData : INetworkSerializable {
   /// </summary>
   public uint MaxPlayers;
 
+  /// <summary>
+  /// Size of the world the game will be played on
+  /// </summary>
+  public uint WorldSize;
+
   /// <summary>
   /// If the game in the lobby has started
   /// </summary>
@@ -89,6 +94,7 @@ public class LobbyData : INetworkSerializable {
   public void Serialize(List<byte> bytes) {
     Id.Serialize(bytes);
     MaxPlayers.Serialize(bytes);
+    WorldSize.Serialize(bytes);
     Started.Serialize(bytes);
     Starting.Serialize(bytes);
     Players.Serialize(bytes);
@@ -97,6 +103,7 @@ public void Serialize(List<byte> bytes) {
   public void Deserialize(byte[] bytes, ref uint index) {
     Id.Deserialize(bytes, ref index);
     MaxPlayers.Deserialize(bytes, ref index);
+    WorldSize.Deserialize(bytes, ref index);
     Started.Deserialize(bytes, ref index);
     Starting.Deserialize(bytes, ref index);
     Players.Deserialize(bytes, ref index);

OpenPolytopia.Common/LobbyRules.cs

@@ -0,0 +1,61 @@
+namespace OpenPolytopia.Common;
+
+/// <summary>
+/// Rules a lobby must respect to be valid
+/// </summary>
+/// <remarks>
+/// Lives in the common assembly so the client can check a lobby before asking the server for it
+/// </remarks>
+public static class LobbyRules {
+  /// <summary>
+  /// Smallest world a game can be played on
+  /// </summary>
+  public const uint MIN_WORLD_SIZE = 11;
+
+  /// <summary>
+  /// Biggest world a game can be played on
+  /// </summary>
+  public const uint MAX_WORLD_SIZE = 30;
+
+  /// <summary>
+  /// Minimum players needed to start a game; solo games aren't allowed
+  /// </summary>
+  public const uint MIN_PLAYERS = 2;
+
+  /// <summary>
+  /// World size from which a lobby can hold <see cref="MAX_PLAYERS_BIG_WORLD"/> players
+  /// </summary>
+  private const uint BIG_WORLD_SIZE = 14;
+
+  /// <summary>
+  /// Max players a world smaller than <see cref="BIG_WORLD_SIZE"/> can hold
+  /// </summary>
+  private const uint MAX_PLAYERS_SMALL_WORLD = 9;
+
+  /// <summary>
+  /// Max players a world of at least <see cref="BIG_WORLD_SIZE"/> can hold
+  /// </summary>
+  private const uint MAX_PLAYERS_BIG_WORLD = 16;
+
+  /// <summary>
+  /// Max players a lobby on a given world can hold
+  /// </summary>
+  /// <param name="worldSize">the size of the world</param>
+  /// <returns>the max players allowed on that world</returns>
+  public static uint MaxPlayersFor(uint worldSize) =>
+    worldSize < BIG_WORLD_SIZE ? MAX_PLAYERS_SMALL_WORLD : MAX_PLAYERS_BIG_WORLD;
+
+  /// <summary>
+  /// Checks if a world can host a game
+  /// </summary>
+  /// <param name="worldSize">the size of the world</param>
+  public static bool IsValidWorldSize(uint worldSize) => worldSize is >= MIN_WORLD_SIZE and <= MAX_WORLD_SIZE;
+
+  /// <summary>
+  /// Checks if a lobby of a given size can be created on a given world
+  /// </summary>
+  /// <param name="maxPlayers">max players that can join the lobby</param>
+  /// <param name="worldSize">the size of the world</param>
+  public static bool IsValidLobby(uint maxPlayers, uint worldSize) =>
+    IsValidWorldSize(worldSize) && maxPlayers >= MIN_PLAYERS && maxPlayers <= MaxPlayersFor(worldSize);
+}

OpenPolytopia.Common/Network/Packets/LobbyPackets.cs

@@ -35,18 +35,25 @@ public class CreateLobbyPacket : IPacket {
   /// </summary>
   public uint MaxPlayers;
 
+  /// <summary>
+  /// Size of the world the game will be played on
+  /// </summary>
+  public uint WorldSize;
+
   /// <summary>
   /// Tribe chosen by the player creating the lobby
   /// </summary>
   public uint Tribe;
 
   public void Serialize(List<byte> bytes) {
     MaxPlayers.Serialize(bytes);
+    WorldSize.Serialize(bytes);
     Tribe.Serialize(bytes);
   }
 
   public void Deserialize(byte[] bytes, ref uint index) {
     MaxPlayers.Deserialize(bytes, ref index);
+    WorldSize.Deserialize(bytes, ref index);
     Tribe.Deserialize(bytes, ref index);
   }
 }

OpenPolytopia.Server/GameServer.cs

@@ -167,7 +167,7 @@ private async Task ManageCreateLobbyAsync(NetworkConnection connection, CreateLo
       if (!_playerNames.TryGetValue(connection.Id, out var name)) {
         result = LobbyActionResult.NotRegistered;
       }
-      else if (packet.MaxPlayers is < 2 or > 16 || !Enum.IsDefined((TribeType)packet.Tribe)) {
+      else if (!Enum.IsDefined((TribeType)packet.Tribe)) {
         result = LobbyActionResult.InvalidParameters;
       }
       // one lobby per player and a global cap, or a client could flood the server
@@ -178,9 +178,9 @@ private async Task ManageCreateLobbyAsync(NetworkConnection connection, CreateLo
         result = LobbyActionResult.TooManyLobbies;
       }
       else {
-        result = LobbyActionResult.Ok;
-        lobby = _lobbyManager.CreateLobby(packet.MaxPlayers,
-          new LobbyPlayerData { PlayerId = connection.Id, Name = name, Tribe = packet.Tribe });
+        // the lobby rules themselves are checked by the manager, so a lobby is never half-valid
+        result = _lobbyManager.CreateLobby(packet.MaxPlayers, packet.WorldSize,
+          new LobbyPlayerData { PlayerId = connection.Id, Name = name, Tribe = packet.Tribe }, out lobby);
       }
 
       _server.SendTo(connection.Id, new CreateLobbyResponsePacket { Result = result, LobbyId = lobby?.Id ?? 0 });
@@ -232,14 +232,14 @@ private async Task ManageLeaveLobbyAsync(NetworkConnection connection, LeaveLobb
 
       _server.SendTo(connection.Id, new LeaveLobbyResponsePacket { Result = result, LobbyId = packet.LobbyId });
 
-      if (result == LobbyActionResult.Ok && _lobbyManager[packet.LobbyId] is { } lobby) {
-        // remove the lobby if it became empty
-        if (lobby.PlayersCount == 0) {
-          _lobbyManager.RemoveLobby(lobby.Id);
-          _server.Broadcast(new LobbyDeletedPacket { LobbyId = lobby.Id });
+      if (result == LobbyActionResult.Ok) {
+        // the manager drops a lobby as soon as its last player leaves, so a missing
+        // lobby here means it became empty
+        if (_lobbyManager[packet.LobbyId] is { } lobby) {
+          _server.Broadcast(new LobbyUpdatedPacket { Lobby = lobby });
         }
         else {
-          _server.Broadcast(new LobbyUpdatedPacket { Lobby = lobby });
+          _server.Broadcast(new LobbyDeletedPacket { LobbyId = packet.LobbyId });
         }
       }
     }

OpenPolytopia.Server/LobbyManager.cs

@@ -10,15 +10,6 @@ namespace OpenPolytopia.Server;
 /// This class isn't thread-safe, <see cref="GameServer"/> serializes every access through its own lock
 /// </remarks>
 public class LobbyManager {
-  /// <summary>
-  /// Minimum players needed to start a game; solo games aren't allowed
-  /// </summary>
-  /// <remarks>
-  /// <see cref="GameServer"/> already refuses to create a lobby smaller than this,
-  /// this keeps the rule from depending on a check living in another class
-  /// </remarks>
-  private const uint MIN_PLAYERS_TO_START = 2;
-
   private readonly Dictionary<ulong, LobbyData> _lobbies = new();
   private ulong _nextId;
 
@@ -41,13 +32,26 @@ public class LobbyManager {
   /// <summary>
   /// Creates a new lobby and adds the creator to it
   /// </summary>
+  /// <remarks>
+  /// A lobby is never created in an invalid state: the world has to be able to host every player
+  /// </remarks>
   /// <param name="maxPlayers">max players that can join the lobby</param>
+  /// <param name="worldSize">size of the world the game will be played on</param>
   /// <param name="creator">the player creating the lobby</param>
-  /// <returns>the new lobby</returns>
-  public LobbyData CreateLobby(uint maxPlayers, LobbyPlayerData creator) {
-    var lobby = new LobbyData { Id = ++_nextId, MaxPlayers = maxPlayers, Players = [creator] };
+  /// <param name="lobby">the new lobby, or <see langword="null"/> if it couldn't be created</param>
+  /// <returns>the result of the operation</returns>
+  public LobbyActionResult CreateLobby(uint maxPlayers, uint worldSize, LobbyPlayerData creator,
+    out LobbyData? lobby) {
+    if (!LobbyRules.IsValidLobby(maxPlayers, worldSize)) {
+      lobby = null;
+      return LobbyActionResult.InvalidParameters;
+    }
+
+    lobby = new LobbyData {
+      Id = ++_nextId, MaxPlayers = maxPlayers, WorldSize = worldSize, Players = [creator]
+    };
     _lobbies[lobby.Id] = lobby;
-    return lobby;
+    return LobbyActionResult.Ok;
   }
 
   /// <summary>
@@ -100,6 +104,12 @@ public LobbyActionResult LeaveLobby(ulong lobbyId, uint playerId) {
     }
 
     lobby.Players.Remove(player);
+
+    // an empty lobby has nothing left to wait for
+    if (lobby.PlayersCount == 0) {
+      _lobbies.Remove(lobby.Id);
+    }
+
     return LobbyActionResult.Ok;
   }
 
@@ -143,7 +153,7 @@ public LobbyActionResult SetReady(ulong lobbyId, uint playerId, bool ready) {
   /// <param name="lobby">the lobby to check</param>
   private static void TryMarkStarting(LobbyData lobby) {
     if (lobby.PlayersCount == lobby.MaxPlayers
-        && lobby.PlayersCount >= MIN_PLAYERS_TO_START
+        && lobby.PlayersCount >= LobbyRules.MIN_PLAYERS
         && lobby.ReadyCount == lobby.PlayersCount) {
       lobby.Starting = true;
     }