rewrite unit generation logic to fix v2/bb behavior

src/CommonItemSet.cc

@@ -33,7 +33,7 @@ CommonItemSet::Table::Table(
     this->offsets.technique_index_prob_table_offset = be_offsets.technique_index_prob_table_offset.load();
     this->offsets.technique_level_ranges_offset = be_offsets.technique_level_ranges_offset.load();
     this->offsets.armor_or_shield_type_bias = be_offsets.armor_or_shield_type_bias;
-    this->offsets.unit_maxes_offset = be_offsets.unit_maxes_offset.load();
+    this->offsets.unit_max_stars_offset = be_offsets.unit_max_stars_offset.load();
     this->offsets.box_item_class_prob_table_offset = be_offsets.box_item_class_prob_table_offset.load();
   } else {
     this->offsets = r.pget<Offsets<false>>(r.pget_u32l(this->r.size() - 0x10));
@@ -171,8 +171,8 @@ const parray<parray<CommonItemSet::Table::Range<uint8_t>, 0x0A>, 0x13>& CommonIt
 uint8_t CommonItemSet::Table::armor_or_shield_type_bias() const {
   return this->offsets.armor_or_shield_type_bias;
 }
-const parray<uint8_t, 0x0A>& CommonItemSet::Table::unit_maxes_table() const {
+const parray<uint8_t, 0x0A>& CommonItemSet::Table::unit_max_stars_table() const {
-  return this->r.pget<parray<uint8_t, 0x0A>>(this->offsets.unit_maxes_offset);
+  return this->r.pget<parray<uint8_t, 0x0A>>(this->offsets.unit_max_stars_offset);
 }
 const parray<parray<uint8_t, 10>, 7>& CommonItemSet::Table::box_item_class_prob_table() const {
   return this->r.pget<parray<parray<uint8_t, 10>, 7>>(this->offsets.box_item_class_prob_table_offset);

src/CommonItemSet.hh

@@ -41,7 +41,7 @@ public:
     const parray<parray<uint8_t, 0x0A>, 0x13>& technique_index_prob_table() const;
     const parray<parray<Range<uint8_t>, 0x0A>, 0x13>& technique_level_ranges() const;
     uint8_t armor_or_shield_type_bias() const;
-    const parray<uint8_t, 0x0A>& unit_maxes_table() const;
+    const parray<uint8_t, 0x0A>& unit_max_stars_table() const;
     const parray<parray<uint8_t, 10>, 7>& box_item_class_prob_table() const;
 
   private:
@@ -219,21 +219,21 @@ public:
       /* 48 */ uint8_t armor_or_shield_type_bias;
       /* 49 */ parray<uint8_t, 3> unused1;
 
-      // These values specify maximum indexes into another array which is
+      // These values specify the maximum number of stars any generated unit can
-      // generated at runtime. The values here are multiplied by a random float in
+      // have in each area. The values here are not inclusive; that is, a value
-      // the range [0, n] to look up the value in the secondary array, which is
+      // of 7 means that only units with 1-6 stars can drop in that area. The
-      // what ends up determining the unit type.
+      // game uniformly chooses a random number of stars in the acceptable
-      // TODO: Figure out and document the exact logic here. Anchor: 80106364
+      // range, then uniformly chooses a random unit with that many stars.
       // V2/V3: -> parray<uint8_t, 0x0A>
-      /* 4C */ U32T unit_maxes_offset;
+      /* 4C */ U32T unit_max_stars_offset;
 
       // This index probability table determines which type of items drop from
-      // boxes. The table is indexed as [item_class][area - 1], with item_class as
+      // boxes. The table is indexed as [item_class][area - 1], with item_class
-      // the result value (that is, in the example below, the game looks at a
+      // as the result value (that is, in the example below, the game looks at a
-      // single column and sums the values going down, then the chosen item class
+      // single column and sums the values going down, then the chosen item
-      // is one of the row indexes based on the weight values in the column.) The
+      // class is one of the row indexes based on the weight values in the
-      // resulting item_class value has the same meaning as in enemy_item_classes
+      // column.) The resulting item_class value has the same meaning as in
-      // above.
+      // enemy_item_classes above.
       // For example, this array might look like the following:
       //   [07 07 08 08 06 07 08 09 09 0A] // Chances per area of a weapon drop
       //   [02 02 02 02 03 02 02 02 03 03] // Chances per area of an armor drop

src/ItemCreator.cc

@@ -38,7 +38,7 @@ ItemCreator::ItemCreator(
       pt(common_item_set->get_table(this->episode, this->mode, this->difficulty, this->section_id)),
       restrictions(restrictions),
       random_crypt(random_seed) {
-  this->generate_unit_weights_tables();
+  this->generate_unit_stars_tables();
 }
 
 void ItemCreator::clear_destroyed_entities() {
@@ -560,11 +560,11 @@ void ItemCreator::generate_common_item_variances(uint32_t area_norm, ItemData& i
       break;
     case 1:
       if (item.data1[1] == 3) {
-        float f1 = 1.0 + this->pt->unit_maxes_table().at(area_norm);
+        float f1 = 1.0 + this->pt->unit_max_stars_table().at(area_norm);
         float f2 = this->rand_float_0_1_from_crypt();
-        uint8_t det = static_cast<uint32_t>(f1 * f2) & 0xFF;
+        uint8_t stars = static_cast<uint32_t>(f1 * f2) & 0xFF;
-        this->log.info("Unit variances determinant: %g * %g = %08" PRIX32, f1, f2, det);
+        this->log.info("Unit stars: %g * %g = %" PRIu32, f1, f2, stars);
-        this->generate_common_unit_variances(det, item);
+        this->generate_common_unit_variances(stars, item);
         if (item.data1[2] == 0xFF) {
           this->log.info("Unit subtype not valid; clearing item");
           item.clear();
@@ -778,111 +778,76 @@ uint8_t ItemCreator::choose_weapon_special(uint8_t det) {
   return 0;
 }
 
-void ItemCreator::generate_unit_weights_tables() {
+void ItemCreator::generate_unit_stars_tables() {
   // Note: This part of the function was originally in a different function,
   // since it had another callsite. Unlike the original code, we generate these
   // tables only once at construction time, so we've inlined the function here.
 
   size_t star_base_index;
+  uint8_t num_units;
   switch (this->version) {
     case GameVersion::DC:
     case GameVersion::PC:
       star_base_index = 0x1D1;
-      this->unit_weights_table1.resize(0x84);
+      num_units = 0x44;
       break;
     case GameVersion::GC:
     case GameVersion::XB:
       star_base_index = 0x2AF;
-      this->unit_weights_table1.resize(0x88);
+      num_units = 0x48;
       break;
     case GameVersion::BB:
       star_base_index = 0x37D;
-      this->unit_weights_table1.resize(0x88);
+      num_units = 0x64;
       break;
     default:
       throw logic_error("invalid game version");
   }
 
-  size_t z;
+  for (auto& vec : this->unit_results_by_star_count) {
-  for (z = 0; z < 0x10; z++) {
+    vec.clear();
-    uint8_t v = this->item_parameter_table->get_item_stars(z + star_base_index);
-    this->unit_weights_table1.at((z * 5) + 0) = v - 1;
-    this->unit_weights_table1.at((z * 5) + 1) = v - 1;
-    this->unit_weights_table1.at((z * 5) + 2) = v;
-    this->unit_weights_table1.at((z * 5) + 3) = v + 1;
-    this->unit_weights_table1.at((z * 5) + 4) = v + 1;
   }
-  for (; z < (this->unit_weights_table1.size() - 0x40); z++) {
-    this->unit_weights_table1.at(z + 0x40) = this->item_parameter_table->get_item_stars(z + star_base_index);
-  }
-  // Note: Inlining ends here
 
-  this->unit_weights_table2.clear(0);
+  for (uint8_t z = 0; z < num_units; z++) {
-  for (size_t z = 0; z < 0x88; z++) {
+    uint8_t stars = this->item_parameter_table->get_item_stars(z + star_base_index);
-    uint8_t index = this->unit_weights_table1[z];
+    if (z < 0x10) {
-    if (index < this->unit_weights_table2.size()) {
+      // Units 00-0F can have modifiers; others can't
-      this->unit_weights_table2[index]++;
+      this->unit_results_by_star_count.at(stars - 1).emplace_back(UnitResult{z, -2});
+      this->unit_results_by_star_count.at(stars - 1).emplace_back(UnitResult{z, -1});
+      this->unit_results_by_star_count.at(stars + 1).emplace_back(UnitResult{z, 1});
+      this->unit_results_by_star_count.at(stars + 1).emplace_back(UnitResult{z, 2});
     }
-    z = z + 1;
+    this->unit_results_by_star_count.at(stars).emplace_back(UnitResult{z, 0});
+  }
+
+  for (size_t z = 0; z < this->unit_results_by_star_count.size(); z++) {
+    fprintf(stderr, "result table %zu\n", z);
+    print_data(stderr, this->unit_results_by_star_count[z].data(), this->unit_results_by_star_count[z].size() * 2);
   }
 }
 
-void ItemCreator::generate_common_unit_variances(uint8_t det, ItemData& item) {
+void ItemCreator::generate_common_unit_variances(uint8_t stars, ItemData& item) {
-  if (det >= 0x0D) {
+  if (stars >= 0x0D) {
     return;
   }
   item.clear();
-  item.data1[0] = 0x01;
-  item.data1[1] = 0x03;
 
-  // Note: The original code calls generate_unit_weights_table1 here (which we
+  const auto& results = this->unit_results_by_star_count.at(stars);
-  // have inlined into generate_unit_weights_tables above). This call seems
+  if (results.empty()) {
-  // unnecessary because the contents of the tables don't depend on anything
+    this->log.info("There are no available units with %hhu stars", stars);
-  // except what appears in ItemPMT, which is essentially constant, so we
-  // don't bother regenerating the table here.
-
-  if (this->unit_weights_table2[det] == 0) {
-    this->log.info("Unit weights table 2 entry is zero; skipping variances");
     return;
   }
 
-  size_t which = this->rand_int(this->unit_weights_table2[det]);
+  const auto& result = (results.size() == 1) ? results[0] : results[this->rand_int(results.size())];
-  this->log.info("Unit values: which=%02zX max=%02hhX", which, this->unit_weights_table2[det]);
+  item.data1[0] = 0x01;
-  size_t current_index = 0;
+  item.data1[1] = 0x03;
-  for (size_t z = 0; z < this->unit_weights_table1.size(); z++) {
+  item.data1[2] = result.unit;
-    if (det != this->unit_weights_table1[z]) {
+  if (result.modifier) {
-      continue;
+    const auto& def = this->item_parameter_table->get_unit(result.unit);
-    }
+    item.set_unit_bonus(def.modifier_amount * result.modifier);
-    if (current_index != which) {
-      current_index++;
-    } else {
-      if (z >= 0x50) {
-        if (det <= 0x87) {
-          item.data1[2] = z + 0xC0;
-        }
-      } else {
-        item.data1[2] = z / 5;
-        const auto& def = this->item_parameter_table->get_unit(item.data1[2]);
-        switch (z % 5) {
-          case 0:
-            item.set_unit_bonus(-(def.modifier_amount * 2));
-            break;
-          case 1:
-            item.set_unit_bonus(-def.modifier_amount);
-            break;
-          case 2:
-            break;
-          case 3:
-            item.set_unit_bonus(def.modifier_amount);
-            break;
-          case 4:
-            item.set_unit_bonus(def.modifier_amount * 2);
-            break;
-        }
-      }
-      break;
-    }
   }
+  this->log.info("Generated unit %02hhX with modifier %hhd, from %zu choices with %hhu stars",
+      result.unit, result.modifier, results.size(), stars);
 }
 
 // Returns a weighted random result, indicating the chosen position in the

src/ItemCreator.hh

@@ -64,8 +64,11 @@ private:
   std::shared_ptr<const CommonItemSet::Table> pt;
   std::shared_ptr<const BattleRules> restrictions;
 
-  std::vector<uint8_t> unit_weights_table1;
+  struct UnitResult {
-  parray<int8_t, 0x0D> unit_weights_table2;
+    uint8_t unit;
+    int8_t modifier;
+  } __attribute__((packed));
+  std::array<std::vector<UnitResult>, 13> unit_results_by_star_count;
 
   // Note: The original implementation uses 17 different random states for some
   // reason. We forego that and use only one for simplicity.
@@ -115,8 +118,8 @@ private:
   void generate_common_weapon_bonuses(ItemData& item, uint8_t area_norm);
   void generate_common_weapon_special(ItemData& item, uint8_t area_norm);
   uint8_t choose_weapon_special(uint8_t det);
-  void generate_unit_weights_tables();
+  void generate_unit_stars_tables();
-  void generate_common_unit_variances(uint8_t det, ItemData& item);
+  void generate_common_unit_variances(uint8_t stars, ItemData& item);
   void choose_tech_disk_level_for_tool_shop(ItemData& item, size_t player_level, uint8_t tech_num_index);
   static void clear_tool_item_if_invalid(ItemData& item);
   void clear_item_if_restricted(ItemData& item) const;