fix incorrect next item IDs in synthesized 6x6D commands

src/Lobby.cc

@@ -483,7 +483,16 @@ void Lobby::add_client(shared_ptr<Client> c, ssize_t required_client_id) {
     }
   }
 
-  this->assign_inventory_and_bank_item_ids(c);
+  // If this is not a game or the joining client is the leader, they will assign
+  // their item IDs BEFORE they process any inbound commands (therefore a 6x6D
+  // command, which we will send during loading, should reflect the item state
+  // AFTER their IDs are assigned). If the joining client is not the leader,
+  // they will not assign their item IDs until they receive a 6x71 command,
+  // which is sent AFTER the 6x6D command, so the 6x6D should reflect the item
+  // state BEFORE their IDs are assigned. (In the latter case, we'll assign the
+  // IDs for real when they send a 6F command, or 6x1F equivalent in the case of
+  // DC NTE and 11/2000.)
+  this->assign_inventory_and_bank_item_ids(c, (!this->is_game() || (c->lobby_client_id == this->leader_id)));
 
   // On BB, we send artificial flag state to fix an Episode 2 bug where the
   // CCA door lock state is overwritten by quests.
@@ -710,17 +719,22 @@ void Lobby::on_item_id_generated_externally(uint32_t item_id) {
   }
 }
 
-void Lobby::assign_inventory_and_bank_item_ids(shared_ptr<Client> c) {
+void Lobby::assign_inventory_and_bank_item_ids(shared_ptr<Client> c, bool consume_ids) {
   auto p = c->character();
+  uint32_t orig_next_item_id = this->next_item_id_for_client.at(c->lobby_client_id);
   for (size_t z = 0; z < p->inventory.num_items; z++) {
     p->inventory.items[z].data.id = this->generate_item_id(c->lobby_client_id);
   }
-  if (c->log.info("Assigned inventory item IDs")) {
-    p->print_inventory(stderr, c->version(), c->require_server_state()->item_name_index);
+  if (!consume_ids) {
+    this->next_item_id_for_client[c->lobby_client_id] = orig_next_item_id;
+  }
+
+  if (c->log.info("Assigned inventory item IDs%s", consume_ids ? "" : " but did not mark IDs as used")) {
+    c->print_inventory(stderr);
     if (p->bank.num_items) {
       p->bank.assign_ids(0x99000000 + (c->lobby_client_id << 20));
       c->log.info("Assigned bank item IDs");
-      p->print_bank(stderr, c->version(), c->require_server_state()->item_name_index);
+      c->print_bank(stderr);
     } else {
       c->log.info("Bank is empty");
     }

src/Lobby.hh

@@ -237,7 +237,7 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
 
   uint32_t generate_item_id(uint8_t client_id);
   void on_item_id_generated_externally(uint32_t item_id);
-  void assign_inventory_and_bank_item_ids(std::shared_ptr<Client> c);
+  void assign_inventory_and_bank_item_ids(std::shared_ptr<Client> c, bool consume_ids);
 
   QuestIndex::IncludeCondition quest_include_condition() const;
 

src/ReceiveCommands.cc

@@ -2033,7 +2033,7 @@ static void on_quest_loaded(shared_ptr<Lobby> l) {
         lc->use_default_bank();
         lc->create_challenge_overlay(lc->version(), l->quest->challenge_template_index, s->level_table);
         lc->log.info("Created challenge overlay");
-        l->assign_inventory_and_bank_item_ids(lc);
+        l->assign_inventory_and_bank_item_ids(lc, true);
       }
     }
   }
@@ -3620,7 +3620,7 @@ static void on_DF_BB(shared_ptr<Client> c, uint16_t command, uint32_t, string& d
           lc->use_default_bank();
           lc->create_challenge_overlay(lc->version(), l->quest->challenge_template_index, s->level_table);
           lc->log.info("Created challenge overlay");
-          l->assign_inventory_and_bank_item_ids(lc);
+          l->assign_inventory_and_bank_item_ids(lc, true);
         }
       }
 
@@ -4361,17 +4361,23 @@ static void on_6F(shared_ptr<Client> c, uint16_t command, uint32_t, string& data
   if (!l->is_game()) {
     throw runtime_error("client sent ready command outside of game");
   }
-  c->config.clear_flag(Client::Flag::LOADING);
+
+  // Episode 3 sends a 6F after a CAx21 (end battle) command, so we shouldn't
+  // reassign the items IDs again in that case (even though item IDs really
+  // don't matter for Ep3)
+  if (c->config.check_flag(Client::Flag::LOADING)) {
+    c->config.clear_flag(Client::Flag::LOADING);
+
+    // The client sends 6F when it has created its TObjPlayer and assigned its
+    // item IDs. For the leader, however, this happens before any inbound commands
+    // are processed, so we already did it when the client was added to the lobby.
+    // So, we only assign item IDs here if the client is not the leader.
+    if ((command == 0x006F) && (c->lobby_client_id != l->leader_id)) {
+      l->assign_inventory_and_bank_item_ids(c, true);
+    }
+  }
 
   send_server_time(c);
-  if (l->base_version == Version::BB_V4) {
-    send_set_exp_multiplier(l);
-  }
-  if (c->version() == Version::BB_V4) {
-    send_update_team_reward_flags(c);
-    send_all_nearby_team_metadatas_to_client(c, false);
-  }
-
   if (c->config.check_flag(Client::Flag::DEBUG_ENABLED)) {
     string variations_str;
     for (size_t z = 0; z < l->variations.size(); z++) {
@@ -4382,6 +4388,10 @@ static void on_6F(shared_ptr<Client> c, uint16_t command, uint32_t, string& data
 
   bool should_resume_game = true;
   if (c->version() == Version::BB_V4) {
+    send_set_exp_multiplier(l);
+    send_update_team_reward_flags(c);
+    send_all_nearby_team_metadatas_to_client(c, false);
+
     // BB sends 016F when the client is done loading a quest. In that case, we
     // shouldn't send the quest to them again!
     if ((command == 0x006F) && l->check_flag(Lobby::Flag::JOINABLE_QUEST_IN_PROGRESS)) {

src/ReceiveSubcommands.cc

@@ -1136,6 +1136,7 @@ static void on_change_floor_6x1F(shared_ptr<Client> c, uint8_t command, uint8_t
     if (c->config.check_flag(Client::Flag::LOADING)) {
       c->config.clear_flag(Client::Flag::LOADING);
       send_resume_game(c->require_lobby(), c);
+      c->require_lobby()->assign_inventory_and_bank_item_ids(c, true);
     }
 
   } else {

src/SendCommands.cc

@@ -2385,23 +2385,7 @@ void send_game_item_state(shared_ptr<Client> c) {
 
   G_SyncItemState_6x6D_Decompressed decompressed_header;
   for (size_t z = 0; z < 12; z++) {
-    // If the player is the leader, they will construct their TObjPlayer BEFORE
-    // they handle the 6x6D, so we should send an appropriate next item ID for
-    // after that has occurred. (We have already done this assignment, so we can
-    // just send our next item ID for the player.) If the player is not the
-    // leader, they will construct their TObjPlayer when they receive a 6x71
-    // command from the leader, so we should adjust the next item ID to what it
-    // should have been before they will assign their inventory item IDs.
-    if ((z == c->lobby_client_id) && (c->lobby_client_id != l->leader_id)) {
-      size_t num_items = c->character()->inventory.num_items;
-      uint32_t next_id = l->next_item_id_for_client[z] - num_items;
-      if ((next_id & 0xFFE00000) != (l->next_item_id_for_client[z] & 0xFFE00000)) {
-        throw runtime_error("next item ID underflow during joining player item state generation");
-      }
-      decompressed_header.next_item_id_per_player[z] = next_id;
-    } else {
-      decompressed_header.next_item_id_per_player[z] = l->next_item_id_for_client[z];
-    }
+    decompressed_header.next_item_id_per_player[z] = l->next_item_id_for_client[z];
   }
   l->log.info("Sending next item IDs to client: %08" PRIX32 " %08" PRIX32 " %08" PRIX32 " %08" PRIX32,
       decompressed_header.next_item_id_per_player[0].load(),