widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

SirVer has proposed merging lp:~widelands-dev/widelands/economy_save_load_debug into lp:widelands.

Commit message:
Small refactoring/simplification and more debug information for GamePlayerEconomiesPacket.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1654897 in widelands: "corrupted saved game: "string ended unexpectedly""
  https://bugs.launchpad.net/widelands/+bug/1654897

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/economy_save_load_debug/+merge/314453
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/economy_save_load_debug into lp:widelands.

=== modified file 'src/game_io/game_player_economies_packet.cc'
--- src/game_io/game_player_economies_packet.cc	2016-08-04 15:49:05 +0000
+++ src/game_io/game_player_economies_packet.cc	2017-01-10 18:04:24 +0000
@@ -20,6 +20,7 @@
 #include "game_io/game_player_economies_packet.h"
 
 #include "base/macros.h"
+#include "economy/economy.h"
 #include "economy/economy_data_packet.h"
 #include "economy/flag.h"
 #include "io/fileread.h"
@@ -31,8 +32,31 @@
 #include "logic/widelands_geometry_io.h"
 
 namespace Widelands {
-
-constexpr uint16_t kCurrentPacketVersion = 3;
+namespace {
+
+constexpr uint16_t kCurrentPacketVersion = 4;
+
+bool write_expedition_ship_economy(Economy* economy, const Map& map, FileWrite* fw) {
+	for (Field const* field = &map[0]; field < &map[map.max_index()]; ++field) {
+		Bob* bob = field->get_first_bob();
+		while (bob) {
+			if (upcast(Ship const, ship, bob)) {
+				if (ship->get_economy() == economy) {
+					// TODO(sirver): the 0xffffffff is ugly and fragile.
+					fw->unsigned_32(0xffffffff);  // Sentinel value.
+					fw->unsigned_32(field - &map[0]);
+					EconomyDataPacket d(economy);
+					d.write(*fw);
+					return true;
+				}
+			}
+			bob = bob->get_next_bob();
+		}
+	}
+	return false;
+}
+
+}  // namespace
 
 void GamePlayerEconomiesPacket::read(FileSystem& fs, Game& game, MapObjectLoader*) {
 	try {
@@ -43,13 +67,27 @@
 		FileRead fr;
 		fr.open(fs, "binary/player_economies");
 		uint16_t const packet_version = fr.unsigned_16();
-		if (packet_version == kCurrentPacketVersion) {
+		if (packet_version == 3 || packet_version == kCurrentPacketVersion) {
 			iterate_players_existing(p, nr_players, game, player) try {
+				// In packet_version 4 we dump the number of economies a player had at
+				// save time to debug
+				// https://bugs.launchpad.net/widelands/+bug/1654897 which is likely
+				// caused by players having more economies at load than they had at
+				// save.
 				Player::Economies& economies = player->economies_;
+				if (packet_version > 3) {
+					const size_t num_economies = fr.unsigned_16();
+					if (num_economies != economies.size()) {
+						throw GameDataError("Num economies on save (%ld) != Num economies on load (%ld)",
+						                    num_economies, economies.size());
+					}
+				}
+
 				for (uint32_t i = 0; i < economies.size(); ++i) {
 					uint32_t value = fr.unsigned_32();
 					if (value < 0xffffffff) {
 						if (upcast(Flag const, flag, map[value].get_immovable())) {
+							assert(flag->get_economy()->owner().player_number() == player->player_number());
 							EconomyDataPacket d(flag->get_economy());
 							d.read(fr);
 						} else {
@@ -57,14 +95,13 @@
 						}
 					} else {
 						bool read_this_economy = false;
-
 						Bob* bob = map[read_map_index_32(&fr, max_index)].get_first_bob();
 						while (bob) {
 							if (upcast(Ship const, ship, bob)) {
-
 								// We are interested only in current player's ships
 								if (ship->get_owner() == player) {
 									assert(ship->get_economy());
+									assert(ship->get_economy()->owner().player_number() == player->player_number());
 									EconomyDataPacket d(ship->get_economy());
 									d.read(fr);
 									read_this_economy = true;
@@ -95,57 +132,28 @@
  */
 void GamePlayerEconomiesPacket::write(FileSystem& fs, Game& game, MapObjectSaver* const) {
 	FileWrite fw;
-
 	fw.unsigned_16(kCurrentPacketVersion);
 
 	const Map& map = game.map();
-	const Field& field_0 = map[0];
 	PlayerNumber const nr_players = map.get_nrplayers();
 	iterate_players_existing_const(p, nr_players, game, player) {
 		const Player::Economies& economies = player->economies_;
-		for (Economy* temp_economy : economies) {
-			bool wrote_this_economy = false;
-
-			// Walk the map so that we find a representative Flag.
-			for (Field const* field = &field_0; field < &map[map.max_index()]; ++field) {
-				if (upcast(Flag const, flag, field->get_immovable())) {
-					if (flag->get_economy() == temp_economy) {
-						fw.unsigned_32(field - &field_0);
-
-						EconomyDataPacket d(flag->get_economy());
-						d.write(fw);
-						wrote_this_economy = true;
-						break;
-					}
-				}
-			}
-			if (wrote_this_economy)
+		fw.unsigned_16(economies.size());
+		for (Economy* economy : economies) {
+			Flag* arbitrary_flag = economy->get_arbitrary_flag();
+			if (arbitrary_flag != nullptr) {
+				fw.unsigned_32(map.get_fcoords(arbitrary_flag->get_position()).field - &map[0]);
+				EconomyDataPacket d(economy);
+				d.write(fw);
 				continue;
+			}
 
 			// No flag found, let's look for a representative Ship. Expeditions
 			// ships are special and have their own economy (which will not have a
 			// flag), therefore we have to special case them.
-			for (Field const* field = &field_0; field < &map[map.max_index()]; ++field) {
-				Bob* bob = field->get_first_bob();
-				while (bob) {
-					if (upcast(Ship const, ship, bob)) {
-						if (ship->get_economy() == temp_economy) {
-							// TODO(sirver): the 0xffffffff is ugly and fragile.
-							fw.unsigned_32(0xffffffff);  // Sentinel value.
-							fw.unsigned_32(field - &field_0);
-							EconomyDataPacket d(ship->get_economy());
-							d.write(fw);
-							wrote_this_economy = true;
-							break;
-						}
-					}
-					bob = bob->get_next_bob();
-				}
+			if (!write_expedition_ship_economy(economy, map, &fw)) {
+				throw GameDataError("economy without representative");
 			}
-
-			// If we have not written this economy, it has no ship and no flag
-			// associated. It should not exist.
-			assert(wrote_this_economy);
 		}
 	}