ignore duplicate item pickup requests

src/Lobby.cc

@@ -313,6 +313,10 @@ uint8_t Lobby::game_event_for_lobby_event(uint8_t lobby_event) {
   return lobby_event;
 }
 
+bool Lobby::item_exists(uint32_t item_id) const {
+  return this->item_id_to_floor_item.count(item_id);
+}
+
 void Lobby::add_item(const ItemData& data, uint8_t area, float x, float z) {
   auto& fi = this->item_id_to_floor_item[data.id];
   fi.data = data;

src/Lobby.hh

@@ -159,9 +159,9 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
       const std::string* identifier = nullptr,
       uint64_t serial_number = 0);
 
+  bool item_exists(uint32_t item_id) const;
   void add_item(const ItemData& item, uint8_t area, float x, float z);
   ItemData remove_item(uint32_t item_id);
-  size_t find_item(uint32_t item_id);
   uint32_t generate_item_id(uint8_t client_id);
   void on_item_id_generated_externally(uint8_t client_id, uint32_t item_id);
 

src/ReceiveCommands.cc

@@ -3420,14 +3420,7 @@ shared_ptr<Lobby> create_game_generic(
     return nullptr;
   }
 
-  // TODO: We disable item tracking for battle and challenge mode because
-  // players' inventories are reset when they start the quests, and the server
-  // is not notified when this happens. We'll have to implement this anyway for
-  // BB, but for now we ignore it.
-  bool item_tracking_enabled =
-      (c->version() == GameVersion::BB) ||
-      (s->item_tracking_enabled && (mode == GameMode::NORMAL || mode == GameMode::SOLO));
-
+  bool item_tracking_enabled = (c->version() == GameVersion::BB) || s->item_tracking_enabled;
   // Only disable drops if the config flag is set and are playing regular
   // multi-mode. Drops are still enabled for battle and challenge modes.
   bool drops_enabled = s->behavior_enabled(s->enable_drops_behavior) || (mode != GameMode::NORMAL);

src/ReceiveSubcommands.cc

@@ -991,15 +991,19 @@ static void on_pick_up_item(shared_ptr<Client> c, uint8_t command, uint8_t flag,
 }
 
 static void on_pick_up_item_request(shared_ptr<Client> c, uint8_t command, uint8_t flag, const void* data, size_t size) {
-  // This is handled by the server on BB, and by the leader on other versions
+  auto& cmd = check_size_t<G_PickUpItemRequest_6x5A>(data, size);
+
   auto l = c->require_lobby();
+  if (!l->is_game() || (cmd.header.client_id != c->lobby_client_id)) {
+    return;
+  }
+
+  // This is handled by the server on BB, and by the leader on other versions.
+  // However, there appears to be a bug in v2 that causes the leader to
+  // sometimes allow players to pick up items that someone else has already
+  // picked up. To account for this, we discard requests to pick up items that
+  // don't exist instead of disconnecting the client.
   if (l->base_version == GameVersion::BB) {
-    auto& cmd = check_size_t<G_PickUpItemRequest_6x5A>(data, size);
-
-    if (!l->is_game() || (cmd.header.client_id != c->lobby_client_id)) {
-      return;
-    }
-
     if (!(l->flags & Lobby::Flag::ITEM_TRACKING_ENABLED)) {
       throw logic_error("item tracking not enabled in BB game");
     }
@@ -1021,6 +1025,10 @@ static void on_pick_up_item_request(shared_ptr<Client> c, uint8_t command, uint8
 
     send_pick_up_item(c, cmd.item_id, cmd.area);
 
+  } else if ((l->flags & Lobby::Flag::ITEM_TRACKING_ENABLED) && !l->item_exists(cmd.item_id)) {
+    l->log.warning("Player %hu requests to pick up %08" PRIX32 ", but the item does not exist; dropping command",
+        cmd.header.client_id.load(), cmd.item_id.load());
+
   } else {
     forward_subcommand(c, command, flag, data, size);
   }