Merge lp:~widelands-dev/widelands/elk_moose into lp:widelands

[GunChleoc]

Rename "elk" to "moose" and add map compatibility code

- Add support for critter renamings depending on packet version to WorldLegacyLookupTable
- New map version property "needs_widelands_version_after" for the website

[Klaus Halfmann (klaus-halfmann)]

Some questions inline Code otherwise LGTM.
Will commpile this and the read that code again.

[Klaus Halfmann (klaus-halfmann)]

I expect some followup change on the website then.
Widelands will always read older Maps.

Did another review with a look into the complete code.
(We need some Widelands historian for all this legacy :-)

A bit more documentation would help.

[Klaus Halfmann (klaus-halfmann)]

I openend some buitin and a new Map,
how can I see this new value in a map?

I am missing some info about wl_map_info / wl_map_object_info

[Klaus Halfmann (klaus-halfmann)]

OTOH the code is OK for me, anything more I can / must do?

review: Approve (review, compile, short test)

[GunChleoc (gunchleoc)]

I have added comments.

If you run ./wl_map_info without parameters, it will give you command line help. It results in a JSON file for the website.

[Klaus Halfmann (klaus-halfmann)]

Ahh, the json file is created in thee same dir as the wmf file, e.g.

{
   "name": "CrossriverA",
   "author": "Hasi50",
   "description": "A nice River flows through this land and water can be found there. Metal in the hills nearby, but you are not alone",
   "hint": "",
   "width": 64,
   "height": 64,
   "nr_players": 2,
   "needs_widelands_version_after": 18,
   "world_name": "",
   "minimap": "/Users/klaus/.widelands/maps/My_Maps/CrossriverA1.wmf.png"
}

A map saved with the new version

{
   "name": "27 zu 3 Inseln",
   "author": "Björn alias the-x",
   "description": "Starting with 27 Islands falling together when you find Ways to pass through the rocks to 3 huge Island. Are you unstoppable to manage all 3 Islands under your control? Tipps: Start to find your dream Island real fast and you wont find Iron on your main Island.",
   "hint": "",
   "width": 144,
   "height": 144,
   "nr_players": 4,
   "needs_widelands_version_after": 20,
   "world_name": "",
   "minimap": "/Users/klaus/.widelands/maps/My_Maps/27 zu 3 Inselnx.wmf.png"
}

Trunk gives me:

Exception: map objects: [../src/logic/map_objects/world/critter.cc:323] loading critter:

UnhandledVersionError: This game was saved using an older version of Widelands and cannot be loaded anymore, or it's a new version that can't be handled yet.

Packet Name: Critter
Saved Version: 2
Current Version: 1.

Fine

review: Approve (testing)

[GunChleoc (gunchleoc)]

Thanks for the review & testing!

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5218. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/549145830.
Appveyor build 4997. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_elk_moose-4997.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 5218. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/549145830.

[Klaus Halfmann (klaus-halfmann)]

Travis:

1 x curl: (6) Could not resolve host: deb.debian.org
Error: An exception occurred within a child process:
  DownloadError: Failed to download resource "isl"

3 x Loading savegame: inputqueues ...
No output has been received in the last 10m0s
-> Adressed elsewhere

All other Builds are fine

@bunnybot merge force

[bunnybot (widelandsofficial)]

Error merging this proposal:

Output:
stdout:

stderr:
Unable to obtain lock held by <email address hidden> on taotie (process #21580), acquired 37 hours, 41 minutes ago.
See "bzr help break-lock" for more.
bzr: ERROR: Could not acquire lock "(remote lock)": bzr+ssh://bazaar.launchpad.net/~widelands-dev/widelands/trunk/

[GunChleoc (gunchleoc)]

@bunnybot merge force

=== renamed directory 'data/world/critters/elk' => 'data/world/critters/moose'
=== modified file 'data/world/critters/moose/init.lua'
--- data/world/critters/elk/init.lua	2019-04-21 14:57:55 +0000
+++ data/world/critters/moose/init.lua	2019-06-22 16:21:53 +0000
@@ -15,8 +15,8 @@
 add_directional_animation(animations, "walk", dirname, "walk", {21, 34}, 20)
 
 world:new_critter_type{
-   name = "elk",
-   descname = _ "Elk",
+   name = "moose",
+   descname = _ "Moose",
    editor_category = "critters_herbivores",
    attributes = { "eatable" },
    programs = {
 
=== modified file 'data/world/init.lua'
--- data/world/init.lua	2017-09-03 10:56:44 +0000
+++ data/world/init.lua	2019-06-22 16:21:53 +0000
@@ -288,7 +288,7 @@
       include "world/critters/deer/init.lua"
       include "world/critters/reindeer/init.lua"
       include "world/critters/stag/init.lua"
-      include "world/critters/elk/init.lua"
+      include "world/critters/moose/init.lua"
 
       -- Carnivores
       world:new_editor_critter_category {
 
=== modified file 'data/world/map_generation.lua'
--- data/world/map_generation.lua	2017-02-12 09:10:57 +0000
+++ data/world/map_generation.lua	2019-06-22 16:21:53 +0000
@@ -77,17 +77,17 @@
             "alder_summer_old", "birch_summer_old", "beech_summer_old",
             "larch_summer_old", "rowan_summer_old"
          },
-         critters = { "brownbear", "chamois", "elk", "lynx", "reindeer", "stag", "wildboar", "wolf" },
+         critters = { "brownbear", "chamois", "moose", "lynx", "reindeer", "stag", "wildboar", "wolf" },
       },
       {
          name = "bushes",
          immovables = { "bush1", "bush2", "bush3", "bush4", "bush5" },
-         critters = { "bunny", "elk", "fox", "lynx", "marten", "sheep", "sheep", "wildboar", "wisent", "wolf", "wolf" },
+         critters = { "bunny", "moose", "fox", "lynx", "marten", "sheep", "sheep", "wildboar", "wisent", "wolf", "wolf" },
       },
       {
          name = "grasses",
          immovables = { "grass1", "grass2", "grass3" },
-         critters = { "bunny", "elk", "fox", "lynx", "marten", "sheep", "sheep", "wolf" },
+         critters = { "bunny", "moose", "fox", "lynx", "marten", "sheep", "sheep", "wolf" },
       },
       {
          name = "standing_stones",
@@ -614,13 +614,13 @@
             "birch_summer_old", "beech_summer_old",
             "larch_summer_old", "rowan_summer_old",
          },
-         critters = { "deer", "elk", "lynx", "reindeer", "wolf" },
+         critters = { "deer", "moose", "lynx", "reindeer", "wolf" },
       },
       {
          name = "bushes",
          immovables = { "bush1", "bush2", "bush3", "bush4", "bush5" },
          critters = {
-            "wisent", "wolf", "bunny", "elk", "fox", "lynx",
+            "wisent", "wolf", "bunny", "moose", "fox", "lynx",
             "wolf", "marten", "sheep"
          },
       },
@@ -628,7 +628,7 @@
          name = "grasses",
          immovables = { "grass1", "grass2", "grass3" },
          critters = {
-            "wisent", "wolf", "bunny", "elk", "fox", "lynx",
+            "wisent", "wolf", "bunny", "moose", "fox", "lynx",
             "wolf", "marten", "sheep"
          },
       },
 
=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2019-05-16 09:15:03 +0000
+++ src/logic/map.cc	2019-06-22 16:21:53 +0000
@@ -714,6 +714,22 @@
 	pathfieldmgr_->set_size(field_size);
 }
 
+int Map::needs_widelands_version_after() const {
+	return map_version_.needs_widelands_version_after;
+}
+
+void Map::calculate_needs_widelands_version_after(bool is_post_one_world) {
+	if (map_version_.needs_widelands_version_after == 0) {
+		if (nrplayers_ > 8) {
+			// We introduced support for 16 players after Build 19
+			map_version_.needs_widelands_version_after = 19;
+		} else if (is_post_one_world) {
+			// We merged the worlds in the engine after Build 18
+			map_version_.needs_widelands_version_after = 18;
+		}
+	}
+}
+
 /*
  * The scenario get/set functions
  */
 
=== modified file 'src/logic/map.h'
--- src/logic/map.h	2019-05-16 09:15:03 +0000
+++ src/logic/map.h	2019-06-22 16:21:53 +0000
@@ -289,6 +289,9 @@
 		return height_;
 	}
 
+	// Map compatibility information for the website
+	int needs_widelands_version_after() const;
+
 	//  The next few functions are only valid when the map is loaded as a
 	//  scenario.
 	const std::string& get_scenario_player_tribe(PlayerNumber) const;
@@ -518,6 +521,11 @@
 	std::map<Coords, FieldData>
 	resize(EditorGameBase& egbase, const Coords coords, int32_t w, int32_t h);
 
+protected:
+	/// Calculate map compatibility information for the website if it wasn't defined in the map packet.
+	/// If is_post_one_world is true, this map wasn't created for a specific world (Widelands versions up to Build 18).
+	void calculate_needs_widelands_version_after(bool is_post_one_world);
+
 private:
 	void recalc_border(const FCoords&);
 	void recalc_brightness(const FCoords&);
 
=== modified file 'src/logic/map_objects/world/critter.cc'
--- src/logic/map_objects/world/critter.cc	2019-05-25 08:51:42 +0000
+++ src/logic/map_objects/world/critter.cc	2019-06-22 16:21:53 +0000
@@ -268,7 +268,8 @@
 ==============================
 */
 
-constexpr uint8_t kCurrentPacketVersion = 1;
+// We need to bump this packet version every time we rename a critter, so that the world legacy lookup table will work.
+constexpr uint8_t kCurrentPacketVersion = 2;
 
 Critter::Loader::Loader() {
 }
@@ -303,7 +304,7 @@
 			const CritterDescr* descr = nullptr;
 
 			if (critter_owner == "world") {
-				critter_name = lookup_table.lookup_critter(critter_name);
+				critter_name = lookup_table.lookup_critter(critter_name, packet_version);
 				descr = egbase.world().get_critter_descr(critter_name);
 			} else {
 				throw GameDataError(
 
=== modified file 'src/logic/map_revision.h'
--- src/logic/map_revision.h	2019-02-23 11:00:49 +0000
+++ src/logic/map_revision.h	2019-06-22 16:21:53 +0000
@@ -37,6 +37,8 @@
 	int32_t map_version_major;
 	int32_t map_version_minor;
 	uint32_t map_version_timestamp;
+	// Map compatibility information for the website
+	int needs_widelands_version_after;
 
 	MapVersion();
 };
 
=== modified file 'src/map_io/map_bob_packet.cc'
--- src/map_io/map_bob_packet.cc	2019-02-23 11:00:49 +0000
+++ src/map_io/map_bob_packet.cc	2019-06-22 16:21:53 +0000
@@ -36,7 +36,7 @@
                             EditorGameBase& egbase,
                             MapObjectLoader&,
                             const Coords& coords,
-                            const WorldLegacyLookupTable& lookup_table) {
+                            const WorldLegacyLookupTable& lookup_table, uint16_t packet_version) {
 	const std::string owner = fr.c_string();
 	char const* const read_name = fr.c_string();
 	uint8_t subtype = fr.unsigned_8();
@@ -47,7 +47,7 @@
 		throw GameDataError("unknown legacy bob %s/%s", owner.c_str(), read_name);
 	}
 
-	const std::string name = lookup_table.lookup_critter(read_name);
+	const std::string name = lookup_table.lookup_critter(read_name, packet_version);
 	try {
 		const World& world = egbase.world();
 		DescriptionIndex const idx = world.get_critter(name.c_str());
@@ -84,7 +84,7 @@
 				for (uint16_t x = 0; x < map->get_width(); ++x) {
 					uint32_t const nr_bobs = fr.unsigned_32();
 					for (uint32_t i = 0; i < nr_bobs; ++i)
-						read_bob(fr, egbase, mol, Coords(x, y), lookup_table);
+						read_bob(fr, egbase, mol, Coords(x, y), lookup_table, packet_version);
 				}
 			}
 		else {
 
=== modified file 'src/map_io/map_bob_packet.h'
--- src/map_io/map_bob_packet.h	2019-02-23 11:00:49 +0000
+++ src/map_io/map_bob_packet.h	2019-06-22 16:21:53 +0000
@@ -42,7 +42,7 @@
 	              EditorGameBase&,
 	              MapObjectLoader&,
 	              const Coords&,
-	              const WorldLegacyLookupTable& lookup_table);
+	              const WorldLegacyLookupTable& lookup_table, uint16_t packet_version);
 };
 }  // namespace Widelands
 
 
=== modified file 'src/map_io/map_saver.cc'
--- src/map_io/map_saver.cc	2019-03-09 10:01:09 +0000
+++ src/map_io/map_saver.cc	2019-06-22 16:21:53 +0000
@@ -156,7 +156,7 @@
 	log("Writing Map Version ... ");
 	{
 		MapVersionPacket p;
-		p.write(fs_, egbase_, *mos_);
+		p.write(fs_, egbase_);
 	}
 	log("took %ums\n ", timer.ms_since_last_query());
 
 
=== modified file 'src/map_io/map_version_packet.cc'
--- src/map_io/map_version_packet.cc	2019-02-23 11:00:49 +0000
+++ src/map_io/map_version_packet.cc	2019-06-22 16:21:53 +0000
@@ -30,11 +30,17 @@
 namespace Widelands {
 
 constexpr uint16_t kCurrentPacketVersion = 1;
+// Map compatibility information for the website
+constexpr int kCurrentNeedsWidelandsVersionAfter = 20;
 
 void MapVersionPacket::read(FileSystem& fs,
                             EditorGameBase& egbase,
-                            bool const skip,
-                            MapObjectLoader&) {
+                            bool const skip, bool is_post_one_world) {
+
+	pre_read(fs, egbase.mutable_map(), skip, is_post_one_world);
+}
+
+void MapVersionPacket::pre_read(FileSystem& fs, Map* map, bool skip, bool is_post_one_world) {
 	if (skip)
 		return;
 
@@ -42,7 +48,6 @@
 	try {
 		prof.read("version", nullptr, fs);
 	} catch (...) {
-		Map* map = egbase.mutable_map();
 		map->map_version_.map_version_timestamp = 0;
 		map->map_version_.map_creator_version = "unknown";
 		return;
@@ -55,7 +60,6 @@
 		if ((packet_version == kCurrentPacketVersion) ||
 		    (packet_version > kCurrentPacketVersion &&
 		     forward_compatibility <= kCurrentPacketVersion)) {
-			Map* map = egbase.mutable_map();
 			map->map_version_.map_source_url = globv.get_safe_string("map_source_url");
 			map->map_version_.map_source_release = globv.get_safe_string("map_release");
 			map->map_version_.map_creator_version = globv.get_safe_string("map_creator_version");
@@ -63,6 +67,8 @@
 			map->map_version_.map_version_minor = globv.get_safe_int("map_version_minor");
 			uint32_t ts = static_cast<uint32_t>(globv.get_safe_int("map_version_timestamp"));
 			map->map_version_.map_version_timestamp = ts;
+			map->map_version_.needs_widelands_version_after = globv.get_int("needs_widelands_version_after", 0);
+			map->calculate_needs_widelands_version_after(is_post_one_world);
 		} else {
 			throw UnhandledVersionError("MapVersionPacket", packet_version, kCurrentPacketVersion);
 		}
@@ -71,7 +77,7 @@
 	}
 }
 
-void MapVersionPacket::write(FileSystem& fs, EditorGameBase& egbase, MapObjectSaver&) {
+void MapVersionPacket::write(FileSystem& fs, EditorGameBase& egbase) {
 	Profile prof;
 	Section& globs = prof.create_section("global");
 
@@ -117,6 +123,7 @@
 	globs.set_int("map_version_timestamp", static_cast<uint32_t>(time(nullptr)));
 	globs.set_int("packet_version", kCurrentPacketVersion);
 	globs.set_int("packet_compatibility", kCurrentPacketVersion);
+	globs.set_int("needs_widelands_version_after", kCurrentNeedsWidelandsVersionAfter);
 
 	prof.write("version", false, fs);
 }
 
=== modified file 'src/map_io/map_version_packet.h'
--- src/map_io/map_version_packet.h	2019-02-23 11:00:49 +0000
+++ src/map_io/map_version_packet.h	2019-06-22 16:21:53 +0000
@@ -20,11 +20,26 @@
 #ifndef WL_MAP_IO_MAP_VERSION_PACKET_H
 #define WL_MAP_IO_MAP_VERSION_PACKET_H
 
-#include "map_io/map_data_packet.h"
+class FileSystem;
+
+namespace Widelands {
+
+class EditorGameBase;
+class Map;
+class MapObjectLoader;
+struct MapObjectSaver;
 
 /*
  * This packet contains the version information of the map.
  */
-MAP_DATA_PACKET(MapVersionPacket)
+class MapVersionPacket {
+public:
+	void read(FileSystem&, EditorGameBase&, bool skip, bool is_post_one_world);
+	void write(FileSystem&, EditorGameBase&);
+
+	void pre_read(FileSystem&, Map*, bool skip, bool is_post_one_world);
+};
+
+} // namespace Widelands
 
 #endif  // end of include guard: WL_MAP_IO_MAP_VERSION_PACKET_H
 
=== modified file 'src/map_io/widelands_map_loader.cc'
--- src/map_io/widelands_map_loader.cc	2019-05-26 03:14:41 +0000
+++ src/map_io/widelands_map_loader.cc	2019-06-22 16:21:53 +0000
@@ -82,6 +82,10 @@
 		mp.pre_read(*fs_, &map_);
 		old_world_name_ = mp.old_world_name();
 	}
+	{
+		MapVersionPacket version_packet;
+		version_packet.pre_read(*fs_, &map_, false, old_world_name_.empty());
+	}
 
 	{
 		MapPlayerNamesAndTribesPacket p;
@@ -198,7 +202,7 @@
 		log("Reading Map Version Data ... ");
 		{
 			MapVersionPacket p;
-			p.read(*fs_, egbase, is_game, *mol_);
+			p.read(*fs_, egbase, is_game, old_world_name_.empty());
 		}
 		log("took %ums\n ", timer.ms_since_last_query());
 
 
=== modified file 'src/map_io/world_legacy_lookup_table.cc'
--- src/map_io/world_legacy_lookup_table.cc	2019-02-23 11:00:49 +0000
+++ src/map_io/world_legacy_lookup_table.cc	2019-06-22 16:21:53 +0000
@@ -33,16 +33,24 @@
 	// Implements OneWorldLegacyLookupTable.
 	std::string lookup_resource(const std::string& resource) const override;
 	std::string lookup_terrain(const std::string& terrain) const override;
-	std::string lookup_critter(const std::string& critter) const override;
+	std::string lookup_critter(const std::string& critter, uint16_t packet_version) const override;
 	std::string lookup_immovable(const std::string& immovable) const override;
 
 private:
+	// Old name, <packet version when it got changed, new name>
+	const std::map<std::string, std::map<uint16_t, std::string>> critters_;
 	const std::map<std::string, std::string> immovables_;
 	const std::map<std::string, std::string> resources_;
 	const std::map<std::string, std::string> terrains_;
 };
 
 PostOneWorldLegacyLookupTable::PostOneWorldLegacyLookupTable() :
+critters_
+{
+	// We need to bump the packet version in Critter every time we rename a critter.
+	// Also, don't forget to edit the OneWorldLegacyLookupTable! You can look up which world had which units in Build 18.
+	{"elk", {{2, "moose"}}},
+},
 immovables_
 {
 	{"blackland_stones1", "blackland_rocks1"},
@@ -113,8 +121,16 @@
 	return i->second;
 }
 
-std::string PostOneWorldLegacyLookupTable::lookup_critter(const std::string& critter) const {
-	return critter;
+std::string PostOneWorldLegacyLookupTable::lookup_critter(const std::string& critter, uint16_t packet_version) const {
+	std::string result = critter;
+	if (critters_.count(critter) == 1) {
+		for (const auto& candidate : critters_.at(result)) {
+			if (candidate.first >= packet_version) {
+				result = candidate.second;
+			}
+		}
+	}
+	return result;
 }
 
 std::string PostOneWorldLegacyLookupTable::lookup_immovable(const std::string& immovable) const {
@@ -132,7 +148,7 @@
 	// Implements OneWorldLegacyLookupTable.
 	std::string lookup_resource(const std::string& resource) const override;
 	std::string lookup_terrain(const std::string& terrain) const override;
-	std::string lookup_critter(const std::string& critter) const override;
+	std::string lookup_critter(const std::string& critter, uint16_t packet_version) const override;
 	std::string lookup_immovable(const std::string& immovable) const override;
 
 private:
@@ -205,6 +221,7 @@
         std::make_pair("greenland",
                        std::map<std::string, std::string>{
                           {"deer", "stag"},
+								  {"elk", "moose"},
                        }),
         std::make_pair("blackland",
                        std::map<std::string, std::string>{
@@ -213,6 +230,7 @@
         std::make_pair("winterland",
                        std::map<std::string, std::string>{
                           {"deer", "stag"},
+								  {"elk", "moose"},
                        }),
         std::make_pair("desert",
                        std::map<std::string, std::string>{
@@ -418,7 +436,7 @@
 	return terrain;
 }
 
-std::string OneWorldLegacyLookupTable::lookup_critter(const std::string& critter) const {
+std::string OneWorldLegacyLookupTable::lookup_critter(const std::string& critter, uint16_t) const {
 	const std::map<std::string, std::string>& world_critters = critters_.at(old_world_name_);
 	const auto& i = world_critters.find(critter);
 	if (i != world_critters.end()) {
 
=== modified file 'src/map_io/world_legacy_lookup_table.h'
--- src/map_io/world_legacy_lookup_table.h	2019-02-23 11:00:49 +0000
+++ src/map_io/world_legacy_lookup_table.h	2019-06-22 16:21:53 +0000
@@ -38,7 +38,7 @@
 	virtual std::string lookup_terrain(const std::string& terrain) const = 0;
 
 	/// Looks up the new name for the 'critter'.
-	virtual std::string lookup_critter(const std::string& critter) const = 0;
+	virtual std::string lookup_critter(const std::string& critter, uint16_t packet_version) const = 0;
 
 	/// Looks up the new name for the 'immovable'.
 	virtual std::string lookup_immovable(const std::string& immovable) const = 0;
 
=== modified file 'src/website/map_info.cc'
--- src/website/map_info.cc	2019-02-23 11:00:49 +0000
+++ src/website/map_info.cc	2019-06-22 16:21:53 +0000
@@ -92,6 +92,7 @@
 			json->add_int("width", map->get_width());
 			json->add_int("height", map->get_height());
 			json->add_int("nr_players", map->get_nrplayers());
+			json->add_int("needs_widelands_version_after", map->needs_widelands_version_after());
 
 			const std::string world_name =
 			   static_cast<Widelands::WidelandsMapLoader*>(ml.get())->old_world_name();