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

Proposed by GunChleoc on 2016-12-06
Status: Merged
Merged at revision: 8210
Proposed branch: lp:~widelands-dev/widelands/bug-1647033-immovable-packet-no
Merge into: lp:widelands
Diff against target: 50 lines (+15/-4)
1 file modified
src/logic/map_objects/immovable.cc (+15/-4)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1647033-immovable-packet-no
Reviewer Review Type Date Requested Status
toptopple (community) 2016-12-06 Approve on 2016-12-08
Review via email: mp+312530@code.launchpad.net

Commit message

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.

To post a comment you must log in.
bunnybot (widelandsofficial) wrote :

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.

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/map_objects/immovable.cc'
2--- src/logic/map_objects/immovable.cc 2016-11-22 19:54:02 +0000
3+++ src/logic/map_objects/immovable.cc 2016-12-06 07:46:30 +0000
4@@ -532,6 +532,11 @@
5 ==============================
6 */
7
8+// We neeed 2 packet versions for map loading: Packet version 7 will load in older versions of
9+// Widelands, so we have a dynamic version number - it is only set higher than
10+// kCurrentPacketVersionImmovableNoFormerBuildings during saving if we have an immovable with
11+// a former building assigned to it.
12+constexpr uint8_t kCurrentPacketVersionImmovableNoFormerBuildings = 7;
13 constexpr uint8_t kCurrentPacketVersionImmovable = 8;
14
15 // Supporting older versions for map loading
16@@ -554,7 +559,7 @@
17 imm.position_ = read_coords_32(&fr, egbase().map().extent());
18 imm.set_position(egbase(), imm.position_);
19
20- if (packet_version >= 8) {
21+ if (packet_version > kCurrentPacketVersionImmovableNoFormerBuildings) {
22 Player* owner = imm.get_owner();
23 if (owner) {
24 DescriptionIndex idx = owner->tribe().building_index(fr.string());
25@@ -645,7 +650,10 @@
26 // This is in front because it is required to obtain the description
27 // necessary to create the Immovable
28 fw.unsigned_8(HeaderImmovable);
29- fw.unsigned_8(kCurrentPacketVersionImmovable);
30+ const uint8_t packet_version = former_building_descr_ == nullptr ?
31+ kCurrentPacketVersionImmovableNoFormerBuildings :
32+ kCurrentPacketVersionImmovable;
33+ fw.unsigned_8(packet_version);
34
35 if (descr().owner_type() == MapObjectDescr::OwnerType::kTribe) {
36 if (get_owner() == nullptr)
37@@ -662,8 +670,11 @@
38
39 fw.unsigned_8(get_owner() ? get_owner()->player_number() : 0);
40 write_coords_32(&fw, position_);
41- if (get_owner()) {
42- fw.string(former_building_descr_ ? former_building_descr_->name() : "");
43+ if (get_owner() && former_building_descr_) {
44+ assert(packet_version > kCurrentPacketVersionImmovableNoFormerBuildings);
45+ fw.string(former_building_descr_->name());
46+ } else {
47+ assert(packet_version == kCurrentPacketVersionImmovableNoFormerBuildings);
48 }
49
50 // Animations