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

[GunChleoc]

Tweak performance for saveloading and introduce an optional "init" function for winconditions with costly calculations

- Affected win conditions: Artifacts, Wood Gnome, Territorial Time, Territorial Lord.
- Saveload long lists of fields via C++ because Lua persistence is very expensive
- Use map index for iteration when writing
- Clean up writing CmdFlagAction
- Bulk skip all commands with greater duetime when writing command queue packet

We will need either this branch in Build 20 or a separate branch to fix Artifacts, because we broke it.

I'd like this branch for its performance improvements, but it is big, so I can be convinced otherwise.

Some stats for the big map "Magic Mountain" with Territorial Time:

This branch:

 Writing Wincondition Data: 146ms
 Writing Scripting Data: 25ms
 Sum 171ms

 Reading Wincondition Data: 997ms
 Reading Scripting Data: 5ms
 Sum 1 002ms

Trunk:
 Writing Scripting Data: 32 218ms
 Reading Scripting Data: 4 116ms

Writing speed factor: ca. 188x
Reading speed factor: ca. 4x

[Toni Förster (stonerl)]

Tested this, also with old savegames. Works like a charm. Tremendously speeds up the saving and loading (territorial & woodgnome).

review: Approve (tested)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4572. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/503990492.
Appveyor build 4359. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_speedup_saveloading-4359.

[Toni Förster (stonerl)]

Regressiontestsfail, though

review: Needs Fixing (travis)

[GunChleoc (gunchleoc)]

I forgot to adapt the tests to the new data types. Should be working now.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4579. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/504410525.
Appveyor build 4366. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_speedup_saveloading-4366.

[Toni Förster (stonerl)]

review: Approve

[GunChleoc (gunchleoc)]

Thanks for the review!

@bunnybot merge

[kaputtnik (franku)]

Here are my values from my old laptop. Seem this branch makes loading slower?

Version bzr9012[speedup_saveloading] (Release)
WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 5136ms
GameLoader::load() took 11895ms
MapSaver::save() for 'Magic Mountain' took 12376ms
MapSaver::save() for 'Magic Mountain' took 12096ms
GameSaver::save() took 13103ms
WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 7798ms
GameLoader::load() took 7921ms

Version bzr9008[trunk] (Release)
WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 5042ms
GameLoader::load() took 12061ms
MapSaver::save() for 'Magic Mountain' took 11165ms
MapSaver::save() for 'Magic Mountain' took 11309ms
GameSaver::save() took 12220ms
WidelandsMapLoader::load_map_complete() for 'Magic Mountain' took 7495ms
GameLoader::load() took 7617ms

[GunChleoc (gunchleoc)]

The only change that could possibly make it slower are the changes to the map resources packet, so I have reverted that. The important speed up is for the win conditions. All the other changes are omitting data, and the extra boolean comparisons should be a lot faster than writing to / reading from disk.

Have you made more than one test run? The numbers are not that radically different.

[Toni Förster (stonerl)]

Well I'd say these numbers are within the margin of error. If you compare saving and loading times, you can see the difference this branch makes, though.

@GunChleoc not sure if r9015 was necessary. I don't really see a difference.

[GunChleoc (gunchleoc)]

It didn't gain us that much anyway, so better safe than sorry this close to the release. The big think here was removing the field lists from Lua ;)

[Toni Förster (stonerl)]

Sound sane :-) are you planning to add them for b21?

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4580. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/504553847.
Appveyor build 4367. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_speedup_saveloading-4367.

[bunnybot (widelandsofficial)]

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

Travis build 4580. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/504553847.

[Toni Förster (stonerl)]

Travis hiccup while downloading brew bottles.

@bunnybot merge force

[GunChleoc (gunchleoc)]

I guess we could.

[GunChleoc (gunchleoc)]

I did some more measurements on my slow machine, and the change in the resources packet didn't matter there. Let's keep it as it is, because the code is more readable.

=== modified file 'data/scripting/win_conditions/territorial_functions.lua'
--- data/scripting/win_conditions/territorial_functions.lua	2019-02-21 08:39:13 +0000
+++ data/scripting/win_conditions/territorial_functions.lua	2019-03-11 07:15:58 +0000
@@ -7,45 +7,13 @@
 set_textdomain("win_conditions")
 
 include "scripting/richtext.lua"
+include "scripting/win_conditions/win_condition_functions.lua"
 include "scripting/win_conditions/win_condition_texts.lua"
 
 local team_str = _"Team %i"
 local wc_has_territory = _"%1$s has %2$3.0f%% of the land (%3$i of %4$i)."
 local wc_had_territory = _"%1$s had %2$3.0f%% of the land (%3$i of %4$i)."
 
--- RST
--- .. function:: count_owned_fields_for_all_players(fields, players)
---
---    Counts all owned fields for each player.
---
---    :arg fields: Table of all buildable fields
---    :arg players: Table of all players
---
---    :returns: a table with ``playernumber = count_of_owned_fields``  entries
---
-local function count_owned_fields_for_all_players(fields, players)
-   local owned_fields = {}
-   -- init the landsizes for each player
-   for idx,plr in ipairs(players) do
-      owned_fields[plr.number] = 0
-   end
-
-   for idx,f in ipairs(fields) do
-      -- check if field is owned by a player
-      local owner = f.owner
-      if owner then
-         local owner_number = owner.number
-         if owned_fields[owner_number] == nil then
-            -- In case player was defeated and lost all their warehouses, make sure they don't count
-            owned_fields[owner_number] = -1
-         elseif owned_fields[owner_number] >= 0 then
-            owned_fields[owner_number] = owned_fields[owner_number] + 1
-         end
-      end
-   end
-   return owned_fields
-end
-
 -- Used by calculate_territory_points keep track of when the winner changes
 local winning_players = {}
 local winning_teams = {}
@@ -91,7 +59,7 @@
 --    First checks if a player was defeated, then fills the ``territory_points`` table
 --    with current data.
 --
---    :arg fields: Table of all buildable fields
+--    :arg fields: Number of all valuable fields
 --    :arg players: Table of all players
 --    :arg wc_descname: String with the win condition's descname
 --    :arg wc_version: Number with the win condition's descname
@@ -100,12 +68,12 @@
    local points = {} -- tracking points of teams and players without teams
    local territory_was_kept = false
 
-   territory_points.all_player_points = count_owned_fields_for_all_players(fields, players)
+   territory_points.all_player_points = count_owned_valuable_fields_for_all_players(players)
    local ranked_players = rank_players(territory_points.all_player_points, players)
 
    -- Check if we have a winner. The table was sorted, so we can simply grab the first entry.
    local winning_points = -1
-   if ranked_players[1].points > ( #fields / 2 ) then
+   if ranked_players[1].points > ( fields / 2 ) then
       winning_points = ranked_players[1].points
    end
 
@@ -161,7 +129,7 @@
 --    Returns a string containing the current land percentages of players/teams
 --    for messages to the players
 --
---    :arg fields: Table of all buildable fields
+--    :arg fields: Number of all valuable fields
 --    :arg has_had: Use "has" for an interim message, "had" for a game over message.
 --
 --    :returns: a richtext-formatted string with information on current points for each player/team
@@ -178,17 +146,17 @@
             li(
                (wc_has_territory):bformat(
                   territory_points.points[i][1],
-                  _percent(territory_points.points[i][2], #fields),
+                  _percent(territory_points.points[i][2], fields),
                   territory_points.points[i][2],
-                  #fields))
+                  fields))
       else
          msg = msg ..
             li(
                (wc_had_territory):bformat(
                   territory_points.points[i][1],
-                  _percent(territory_points.points[i][2], #fields),
+                  _percent(territory_points.points[i][2], fields),
                   territory_points.points[i][2],
-                  #fields))
+                  fields))
       end
 
    end
@@ -246,7 +214,7 @@
 --
 --    Updates the territory points and sends game over reports
 --
---    :arg fields: Table of all buildable fields
+--    :arg fields: Number of all valuable fields
 --    :arg players: Table of all players
 --
 function territory_game_over(fields, players, wc_descname, wc_version)
 
=== modified file 'data/scripting/win_conditions/territorial_lord.lua'
--- data/scripting/win_conditions/territorial_lord.lua	2019-03-02 08:52:08 +0000
+++ data/scripting/win_conditions/territorial_lord.lua	2019-03-11 07:15:58 +0000
@@ -23,18 +23,20 @@
    "that area for at least 20 minutes."
 )
 
+local fields = 0
+
 return {
    name = wc_name,
    description = wc_desc,
+   init = function()
+      fields = wl.Game().map:count_conquerable_fields()
+   end,
    func = function()
       local plrs = wl.Game().players
 
       -- set the objective with the game type for all players
       broadcast_objective("win_condition", wc_descname, wc_desc)
 
-      -- Table of fields that are worth conquering
-      local fields = wl.Game().map.conquerable_fields
-
       -- Configure how long the winner has to hold on to the territory
       local time_to_keep_territory = 20 * 60 -- 20 minutes
       -- time in secs, if == 0 -> victory
 
=== modified file 'data/scripting/win_conditions/territorial_time.lua'
--- data/scripting/win_conditions/territorial_time.lua	2019-03-02 08:52:08 +0000
+++ data/scripting/win_conditions/territorial_time.lua	2019-03-11 07:15:58 +0000
@@ -27,18 +27,20 @@
    "after 4 hours, whichever comes first."
 )
 
+local fields = 0
+
 return {
    name = wc_name,
    description = wc_desc,
+   init = function()
+      fields = wl.Game().map:count_conquerable_fields()
+   end,
    func = function()
       local plrs = wl.Game().players
 
       -- set the objective with the game type for all players
       broadcast_objective("win_condition", wc_descname, wc_desc)
 
-      -- Table of fields that are worth conquering
-      local fields = wl.Game().map.conquerable_fields
-
       -- variables to track the maximum 4 hours of gametime
       local remaining_max_time = 4 * 60 * 60 -- 4 hours
 
 
=== modified file 'data/scripting/win_conditions/win_condition_functions.lua'
--- data/scripting/win_conditions/win_condition_functions.lua	2019-02-12 17:30:21 +0000
+++ data/scripting/win_conditions/win_condition_functions.lua	2019-03-11 07:15:58 +0000
@@ -147,6 +147,38 @@
 
 
 -- RST
+-- .. function:: count_owned_valuable_fields_for_all_players(players[, attribute])
+--
+--    Counts all owned fields for each player.
+--
+--    :arg players: Table of all players
+--    :arg attribute: If this is set, only count fields that have an immovable with this attribute
+--
+--    :returns: a table with ``playernumber = count_of_owned_fields``  entries
+--
+function count_owned_valuable_fields_for_all_players(players, attribute)
+   attribute = attribute or ""
+
+   local owned_fields = {}
+
+   -- Get number of currently owned valuable fields per player.
+   -- This table can contain defeated players.
+   local all_plrpoints = wl.Game().map:count_owned_valuable_fields(attribute)
+
+   -- Insert points for all players who are still in the game, and 0 points for defeated players.
+   for idx,plr in ipairs(players) do
+      if (plr.defeated) then
+         owned_fields[plr.number] = 0
+      else
+         owned_fields[plr.number] = all_plrpoints[plr.number]
+      end
+   end
+   return owned_fields
+end
+
+
+
+-- RST
 -- .. function:: rank_players(all_player_points, plrs)
 --
 --    Rank the players and teams according to the highest points
 
=== modified file 'data/scripting/win_conditions/wood_gnome.lua'
--- data/scripting/win_conditions/wood_gnome.lua	2019-03-02 08:52:08 +0000
+++ data/scripting/win_conditions/wood_gnome.lua	2019-03-11 07:15:58 +0000
@@ -24,6 +24,10 @@
 return {
    name = wc_name,
    description = wc_desc,
+   init = function()
+      -- Calculate valuable fields
+      wl.Game().map:count_terrestrial_fields()
+   end,
    func = function()
    local plrs = wl.Game().players
    local game = wl.Game()
@@ -34,51 +38,21 @@
    -- set the objective with the game type for all players
    broadcast_objective("win_condition", wc_descname, wc_desc)
 
-   -- Table of terrestrial fields
-   local fields = wl.Game().map.terrestrial_fields
-
    -- The function to calculate the current points.
    local _last_time_calculated = -100000
-   local _plrpoints = {}
+   local playerpoints = {}
    local function _calc_points()
-      local game = wl.Game()
 
       if _last_time_calculated > game.time - 5000 then
-         return _plrpoints
-      end
-
-      -- clear out the table. We count afresh.
-      for k,v in pairs(_plrpoints) do
-         _plrpoints[k] = 0
-      end
-
-      -- Insert all players who are still in the game.
-      for idx,plr in ipairs(plrs) do
-         _plrpoints[plr.number] = 0
-      end
-
-      for idf,f in ipairs(fields) do
-         -- check if field is owned by a player
-         local owner = f.owner
-         if owner and not owner.defeated then
-            owner = owner.number
-            -- check if field has an immovable
-            local imm = f.immovable
-            if imm then
-               -- check if immovable is a tree
-               if imm:has_attribute("tree") then
-                  _plrpoints[owner] = _plrpoints[owner] + 1
-               end
-            end
-         end
-      end
-
+         return
+      end
+
+      playerpoints = count_owned_valuable_fields_for_all_players(plrs, "tree")
       _last_time_calculated = game.time
-      return _plrpoints
    end
 
    local function _send_state(remaining_time, plrs, show_popup)
-      local playerpoints = _calc_points()
+      _calc_points()
       local msg = format_remaining_time(remaining_time) .. vspace(8) .. game_status.body
 
       for idx,plr in ipairs(plrs) do
@@ -92,7 +66,7 @@
    end
 
    local function _game_over(plrs)
-      local playerpoints = _calc_points()
+      _calc_points()
       local points = {}
       for idx,plr in ipairs(plrs) do
          points[#points + 1] = { plr, playerpoints[plr.number] }
@@ -131,8 +105,8 @@
       name = wc_trees_owned,
       pic = "images/wui/stats/genstats_trees.png",
       calculator = function(p)
-         local pts = _calc_points(p)
-         return pts[p.number]
+         _calc_points(p)
+         return playerpoints[p.number] or 0
       end,
    }
 
 
=== modified file 'src/game_io/game_cmd_queue_packet.cc'
--- src/game_io/game_cmd_queue_packet.cc	2019-02-23 11:00:49 +0000
+++ src/game_io/game_cmd_queue_packet.cc	2019-03-11 07:15:58 +0000
@@ -98,6 +98,10 @@
 
 		while (!p.empty()) {
 			const CmdQueue::CmdItem& it = p.top();
+			if (it.cmd->duetime() > time) {
+				// Time is the primary sorting key, so we can't have any additional commands in this queue for this time
+				break;
+			}
 			if (it.cmd->duetime() == time) {
 				if (upcast(GameLogicCommand, cmd, it.cmd)) {
 					// The id (aka command type)
 
=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2019-03-02 10:34:39 +0000
+++ src/logic/game.cc	2019-03-11 07:15:58 +0000
@@ -314,10 +314,14 @@
 
 	// Check for win_conditions
 	if (!settings.scenario) {
+		loader_ui->step(_("Initializing game…"));
 		std::unique_ptr<LuaTable> table(lua().run_script(settings.win_condition_script));
-		get_ibase()->log_message(_("Initializing game…"));
 		table->do_not_warn_about_unaccessed_keys();
 		win_condition_displayname_ = table->get_string("name");
+		if (table->has_key<std::string>("init")) {
+			std::unique_ptr<LuaCoroutine> cr = table->get_coroutine("init");
+			cr->resume();
+		}
 		std::unique_ptr<LuaCoroutine> cr = table->get_coroutine("func");
 		enqueue_command(new CmdLuaCoroutine(get_gametime() + 100, std::move(cr)));
 	} else {
 
=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2019-03-02 09:31:11 +0000
+++ src/logic/map.cc	2019-03-11 07:15:58 +0000
@@ -257,8 +257,11 @@
 		}
 }
 
-std::set<FCoords> Map::calculate_all_conquerable_fields() const {
-	std::set<FCoords> result;
+size_t Map::count_all_conquerable_fields() {
+	if (!valuable_fields_.empty()) {
+		// Already calculated
+		return valuable_fields_.size();
+	}
 
 	std::set<FCoords> coords_to_check;
 
@@ -267,17 +270,17 @@
 
 	// If we don't have the given coordinates yet, walk the map and collect conquerable fields,
 	// initialized with the given radius around the coordinates
-	const auto walk_starting_coords = [this, &result, &coords_to_check](
+	const auto walk_starting_coords = [this, &coords_to_check](
 	                                     const Coords& coords, int radius) {
 		FCoords fcoords = get_fcoords(coords);
 
 		// We already have these coordinates
-		if (result.count(fcoords) == 1) {
+		if (valuable_fields_.count(fcoords) == 1) {
 			return;
 		}
 
 		// Add starting field
-		result.insert(fcoords);
+		valuable_fields_.insert(fcoords);
 
 		// Add outer land coordinates around the starting field for the given radius
 		std::unique_ptr<Widelands::HollowArea<>> hollow_area(
@@ -317,8 +320,8 @@
 
 					// We do the caps check first, because the comparison is faster than the container
 					// check
-					if ((fcoords.field->maxcaps() & MOVECAPS_WALK) && (result.count(fcoords) == 0)) {
-						result.insert(fcoords);
+					if ((fcoords.field->maxcaps() & MOVECAPS_WALK) && (valuable_fields_.count(fcoords) == 0)) {
+						valuable_fields_.insert(fcoords);
 						coords_to_check.insert(fcoords);
 					}
 				} while (map_region->advance(*this));
@@ -342,23 +345,44 @@
 		}
 	}
 
-	log("%" PRIuS " found ... ", result.size());
-	return result;
+	log("%" PRIuS " found ... ", valuable_fields_.size());
+	return valuable_fields_.size();
 }
 
-std::set<FCoords> Map::calculate_all_fields_excluding_caps(NodeCaps caps) const {
+size_t Map::count_all_fields_excluding_caps(NodeCaps caps) {
+	if (!valuable_fields_.empty()) {
+		// Already calculated
+		return valuable_fields_.size();
+	}
+
 	log("Collecting valuable fields ... ");
 	ScopedTimer timer("took %ums");
 
-	std::set<FCoords> result;
 	for (MapIndex i = 0; i < max_index(); ++i) {
 		Field& field = fields_[i];
 		if (!(field.nodecaps() & caps)) {
-			result.insert(get_fcoords(field));
-		}
-	}
-
-	log("%" PRIuS " found ... ", result.size());
+			valuable_fields_.insert(get_fcoords(field));
+		}
+	}
+
+	log("%" PRIuS " found ... ", valuable_fields_.size());
+	return valuable_fields_.size();
+}
+
+std::map<PlayerNumber, size_t> Map::count_owned_valuable_fields(const std::string& immovable_attribute) const {
+	std::map<PlayerNumber, size_t> result;
+	const bool use_attribute = !immovable_attribute.empty();
+	const uint32_t attribute_id = use_attribute ? MapObjectDescr::get_attribute_id(immovable_attribute) : 0U;
+	for (const FCoords& fcoords : valuable_fields_) {
+		if (use_attribute) {
+			const BaseImmovable* imm = fcoords.field->get_immovable();
+			if (imm != nullptr && imm->has_attribute(attribute_id)) {
+				result[fcoords.field->get_owned_by()] += 1;
+			}
+		} else {
+			result[fcoords.field->get_owned_by()] += 1;
+		}
+	}
 	return result;
 }
 
 
=== modified file 'src/logic/map.h'
--- src/logic/map.h	2019-02-26 05:56:01 +0000
+++ src/logic/map.h	2019-03-11 07:15:58 +0000
@@ -165,11 +165,25 @@
 	void recalc_whole_map(const World& world);
 	void recalc_for_field_area(const World& world, Area<FCoords>);
 
-	/// Calculates and returns a list of the fields that could be conquered by a player throughout a
-	/// game. Useful for territorial win conditions.
-	std::set<FCoords> calculate_all_conquerable_fields() const;
-	/// Calculates and returns a list of the fields that do not have the given caps.
-	std::set<FCoords> calculate_all_fields_excluding_caps(NodeCaps caps) const;
+	/**
+	 *  If the valuable fields are empty, calculates all fields that could be conquered by a player throughout a game.
+	 *  Useful for territorial win conditions.
+	 *  Returns the amount of valuable fields.
+	 */
+	size_t count_all_conquerable_fields();
+
+	/**
+	 *  If the valuable fields are empty, calculates which fields do not have the given caps and adds the to the list of valuable fields.
+	 *  Useful for win conditions.
+	 *  Returns the amount of valuable fields.
+	 */
+	size_t count_all_fields_excluding_caps(NodeCaps caps);
+
+	/**
+	 * Counts the valuable fields that are owned by each player. Only players that currently own a field are added.
+	 * Returns a map of <player number, number of owned fields>.
+	 */
+	std::map<PlayerNumber, size_t> count_owned_valuable_fields(const std::string& immovable_attribute) const;
 
 	/***
 	 * Ensures that resources match their adjacent terrains.
@@ -438,6 +452,10 @@
 		return &objectives_;
 	}
 
+	std::set<FCoords>* mutable_valuable_fields() {
+		return &valuable_fields_;
+	}
+
 	/// Returns the military influence on a location from an area.
 	MilitaryInfluence calc_influence(Coords, Area<>) const;
 
@@ -535,6 +553,9 @@
 
 	Objectives objectives_;
 
+	// Fields that are important for the player to own in a win condition
+	std::set<FCoords> valuable_fields_;
+
 	MapVersion map_version_;
 };
 
 
=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2019-02-23 11:00:49 +0000
+++ src/logic/player.cc	2019-03-11 07:15:58 +0000
@@ -253,6 +253,15 @@
 	return &other != this && (!team_number_ || team_number_ != other.team_number_);
 }
 
+bool Player::is_defeated() const {
+	for (const auto& economy : economies()) {
+		if (!economy.second->warehouses().empty()) {
+			return false;
+		}
+	}
+	return true;
+}
+
 void Player::AiPersistentState::initialize() {
 	colony_scan_area = kColonyScanStartArea;
 	trees_around_cutters = 0;
 
=== modified file 'src/logic/player.h'
--- src/logic/player.h	2019-02-23 11:00:49 +0000
+++ src/logic/player.h	2019-03-11 07:15:58 +0000
@@ -148,6 +148,11 @@
 
 	bool is_hostile(const Player&) const;
 
+	/**
+	 * Returns whether the player lost the last warehouse.
+	 */
+	bool is_defeated() const;
+
 	// For cheating
 	void set_see_all(bool const t) {
 		see_all_ = t;
 
=== modified file 'src/logic/playercommand.cc'
--- src/logic/playercommand.cc	2019-02-23 11:00:49 +0000
+++ src/logic/playercommand.cc	2019-03-11 07:15:58 +0000
@@ -450,14 +450,17 @@
 	ser.unsigned_32(serial);
 }
 
-constexpr uint16_t kCurrentPacketVersionCmdFlagAction = 1;
+constexpr uint16_t kCurrentPacketVersionCmdFlagAction = 2;
 
 void CmdFlagAction::read(FileRead& fr, EditorGameBase& egbase, MapObjectLoader& mol) {
 	try {
 		const uint16_t packet_version = fr.unsigned_16();
-		if (packet_version == kCurrentPacketVersionCmdFlagAction) {
+		// TODO(GunChleoc): Savegame compatibility, remove after Build 21
+		if (packet_version >= 1 && packet_version <= kCurrentPacketVersionCmdFlagAction) {
 			PlayerCommand::read(fr, egbase, mol);
-			fr.unsigned_8();
+			if (packet_version == 1) {
+				fr.unsigned_8();
+			}
 			serial = get_object_serial_or_zero<Flag>(fr.unsigned_32(), mol);
 		} else {
 			throw UnhandledVersionError(
@@ -472,9 +475,6 @@
 	fw.unsigned_16(kCurrentPacketVersionCmdFlagAction);
 	// Write base classes
 	PlayerCommand::write(fw, egbase, mos);
-	// Now action
-	fw.unsigned_8(0);
-
 	// Now serial
 	fw.unsigned_32(mos.get_object_file_index_or_zero(egbase.objects().get_object(serial)));
 }
 
=== modified file 'src/map_io/CMakeLists.txt'
--- src/map_io/CMakeLists.txt	2017-11-20 13:50:51 +0000
+++ src/map_io/CMakeLists.txt	2019-03-11 07:15:58 +0000
@@ -87,6 +87,8 @@
     map_terrain_packet.h
     map_version_packet.cc
     map_version_packet.h
+    map_wincondition_packet.cc
+    map_wincondition_packet.h
   DEPENDS
     base_exceptions
     base_log
 
=== modified file 'src/map_io/map_saver.cc'
--- src/map_io/map_saver.cc	2019-02-23 11:00:49 +0000
+++ src/map_io/map_saver.cc	2019-03-11 07:15:58 +0000
@@ -58,6 +58,7 @@
 #include "map_io/map_scripting_packet.h"
 #include "map_io/map_terrain_packet.h"
 #include "map_io/map_version_packet.h"
+#include "map_io/map_wincondition_packet.h"
 
 namespace Widelands {
 
@@ -220,6 +221,14 @@
 	log("took %ums\n ", timer.ms_since_last_query());
 
 	if (is_game) {
+		// Map data used by win conditions
+		log("Writing Wincondition Data ... ");
+		{
+			MapWinconditionPacket p;
+			p.write(fs_, *egbase_.mutable_map(), *mos_);
+		}
+		log("took %ums\n ", timer.ms_since_last_query());
+
 		// DATA PACKETS
 		if (mos_->get_nr_flags()) {
 			log("Writing Flagdata Data ... ");
 
=== added file 'src/map_io/map_wincondition_packet.cc'
--- src/map_io/map_wincondition_packet.cc	1970-01-01 00:00:00 +0000
+++ src/map_io/map_wincondition_packet.cc	2019-03-11 07:15:58 +0000
@@ -0,0 +1,75 @@
+/*
+ * Copyright (C) 2019 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#include "map_io/map_wincondition_packet.h"
+
+#include "io/fileread.h"
+#include "io/filewrite.h"
+#include "logic/game_data_error.h"
+
+namespace Widelands {
+
+constexpr uint8_t kCurrentPacketVersion = 1;
+
+void MapWinconditionPacket::read(FileSystem& fs, Map& map, MapObjectLoader&) {
+	if (!fs.file_exists("binary/wincondition")) {
+		// This can be empty when win conditions don't need any special map data
+		return;
+	}
+
+	try {
+		FileRead fr;
+		fr.open(fs, "binary/wincondition");
+
+		const uint8_t packet_version = fr.unsigned_8();
+		if (packet_version == kCurrentPacketVersion) {
+			const size_t no_of_fields = fr.unsigned_32();
+			std::set<FCoords>* fields = map.mutable_valuable_fields();
+			fields->clear();
+
+			for (size_t i = 0; i < no_of_fields; ++i) {
+				const int32_t x = fr.signed_16();
+				const int32_t y = fr.signed_16();
+				fields->insert(map.get_fcoords(Coords(x, y)));
+			}
+		} else {
+			throw UnhandledVersionError("MapWinconditionPacket", packet_version, kCurrentPacketVersion);
+		}
+	} catch (const WException& e) {
+		throw GameDataError("win condition data: %s", e.what());
+	}
+}
+
+void MapWinconditionPacket::write(FileSystem& fs, Map& map, MapObjectSaver&) {
+	// We only write this packet if we have something interesting to write to it.
+	std::set<FCoords>& fields = *map.mutable_valuable_fields();
+	if (!fields.empty()) {
+		FileWrite fw;
+		fw.unsigned_8(kCurrentPacketVersion);
+
+		fw.unsigned_32(fields.size());
+		for (const auto& field : fields) {
+			fw.signed_16(field.x);
+			fw.signed_16(field.y);
+		}
+
+		fw.write(fs, "binary/wincondition");
+	}
+}
+}  // namespace Widelands
 
=== added file 'src/map_io/map_wincondition_packet.h'
--- src/map_io/map_wincondition_packet.h	1970-01-01 00:00:00 +0000
+++ src/map_io/map_wincondition_packet.h	2019-03-11 07:15:58 +0000
@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2019 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#ifndef WL_MAP_IO_MAP_WINCONDITION_PACKET_H
+#define WL_MAP_IO_MAP_WINCONDITION_PACKET_H
+
+#include "logic/map.h"
+
+namespace Widelands {
+
+/// The anything needed by win conditions
+struct MapWinconditionPacket {
+	void read(FileSystem&, Map& map, MapObjectLoader&);
+	void write(FileSystem&, Map& map, MapObjectSaver&);
+};
+}  // namespace Widelands
+
+#endif  // end of include guard: WL_MAP_IO_MAP_WINCONDITION_PACKET_H
 
=== modified file 'src/map_io/widelands_map_loader.cc'
--- src/map_io/widelands_map_loader.cc	2019-02-23 11:00:49 +0000
+++ src/map_io/widelands_map_loader.cc	2019-03-11 07:15:58 +0000
@@ -55,6 +55,7 @@
 #include "map_io/map_scripting_packet.h"
 #include "map_io/map_terrain_packet.h"
 #include "map_io/map_version_packet.h"
+#include "map_io/map_wincondition_packet.h"
 #include "map_io/tribes_legacy_lookup_table.h"
 #include "map_io/world_legacy_lookup_table.h"
 
@@ -310,6 +311,14 @@
 		}
 		log("took %ums\n ", timer.ms_since_last_query());
 
+		// Map data used by win conditions.
+		log("Reading Wincondition Data ... ");
+		{
+			MapWinconditionPacket p;
+			p.read(*fs_, *egbase.mutable_map(), *mol_);
+		}
+		log("took %ums\n ", timer.ms_since_last_query());
+
 		// Objectives. They are not needed in the Editor, since they are fully
 		// defined through Lua scripting. They are also not required for a game,
 		// since they will be only be set after it has started.
 
=== modified file 'src/scripting/lua_game.cc'
--- src/scripting/lua_game.cc	2019-02-23 11:00:49 +0000
+++ src/scripting/lua_game.cc	2019-03-11 07:15:58 +0000
@@ -173,17 +173,7 @@
       (RO) :const:`true` if this player was defeated, :const:`false` otherwise
 */
 int LuaPlayer::get_defeated(lua_State* L) {
-	Player& p = get(L, get_egbase(L));
-	bool is_defeated = true;
-
-	for (const auto& economy : p.economies()) {
-		if (!economy.second->warehouses().empty()) {
-			is_defeated = false;
-			break;
-		}
-	}
-
-	lua_pushboolean(L, is_defeated);
+	lua_pushboolean(L, get(L, get_egbase(L)).is_defeated());
 	return 1;
 }
 
 
=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc	2019-02-26 12:02:52 +0000
+++ src/scripting/lua_map.cc	2019-03-11 07:15:58 +0000
@@ -1186,9 +1186,11 @@
 */
 const char LuaMap::className[] = "Map";
 const MethodType<LuaMap> LuaMap::Methods[] = {
+   METHOD(LuaMap, count_conquerable_fields), METHOD(LuaMap, count_terrestrial_fields),
+   METHOD(LuaMap, count_owned_valuable_fields),
    METHOD(LuaMap, place_immovable), METHOD(LuaMap, get_field),
    METHOD(LuaMap, recalculate),     METHOD(LuaMap, recalculate_seafaring),
-   METHOD(LuaMap, set_port_space),  {nullptr, nullptr},
+   METHOD(LuaMap, set_port_space), {nullptr, nullptr},
 };
 const PropertyType<LuaMap> LuaMap::Properties[] = {
    PROP_RO(LuaMap, allows_seafaring),
@@ -1197,8 +1199,6 @@
    PROP_RO(LuaMap, width),
    PROP_RO(LuaMap, height),
    PROP_RO(LuaMap, player_slots),
-   PROP_RO(LuaMap, conquerable_fields),
-   PROP_RO(LuaMap, terrestrial_fields),
    {nullptr, nullptr, nullptr},
 };
 
@@ -1299,47 +1299,69 @@
 	return 1;
 }
 
-/* RST
-   .. attribute:: conquerable_fields
-
-      (RO) Calculates and returns all reachable fields that a player could build on.
-
-      **Note:** This function is expensive, so call it seldom.
-*/
-int LuaMap::get_conquerable_fields(lua_State* L) {
-	lua_newtable(L);
-	int counter = 0;
-	for (const Widelands::FCoords& fcoords :
-	     get_egbase(L).map().calculate_all_conquerable_fields()) {
-		lua_pushinteger(L, ++counter);
-		to_lua<LuaMaps::LuaField>(L, new LuaMaps::LuaField(fcoords));
-		lua_settable(L, -3);
-	}
-	return 1;
-}
-
-/* RST
-   .. attribute:: terrestrial_fields
-
-      (RO) Calculates and returns all fields that are not swimmable.
-*/
-int LuaMap::get_terrestrial_fields(lua_State* L) {
-	lua_newtable(L);
-	int counter = 0;
-	for (const Widelands::FCoords& fcoords :
-	     get_egbase(L).map().calculate_all_fields_excluding_caps(MOVECAPS_SWIM)) {
-		lua_pushinteger(L, ++counter);
-		to_lua<LuaMaps::LuaField>(L, new LuaMaps::LuaField(fcoords));
-		lua_settable(L, -3);
-	}
-	return 1;
-}
 
 /*
  ==========================================================
  LUA METHODS
  ==========================================================
  */
+
+/* RST
+   .. method:: count_conquerable_fields()
+
+      (RO) Counts all reachable fields that a player could build on.
+
+      **Note:** The fields are only calculated afresh when this is called for the first time.
+
+	  :returns: An integer with the amount of fields.
+*/
+int LuaMap::count_conquerable_fields(lua_State* L) {
+	lua_pushinteger(L, get_egbase(L).mutable_map()->count_all_conquerable_fields());
+	return 1;
+}
+
+/* RST
+   .. method:: count_terrestrial_fields()
+
+      (RO) Counts all fields that are not swimmable.
+
+	  **Note:** The fields are only calculated afresh when this is called for the first time.
+
+	  :returns: An integer with the amount of fields.
+*/
+int LuaMap::count_terrestrial_fields(lua_State* L) {
+	lua_pushinteger(L, get_egbase(L).mutable_map()->count_all_fields_excluding_caps(MOVECAPS_SWIM));
+	return 1;
+}
+
+
+/* RST
+   .. method:: count_owned_valuable_fields([immovable_attribute])
+
+      (RO) Counts the number of owned valuable fields for all players.
+
+      :arg name: *Optional*. If this is set, only count fields that have an
+        immovable with the given atttribute.
+      :type name: :class:`string`
+
+	  :returns: A table mapping player numbers to their number of owned fields.
+*/
+int LuaMap::count_owned_valuable_fields(lua_State* L) {
+	if (lua_gettop(L) > 2) {
+		report_error(L, "Does not take more than one argument.");
+	}
+	const std::string attribute = lua_gettop(L) == 2 ? luaL_checkstring(L, -1) : "";
+
+	lua_newtable(L);
+	for (const auto& fieldinfo :
+	     get_egbase(L).map().count_owned_valuable_fields(attribute)) {
+		lua_pushinteger(L, fieldinfo.first);
+		lua_pushinteger(L, fieldinfo.second);
+		lua_settable(L, -3);
+	}
+	return 1;
+}
+
 /* RST
    .. method:: place_immovable(name, field, from_where)
 
 
=== modified file 'src/scripting/lua_map.h'
--- src/scripting/lua_map.h	2019-02-24 22:50:04 +0000
+++ src/scripting/lua_map.h	2019-03-11 07:15:58 +0000
@@ -92,12 +92,13 @@
 	int get_width(lua_State*);
 	int get_height(lua_State*);
 	int get_player_slots(lua_State*);
-	int get_conquerable_fields(lua_State*);
-	int get_terrestrial_fields(lua_State*);
 
 	/*
 	 * Lua methods
 	 */
+	int count_conquerable_fields(lua_State*);
+	int count_terrestrial_fields(lua_State*);
+	int count_owned_valuable_fields(lua_State*);
 	int place_immovable(lua_State*);
 	int get_field(lua_State*);
 	int recalculate(lua_State*);
 
=== modified file 'test/maps/lua_testsuite.wmf/scripting/cfield.lua'
--- test/maps/lua_testsuite.wmf/scripting/cfield.lua	2019-02-23 09:58:39 +0000
+++ test/maps/lua_testsuite.wmf/scripting/cfield.lua	2019-03-11 07:15:58 +0000
@@ -330,13 +330,17 @@
 end
 
 function field_caps_tests:test_conquerable_fields_does_not_crash()
-   local fields = map.conquerable_fields
-   assert_equal(false, fields[1] == nil)
+   assert_equal(5028, map:count_conquerable_fields())
+
+   local owned_fields = map:count_owned_valuable_fields()
+   assert_equal(404, owned_fields[1])
 end
 
 function field_caps_tests:test_terrestrial_fields_does_not_crash()
-   local fields = map.terrestrial_fields
-   assert_equal(false, fields[1] == nil)
+   assert_equal(5028, map:count_terrestrial_fields())
+
+   local owned_fields = map:count_owned_valuable_fields("tree")
+   assert_equal(nil, owned_fields[1])
 end
 
 -- ===============