reformat more files

src/ItemData.cc

@@ -8,10 +8,8 @@
 
 using namespace std;
 
-const vector<uint8_t> ItemData::StackLimits::DEFAULT_TOOL_LIMITS_DC_NTE(
+const vector<uint8_t> ItemData::StackLimits::DEFAULT_TOOL_LIMITS_DC_NTE({10});
-    {10});
+const vector<uint8_t> ItemData::StackLimits::DEFAULT_TOOL_LIMITS_V1_V2({10, 10, 1, 10, 10, 10, 10, 10, 10, 1});
-const vector<uint8_t> ItemData::StackLimits::DEFAULT_TOOL_LIMITS_V1_V2(
-    {10, 10, 1, 10, 10, 10, 10, 10, 10, 1});
 const vector<uint8_t> ItemData::StackLimits::DEFAULT_TOOL_LIMITS_V3_V4(
     {10, 10, 1, 10, 10, 10, 10, 10, 10, 1, 1, 1, 1, 1, 1, 1, 99, 1});
 
@@ -28,8 +26,7 @@ ItemData::StackLimits::StackLimits(
       max_tool_stack_sizes_by_data1_1(max_tool_stack_sizes_by_data1_1),
       max_meseta_stack_size(max_meseta_stack_size) {}
 
-ItemData::StackLimits::StackLimits(Version version, const phosg::JSON& json)
+ItemData::StackLimits::StackLimits(Version version, const phosg::JSON& json) : version(version) {
-    : version(version) {
   this->max_tool_stack_sizes_by_data1_1.clear();
   for (const auto& limit_json : json.at("ToolLimits").as_list()) {
     this->max_tool_stack_sizes_by_data1_1.emplace_back(limit_json->as_int());
@@ -116,8 +113,7 @@ uint32_t ItemData::primary_identifier() const {
   // - 03TTSS00 = tool
   // - 04000000 = meseta
 
-  // The game treats any item starting with 04 as Meseta, and ignores the rest
+  // The game treats any item starting with 04 as Meseta, and ignores the rest of data1 (the value is in data2)
-  // of data1 (the value is in data2)
   if (this->data1[0] == 0x04) {
     return 0x04000000;
   }
@@ -150,17 +146,14 @@ void ItemData::change_primary_identifier(uint32_t primary_identifier) {
       }
       break;
     }
-    case 0x01: // Armor/shield/unit
+    case 0x01: // Armor/shield/unit; apply data1[1] and data1[2]
-      // Apply data1[1] and data1[2]
       this->data1[1] = (primary_identifier >> 16) & 0xFF;
       this->data1[2] = (primary_identifier >> 8) & 0xFF;
       break;
-    case 0x02: // Mag
+    case 0x02: // Mag; apply data1[1] only
-      // Apply data1[1] only
       this->data1[1] = (primary_identifier >> 16) & 0xFF;
       break;
-    case 0x03: // Tool
+    case 0x03: // Tool; apply data1[1] and data1[2] (or data1[4] if it's a tech disk)
-      // Apply data1[1] and data1[2] (or data1[4] if it's a tech disk)
       this->data1[1] = (primary_identifier >> 16) & 0xFF;
       if (this->data1[1] == 0x02) {
         this->data1[4] = (primary_identifier >> 8) & 0xFF;
@@ -169,8 +162,7 @@ void ItemData::change_primary_identifier(uint32_t primary_identifier) {
         this->data1[2] = (primary_identifier >> 8) & 0xFF;
       }
       break;
-    case 0x04: // Meseta
+    case 0x04: // Meseta; nothing to apply
-      // Nothing to apply
       break;
     default:
       throw std::runtime_error("invalid item class");
@@ -298,10 +290,7 @@ void ItemData::clear_mag_stats() {
 }
 
 uint16_t ItemData::compute_mag_level() const {
-  return (this->data1w[2] / 100) +
+  return (this->data1w[2] / 100) + (this->data1w[3] / 100) + (this->data1w[4] / 100) + (this->data1w[5] / 100);
-      (this->data1w[3] / 100) +
-      (this->data1w[4] / 100) +
-      (this->data1w[5] / 100);
 }
 
 uint16_t ItemData::compute_mag_strength_flags() const {
@@ -432,8 +421,7 @@ void ItemData::decode_for_version(Version from_version) {
       }
 
       if (is_v1(from_version) || is_v2(from_version)) {
-        // PSO PC and GC NTE encode mags in a tediously annoying manner. The
+        // PSO PC and GC NTE encode mags in a tediously annoying manner. The first four bytes are the same, but then:
-        // first four bytes are the same, but then...
         // V2: pHHHHHHHHHHHHHHc pIIIIIIIIIIIIIIc JJJJJJJJJJJJJJJc KKKKKKKKKKKKKKKc QQQQQQQQ QQQQQQQQ YYYYYYYY pYYYYYYY
         // V3: HHHHHHHHHHHHHHHH IIIIIIIIIIIIIIII JJJJJJJJJJJJJJJJ KKKKKKKKKKKKKKKK YYYYYYYY QQQQQQQQ PPPPPPPP CCCCCCCC
         // c = color in V2 (4 bits; low bit first)
@@ -455,10 +443,8 @@ void ItemData::decode_for_version(Version from_version) {
         this->data1w[5] &= 0xFFFE;
 
       } else if (is_big_endian(from_version)) {
-        // PSO GC (but not GC NTE, which uses the above logic) byteswaps the
+        // PSO GC (but not GC NTE, which uses the above logic) byteswaps the data2d field, since internally it's
-        // data2d field, since internally it's actually a uint32_t. We treat it
+        // actually a uint32_t. We treat it as individual bytes, so we correct for the client's byteswapping here.
-        // as individual bytes instead, so we correct for the client's
-        // byteswapping here.
         this->data2d = phosg::bswap32(this->data2d);
       }
       break;
@@ -529,9 +515,8 @@ void ItemData::encode_for_version(Version to_version, shared_ptr<const ItemParam
         this->data1[1] = 0x00;
       }
 
-      // This logic is the inverse of the corresponding logic in
+      // This logic is the inverse of the corresponding logic in decode_for_version; see that function for a
-      // decode_for_version; see that function for a description of what's
+      // description of what's going on here.
-      // going on here.
       if (is_v1(to_version) || is_v2(to_version)) {
         this->data1w[2] = (this->data1w[2] & 0x7FFE) | ((this->data2[2] << 14) & 0x8000) | (this->data2[3] & 1);
         this->data1w[3] = (this->data1w[3] & 0x7FFE) | ((this->data2[2] << 13) & 0x8000) | ((this->data2[3] >> 1) & 1);

src/ItemData.hh

@@ -10,13 +10,11 @@
 class ItemParameterTable;
 
 enum class EquipSlot {
-  // When equipping items through the Item Pack pause menu, the client sends
+  // When equipping items through the Item Pack pause menu, the client sends UNKNOWN for the slot. The receiving client
-  // UNKNOWN for the slot. The receiving client (and server, in our case) have
+  // (and server, in our case) have to analyze the item being equipped and put it in the appropriate slot in this case.
-  // to analyze the item being equipped and put it in the appropriate slot in
+  // See ItemData::default_equip_slot() for this computation.
-  // this case. See ItemData::default_equip_slot() for this computation.
   UNKNOWN = 0x00,
-  // When equipping items through the quick menu or Equip pause menu, the client
+  // When equipping items through the quick menu or Equip pause menu, the client sends one of the slots below.
-  // sends one of the slots below.
   MAG = 0x01,
   ARMOR = 0x02,
   SHIELD = 0x03,
@@ -80,15 +78,15 @@ struct ItemData {
   };
 
   // QUICK ITEM FORMAT REFERENCE
-  //           data1/0  data1/4  data1/8  data2
+  //              data1/0  data1/4  data1/8  data2
-  //   Weapon:    00ZZZZGG SSNNAABB AABBAABB 00000000
+  // Weapon:    00ZZZZGG SSNNAABB AABBAABB 00000000
-  //   Armor:     0101ZZ00 FFTTDDDD EEEEXXXX 00000000
+  // Armor:     0101ZZ00 FFTTDDDD EEEEXXXX 00000000
-  //   Shield:    0102ZZ00 FFTTDDDD EEEEXXXX 00000000
+  // Shield:    0102ZZ00 FFTTDDDD EEEEXXXX 00000000
-  //   Unit:      0103ZZ00 FF00RRRR 0000XXXX 00000000
+  // Unit:      0103ZZ00 FF00RRRR 0000XXXX 00000000
-  //   Mag:       02ZZLLWW HHHHIIII JJJJKKKK YYQQPPVV
+  // Mag:       02ZZLLWW HHHHIIII JJJJKKKK YYQQPPVV
-  //   Tool:      03ZZZZUU 00CC0000 0000XXXX 00000000
+  // Tool:      03ZZZZUU 00CC0000 0000XXXX 00000000
-  //   Tech disk: 0302&&UU %%CC0000 0000XXXX 00000000
+  // Tech disk: 0302&&UU %%CC0000 0000XXXX 00000000
-  //   Meseta:    04000000 00000000 00000000 MMMMMMMM
+  // Meseta:    04000000 00000000 00000000 MMMMMMMM
   // A = attribute type (for S-ranks, custom name; last pair is kill count for some weapons)
   // B = attribute amount (for S-ranks, custom name; last pair is kill count for some weapons)
   // C = stack size (for tools)
@@ -116,13 +114,10 @@ struct ItemData {
   // Z = item ID
   // & = technique level
   // % = technique number
-  // Note: PSO GC erroneously byteswaps data2 even when the item is a mag. This
+  // Note: PSO GC byteswaps data2 even when the item is a mag. This makes it incompatible with little-endian versions
-  // makes it incompatible with little-endian versions of PSO (i.e. all other
+  // of PSO (i.e. all other versions). We manually byteswap data2 upon receipt and before sending where needed.
-  // versions). We manually byteswap data2 upon receipt and immediately before
+  // Related note: PSO V2 has an annoyingly complicated format for mags that doesn't match the above table. We decode
-  // sending where needed.
+  // this upon receipt and encode it immediately before sending when interacting with V2 clients; see the
-  // Related note: PSO V2 has an annoyingly complicated format for mags that
-  // doesn't match the above table. We decode this upon receipt and encode it
-  // immediately before sending when interacting with V2 clients; see the
   // implementation of decode_for_version() for details.
 
   union {

src/ItemNameIndex.cc

@@ -146,9 +146,8 @@ std::string ItemNameIndex::describe_item(const ItemData& item, uint8_t flags) co
     }
   }
 
-  // Armors, shields, and units (0x01) can be wrapped, as can mags (0x02) and
+  // Armors, shields, and units (0x01) can be wrapped, as can mags (0x02) and non-stackable tools (0x03). However, each
-  // non-stackable tools (0x03). However, each of these item classes has its
+  // of these item classes has its flags in a different location.
-  // flags in a different location.
   if (!name_only &&
       (((item.data1[0] == 0x01) && (item.data1[4] & 0x40)) ||
           ((item.data1[0] == 0x02) && (item.data2[2] & 0x40)) ||
@@ -188,12 +187,10 @@ std::string ItemNameIndex::describe_item(const ItemData& item, uint8_t flags) co
     }
 
     if (item.is_s_rank_weapon()) {
-      // S-rank weapons have names instead of percent bonuses. The name is
+      // S-rank weapons have names instead of percent bonuses. The name is encoded as 9 5-bit characters in the bytes
-      // encoded as 9 5-bit characters in the bytes where the bonuses usually
+      // where the bonuses usually go. The first character does not appear when the name is rendered; instead, it's
-      // go. The first character does not appear when the name is rendered;
+      // used to determine if the weapon type name should appear or not. Unlike the client, we check the first
-      // instead, it's used to determine if the weapon type name should appear
+      // character directly and don't bother decoding it.
-      // or not. Unlike the client, we check the first character directly and
-      // don't bother decoding it.
       uint16_t be_data1w3 = phosg::bswap16(item.data1w[3]);
       uint16_t be_data1w4 = phosg::bswap16(item.data1w[4]);
       uint16_t be_data1w5 = phosg::bswap16(item.data1w[5]);
@@ -324,8 +321,7 @@ std::string ItemNameIndex::describe_item(const ItemData& item, uint8_t flags) co
 
     uint8_t flags = item.data2[2];
     if (flags & 7) {
-      static const vector<const char*> pb_shortnames = {
+      static const vector<const char*> pb_shortnames = {"F", "E", "G", "P", "L", "M&Y", "MG", "GR"};
-          "F", "E", "G", "P", "L", "M&Y", "MG", "GR"};
 
       const char* pb_names[3] = {nullptr, nullptr, nullptr};
       uint8_t left_pb = item.mag_photon_blast_for_slot(2);
@@ -587,9 +583,8 @@ ItemData ItemNameIndex::parse_item_description_phase(const std::string& descript
   }
 
   auto name_it = this->name_index.lower_bound(desc);
-  // Look up to 3 places before the lower bound. We have to do this to catch
+  // Look up to 3 places before the lower bound. We have to do this to catch cases like Sange vs. Sange & Yasha - if
-  // cases like Sange vs. Sange & Yasha - if the input is like "Sange 0/...",
+  // the input is like "Sange 0/...", then we'll see Sange & Yasha first, which we should skip.
-  // then we'll see Sange & Yasha first, which we should skip.
   size_t lookback = 0;
   while (lookback < 4) {
     if (name_it != this->name_index.end() && desc.starts_with(name_it->first)) {
@@ -610,16 +605,14 @@ ItemData ItemNameIndex::parse_item_description_phase(const std::string& descript
     desc = desc.substr(1);
   }
 
-  // Tech disks should have already been handled above, so we don't need to
+  // Tech disks should have already been handled above, so we don't need to special-case 0302xxxx identifiers here.
-  // special-case 0302xxxx identifiers here.
   uint32_t primary_identifier = name_it->second->primary_identifier;
   ret.data1[0] = (primary_identifier >> 24) & 0xFF;
   ret.data1[1] = (primary_identifier >> 16) & 0xFF;
   ret.data1[2] = (primary_identifier >> 8) & 0xFF;
 
   if (ret.data1[0] == 0x00) {
-    // Weapons: add special, grind and percentages (or name, if S-rank) and
+    // Weapons: add special, grind and percentages (or name, if S-rank) and kill count if unsealable
-    // kill count if unsealable
     ret.data1[4] = weapon_special | (is_wrapped ? 0x40 : 0x00) | (is_unidentified ? 0x80 : 0x00);
 
     bool kill_count_set = false;
@@ -728,16 +721,8 @@ ItemData ItemNameIndex::parse_item_description_phase(const std::string& descript
         if (pb_tokens.size() > 3) {
           throw runtime_error("too many photon blasts specified");
         }
-        static const unordered_map<string, uint8_t> name_to_pb_num({
+        static const unordered_map<string, uint8_t> name_to_pb_num(
-            {"f", 0},
+            {{"f", 0}, {"e", 1}, {"g", 2}, {"p", 3}, {"l", 4}, {"m", 5}, {"my", 5}, {"m&y", 5}});
-            {"e", 1},
-            {"g", 2},
-            {"p", 3},
-            {"l", 4},
-            {"m", 5},
-            {"my", 5},
-            {"m&y", 5},
-        });
         for (const auto& pb_token : pb_tokens) {
           ret.add_mag_photon_blast(name_to_pb_num.at(pb_token));
         }
@@ -1044,8 +1029,7 @@ void ItemNameIndex::print_table(FILE* stream) const {
   phosg::fwrite_fmt(stream, "MAG FEED TABLES\n");
   for (size_t table_index = 0; table_index < 8; table_index++) {
     static const char* names[11] = {
-        "Monomate", "Dimate", "Trimate", "Monofluid",
+        "Monomate", "Dimate", "Trimate", "Monofluid", "Difluid", "Trifluid", "Antidote", "Antiparalysis",
-        "Difluid", "Trifluid", "Antidote", "Antiparalysis",
         "Sol Atomizer", "Moon Atomizer", "Star Atomizer"};
     phosg::fwrite_fmt(stream, "  TABLE {:02X}         => -DEF -POW -DEX MIND -IQ- SYNC\n", table_index);
     for (size_t which = 0; which < 11; which++) {

src/ItemParameterTable.cc

@@ -791,8 +791,8 @@ ItemParameterTable::definition_for_primary_identifier(uint32_t primary_identifie
     case 2:
       return &this->get_mag(data1_1);
     case 3:
-      // NOTE: Unlike in ItemData, the tech number comes first in primary
+      // NOTE: Unlike in ItemData, the tech number comes first in primary identifiers, so we don't need to special-case
-      // identifiers, so we don't need to special-case 0302XXYY here
+      // 0302XXYY here
       return &this->get_tool(data1_1, data1_2);
     default:
       throw runtime_error("invalid primary identifier");
@@ -908,9 +908,7 @@ uint8_t ItemParameterTable::get_item_stars(uint32_t item_id) const {
 }
 
 uint8_t ItemParameterTable::get_special_stars(uint8_t special) const {
-  return ((special & 0x3F) && !(special & 0x80))
+  return ((special & 0x3F) && !(special & 0x80)) ? this->get_item_stars(special + this->special_stars_begin_index) : 0;
-      ? this->get_item_stars(special + this->special_stars_begin_index)
-      : 0;
 }
 
 const ItemParameterTable::Special& ItemParameterTable::get_special(uint8_t special) const {
@@ -1029,9 +1027,7 @@ uint8_t ItemParameterTable::get_weapon_v1_replacement(uint8_t data1_1) const {
     throw logic_error("table is not v2, v3, or v4");
   }
 
-  return (data1_1 < this->num_weapon_classes)
+  return (data1_1 < this->num_weapon_classes) ? this->r.pget_u8(offset + data1_1) : 0x00;
-      ? this->r.pget_u8(offset + data1_1)
-      : 0x00;
 }
 
 uint32_t ItemParameterTable::get_item_id(const ItemData& item) const {
@@ -1157,9 +1153,7 @@ bool ItemParameterTable::is_unsealable_item(uint8_t data1_0, uint8_t data1_1, ui
 
   const auto* defs = &this->r.pget<UnsealableItem>(offset, count * sizeof(UnsealableItem));
   for (size_t z = 0; z < count; z++) {
-    if ((defs[z].item[0] == data1_0) &&
+    if ((defs[z].item[0] == data1_0) && (defs[z].item[1] == data1_1) && (defs[z].item[2] == data1_2)) {
-        (defs[z].item[1] == data1_1) &&
-        (defs[z].item[2] == data1_2)) {
       return true;
     }
   }

src/ItemParameterTable.hh

@@ -18,8 +18,8 @@
 #include "Types.hh"
 #include "Version.hh"
 
-// TODO: These don't really belong here, but putting them anywhere else creates
+// TODO: These don't really belong here, but putting them anywhere else creates annoying dependency cycles. Find or
-// annoying dependency cycles. Find or make a better place for these.
+// make a better place for these.
 enum class ServerDropMode {
   DISABLED = 0,
   CLIENT = 1, // Not allowed for BB games
@@ -40,14 +40,13 @@ const char* phosg::name_for_enum<ServerDropMode>(ServerDropMode value);
 
 class ItemParameterTable {
 public:
-  // TODO: This implementation is ugly. We should use real classes and virtual
+  // TODO: This implementation is ugly. We should use real classes and virtual functions instead of manually branching
-  // functions instead of manually branching on various offset table pointers
+  // on various offset table pointers being null or not in each public function. Rewrite this and make it better.
-  // being null or not in each public function. Rewrite this and make it better.
 
   template <bool BE>
   struct ItemBaseV2T {
-    // id specifies several things; notably, it doubles as the index of the
+    // id specifies several things; notably, it doubles as the index of the item's name in the text archive (e.g.
-    // item's name in the text archive (e.g. TextEnglish) collection 0.
+    // TextEnglish) collection 0.
     /* 00 */ U32T<BE> id = 0xFFFFFFFF;
     /* 04 */
   } __attribute__((packed));
@@ -291,15 +290,13 @@ public:
     /* 09 */ uint8_t on_low_hp = 0;
     /* 0A */ uint8_t on_death = 0;
     /* 0B */ uint8_t on_boss = 0;
-    // These flags control how likely each effect is to activate. First, the
+    // These flags control how likely each effect is to activate. First, the game computes step_synchro as follows:
-    // game computes step_synchro as follows:
     //   if synchro in [0, 30], step_synchro = 0
     //   if synchro in [31, 60], step_synchro = 15
     //   if synchro in [61, 80], step_synchro = 25
     //   if synchro in [81, 100], step_synchro = 30
     //   if synchro in [101, 120], step_synchro = 35
-    // Then, the percent chance of the effect occurring upon its trigger (e.g.
+    // Then, the percent chance of the effect occurring upon its trigger (e.g. entering a boss arena) is:
-    // entering a boss arena) is:
     //   flag == 0 => activation
     //   flag == 1 => activation + step_synchro
     //   flag == 2 => step_synchro
@@ -407,10 +404,9 @@ public:
 
   template <bool BE>
   struct StatBoostT {
-    // Only the first of these stat/amount pairs is used in most versions of
+    // Only the first of these stat/amount pairs is used in most versions of the game. In DC 11/2000 Sega apparently
-    // the game. In DC 11/2000 Sega apparently changed the loop from
+    // changed the loop from `for (z = 0; z != 2; z++)` to `for (z = 0; z != 1; z++)`, so only the first stat/amount
-    // `for (z = 0; z != 2; z++)` to `for (z = 0; z != 1; z++)`, so only the
+    // pair is used on all versions after DC NTE.
-    // first stat/amount pair is used on all versions after DC NTE.
     // Values for stats:
     //   01 = ATP bonus
     //   02 = ATA bonus
@@ -565,8 +561,7 @@ protected:
   } __packed_ws__(TableOffsetsDCProtos, 0x50);
 
   struct TableOffsetsV1V2 {
-    // TODO: Is weapon count 0x89 or 0x8A? It could be that the last entry in
+    // TODO: Is weapon count 0x89 or 0x8A? It could be that the last entry in weapon_table is used for ???? items.
-    // weapon_table is used for ???? items.
     /* ## / V1   / V2*/
     /* 00 / 0013 / 0013 */ le_uint32_t unknown_a0;
     /* 04 / 32E8 / 5AFC */ le_uint32_t weapon_table; // -> [{count, offset -> [WeaponV2]}](0x89)
@@ -652,8 +647,7 @@ protected:
   const TableOffsetsV3V4BE* offsets_v3_be;
   const TableOffsetsV3V4* offsets_v4;
 
-  // These are unused if offsets_v4 is not null (in that case, we just return
+  // These are unused if offsets_v4 is not null (in that case, we just return references to within the data string)
-  // references pointing inside the data string)
   mutable std::unordered_map<uint16_t, WeaponV4> parsed_weapons;
   mutable std::vector<ArmorOrShieldV4> parsed_armors;
   mutable std::vector<ArmorOrShieldV4> parsed_shields;
@@ -663,8 +657,8 @@ protected:
   mutable std::vector<Special> parsed_specials;
   mutable std::vector<StatBoost> parsed_stat_boosts;
 
-  // Key is used_item. We can't index on (used_item, equipped_item) because
+  // Key is used_item. We can't index on (used_item, equipped_item) because equipped_item may contain wildcards, and
-  // equipped_item may contain wildcards, and the matching order matters.
+  // the matching order matters.
   mutable std::map<uint32_t, std::vector<ItemCombination>> item_combination_index;
 
   template <typename ToolDefT, bool BE>
@@ -682,8 +676,8 @@ public:
   // TODO: V1 format is different! Offsets are 0438 0440 0498 0520 054C
   struct MotionReference {
     struct Side {
-      // This specifies which entry in ItemMagMotion.dat is used. The file is
+      // This specifies which entry in ItemMagMotion.dat is used. The file is just a list of 0x64-byte structures.
-      // just a list of 0x64-byte structures. 0xFF = no TItemMagSub is created
+      // 0xFF = no TItemMagSub is created
       uint8_t motion_table_entry = 0xFF;
       parray<uint8_t, 5> unknown_a1 = 0;
     } __packed_ws__(Side, 0x06);
@@ -691,37 +685,34 @@ public:
   } __packed_ws__(MotionReference, 0x0C);
 
   struct MotionReferenceTables {
-    // It seems that there are two definition tables, but only the first is
+    // It seems that there are two definition tables, but only the first is used on any version of PSO. On v3 and
-    // used on any version of PSO. On v3 and later, the two offsets point to
+    // later, the two offsets point to the same table, but on v2 they don't and the second table contains different
-    // the same table, but on v2 they don't and the second table contains
+    // data. TODO: Figure out what the deal is with the different v2 tables.
-    // different data.
-    // TODO: Figure out what the deal is with the different v2 tables.
     le_uint32_t ref_table; // -> MotionReference[num_mags]
     le_uint32_t unused_ref_table; // -> MotionReference[num_mags]
   } __packed_ws__(MotionReferenceTables, 0x08);
 
   struct ColorEntry {
-    // Colors are specified as 4 floats, each in the range [0, 1], for each
+    // Colors are specified as 4 floats, each in the range [0, 1], for each color channel. The default colors are:
-    // color channel. The default colors are:
+    //   alpha   red     green   blue    color (see StaticGameData.cc)
-    // alpha   red     green   blue    color (see StaticGameData.cc)
+    //   1.0     1.0     0.2     0.1     red
-    // 1.0     1.0     0.2     0.1     red
+    //   1.0     0.2     0.2     1.0     blue
-    // 1.0     0.2     0.2     1.0     blue
+    //   1.0     1.0     0.9     0.1     yellow
-    // 1.0     1.0     0.9     0.1     yellow
+    //   1.0     0.1     1.0     0.1     green
-    // 1.0     0.1     1.0     0.1     green
+    //   1.0     0.8     0.1     1.0     purple
-    // 1.0     0.8     0.1     1.0     purple
+    //   1.0     0.1     0.1     0.2     black
-    // 1.0     0.1     0.1     0.2     black
+    //   1.0     0.9     1.0     1.0     white
-    // 1.0     0.9     1.0     1.0     white
+    //   1.0     0.1     0.9     1.0     cyan
-    // 1.0     0.1     0.9     1.0     cyan
+    //   1.0     0.5     0.3     0.2     brown
-    // 1.0     0.5     0.3     0.2     brown
+    //   1.0     1.0     0.4     0.0     orange (v3+)
-    // 1.0     1.0     0.4     0.0     orange (v3+)
+    //   1.0     0.502   0.545   0.977   light-blue (v3+)
-    // 1.0     0.502   0.545   0.977   light-blue (v3+)
+    //   1.0     0.502   0.502   0.0     olive (v3+)
-    // 1.0     0.502   0.502   0.0     olive (v3+)
+    //   1.0     0.0     0.941   0.714   turquoise (v3+)
-    // 1.0     0.0     0.941   0.714   turquoise (v3+)
+    //   1.0     0.8     0.098   0.392   fuchsia (v3+)
-    // 1.0     0.8     0.098   0.392   fuchsia (v3+)
+    //   1.0     0.498   0.498   0.498   grey (v3+)
-    // 1.0     0.498   0.498   0.498   grey (v3+)
+    //   1.0     0.996   0.996   0.832   cream (v3+)
-    // 1.0     0.996   0.996   0.832   cream (v3+)
+    //   1.0     0.996   0.498   0.784   pink (v3+)
-    // 1.0     0.996   0.498   0.784   pink (v3+)
+    //   1.0     0.0     0.498   0.322   dark-green (v3+)
-    // 1.0     0.0     0.498   0.322   dark-green (v3+)
     le_float alpha;
     le_float red;
     le_float green;

src/Items.cc

@@ -9,9 +9,8 @@ using namespace std;
 void player_use_item(shared_ptr<Client> c, size_t item_index, shared_ptr<RandomGenerator> rand_crypt) {
   auto s = c->require_server_state();
 
-  // On PC (and presumably DC), the client sends a 6x29 after this to delete the
+  // On PC (and presumably DC), the client sends a 6x29 after this to delete the used item. On GC and later versions,
-  // used item. On GC and later versions, this does not happen, so we should
+  // this does not happen, so we should delete the item here.
-  // delete the item here.
   bool is_v4 = ::is_v4(c->version());
   bool is_v3_or_later = is_v3(c->version()) || is_v4;
   bool should_delete_item = is_v3_or_later;
@@ -37,8 +36,7 @@ void player_use_item(shared_ptr<Client> c, size_t item_index, shared_ptr<RandomG
     }
 
     auto& weapon = player->inventory.items[player->inventory.find_equipped_item(EquipSlot::WEAPON)];
-    // Only enforce grind limits on BB, since the server doesn't have direct
+    // Only enforce grind limits on BB, since the server doesn't have direct control over inventories on other versions
-    // control over players' inventories on other versions
     auto item_parameter_table = s->item_parameter_table(c->version());
     auto weapon_def = item_parameter_table->get_weapon(weapon.data.data1[1], weapon.data.data1[2]);
     if (is_v4 && (weapon.data.data1[3] >= weapon_def.max_grind)) {
@@ -77,8 +75,6 @@ void player_use_item(shared_ptr<Client> c, size_t item_index, shared_ptr<RandomG
       case 6: // Hit Material (v1/v2) or Luck Material (v3/v4)
         type = Type::LUCK;
         if (!is_v3_or_later && (c->version() != Version::GC_NTE)) {
-          // Hit material doesn't exist on v3/v4, but we'll ignore type anyway
-          // in this case because track_non_hp_tp_materials is false
           p->disp.stats.char_stats.ata += 2;
         } else {
           p->disp.stats.char_stats.lck += 2;
@@ -173,10 +169,9 @@ void player_use_item(shared_ptr<Client> c, size_t item_index, shared_ptr<RandomG
     if (sum == 0) {
       throw runtime_error("no unwrap results available for event");
     }
-    // TODO: It seems that on non-BB, clients don't synchronize this at all, so
+    // TODO: It seems that on non-BB, clients don't synchronize this at all, so they could end up thinking the
-    // they could end up thinking the unwrapped item is something completely
+    // unwrapped item is something completely different. (They don't even use a fixed random seed, like for rares; they
-    // different. (They don't even use a fixed random seed, like for rares;
+    // just call rand().) How does this actually work on console PSO?
-    // they just call rand().) How does this actually work on console PSO?
     size_t det = rand_crypt->next() % sum;
     for (size_t z = 0; z < table.second; z++) {
       const auto& entry = table.first[z];
@@ -264,8 +259,8 @@ void player_use_item(shared_ptr<Client> c, size_t item_index, shared_ptr<RandomG
   }
 
   if (should_delete_item) {
-    // Allow overdrafting meseta if the client is not BB, since the server isn't
+    // Allow overdrafting meseta if the client is not BB, since the server isn't informed when meseta is added or
-    // informed when meseta is added or removed from the bank.
+    // removed from the bank.
     player->remove_item(item.data.id, 1, *s->item_stack_limits(c->version()));
   }
 }
@@ -323,8 +318,8 @@ void apply_mag_feed_result(
   uint8_t evolution_number = mag_evolution_table->get_evolution_number(mag_item.data1[1]);
   uint8_t mag_number = mag_item.data1[1];
 
-  // Note: Sega really did just hardcode all these rules into the client. There
+  // Note: Sega really did just hardcode all these rules into the client. There is no data file describing these
-  // is no data file describing these evolutions, unfortunately.
+  // evolutions, unfortunately.
 
   if (mag_level < 10) {
     // Nothing to do
@@ -408,9 +403,8 @@ void apply_mag_feed_result(
           throw logic_error("char class is not any of the top-level classes");
         }
 
-        // Note: The original code checks the class (hunter/ranger/force) again
+        // Note: The original code checks the class (hunter/ranger/force) again here, and goes into 3 branches that
-        // here, and goes into 3 branches that each do these same checks.
+        // each do these same checks. However, the result of all 3 branches is exactly the same!
-        // However, the result of all 3 branches is exactly the same!
         if (((section_id_group == 0) && (pow + mind == def + dex)) ||
             ((section_id_group == 1) && (pow + dex == mind + def)) ||
             ((section_id_group == 2) && (pow + def == mind + dex))) {
@@ -443,54 +437,36 @@ void apply_mag_feed_result(
 
         if (is_hunter) {
           if (flags & 0x108) {
-            mag_item.data1[1] = (section_id & 1)
+            mag_item.data1[1] = (section_id & 1) ? ((dex < mind) ? 0x08 : 0x06) : ((dex < mind) ? 0x0C : 0x05);
-                ? ((dex < mind) ? 0x08 : 0x06)
-                : ((dex < mind) ? 0x0C : 0x05);
           } else if (flags & 0x010) {
-            mag_item.data1[1] = (section_id & 1)
+            mag_item.data1[1] = (section_id & 1) ? ((mind < pow) ? 0x12 : 0x10) : ((mind < pow) ? 0x17 : 0x13);
-                ? ((mind < pow) ? 0x12 : 0x10)
-                : ((mind < pow) ? 0x17 : 0x13);
           } else if (flags & 0x020) {
-            mag_item.data1[1] = (section_id & 1)
+            mag_item.data1[1] = (section_id & 1) ? ((pow < dex) ? 0x16 : 0x24) : ((pow < dex) ? 0x07 : 0x1E);
-                ? ((pow < dex) ? 0x16 : 0x24)
-                : ((pow < dex) ? 0x07 : 0x1E);
           }
         } else if (is_ranger) {
           if (flags & 0x110) {
-            mag_item.data1[1] = (section_id & 1)
+            mag_item.data1[1] = (section_id & 1) ? ((mind < pow) ? 0x0A : 0x05) : ((mind < pow) ? 0x0C : 0x06);
-                ? ((mind < pow) ? 0x0A : 0x05)
-                : ((mind < pow) ? 0x0C : 0x06);
           } else if (flags & 0x008) {
-            mag_item.data1[1] = (section_id & 1)
+            mag_item.data1[1] = (section_id & 1) ? ((dex < mind) ? 0x0A : 0x26) : ((dex < mind) ? 0x0C : 0x06);
-                ? ((dex < mind) ? 0x0A : 0x26)
-                : ((dex < mind) ? 0x0C : 0x06);
           } else if (flags & 0x020) {
-            mag_item.data1[1] = (section_id & 1)
+            mag_item.data1[1] = (section_id & 1) ? ((pow < dex) ? 0x18 : 0x1E) : ((pow < dex) ? 0x08 : 0x05);
-                ? ((pow < dex) ? 0x18 : 0x1E)
-                : ((pow < dex) ? 0x08 : 0x05);
           }
         } else if (is_force) {
           if (flags & 0x120) {
             if (def < 45) {
-              mag_item.data1[1] = (section_id & 1)
+              mag_item.data1[1] = (section_id & 1) ? ((pow < dex) ? 0x17 : 0x09) : ((pow < dex) ? 0x1E : 0x1C);
-                  ? ((pow < dex) ? 0x17 : 0x09)
-                  : ((pow < dex) ? 0x1E : 0x1C);
             } else {
               mag_item.data1[1] = 0x24;
             }
           } else if (flags & 0x008) {
             if (def < 45) {
-              mag_item.data1[1] = (section_id & 1)
+              mag_item.data1[1] = (section_id & 1) ? ((dex < mind) ? 0x1C : 0x20) : ((dex < mind) ? 0x1F : 0x25);
-                  ? ((dex < mind) ? 0x1C : 0x20)
-                  : ((dex < mind) ? 0x1F : 0x25);
             } else {
               mag_item.data1[1] = 0x23;
             }
           } else if (flags & 0x010) {
             if (def < 45) {
-              mag_item.data1[1] = (section_id & 1)
+              mag_item.data1[1] = (section_id & 1) ? ((mind < pow) ? 0x12 : 0x0C) : ((mind < pow) ? 0x15 : 0x11);
-                  ? ((mind < pow) ? 0x12 : 0x0C)
-                  : ((mind < pow) ? 0x15 : 0x11);
             } else {
               mag_item.data1[1] = 0x24;
             }

src/LevelTable.cc

@@ -19,24 +19,23 @@ void LevelTable::reset_to_base(PlayerStats& stats, uint8_t char_class) const {
 void LevelTable::advance_to_level(PlayerStats& stats, uint32_t level, uint8_t char_class) const {
   for (; stats.level < level; stats.level++) {
     const auto& level_stats = this->stats_delta_for_level(char_class, stats.level + 1);
-    // The original code clamps the resulting stat values to [0, max_stat]; we
+    // The original code clamps the resulting stat values to [0, max_stat]; we don't have max_stat handy so we just
-    // don't have max_stat handy so we just allow them to be unbounded
+    // allow them to be unbounded
     stats.char_stats.atp += level_stats.atp;
     stats.char_stats.mst += level_stats.mst;
     stats.char_stats.evp += level_stats.evp;
     stats.char_stats.hp += level_stats.hp;
     stats.char_stats.dfp += level_stats.dfp;
     stats.char_stats.ata += level_stats.ata;
-    // Note: It is not a bug that lck is ignored here; the original code
+    // Note: It is not a bug that lck is ignored here; the original code ignores it too.
-    // ignores it too.
     stats.experience = level_stats.experience;
   }
 }
 
 LevelTableV2::LevelTableV2(const string& data, bool compressed) {
   struct Offsets {
-    // TODO: The overall format of this file on V2 has much more data than we
+    // TODO: The overall format of this file on V2 has much more data than we actually use. What's known of the
-    // actually use. What's known of the structure so far:
+    // structure so far:
     le_uint32_t level_deltas; // (5468) -> u32[9] -> LevelStatsDelta[200]
     le_uint32_t unknown_a1; // (548C) -> float[6]
     le_uint32_t max_stats; // (54A4) -> PlayerStats[9]

src/LevelTable.hh

@@ -96,11 +96,9 @@ check_struct_size(LevelStatsDelta, 0x0C);
 check_struct_size(LevelStatsDeltaBE, 0x0C);
 
 class LevelTable {
-  // This is the base class for all the LevelTable implementations. The public
+  // This is the base class for all the LevelTable implementations. The public interface here only defines functions
-  // interface here only defines functions that the server needs to handle
+  // that the server needs to handle requests, but some subclasses implement more functionality. See the comments and
-  // requests, but some subclasses implement more functionality. See the
+  // Offsets structures inside the subclasses' constructor implementations for more details on the file formats.
-  // comments and Offsets structures inside the subclasses' constructor
-  // implementations for more details on the file formats.
 public:
   virtual ~LevelTable() = default;
   virtual const CharacterStats& base_stats_for_class(uint8_t char_class) const = 0;

src/Lobby.cc

@@ -424,8 +424,7 @@ void Lobby::add_client(shared_ptr<Client> c, ssize_t required_client_id) {
     this->create_item_creator();
   }
 
-  // If this is a lobby or no one was here before this, reassign all the floor
+  // If this is a lobby or no one was here before this, reassign all the floor item IDs and reset the next item IDs
-  // item IDs and reset the next item IDs
   if (!this->is_game() || (leader_index >= this->max_clients)) {
     this->reset_next_item_ids();
     for (auto& m : this->floor_item_managers) {
@@ -433,19 +432,15 @@ void Lobby::add_client(shared_ptr<Client> c, ssize_t required_client_id) {
     }
   }
 
-  // If this is not a game or the joining client is the leader, they will assign
+  // If this is not a game or the joining client is the leader, they will assign their item IDs BEFORE they process any
-  // their item IDs BEFORE they process any inbound commands (therefore a 6x6D
+  // inbound commands (therefore a 6x6D command, which we will send during loading, should reflect the item state AFTER
-  // command, which we will send during loading, should reflect the item state
+  // their IDs are assigned). If the joining client is not the leader, they will not assign their item IDs until they
-  // AFTER their IDs are assigned). If the joining client is not the leader,
+  // receive a 6x71 command, which is sent AFTER the 6x6D command, so the 6x6D should reflect the item state BEFORE
-  // they will not assign their item IDs until they receive a 6x71 command,
+  // their IDs are assigned. (In the latter case, we'll assign the IDs for real when they send a 6F command, or 6x1F
-  // which is sent AFTER the 6x6D command, so the 6x6D should reflect the item
+  // equivalent in the case of DC NTE and 11/2000.)
-  // 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
+  // On BB, we send flag state to fix an Episode 2 bug where the CCA door lock state is overwritten by quests.
-  // CCA door lock state is overwritten by quests.
   if (this->is_game() && (c->version() == Version::BB_V4)) {
     c->set_flag(Client::Flag::SHOULD_SEND_ARTIFICIAL_FLAG_STATE);
   }
@@ -492,9 +487,8 @@ void Lobby::remove_client(shared_ptr<Client> c) {
   }
   this->clients[c->lobby_client_id] = nullptr;
 
-  // Unassign the client's lobby if it matches the current lobby (it may not
+  // Unassign the client's lobby if it matches the current lobby (it may not match if the client was already added to
-  // match if the client was already added to another lobby - this can happen
+  // another lobby - this can happen during the lobby change procedure)
-  // during the lobby change procedure)
   {
     auto c_lobby = c->lobby.lock();
     if (c_lobby.get() == this) {
@@ -521,9 +515,8 @@ void Lobby::remove_client(shared_ptr<Client> c) {
     }
   }
 
-  // If there are still players left in the lobby, delete all items that only
+  // If there are still players left in the lobby, delete all items that only the leaving player could see. Don't do
-  // the leaving player could see. Don't do this if no one is left in the lobby,
+  // this if no one is left in the lobby, since that would mean items could not persist in empty lobbies.
-  // since that would mean items could not persist in empty lobbies.
   uint16_t remaining_clients_mask = 0;
   for (size_t z = 0; z < 12; z++) {
     if (this->clients[z]) {
@@ -544,8 +537,7 @@ void Lobby::remove_client(shared_ptr<Client> c) {
       this->check_flag(Flag::PERSISTENT) &&
       !this->check_flag(Flag::DEFAULT) &&
       (this->idle_timeout_usecs > 0)) {
-    // If the lobby is persistent but has an idle timeout, make it expire after
+    // If the lobby is persistent but has an idle timeout, make it expire after the specified time
-    // the specified time
     this->idle_timeout_timer.expires_after(std::chrono::microseconds(this->idle_timeout_usecs));
     this->idle_timeout_timer.async_wait([this](std::error_code ec) {
       if (!ec) {
@@ -561,10 +553,7 @@ void Lobby::remove_client(shared_ptr<Client> c) {
   }
 }
 
-void Lobby::move_client_to_lobby(
+void Lobby::move_client_to_lobby(shared_ptr<Lobby> dest_lobby, shared_ptr<Client> c, ssize_t required_client_id) {
-    shared_ptr<Lobby> dest_lobby,
-    shared_ptr<Client> c,
-    ssize_t required_client_id) {
   if (dest_lobby.get() == this) {
     return;
   }
@@ -648,8 +637,7 @@ Lobby::JoinError Lobby::join_error_for_client(std::shared_ptr<Client> c, const s
         }
       }
     }
-    // Only prevent joining during loading if the client is actually trying to
+    // Only prevent joining during loading if the client is actually trying to join (not just loading the game list)
-    // join (not just loading the game list)
     if (password && this->any_client_loading()) {
       return JoinError::LOADING;
     }
@@ -714,9 +702,8 @@ uint32_t Lobby::generate_item_id(uint8_t client_id) {
 }
 
 void Lobby::on_item_id_generated_externally(uint32_t item_id) {
-  // Note: The client checks for the range (0x00010000, 0x02010000) here, but
+  // Note: The client checks for the range (0x00010000, 0x02010000) here, but server-side item drop logic uses
-  // server-side item drop logic uses 0x00810000 as its base ID, so we restrict
+  // 0x00810000 as its base ID, so we restrict the range further here.
-  // the range further here.
   if ((item_id > 0x00010000) && (item_id < 0x00810000)) {
     uint16_t item_client_id = (item_id >> 21) & 0x7FF;
     uint32_t& next_item_id = this->next_item_id_for_client.at(item_client_id);

src/Lobby.hh

@@ -30,12 +30,10 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
     // At most one of the following will be non-null
     std::shared_ptr<const MapState::ObjectState> from_obj;
     std::shared_ptr<const MapState::EnemyState> from_ene;
-    // The low 12 bits of flags are visibility flags, specifying which clients
+    // The low 12 bits of flags are visibility flags, specifying which clients can see the item. (In practice, only the
-    // can see the item. (In practice, only the lowest 4 of these bits are used,
+    // lowest 4 of these bits are used, but the game has fields for 12 players so we do too.) The 13th bit (0x1000)
-    // but the game has fields for 12 players so we do too.)
+    // specifies whether a rare item notification should be sent to all players when the item is picked up. This has no
-    // The 13th bit (0x1000) specifies whether a rare item notification should
+    // effect for non-rare items.
-    // be sent to all players when the item is picked up. This has no effect for
-    // non-rare items.
     uint16_t flags;
 
     bool visible_to_client(uint8_t client_id) const;
@@ -43,8 +41,7 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
   struct FloorItemManager {
     phosg::PrefixedLogger log;
     uint64_t next_drop_number;
-    // It's important that this is a map and not an unordered_map. See the
+    // It's important that this is a map and not an unordered_map. See the comment in send_game_item_state for details.
-    // comment in send_game_item_state for more details.
     std::map<uint32_t, std::shared_ptr<FloorItem>> items; // Keyed on item_id
     std::array<std::map<uint64_t, std::shared_ptr<FloorItem>>, 12> queue_for_client;
 
@@ -111,9 +108,8 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
   std::unique_ptr<SwitchFlags> switch_flags;
 
   // Game config
-  // Bits in allowed_versions specify who is allowed to join this game. The
+  // Bits in allowed_versions specify who is allowed to join this game. The bits are indexed as (1 << version), where
-  // bits are indexed as (1 << version), where version is a value from the
+  // version is a value from the Version enum.
-  // Version enum.
   uint16_t allowed_versions = 0x0000;
   uint8_t creator_section_id = 0xFF;
   uint8_t override_section_id = 0xFF;
@@ -145,16 +141,14 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
   std::shared_ptr<ChallengeParameters> challenge_params;
 
   // Ep3 stuff
-  // There are three kinds of Episode 3 games. All of these types have episode
+  // There are three kinds of Episode 3 games. All of these types have episode set to EP3; types 2 and 3 additionally
-  // set to EP3; types 2 and 3 additionally have the IS_SPECTATOR_TEAM flag.
+  // have the IS_SPECTATOR_TEAM flag.
   // 1. Primary games. These are the lobbies where battles may take place.
-  // 2. Watcher games. These lobbies receive all the battle and chat commands
+  // 2. Watcher games. These lobbies receive all the battle and chat commands from a primary game. (This the
-  //    from a primary game. (This the implementation of spectator teams.)
+  //    implementation of spectator teams.)
-  // 3. Replay games. These lobbies replay a sequence of battle commands and
+  // 3. Replay games. These lobbies replay a sequence of battle and chat commands from a previous primary game.
-  //    chat commands from a previous primary game.
+  // Types 2 and 3 are distinguished by the presence of the battle_record field - in replay games, it will be present;
-  // Types 2 and 3 may be distinguished by the presence of the battle_record
+  // in watcher games it will be absent.
-  // field - in replay games, it will be present; in watcher games it will be
-  // absent.
   std::shared_ptr<Episode3::Server> ep3_server; // Only used in primary games
   std::weak_ptr<Lobby> watched_lobby; // Only used in watcher games
   std::unordered_set<std::shared_ptr<Lobby>> watcher_lobbies; // Only used in primary games
@@ -174,8 +168,7 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
   // Keys in this map are client_id
   std::unordered_map<size_t, std::weak_ptr<Client>> clients_to_add;
 
-  // This is only used when the PERSISTENT flag is set and idle_timeout_usecs
+  // This is only used when the PERSISTENT flag is set and idle_timeout_usecs is not zero
-  // is not zero
   uint64_t idle_timeout_usecs = 0;
   asio::steady_timer idle_timeout_timer;
 
@@ -239,9 +232,7 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
   void remove_client(std::shared_ptr<Client> c);
 
   void move_client_to_lobby(
-      std::shared_ptr<Lobby> dest_lobby,
+      std::shared_ptr<Lobby> dest_lobby, std::shared_ptr<Client> c, ssize_t required_client_id = -1);
-      std::shared_ptr<Client> c,
-      ssize_t required_client_id = -1);
 
   std::shared_ptr<Client> find_client(const std::string* identifier = nullptr, uint64_t account_id = 0);
 

src/Main.cc

@@ -54,12 +54,6 @@
 
 using namespace std;
 
-#ifdef PHOSG_WINDOWS
-static constexpr bool IS_WINDOWS = true;
-#else
-static constexpr bool IS_WINDOWS = false;
-#endif
-
 bool use_terminal_colors = false;
 
 void print_version_info();
@@ -210,8 +204,7 @@ void write_output_data(phosg::Arguments& args, const void* data, size_t size, co
     phosg::save_file(output_filename, data, size);
 
   } else if (output_filename.empty() && (output_filename != "-") && !input_filename.empty() && (input_filename != "-")) {
-    // If no output filename is given and an input filename is given, write to
+    // If no output filename is given and an input filename is given, write to <input_filename>.<extension>
-    // <input_filename>.<extension>
     if (!extension) {
       throw runtime_error("an output filename is required");
     }
@@ -221,8 +214,7 @@ void write_output_data(phosg::Arguments& args, const void* data, size_t size, co
     phosg::save_file(filename, data, size);
 
   } else if (isatty(fileno(stdout)) && (!extension || !is_text_extension(extension))) {
-    // If stdout is a terminal and the data is not known to be text, use
+    // If stdout is a terminal and the data is not known to be text, use print_data to write the result
-    // print_data to write the result
     phosg::print_data(stdout, data, size);
     fflush(stdout);
 
@@ -832,8 +824,8 @@ static void a_encrypt_decrypt_vms_save_fn(phosg::Arguments& args) {
     phosg::fwrite_fmt(stderr, "File type: DC v2 character\n");
     process_file.template operator()<PSODCV2CharacterFile, true>();
   } else if (header.data_size == sizeof(PSODCV1V2GuildCardFile)) {
-    // There appears to be a copy/paste error here: the game uses the character
+    // There appears to be a copy/paste error here: the game uses the character file size when checksumming the Guild
-    // file size when checksumming the Guild Card file, so we must do the same
+    // Card file, so we must do the same
     if (!is_v2) {
       phosg::fwrite_fmt(stderr, "File type: DC v1 Guild Card list\n");
       static_assert(sizeof(PSODCV1CharacterFile) <= sizeof(PSODCV1V2GuildCardFile::EncryptedSection));
@@ -1343,15 +1335,12 @@ Action a_salvage_gci(
               0, 0x100000000, 0x1000, num_threads);
 
         } else if (!exhaustive) {
-          // The pseudorandom number generator used by PSO to encrypt its save
+          // The pseudorandom number generator used by PSO to encrypt its save files has a weakness: if the low bits of
-          // files has a weakness: if the low bits of the seed are correct, the
+          // the seed are correct, the low bits of each 32-bit integer in the plaintext will also be correct, even if
-          // low bits of each 32-bit integer in the plaintext will also be
+          // the high bits of the seed are wrong. Using this, we can brute-force the low half of the seed, then the
-          // correct, even if the high bits of the seed are wrong. Using this,
+          // high half, which is much faster than trying all possible seeds. Unfortunately, this relies on the
-          // we can brute-force the low half of the seed, then the high half,
+          // distribution of values in the plaintext, so it only works for the round-2 seed - the decrypted data after
-          // which is much faster than trying all possible seeds.
+          // round 1 is still essentially random.
-          // Unfortunately, this relies on the distribution of values in the
-          // plaintext, so it only works for the round-2 seed - the decrypted
-          // data after round 1 is still essentially random.
           phosg::parallel_range_blocks<uint64_t>(try_round2_seed, 0, 0x100000, 0x1000, num_threads);
           auto intermediate_top_seeds = merge_top_seeds(top_seeds_by_thread);
           if (intermediate_top_seeds.empty()) {
@@ -1370,8 +1359,7 @@ Action a_salvage_gci(
               0, 0x10000, 0x80, num_threads);
 
         } else {
-          // The user requested not to take any shortcuts, so burn a lot of CPU
+          // The user requested not to take any shortcuts, so burn a lot of CPU power
-          // power
           phosg::parallel_range_blocks<uint64_t>(try_round2_seed, 0, 0x100000000, 0x1000, num_threads);
         }
 
@@ -1446,14 +1434,10 @@ Action a_find_decryption_seed(
         string le_decrypt_buf = ciphertext.substr(0, max_plaintext_size);
         if (uses_v3_encryption(version)) {
           PSOV3Encryption(seed).encrypt_both_endian(
-              le_decrypt_buf.data(),
+              le_decrypt_buf.data(), be_decrypt_buf.data(), be_decrypt_buf.size());
-              be_decrypt_buf.data(),
-              be_decrypt_buf.size());
         } else {
           PSOV2Encryption(seed).encrypt_both_endian(
-              le_decrypt_buf.data(),
+              le_decrypt_buf.data(), be_decrypt_buf.data(), be_decrypt_buf.size());
-              be_decrypt_buf.data(),
-              be_decrypt_buf.size());
         }
 
         for (const auto& plaintext : plaintexts) {
@@ -3554,8 +3538,7 @@ Action a_replay_ep3_battle_record(
       auto server = make_shared<Episode3::Server>(nullptr, std::move(options));
       server->init();
 
-      // Ignore commands generated by the server when it's constructed (these
+      // Ignore commands generated by the server when it's constructed (these are not included in the battle record)
-      // are not included in the battle record)
       output_queue->clear();
 
       std::array<bool, 4> players_present = {false, false, false, false};
@@ -3576,8 +3559,7 @@ Action a_replay_ep3_battle_record(
             ev.print(stdout);
             break;
           case Episode3::BattleRecord::Event::Type::BATTLE_COMMAND:
-            // Ignore the map command (this is handled separately) and 6xB4x4B
+            // Ignore the map command (handled separately) and 6xB4x4B (only needed when a lobby is present)
-            // (which is only generated when a lobby is present)
             if (ev.data.empty() || (static_cast<uint8_t>(ev.data[0]) == 0xB6) || (ev.data.at(4) == 0x4B)) {
               ev.print(stdout);
             } else {
@@ -3594,8 +3576,7 @@ Action a_replay_ep3_battle_record(
                 phosg::print_data(stderr, ev.data, 0, nullptr, phosg::PrintDataFlags::OFFSET_16_BITS | phosg::PrintDataFlags::PRINT_ASCII);
                 throw std::runtime_error("Output did not match expectations");
               }
-              // Hack: don't check the last field in 6xB4x46 since it contains
+              // Hack: don't check the last field in 6xB4x46 since it contains a timestamp on non-NTE
-              // a timestamp on non-NTE
               bool matched = false;
               if ((ev.data.at(4) == 0x46) && !is_nte) {
                 auto received_cmd = check_size_t<G_ServerVersionStrings_Ep3_6xB4x46>(output_queue->front());
@@ -3631,9 +3612,8 @@ Action a_replay_ep3_battle_record(
               phosg::print_data(stderr, output_queue->front());
               throw std::runtime_error("Output did not match expectations");
             }
-            // Hack: Set the CPU player flag if the player isn't present in the
+            // Hack: Set the CPU player flag if the player isn't present in the recording (normally this is done by
-            // recording (normally this is done by checking the Lobby, but
+            // checking the Lobby, but there's no Lobby during a replay)
-            // there's no Lobby during a replay)
             if (ev.data.at(4) == 0x1B) {
               string mutable_data = ev.data;
               auto& cmd = check_size_t<G_SetPlayerName_Ep3_CAx1B>(mutable_data);
@@ -3894,30 +3874,6 @@ int main(int argc, char** argv) {
     phosg::log_error_f("Unknown or invalid action; try --help");
     return 1;
   }
-  if (IS_WINDOWS) {
+  a->run(args);
-    // Cygwin just gives a stackdump when an exception falls out of main(), so
-    // unlike Linux and macOS, we have to manually catch exceptions here just to
-    // see what the exception message was.
-    try {
-      a->run(args);
-    } catch (const phosg::cannot_open_file& e) {
-      phosg::log_error_f("Top-level exception (cannot_open_file): {}", e.what());
-      throw;
-    } catch (const invalid_argument& e) {
-      phosg::log_error_f("Top-level exception (invalid_argument): {}", e.what());
-      throw;
-    } catch (const out_of_range& e) {
-      phosg::log_error_f("Top-level exception (out_of_range): {}", e.what());
-      throw;
-    } catch (const runtime_error& e) {
-      phosg::log_error_f("Top-level exception (runtime_error): {}", e.what());
-      throw;
-    } catch (const exception& e) {
-      phosg::log_error_f("Top-level exception: {}", e.what());
-      throw;
-    }
-  } else {
-    a->run(args);
-  }
   return 0;
 }

src/Menu.hh

@@ -7,11 +7,10 @@
 #include <string>
 #include <vector>
 
-// Note: These aren't enums because neither enum nor enum class does what we
+// Note: These aren't enums because neither enum nor enum class does what we want. Specifically, we need GO_BACK to be
-// want. Specifically, we need GO_BACK to be valid in multiple enums (and enums
+// valid in multiple enums (and enums aren't namespaced unless they're enum classes), so we can't use enums. But we
-// aren't namespaced unless they're enum classes), so we can't use enums. But we
+// also want to be able to use non-enum values in switch statements without casting values all over the place, so we
-// also want to be able to use non-enum values in switch statements without
+// can't use enum classes either.
-// casting values all over the place, so we can't use enum classes either.
 
 namespace MenuID {
 constexpr uint32_t MAIN = 0x11000011;
@@ -22,8 +21,7 @@ constexpr uint32_t GAME = 0x44000044;
 constexpr uint32_t QUEST_EP1 = 0x55010155;
 constexpr uint32_t QUEST_EP2 = 0x55020255;
 constexpr uint32_t QUEST_EP3 = 0x55030355;
-// See the decsription of the A2 command in CommandFormats.hh for why these
+// See the decsription of the A2 command in CommandFormats.hh for why these menu IDs don't fit the rest of the pattern.
-// menu IDs don't fit the rest of the pattern.
 constexpr uint32_t QUEST_CATEGORIES_EP1_EP3_EP4 = 0x01000001;
 constexpr uint32_t QUEST_CATEGORIES_EP2 = 0x02000002;
 constexpr uint32_t PROXY_DESTINATIONS = 0x77000077;
@@ -92,20 +90,10 @@ constexpr uint32_t MEMBERS_20_LEADERS_3 = 0x03030303;
 constexpr uint32_t MEMBERS_40_LEADERS_5 = 0x04040404;
 constexpr uint32_t MEMBERS_70_LEADERS_8 = 0x05050505;
 constexpr uint32_t MEMBERS_100_LEADERS_10 = 0x06060606;
-// constexpr uint32_t POINT_OF_DISASTER = ...;
-// constexpr uint32_t TOYS_TWILIGHT = ...;
-// constexpr uint32_t COMMANDER_BLADE = ...;
-// constexpr uint32_t UNION_GUARD = ...;
-// constexpr uint32_t TEAM_POINTS_500 = ...;
-// constexpr uint32_t TEAM_POINTS_1000 = ...;
-// constexpr uint32_t TEAM_POINTS_5000 = ...;
-// constexpr uint32_t TEAM_POINTS_10000 = ...;
 } // namespace TeamRewardMenuItemID
 
 struct MenuItem {
   enum Flag {
-    // For menu items to be visible on DC NTE, they must not have either of the
-    // following two flags. (The INVISIBLE_ON_GC_NTE flag behaves similarly.)
     INVISIBLE_ON_DC_PROTOS = 0x001,
     INVISIBLE_ON_DC = 0x002,
     INVISIBLE_ON_PC_NTE = 0x004,
@@ -131,16 +119,8 @@ struct MenuItem {
   std::function<std::string()> get_description;
   uint32_t flags;
 
-  MenuItem(
+  MenuItem(uint32_t item_id, const std::string& name, const std::string& description, uint32_t flags);
-      uint32_t item_id,
+  MenuItem(uint32_t item_id, const std::string& name, std::function<std::string()> get_description, uint32_t flags);
-      const std::string& name,
-      const std::string& description,
-      uint32_t flags);
-  MenuItem(
-      uint32_t item_id,
-      const std::string& name,
-      std::function<std::string()> get_description,
-      uint32_t flags);
 };
 
 struct Menu {

src/NetworkAddresses.cc

@@ -103,8 +103,7 @@ uint32_t address_for_string(const char* address) {
 }
 
 uint64_t devolution_phone_number_for_netloc(uint32_t addr, uint16_t port) {
-  // It seems the address part of the number is fixed-width, but the port is
+  // It seems the address part of the number is fixed-width, but the port is not. Why did they do it this way?
-  // not. Why did they do it this way?
   if (port & 0xF000) {
     return (static_cast<uint64_t>(addr) << 16) | port;
   } else if (port & 0x0F00) {

src/NetworkAddresses.hh

@@ -6,8 +6,7 @@
 #include <map>
 #include <string>
 
-// PSO is IPv4-only, so we just treat addresses as uint32_t everywhere because
+// PSO is IPv4-only, so we just treat addresses as uint32_t everywhere because it's easier
-// it's easier
 
 std::map<std::string, uint32_t> get_local_addresses();
 uint32_t get_connected_address(int fd);

src/PPKArchive.cc

@@ -19,7 +19,7 @@ struct Entry {
   le_uint32_t compressed_size;
   le_uint32_t checksum;
   // Data follows immediately here
-  // Trailer: le_uint32_t entry_size; //
+  // Trailer: le_uint32_t entry_size;
 };
 
 static void decrypt_ppk_data(std::string& data, const std::string& filename, const std::string& password) {

src/PatchFileIndex.cc

@@ -51,8 +51,8 @@ PatchFileIndex::PatchFileIndex(const string& root_dir)
     patch_index_log.warning_f("Cannot load patch metadata cache from {}: {}", metadata_cache_filename, e.what());
   }
 
-  // Assuming it's rare for patch files to change, we skip writing the metadata
+  // Assuming it's rare for patch files to change, we skip writing the metadata cache if no files were changed at all
-  // cache if no files were changed at all (which should usually be the case)
+  // (which should usually be the case)
   bool should_write_metadata_cache = false;
   phosg::JSON new_metadata_cache_json = phosg::JSON::dict();
 
@@ -124,10 +124,8 @@ PatchFileIndex::PatchFileIndex(const string& root_dir)
           should_write_metadata_cache = true;
 
         } else {
-          // File was not modified and cache item was valid; just use the
+          // File was not modified and cache item was valid; just use the existing cache item
-          // existing cache item
+          new_metadata_cache_json.emplace(relative_item_path, std::move(cache_item_json));
-          new_metadata_cache_json.emplace(
-              relative_item_path, std::move(cache_item_json));
         }
 
         this->files_by_patch_order.emplace_back(f);
@@ -159,12 +157,10 @@ PatchFileIndex::PatchFileIndex(const string& root_dir)
   }
 }
 
-const vector<shared_ptr<PatchFileIndex::File>>&
+const vector<shared_ptr<PatchFileIndex::File>>& PatchFileIndex::all_files() const {
-PatchFileIndex::all_files() const {
   return this->files_by_patch_order;
 }
 
-shared_ptr<PatchFileIndex::File> PatchFileIndex::get(
+shared_ptr<PatchFileIndex::File> PatchFileIndex::get(const string& filename) const {
-    const string& filename) const {
   return this->files_by_name.at(filename);
 }

src/PatchFileIndex.hh

@@ -47,8 +47,6 @@ struct PatchFileChecksumRequest {
         size(0),
         response_received(false) {}
   inline bool needs_update() const {
-    return !this->response_received ||
+    return !this->response_received || (this->crc32 != this->file->crc32) || (this->size != this->file->size);
-        (this->crc32 != this->file->crc32) ||
-        (this->size != this->file->size);
   }
 };

src/PlayerInventory.hh

@@ -21,27 +21,22 @@
 class Client;
 class ItemParameterTable;
 
-// PSO V2 stored some extra data in the character structs in a format that I'm
+// PSO V2 stored some extra data in the character structs in a format that I'm sure Sega thought was very clever for
-// sure Sega thought was very clever for backward compatibility, but for us is
+// backward compatibility, but for us is just plain annoying. Specifically, they used the third and fourth bytes of the
-// just plain annoying. Specifically, they used the third and fourth bytes of
+// InventoryItem struct to store some things not present in V1. The game stores arrays of bytes striped across these
-// the InventoryItem struct to store some things not present in V1. The game
+// structures. In newserv, we call those fields extension_data. They contain:
-// stores arrays of bytes striped across these structures. In newserv, we call
-// those fields extension_data. They contain:
 //   items[0].extension_data1 through items[19].extension_data1:
-//       Extended technique levels. The values in the technique_levels_v1 array
+//       Extended technique levels. The values in the technique_levels_v1 array only go up to 14 (tech level 15); if
-//       only go up to 14 (tech level 15); if the player has a technique above
+//       the player has a technique above level 15, the extension_data1 field holds the remaining levels (so a level 20
-//       level 15, the corresponding extension_data1 field holds the remaining
+//       technique would have 14 in technique_levels_v1 and 5 in the corresponding item's extension_data1 field).
-//       levels (so a level 20 tech would have 14 in technique_levels_v1 and 5
-//       in the corresponding item's extension_data1 field).
 //   items[0].extension_data2 through items[3].extension_data2:
-//       The flags field from the PSOGCCharacterFile::Character struct; see
+//       The flags field from the PSOGCCharacterFile::Character struct; see SaveFileFormats.hh for details.
-//       SaveFileFormats.hh for details.
 //   items[4].extension_data2 through items[7].extension_data2:
-//       The timestamp when the character was last saved, in seconds since
+//       The timestamp when the character was last saved, in seconds since January 1, 2000. Stored little-endian, so
-//       January 1, 2000. Stored little-endian, so items[4] contains the LSB.
+//       items[4] contains the LSB.
 //   items[8].extension_data2 through items[12].extension_data2:
-//       Number of power materials, mind materials, evade materials, def
+//       Number of power materials, mind materials, evade materials, def materials, and luck materials (respectively)
-//       materials, and luck materials (respectively) used by the player.
+//       used by the player.
 //   items[13].extension_data2 through items[15].extension_data2:
 //       Unknown. These are not an array, but do appear to be related.
 
@@ -149,8 +144,7 @@ struct PlayerInventoryT {
         continue;
       }
 
-      // Units can be equipped in multiple slots, so the currently-equipped slot
+      // Units can be equipped in multiple slots, so the currently-equipped slot is stored in the item data itself.
-      // is stored in the item data itself.
       if (((slot == EquipSlot::UNIT_1) && (i.data.data1[4] != 0x00)) ||
           ((slot == EquipSlot::UNIT_2) && (i.data.data1[4] != 0x01)) ||
           ((slot == EquipSlot::UNIT_3) && (i.data.data1[4] != 0x02)) ||
@@ -258,11 +252,10 @@ struct PlayerInventoryT {
 
   void encode_for_client(Version v, std::shared_ptr<const ItemParameterTable> item_parameter_table) {
     if (v == Version::DC_NTE) {
-      // DC NTE has the item count as a 32-bit value here, whereas every other
+      // DC NTE has the item count as a 32-bit value here, whereas every other version uses a single byte. To stop DC
-      // version uses a single byte. To stop DC NTE from crashing by trying to
+      // NTE from crashing by trying to construct far more than 30 TItem objects, we clear the fields DC NTE doesn't
-      // construct far more than 30 TItem objects, we clear the fields DC NTE
+      // know about. Note that the 11/2000 prototype does not have this issue - its inventory format matches the rest
-      // doesn't know about. Note that the 11/2000 prototype does not have this
+      // of the versions.
-      // issue - its inventory format matches the rest of the versions.
       this->hp_from_materials = 0;
       this->tp_from_materials = 0;
       this->language = Language::JAPANESE;
@@ -276,8 +269,8 @@ struct PlayerInventoryT {
       }
     }
 
-    // For pre-V2 clients, use the V2 parameter table, since the V1 table
+    // For pre-V2 clients, use the V2 parameter table, since the V1 table doesn't have correct encodings for backward-
-    // doesn't have correct encodings for backward-compatible V2 items.
+    // compatible V2 items.
     for (size_t z = 0; z < this->items.size(); z++) {
       this->items[z].data.encode_for_version(v, item_parameter_table);
     }

src/PlayerSubordinates.cc

@@ -330,27 +330,19 @@ const QuestFlagsForDifficulty BB_QUEST_FLAG_APPLY_MASK{{
     // clang-format on
 
     // The flags in the above mask are:
-    // 000A 000B 000C 000D 000E 000F 0010 0011 0012 0013 0014 0015 0016 0017
+    // 000A 000B 000C 000D 000E 000F 0010 0011 0012 0013 0014 0015 0016 0017 0018 0019 001A 001E 001F 0020 0021 0022
-    // 0018 0019 001A 001E 001F 0020 0021 0022 0028 0029 002A 002B 002C 002D
+    // 0028 0029 002A 002B 002C 002D 002E 002F 0030 0031 0032 0033 0034 0035 0036 0037 0046 0047 0048 0049 004A 004B
-    // 002E 002F 0030 0031 0032 0033 0034 0035 0036 0037 0046 0047 0048 0049
+    // 004C 004D 004E 004F 0050 0051 0052 0053 0054 0055 0056 0057 0058 0059 005A 005B 005C 005D 005E 005F 0060 0061
-    // 004A 004B 004C 004D 004E 004F 0050 0051 0052 0053 0054 0055 0056 0057
+    // 0062 0063 0097 0098 0099 009A 012D 012E 012F 0130 0131 0132 0133 0134 0135 0140 0141 0142 0143 0144 0145 0146
-    // 0058 0059 005A 005B 005C 005D 005E 005F 0060 0061 0062 0063 0097 0098
+    // 0147 0148 0149 014A 014B 014C 014D 014E 014F 0150 0151 0152 0153 0154 0155 0156 0157 0158 0159 015A 015B 015C
-    // 0099 009A 012D 012E 012F 0130 0131 0132 0133 0134 0135 0140 0141 0142
+    // 015D 015E 015F 0160 0161 0162 0163 0164 0165 0166 0167 0168 0169 016A 016B 016C 016D 016E 016F 0170 0171 0172
-    // 0143 0144 0145 0146 0147 0148 0149 014A 014B 014C 014D 014E 014F 0150
+    // 0173 0174 0175 0176 0177 0178 0179 017A 017B 017C 017D 017E 017F 0180 0181 0182 0183 0184 0185 0186 0191 0192
-    // 0151 0152 0153 0154 0155 0156 0157 0158 0159 015A 015B 015C 015D 015E
+    // 0193 0194 0195 0196 0197 0198 0199 019A 019B 019C 019D 019E 01A4 01A5 01A6 01A7 01A8 01A9 01AA 01AB 01AC 01AD
-    // 015F 0160 0161 0162 0163 0164 0165 0166 0167 0168 0169 016A 016B 016C
+    // 01AE 01AF 01B0 01B1 01B2 01B3 01B4 01B5 01B6 01B7 01B8 01C2 01C3 01C4 01C5 01C6 01C7 01C8 01C9 01CA 01CB 01CC
-    // 016D 016E 016F 0170 0171 0172 0173 0174 0175 0176 0177 0178 0179 017A
+    // 01CD 01CE 01CF 01D0 01D1 01D2 01D3 01D4 01D5 01D6 01F4 01F5 01F6 01F7 01F8 01F9 01FA 01FB 01FC 01FD 01FE 01FF
-    // 017B 017C 017D 017E 017F 0180 0181 0182 0183 0184 0185 0186 0191 0192
+    // 0200 0201 0202 0203 0204 0205 0206 0207 0208 0209 020A 020B 020C 020D 020E 020F 0210 0211 0212 0213 0214 0215
-    // 0193 0194 0195 0196 0197 0198 0199 019A 019B 019C 019D 019E 01A4 01A5
+    // 0216 0217 0218 0219 021A 021B 021C 021D 021E 021F 0220 0221 0222 0223 0224 0225 0226 0227 0228 0229 022A 022B
-    // 01A6 01A7 01A8 01A9 01AA 01AB 01AC 01AD 01AE 01AF 01B0 01B1 01B2 01B3
+    // 022C 022D 022E 022F 0230 0231 0232 0233 0234 0235 02BD 02BE 02BF 02C0 02C1 02C2 02C3 02C4
-    // 01B4 01B5 01B6 01B7 01B8 01C2 01C3 01C4 01C5 01C6 01C7 01C8 01C9 01CA
-    // 01CB 01CC 01CD 01CE 01CF 01D0 01D1 01D2 01D3 01D4 01D5 01D6 01F4 01F5
-    // 01F6 01F7 01F8 01F9 01FA 01FB 01FC 01FD 01FE 01FF 0200 0201 0202 0203
-    // 0204 0205 0206 0207 0208 0209 020A 020B 020C 020D 020E 020F 0210 0211
-    // 0212 0213 0214 0215 0216 0217 0218 0219 021A 021B 021C 021D 021E 021F
-    // 0220 0221 0222 0223 0224 0225 0226 0227 0228 0229 022A 022B 022C 022D
-    // 022E 022F 0230 0231 0232 0233 0234 0235 02BD 02BE 02BF 02C0 02C1 02C2
-    // 02C3 02C4
 }};
 
 BattleRules::BattleRules(const phosg::JSON& json) {

src/PlayerSubordinates.hh

@@ -32,10 +32,9 @@ struct PlayerVisualConfigT {
   /* 10 */ parray<uint8_t, 8> unknown_a2;
   /* 18 */ U32T<BE> name_color = 0xFFFFFFFF; // ARGB
   /* 1C */ uint8_t extra_model = 0;
-  // Some NPCs can crash the client if the character's class is incorrect. To
+  // Some NPCs can crash the client if the character's class is incorrect. To handle this, we save the affected fields
-  // handle this, we save the affected fields in the unused bytes after
+  // in the unused bytes after extra_model. This is a newserv-specific extension; it appears the following 15 bytes
-  // extra_model. This is a newserv-specific extension; it appears the
+  // were simply never used by Sega.
-  // following 15 bytes were simply never used by Sega.
   /* 1D */ uint8_t npc_saved_data_type = 0;
   /* 1E */ uint8_t npc_saved_costume = 0;
   /* 1F */ uint8_t npc_saved_skin = 0;
@@ -47,16 +46,13 @@ struct PlayerVisualConfigT {
   /* 25 */ uint8_t npc_saved_hair_b = 0;
   /* 26 */ parray<uint8_t, 2> unused;
   /* 28 */ F32T<BE> npc_saved_proportion_y = 0.0;
-  // See compute_name_color_checksum for details on how this is computed. If the
+  // See compute_name_color_checksum for details on how this is computed. If the value is incorrect, V1 and V2 will
-  // value is incorrect, V1 and V2 will ignore the name_color field and use the
+  // ignore the name_color field and use the default color instead. This field is ignored on GC; on BB (and presumably
-  // default color instead. This field is ignored on GC; on BB (and presumably
+  // Xbox), if this has a nonzero value, the "Change Name" option appears in the character selection menu.
-  // Xbox), if this has a nonzero value, the "Change Name" option appears in the
-  // character selection menu.
   /* 2C */ U32T<BE> name_color_checksum = 0;
   /* 30 */ uint8_t section_id = 0;
   /* 31 */ uint8_t char_class = 0;
-  // validation_flags specifies that some parts of this structure are not valid
+  // validation_flags specifies that some parts of this structure are not valid and should be ignored. The bits are:
-  // and should be ignored. The bits are:
   //   -----FCS
   //   F = class_flags is incorrect for the character's char_class value
   //   C = char_class is out of range
@@ -338,8 +334,8 @@ check_struct_size(PlayerDispDataDCPCV3BE, 0xD0);
 struct PlayerDispDataBBPreview {
   /* 00 */ le_uint32_t experience = 0;
   /* 04 */ le_uint32_t level = 0;
-  // The name field in this structure is used for the player's Guild Card
+  // The name field in this structure is used for the player's Guild Card number, apparently (possibly because it's a
-  // number, apparently (possibly because it's a char array and this is BB)
+  // char array and this is BB)
   /* 08 */ PlayerVisualConfig visual;
   /* 58 */ pstring<TextEncoding::UTF16_ALWAYS_MARKED, 0x10> name;
   /* 78 */ uint32_t play_time_seconds = 0;
@@ -436,18 +432,6 @@ struct GuildCardPC {
   operator GuildCardBB() const;
 } __packed_ws__(GuildCardPC, 0xF0);
 
-// 0000 | 62 00 AC 00 06 2A 00 00 00 00 01 00 90 96 66 8C | b    *        f
-// 0010 | 31 31 31 31 00 00 00 00 00 00 00 00 00 00 00 00 | 1111
-// 0020 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0030 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0040 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0050 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0060 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0070 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0080 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 0090 | 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |
-// 00A0 | 00 00 00 00 00 00 00 00 01 00 06 00             |
-
 template <bool BE, size_t DescriptionLength>
 struct GuildCardGCT {
   /* NTE:Final */
@@ -536,11 +520,9 @@ GuildCardGCT<BE, DescriptionLength>::operator GuildCardBB() const {
 struct PlayerLobbyDataPC {
   le_uint32_t player_tag = 0;
   le_uint32_t guild_card_number = 0;
-  // There's a strange behavior (bug? "feature"?) in Episode 3 where the start
+  // There's a strange behavior (bug? "feature"?) in Episode 3 where the start button does nothing in the lobby (hence
-  // button does nothing in the lobby (hence you can't "quit game") if the
+  // you can't "quit game") if the client's IP address is zero. So, we fill it in with a fake nonzero value to avoid
-  // client's IP address is zero. So, we fill it in with a fake nonzero value to
+  // this behavior, and to be consistent, we make IP addresses fake and nonzero on all other versions too.
-  // avoid this behavior, and to be consistent, we make IP addresses fake and
-  // nonzero on all other versions too.
   be_uint32_t ip_address = 0x7F000001;
   le_uint32_t client_id = 0;
   pstring<TextEncoding::UTF16, 0x10> name;
@@ -563,8 +545,7 @@ struct XBNetworkLocation {
   /* 04 */ le_uint32_t external_ipv4_address = 0x23232323;
   /* 08 */ le_uint16_t port = 9500;
   /* 0A */ parray<uint8_t, 6> mac_address = 0x77;
-  // The remainder of this struct appears to be private/opaque in the XDK (and
+  // The remainder of this struct appears to be private/opaque in the XDK (and newserv doesn't use it either)
-  // newserv doesn't use it either)
   /* 10 */ le_uint32_t sg_ip_address = 0x0B0B0B0B;
   /* 14 */ le_uint32_t spi = 0xCCCCCCCC;
   /* 18 */ le_uint64_t account_id = 0xFFFFFFFFFFFFFFFF;
@@ -593,8 +574,7 @@ struct PlayerLobbyDataBB {
   /* 10 */ parray<uint8_t, 0x0C> unknown_a1;
   /* 1C */ le_uint32_t client_id = 0;
   /* 20 */ pstring<TextEncoding::UTF16_ALWAYS_MARKED, 0x10> name;
-  // If this field is zero, the "Press F1 for help" prompt appears in the corner
+  // If this is zero, the "Press F1 for help" prompt appears in the corner of the screen in the lobby and Pioneer 2.
-  // of the screen in the lobby and on Pioneer 2.
   /* 40 */ le_uint32_t hide_help_prompt = 1;
   /* 44 */
 
@@ -653,8 +633,7 @@ check_struct_size(PlayerRecordsChallengePC, 0xD8);
 
 template <bool BE>
 struct PlayerRecordsChallengeV3T {
-  // Offsets are (1) relative to start of C5 entry, and (2) relative to start
+  // Offsets are (1) relative to start of C5 entry, and (2) relative to start of save file structure
-  // of save file structure
   /* 0000:001C */ U16T<BE> title_color = 0x7FFF; // XRGB1555
   /* 0002:001E */ parray<uint8_t, 2> unknown_u0;
   /* 0004:0020 */ parray<ChallengeTimeT<BE>, 9> times_ep1_online;
@@ -679,9 +658,8 @@ struct PlayerRecordsChallengeV3T {
   /* 00C8:00E4 */ ChallengeAwardStateT<BE> ep2_online_award_state;
   /* 00D0:00EC */ ChallengeAwardStateT<BE> ep1_offline_award_state;
   /* 00D8:00F4 */
-  // On Episode 3, there are special cases that apply to this field - if the
+  // On Episode 3, there are special cases that apply to this field - if the text ends with certain strings, the player
-  // text ends with certain strings, the player will have particle effects
+  // will have particle effects emanate from their character in the lobby every 2 seconds. The effects are:
-  // emanate from their character in the lobby every 2 seconds. The effects are:
   //   Ends with ":GOD" => blue circle
   //   Ends with ":KING" => white particles
   //   Ends with ":LORD" => rising yellow sparkles
@@ -862,7 +840,8 @@ DestT convert_player_disp_data(const SrcT&, Language, Language) {
 }
 
 template <>
-inline PlayerDispDataDCPCV3 convert_player_disp_data<PlayerDispDataDCPCV3>(const PlayerDispDataDCPCV3& src, Language, Language) {
+inline PlayerDispDataDCPCV3 convert_player_disp_data<PlayerDispDataDCPCV3>(
+    const PlayerDispDataDCPCV3& src, Language, Language) {
   return src;
 }
 
@@ -879,8 +858,7 @@ inline PlayerDispDataBB convert_player_disp_data<PlayerDispDataBB, PlayerDispDat
 }
 
 template <>
-inline PlayerDispDataBB convert_player_disp_data<PlayerDispDataBB>(
+inline PlayerDispDataBB convert_player_disp_data<PlayerDispDataBB>(const PlayerDispDataBB& src, Language, Language) {
-    const PlayerDispDataBB& src, Language, Language) {
   return src;
 }
 
@@ -1117,10 +1095,7 @@ struct SymbolChatT {
   /* 0C */ parray<SymbolChatFacePart, 12> face_parts;
   /* 3C */
 
-  SymbolChatT()
+  SymbolChatT() : spec(0), corner_objects(0x00FF), face_parts() {}
-      : spec(0),
-        corner_objects(0x00FF),
-        face_parts() {}
 
   operator SymbolChatT<!BE>() const {
     SymbolChatT<!BE> ret;