Merge lp:~widelands-dev/widelands/bug-1647033-immovable-packet-no into lp:widelands

[GunChleoc]

Immovables now have a dynamic packet number, so maps will still load with Build 19: The packet number is only increased if the immovable has a former building.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1719. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/181581877.
Appveyor build 1559. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1647033_immovable_packet_no-1559.

[toptopple (7010622-q-deactivatedaccount)]

I could create a stable version of Gold Rush (online as version 1.2) with this branch and it loads into B19. Noteworthy, the save games of later versions still report the same error when loaded in B19. Not necessarily a problem. Savegames probably will grow incompatible anyway soon, so it's not worth fixing this part.

review: Approve

[GunChleoc (gunchleoc)]

Thanks for testing! Savegames as a rule aren't backwards compatible, only forwards compatible. The important thing is that maps can be loaded with older versions :)

@bunnybot merge

[SirVer (sirver)]

All other gold rush maps have been deleted, so the bug should not have any more impact.

=== modified file 'src/logic/map_objects/immovable.cc'
--- src/logic/map_objects/immovable.cc	2016-11-22 19:54:02 +0000
+++ src/logic/map_objects/immovable.cc	2016-12-06 07:46:30 +0000
@@ -532,6 +532,11 @@
 ==============================
 */
 
+// We neeed 2 packet versions for map loading: Packet version 7 will load in older versions of
+// Widelands, so we have a dynamic version number - it is only set higher than
+// kCurrentPacketVersionImmovableNoFormerBuildings during saving if we have an immovable with
+// a former building assigned to it.
+constexpr uint8_t kCurrentPacketVersionImmovableNoFormerBuildings = 7;
 constexpr uint8_t kCurrentPacketVersionImmovable = 8;
 
 // Supporting older versions for map loading
@@ -554,7 +559,7 @@
 	imm.position_ = read_coords_32(&fr, egbase().map().extent());
 	imm.set_position(egbase(), imm.position_);
 
-	if (packet_version >= 8) {
+	if (packet_version > kCurrentPacketVersionImmovableNoFormerBuildings) {
 		Player* owner = imm.get_owner();
 		if (owner) {
 			DescriptionIndex idx = owner->tribe().building_index(fr.string());
@@ -645,7 +650,10 @@
 	// This is in front because it is required to obtain the description
 	// necessary to create the Immovable
 	fw.unsigned_8(HeaderImmovable);
-	fw.unsigned_8(kCurrentPacketVersionImmovable);
+	const uint8_t packet_version = former_building_descr_ == nullptr ?
+	                                  kCurrentPacketVersionImmovableNoFormerBuildings :
+	                                  kCurrentPacketVersionImmovable;
+	fw.unsigned_8(packet_version);
 
 	if (descr().owner_type() == MapObjectDescr::OwnerType::kTribe) {
 		if (get_owner() == nullptr)
@@ -662,8 +670,11 @@
 
 	fw.unsigned_8(get_owner() ? get_owner()->player_number() : 0);
 	write_coords_32(&fw, position_);
-	if (get_owner()) {
-		fw.string(former_building_descr_ ? former_building_descr_->name() : "");
+	if (get_owner() && former_building_descr_) {
+		assert(packet_version > kCurrentPacketVersionImmovableNoFormerBuildings);
+		fw.string(former_building_descr_->name());
+	} else {
+		assert(packet_version == kCurrentPacketVersionImmovableNoFormerBuildings);
 	}
 
 	// Animations