remove player from game on 98 command, not 84 command

src/Lobby.cc

@@ -12,11 +12,24 @@ using namespace std;
 
 
 
-Lobby::Lobby() : lobby_id(0), min_level(0), max_level(0xFFFFFFFF),
+Lobby::Lobby(uint32_t id)
-    next_game_item_id(0x00810000), version(GameVersion::GC), section_id(0),
+  : log(string_printf("[Lobby/%" PRIX32 "] ", id), lobby_log.min_level),
-    episode(1), difficulty(0), mode(0), rare_seed(random_object<uint32_t>()),
+    lobby_id(id),
-    event(0), block(0), type(0), leader_id(0), max_clients(12), flags(0) {
+    min_level(0),
-
+    max_level(0xFFFFFFFF),
+    next_game_item_id(0x00810000),
+    version(GameVersion::GC),
+    section_id(0),
+    episode(1),
+    difficulty(0),
+    mode(0),
+    rare_seed(random_object<uint32_t>()),
+    event(0),
+    block(0),
+    type(0),
+    leader_id(0),
+    max_clients(12),
+    flags(0) {
   for (size_t x = 0; x < 12; x++) {
     this->next_item_id[x] = 0x00010000 + 0x00200000 * x;
   }

src/ReceiveCommands.cc

@@ -950,7 +950,9 @@ void process_menu_selection(shared_ptr<ServerState> s, shared_ptr<Client> c,
         }
       }
 
-      s->change_client_lobby(c, game);
+      if (!s->change_client_lobby(c, game)) {
+        throw logic_error("client cannot join game after all preconditions satisfied");
+      }
       c->flags |= Client::Flag::LOADING;
       break;
     }
@@ -1110,20 +1112,37 @@ void process_change_lobby(shared_ptr<ServerState> s, shared_ptr<Client> c,
     return;
   }
 
-  shared_ptr<Lobby> new_lobby;
+  // If the client isn't in any lobby, then they just left a game. Ignore their
-  try {
+  // selection and add them to any lobby with room. If they're already in a
-    new_lobby = s->find_lobby(cmd.item_id);
+  // lobby, then they used the lobby teleporter - add them to a specific lobby.
-  } catch (const out_of_range&) {
+  if (c->lobby_id == 0) {
-    send_lobby_message_box(c, u"$C6Can't change lobby\n\n$C7The lobby does not\nexist.");
+    shared_ptr<Lobby> new_lobby;
-    return;
+    try {
-  }
+      new_lobby = s->find_lobby(cmd.item_id);
+    } catch (const out_of_range&) { }
 
-  if ((new_lobby->flags & Lobby::Flag::EPISODE_3_ONLY) && !(c->flags & Client::Flag::EPISODE_3)) {
+    s->add_client_to_available_lobby(c, new_lobby);
-    send_lobby_message_box(c, u"$C6Can't change lobby\n\n$C7The lobby is for\nEpisode 3 only.");
-    return;
-  }
 
-  s->change_client_lobby(c, new_lobby);
+  // If the client already is in a lobby, then they're using the lobby
+  // teleporter; add them to the lobby they requested or send a failure message.
+  } else {
+    shared_ptr<Lobby> new_lobby;
+    try {
+      new_lobby = s->find_lobby(cmd.item_id);
+    } catch (const out_of_range&) {
+      send_lobby_message_box(c, u"$C6Can't change lobby\n\n$C7The lobby does not\nexist.");
+      return;
+    }
+
+    if ((new_lobby->flags & Lobby::Flag::EPISODE_3_ONLY) && !(c->flags & Client::Flag::EPISODE_3)) {
+      send_lobby_message_box(c, u"$C6Can't change lobby\n\n$C7The lobby is for\nEpisode 3 only.");
+      return;
+    }
+
+    if (!s->change_client_lobby(c, new_lobby)) {
+      send_lobby_message_box(c, u"$C6Can\'t change lobby\n\n$C7The lobby is full.");
+    }
+  }
 }
 
 void process_game_list_request(shared_ptr<ServerState> s, shared_ptr<Client> c,
@@ -1377,38 +1396,46 @@ void process_player_data(shared_ptr<ServerState> s, shared_ptr<Client> c,
     c->channel.name = remove_language_marker(encode_sjis(player->disp.name));
   }
 
-  if (command == 0x61 && !c->pending_bb_save_username.empty()) {
+  // 98 should only be sent when leaving a game, and we should leave the client
-    string prev_bb_username = c->game_data.bb_username;
+  // in no lobby (they will send an 84 soon afterward to choose a lobby).
-    size_t prev_bb_player_index = c->game_data.bb_player_index;
+  if (command == 0x98) {
+    s->remove_client_from_lobby(c);
 
-    c->game_data.bb_username = c->pending_bb_save_username;
+  } else if (command == 0x61) {
-    c->game_data.bb_player_index = c->pending_bb_save_player_index;
+    if (!c->pending_bb_save_username.empty()) {
+      string prev_bb_username = c->game_data.bb_username;
+      size_t prev_bb_player_index = c->game_data.bb_player_index;
 
-    bool failure = false;
+      c->game_data.bb_username = c->pending_bb_save_username;
-    try {
+      c->game_data.bb_player_index = c->pending_bb_save_player_index;
-      c->game_data.save_player_data();
+
-    } catch (const exception& e) {
+      bool failure = false;
-      u16string buffer = u"$C6PSOBB player data could\nnot be saved:\n" + decode_sjis(e.what());
+      try {
-      send_text_message(c, buffer.c_str());
+        c->game_data.save_player_data();
-      failure = true;
+      } catch (const exception& e) {
+        u16string buffer = u"$C6PSOBB player data could\nnot be saved:\n" + decode_sjis(e.what());
+        send_text_message(c, buffer.c_str());
+        failure = true;
+      }
+
+      if (!failure) {
+        send_text_message_printf(c,
+            "$C6BB player data saved\nas player %hhu for user\n%s",
+            static_cast<uint8_t>(c->pending_bb_save_player_index + 1),
+            c->pending_bb_save_username.c_str());
+      }
+
+      c->game_data.bb_username = prev_bb_username;
+      c->game_data.bb_player_index = prev_bb_player_index;
+
+      c->pending_bb_save_username.clear();
     }
 
-    if (!failure) {
+    // We use 61 during the lobby server init sequence to trigger joining an
-      send_text_message_printf(c,
+    // available lobby
-          "$C6BB player data saved\nas player %hhu for user\n%s",
+    if (!c->lobby_id && (c->server_behavior == ServerBehavior::LOBBY_SERVER)) {
-          static_cast<uint8_t>(c->pending_bb_save_player_index + 1),
+      s->add_client_to_available_lobby(c);
-          c->pending_bb_save_username.c_str());
     }
-
-    c->game_data.bb_username = prev_bb_username;
-    c->game_data.bb_player_index = prev_bb_player_index;
-
-    c->pending_bb_save_username.clear();
-  }
-
-  // if the client isn't in a lobby, add them to an available lobby
-  if (!c->lobby_id && (c->server_behavior == ServerBehavior::LOBBY_SERVER)) {
-    s->add_client_to_available_lobby(c);
   }
 }
 
@@ -1809,7 +1836,7 @@ shared_ptr<Lobby> create_game_generic(shared_ptr<ServerState> s,
 
   bool item_tracking_enabled = (c->version == GameVersion::BB) | s->item_tracking_enabled;
 
-  shared_ptr<Lobby> game(new Lobby());
+  shared_ptr<Lobby> game = s->create_lobby();
   game->name = name;
   game->password = password;
   game->version = c->version;
@@ -1896,6 +1923,9 @@ shared_ptr<Lobby> create_game_generic(shared_ptr<ServerState> s,
     }
   }
 
+  s->change_client_lobby(c, game);
+  c->flags |= Client::Flag::LOADING;
+
   return game;
 }
 
@@ -1903,12 +1933,8 @@ void process_create_game_pc(shared_ptr<ServerState> s, shared_ptr<Client> c,
     uint16_t, uint32_t, const string& data) { // C1
   const auto& cmd = check_size_t<C_CreateGame_PC_C1>(data);
 
-  auto game = create_game_generic(s, c, cmd.name, cmd.password, 1,
+  create_game_generic(s, c, cmd.name, cmd.password, 1,
       cmd.difficulty, cmd.battle_mode, cmd.challenge_mode, 0);
-
-  s->add_lobby(game);
-  s->change_client_lobby(c, game);
-  c->flags |= Client::Flag::LOADING;
 }
 
 void process_create_game_dc_gc(shared_ptr<ServerState> s, shared_ptr<Client> c,
@@ -1932,24 +1958,16 @@ void process_create_game_dc_gc(shared_ptr<ServerState> s, shared_ptr<Client> c,
   u16string name = decode_sjis(cmd.name);
   u16string password = decode_sjis(cmd.password);
 
-  auto game = create_game_generic(s, c, name.c_str(), password.c_str(),
+  create_game_generic(s, c, name.c_str(), password.c_str(),
       episode, cmd.difficulty, cmd.battle_mode, cmd.challenge_mode, 0);
-
-  s->add_lobby(game);
-  s->change_client_lobby(c, game);
-  c->flags |= Client::Flag::LOADING;
 }
 
 void process_create_game_bb(shared_ptr<ServerState> s, shared_ptr<Client> c,
     uint16_t, uint32_t, const string& data) { // C1
   const auto& cmd = check_size_t<C_CreateGame_BB_C1>(data);
 
-  auto game = create_game_generic(s, c, cmd.name, cmd.password, cmd.episode,
+  create_game_generic(s, c, cmd.name, cmd.password, cmd.episode,
       cmd.difficulty, cmd.battle_mode, cmd.challenge_mode, cmd.solo_mode);
-
-  s->add_lobby(game);
-  s->change_client_lobby(c, game);
-  c->flags |= Client::Flag::LOADING;
 }
 
 void process_lobby_name_request(shared_ptr<ServerState> s, shared_ptr<Client> c,

src/ServerState.cc

@@ -29,14 +29,13 @@ ServerState::ServerState()
     auto lobby_name = decode_sjis(string_printf("LOBBY%zu", x + 1));
     bool is_ep3_only = (x > 14);
 
-    shared_ptr<Lobby> l(new Lobby());
+    shared_ptr<Lobby> l = this->create_lobby();
     l->flags |= Lobby::Flag::PUBLIC | Lobby::Flag::DEFAULT | Lobby::Flag::PERSISTENT |
         (is_ep3_only ? Lobby::Flag::EPISODE_3_ONLY : 0);
     l->block = x + 1;
     l->type = x;
     l->name = lobby_name;
     l->max_clients = 12;
-    this->add_lobby(l);
 
     if (!is_ep3_only) {
       this->public_lobby_search_order.emplace_back(l);
@@ -52,20 +51,30 @@ ServerState::ServerState()
       ep3_only_lobbies.end());
 }
 
-void ServerState::add_client_to_available_lobby(shared_ptr<Client> c) {
+void ServerState::add_client_to_available_lobby(
-  const auto& search_order = (c->flags & Client::Flag::EPISODE_3)
+    shared_ptr<Client> c, shared_ptr<Lobby> preferred_lobby) {
-      ? this->public_lobby_search_order_ep3
-      : this->public_lobby_search_order;
-
   shared_ptr<Lobby> added_to_lobby;
-  for (const auto& l : search_order) {
+
+  if (preferred_lobby) {
     try {
-      l->add_client(c);
+      preferred_lobby->add_client(c);
-      added_to_lobby = l;
+      added_to_lobby = preferred_lobby;
-      break;
     } catch (const out_of_range&) { }
   }
 
+  if (!added_to_lobby.get()) {
+    const auto& search_order = (c->flags & Client::Flag::EPISODE_3)
+        ? this->public_lobby_search_order_ep3
+        : this->public_lobby_search_order;
+    for (const auto& l : search_order) {
+      try {
+        l->add_client(c);
+        added_to_lobby = l;
+        break;
+      } catch (const out_of_range&) { }
+    }
+  }
+
   if (!added_to_lobby) {
     // TODO: Add the user to a dynamically-created private lobby instead
     throw out_of_range("all lobbies full");
@@ -85,7 +94,7 @@ void ServerState::remove_client_from_lobby(shared_ptr<Client> c) {
   }
 }
 
-void ServerState::change_client_lobby(shared_ptr<Client> c, shared_ptr<Lobby> new_lobby) {
+bool ServerState::change_client_lobby(shared_ptr<Client> c, shared_ptr<Lobby> new_lobby) {
   uint8_t old_lobby_client_id = c->lobby_client_id;
 
   shared_ptr<Lobby> current_lobby = this->find_lobby(c->lobby_id);
@@ -96,8 +105,7 @@ void ServerState::change_client_lobby(shared_ptr<Client> c, shared_ptr<Lobby> ne
       new_lobby->add_client(c);
     }
   } catch (const out_of_range&) {
-    send_lobby_message_box(c, u"$C6Can't change lobby\n\n$C7The lobby is full.");
+    return false;
-    return;
   }
 
   if (current_lobby) {
@@ -108,6 +116,7 @@ void ServerState::change_client_lobby(shared_ptr<Client> c, shared_ptr<Lobby> ne
     }
   }
   this->send_lobby_join_notifications(new_lobby, c);
+  return true;
 }
 
 void ServerState::send_lobby_join_notifications(shared_ptr<Lobby> l,
@@ -135,16 +144,22 @@ vector<shared_ptr<Lobby>> ServerState::all_lobbies() {
   return ret;
 }
 
-void ServerState::add_lobby(shared_ptr<Lobby> l) {
+shared_ptr<Lobby> ServerState::create_lobby() {
-  l->lobby_id = this->next_lobby_id++;
+  shared_ptr<Lobby> l(new Lobby(this->next_lobby_id++));
-  if (this->id_to_lobby.count(l->lobby_id)) {
+  if (!this->id_to_lobby.emplace(l->lobby_id, l).second) {
     throw logic_error("lobby already exists with the given id");
   }
-  this->id_to_lobby.emplace(l->lobby_id, l);
+  l->log.info("Created lobby");
+  return l;
 }
 
 void ServerState::remove_lobby(uint32_t lobby_id) {
-  this->id_to_lobby.erase(lobby_id);
+  auto lobby_it = this->id_to_lobby.find(lobby_id);
+  if (lobby_it == this->id_to_lobby.end()) {
+    throw logic_error("attempted to remove nonexistent lobby");
+  }
+  lobby_it->second->log.info("Deleted lobby");
+  this->id_to_lobby.erase(lobby_it);
 }
 
 shared_ptr<Client> ServerState::find_client(const std::u16string* identifier,

src/ServerState.hh

@@ -74,6 +74,9 @@ struct ServerState {
   std::map<int64_t, std::shared_ptr<Lobby>> id_to_lobby;
   std::vector<std::shared_ptr<Lobby>> public_lobby_search_order;
   std::vector<std::shared_ptr<Lobby>> public_lobby_search_order_ep3;
+  // TODO: Use a free-list instead of an incrementer to prevent wrap-around
+  // behavioral bugs. This... will probably never be an issue for anyone, but we
+  // technically should handle it.
   std::atomic<int32_t> next_lobby_id;
   uint8_t pre_lobby_event;
   int32_t ep3_menu_song;
@@ -87,9 +90,10 @@ struct ServerState {
 
   ServerState();
 
-  void add_client_to_available_lobby(std::shared_ptr<Client> c);
+  void add_client_to_available_lobby(std::shared_ptr<Client> c,
+      std::shared_ptr<Lobby> preferred_lobby = nullptr);
   void remove_client_from_lobby(std::shared_ptr<Client> c);
-  void change_client_lobby(std::shared_ptr<Client> c,
+  bool change_client_lobby(std::shared_ptr<Client> c,
       std::shared_ptr<Lobby> new_lobby);
 
   void send_lobby_join_notifications(std::shared_ptr<Lobby> l,
@@ -98,7 +102,7 @@ struct ServerState {
   std::shared_ptr<Lobby> find_lobby(uint32_t lobby_id);
   std::vector<std::shared_ptr<Lobby>> all_lobbies();
 
-  void add_lobby(std::shared_ptr<Lobby> l);
+  std::shared_ptr<Lobby> create_lobby();
   void remove_lobby(uint32_t lobby_id);
 
   std::shared_ptr<Client> find_client(