widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Klaus Halfmann has proposed merging lp:~widelands-dev/widelands/bug_1725724_ai_data into lp:widelands.

Requested reviews:
  TiborB (tiborb95)
Related bugs:
  Bug #1725724 in widelands: "assert ai_data.remaining_buildings_size in bzr8464[trunk]"
  https://bugs.launchpad.net/widelands/+bug/1725724

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug_1725724_ai_data/+merge/333681

Refactored game_player_ai_persistent_packet for better readbility and type safety.
This fiexe Bug 1725724 for me.

My only Problem is, I do not know _why_ this is fixed now :-).

I will go back to trunk and reproduce this with the debugger.
-- 
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug_1725724_ai_data.

=== modified file 'src/game_io/game_player_ai_persistent_packet.cc'
--- src/game_io/game_player_ai_persistent_packet.cc	2017-08-20 08:34:02 +0000
+++ src/game_io/game_player_ai_persistent_packet.cc	2017-11-14 14:21:26 +0000
@@ -29,6 +29,74 @@
 namespace Widelands {
 
 constexpr uint16_t kCurrentPacketVersion = 3;
+constexpr uint16_t kPacketVersion2 = 2; // TODO(TiborB): what exactly does this version stand for -> please rename
+
+/**
+ * skip and ignore old data, we will just read other variables
+ */
+static void skipVersion2(FileRead& fr) {
+    fr.unsigned_8();
+    fr.unsigned_32();
+    fr.unsigned_32();
+    fr.unsigned_32();
+    fr.unsigned_16();
+    fr.unsigned_8();
+    fr.signed_16();
+    fr.unsigned_32();
+    fr.unsigned_32();
+    fr.signed_16();
+    fr.signed_32();
+    fr.unsigned_32();
+    fr.signed_32();
+    fr.unsigned_32();
+    fr.unsigned_32();
+}
+
+    static void readCurrentVersion(FileRead& fr, Player::AiPersistentState& ai_data) {
+    ai_data.initialized                    = fr.unsigned_8();
+    ai_data.colony_scan_area               = fr.unsigned_32();
+    ai_data.trees_around_cutters           = fr.unsigned_32();
+    ai_data.expedition_start_time          = fr.unsigned_32();
+    ai_data.ships_utilization              = fr.unsigned_16();
+    ai_data.no_more_expeditions            = fr.unsigned_8();
+    ai_data.last_attacked_player           = fr.signed_16();
+    ai_data.least_military_score           = fr.unsigned_32();
+    ai_data.target_military_score          = fr.unsigned_32();
+    ai_data.ai_productionsites_ratio       = fr.unsigned_32();
+    ai_data.ai_personality_mil_upper_limit = fr.signed_32();
+    // Magic numbers
+    ai_data.magic_numbers_size = fr.unsigned_32();
+    for (uint16_t i = 0; i < ai_data.magic_numbers_size; ++i) {
+        ai_data.magic_numbers.push_back(fr.signed_16());
+    }
+    assert(ai_data.magic_numbers_size == ai_data.magic_numbers.size());
+    // Neurons
+    ai_data.neuron_pool_size = fr.unsigned_32();
+    for (uint16_t i = 0; i < ai_data.neuron_pool_size; ++i) {
+        ai_data.neuron_weights.push_back(fr.signed_8());
+    }
+    for (uint16_t i = 0; i < ai_data.neuron_pool_size; ++i) {
+        ai_data.neuron_functs.push_back(fr.signed_8());
+    }
+    assert(ai_data.neuron_pool_size == ai_data.neuron_weights.size());
+    assert(ai_data.neuron_pool_size == ai_data.neuron_functs.size());
+
+    // F-neurons
+    ai_data.f_neuron_pool_size = fr.unsigned_32();
+    for (uint16_t i = 0; i < ai_data.f_neuron_pool_size; ++i) {
+        ai_data.f_neurons.push_back(fr.unsigned_32());
+    }
+    assert(ai_data.f_neuron_pool_size == ai_data.f_neurons.size());
+
+    // remaining buildings for basic economy
+    ai_data.remaining_buildings_size = fr.unsigned_32();
+    for (uint16_t i = 0; i < ai_data.remaining_buildings_size; ++i) {
+        ai_data.remaining_basic_buildings.emplace(static_cast<Widelands::DescriptionIndex>(fr.unsigned_32()), fr.unsigned_32());
+    }
+    assert(ai_data.remaining_buildings_size ==
+           ai_data.remaining_basic_buildings.size());
+
+}
 
 void GamePlayerAiPersistentPacket::read(FileSystem& fs, Game& game, MapObjectLoader*) {
 	try {
@@ -38,71 +106,16 @@
 		fr.open(fs, "binary/player_ai");
 		uint16_t const packet_version = fr.unsigned_16();
 		// TODO(GunChleoc): Savegame compatibility, remove after Build20
-		if (packet_version >= 2 && packet_version <= kCurrentPacketVersion) {
+		if (packet_version >= kPacketVersion2 && packet_version <= kCurrentPacketVersion) {
 			iterate_players_existing(p, nr_players, game, player) try {
-				if (packet_version == 2) {
-
-					// zero here says the AI has not been initialized
-					player->ai_data.initialized = 0;
-					// we will just read other variables
-					fr.unsigned_8();
-					fr.unsigned_32();
-					fr.unsigned_32();
-					fr.unsigned_32();
-					fr.unsigned_16();
-					fr.unsigned_8();
-					fr.signed_16();
-					fr.unsigned_32();
-					fr.unsigned_32();
-					fr.signed_16();
-					fr.signed_32();
-					fr.unsigned_32();
-					fr.signed_32();
-					fr.unsigned_32();
-					fr.unsigned_32();
-				} else {
-					player->ai_data.initialized = fr.unsigned_8();
-					player->ai_data.colony_scan_area = fr.unsigned_32();
-					player->ai_data.trees_around_cutters = fr.unsigned_32();
-					player->ai_data.expedition_start_time = fr.unsigned_32();
-					player->ai_data.ships_utilization = fr.unsigned_16();
-					player->ai_data.no_more_expeditions = fr.unsigned_8();
-					player->ai_data.last_attacked_player = fr.signed_16();
-					player->ai_data.least_military_score = fr.unsigned_32();
-					player->ai_data.target_military_score = fr.unsigned_32();
-					player->ai_data.ai_productionsites_ratio = fr.unsigned_32();
-					player->ai_data.ai_personality_mil_upper_limit = fr.signed_32();
-					// Magic numbers
-					player->ai_data.magic_numbers_size = fr.unsigned_32();
-					for (uint16_t i = 0; i < player->ai_data.magic_numbers_size; ++i) {
-						player->ai_data.magic_numbers.push_back(fr.signed_16());
-					}
-					assert(player->ai_data.magic_numbers_size == player->ai_data.magic_numbers.size());
-					// Neurons
-					player->ai_data.neuron_pool_size = fr.unsigned_32();
-					for (uint16_t i = 0; i < player->ai_data.neuron_pool_size; ++i) {
-						player->ai_data.neuron_weights.push_back(fr.signed_8());
-					}
-					for (uint16_t i = 0; i < player->ai_data.neuron_pool_size; ++i) {
-						player->ai_data.neuron_functs.push_back(fr.signed_8());
-					}
-					assert(player->ai_data.neuron_pool_size == player->ai_data.neuron_weights.size());
-					assert(player->ai_data.neuron_pool_size == player->ai_data.neuron_functs.size());
-
-					// F-neurons
-					player->ai_data.f_neuron_pool_size = fr.unsigned_32();
-					for (uint16_t i = 0; i < player->ai_data.f_neuron_pool_size; ++i) {
-						player->ai_data.f_neurons.push_back(fr.unsigned_32());
-					}
-					assert(player->ai_data.f_neuron_pool_size == player->ai_data.f_neurons.size());
-
-					// remaining buildings for basic economy
-					player->ai_data.remaining_buildings_size = fr.unsigned_32();
-					for (uint16_t i = 0; i < player->ai_data.remaining_buildings_size; ++i) {
-						player->ai_data.remaining_basic_buildings[fr.unsigned_32()] = fr.unsigned_32();
-					}
-					assert(player->ai_data.remaining_buildings_size ==
-					       player->ai_data.remaining_basic_buildings.size());
+				if (packet_version == kPacketVersion2) {
+
+					player->ai_data.initialized = 0; // AI has not been initialized
+                    skipVersion2(fr);
+                }
+                else // kCurrentPacketVersion
+                {
+                    readCurrentVersion(fr, player->ai_data);
 				}
 			} catch (const WException& e) {
 				throw GameDataError("player %u: %s", p, e.what());
@@ -116,6 +129,54 @@
 	}
 }
 
+static void writeCurrentVersion(FileWrite& fw, const Player::AiPersistentState& ai_data) {
+    fw.unsigned_8(ai_data.initialized);
+    fw.unsigned_32(ai_data.colony_scan_area);
+    fw.unsigned_32(ai_data.trees_around_cutters);
+    fw.unsigned_32(ai_data.expedition_start_time);
+    fw.unsigned_16(ai_data.ships_utilization);
+    fw.unsigned_8(ai_data.no_more_expeditions);
+    fw.signed_16(ai_data.last_attacked_player);
+    fw.unsigned_32(ai_data.least_military_score);
+    fw.unsigned_32(ai_data.target_military_score);
+    fw.unsigned_32(ai_data.ai_productionsites_ratio);
+    fw.signed_32(ai_data.ai_personality_mil_upper_limit);
+
+    // Magic numbers
+    fw.unsigned_32(ai_data.magic_numbers_size);
+    assert(ai_data.magic_numbers_size == ai_data.magic_numbers.size());
+    for (uint16_t i = 0; i < ai_data.magic_numbers_size; ++i) {
+        fw.signed_16(ai_data.magic_numbers[i]);
+    }
+    // Neurons
+    fw.unsigned_32(ai_data.neuron_pool_size);
+    assert(ai_data.neuron_pool_size == ai_data.neuron_weights.size());
+    assert(ai_data.neuron_pool_size == ai_data.neuron_functs.size());
+    for (uint16_t i = 0; i < ai_data.neuron_pool_size; ++i) {
+        fw.signed_8(ai_data.neuron_weights[i]);
+    }
+    for (uint16_t i = 0; i < ai_data.neuron_pool_size; ++i) {
+        fw.signed_8(ai_data.neuron_functs[i]);
+    }
+
+    // F-Neurons
+    fw.unsigned_32(ai_data.f_neuron_pool_size);
+    assert(ai_data.f_neuron_pool_size == ai_data.f_neurons.size());
+
+    for (uint16_t i = 0; i < ai_data.f_neuron_pool_size; ++i) {
+        fw.unsigned_32(ai_data.f_neurons[i]);
+    }
+
+    // Remaining buildings for basic economy
+    assert(ai_data.remaining_buildings_size ==
+           ai_data.remaining_basic_buildings.size());
+    fw.unsigned_32(ai_data.remaining_buildings_size);
+    for (auto bb : ai_data.remaining_basic_buildings) {
+        fw.unsigned_32(bb.first);
+        fw.unsigned_32(bb.second);
+    }
+}
+
 /*
  * Write Function
  */
@@ -126,51 +187,7 @@
 
 	PlayerNumber const nr_players = game.map().get_nrplayers();
 	iterate_players_existing_const(p, nr_players, game, player) {
-		fw.unsigned_8(player->ai_data.initialized);
-		fw.unsigned_32(player->ai_data.colony_scan_area);
-		fw.unsigned_32(player->ai_data.trees_around_cutters);
-		fw.unsigned_32(player->ai_data.expedition_start_time);
-		fw.unsigned_16(player->ai_data.ships_utilization);
-		fw.unsigned_8(player->ai_data.no_more_expeditions);
-		fw.signed_16(player->ai_data.last_attacked_player);
-		fw.unsigned_32(player->ai_data.least_military_score);
-		fw.unsigned_32(player->ai_data.target_military_score);
-		fw.unsigned_32(player->ai_data.ai_productionsites_ratio);
-		fw.signed_32(player->ai_data.ai_personality_mil_upper_limit);
-
-		// Magic numbers
-		fw.unsigned_32(player->ai_data.magic_numbers_size);
-		assert(player->ai_data.magic_numbers_size == player->ai_data.magic_numbers.size());
-		for (uint16_t i = 0; i < player->ai_data.magic_numbers_size; ++i) {
-			fw.signed_16(player->ai_data.magic_numbers[i]);
-		}
-		// Neurons
-		fw.unsigned_32(player->ai_data.neuron_pool_size);
-		assert(player->ai_data.neuron_pool_size == player->ai_data.neuron_weights.size());
-		assert(player->ai_data.neuron_pool_size == player->ai_data.neuron_functs.size());
-		for (uint16_t i = 0; i < player->ai_data.neuron_pool_size; ++i) {
-			fw.signed_8(player->ai_data.neuron_weights[i]);
-		}
-		for (uint16_t i = 0; i < player->ai_data.neuron_pool_size; ++i) {
-			fw.signed_8(player->ai_data.neuron_functs[i]);
-		}
-
-		// F-Neurons
-		fw.unsigned_32(player->ai_data.f_neuron_pool_size);
-		assert(player->ai_data.f_neuron_pool_size == player->ai_data.f_neurons.size());
-
-		for (uint16_t i = 0; i < player->ai_data.f_neuron_pool_size; ++i) {
-			fw.unsigned_32(player->ai_data.f_neurons[i]);
-		}
-
-		// Remaining buildings for basic economy
-		assert(player->ai_data.remaining_buildings_size ==
-		       player->ai_data.remaining_basic_buildings.size());
-		fw.unsigned_32(player->ai_data.remaining_buildings_size);
-		for (auto bb : player->ai_data.remaining_basic_buildings) {
-			fw.unsigned_32(bb.first);
-			fw.unsigned_32(bb.second);
-		}
+        writeCurrentVersion(fw, player->ai_data);
 	}
 
 	fw.write(fs, "binary/player_ai");

=== modified file 'src/game_io/game_player_ai_persistent_packet.h'
--- src/game_io/game_player_ai_persistent_packet.h	2017-01-25 18:55:59 +0000
+++ src/game_io/game_player_ai_persistent_packet.h	2017-11-14 14:21:26 +0000
@@ -24,8 +24,8 @@
 
 namespace Widelands {
 
-/*
- * stores data that are needed for AI
+/**
+ * Stores and loads data that are needed for AI.
  */
 struct GamePlayerAiPersistentPacket : public GameDataPacket {
 	void read(FileSystem&, Game&, MapObjectLoader* = nullptr) override;