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

[SirVer]

Refactor Attackable.

- Getting rid of some multiple inheritance in Warehouse and MilitarySite by
  composing Attackable as a member into Building.
- Rename Attackable -> AttackTarget and change the interface to be cleaner.
- Mild related cleanups.

[SirVer (sirver)]

I was looking into how Warehouse works - thinking about trading. And the design of this irked me, so I improved it.

[GunChleoc (gunchleoc)]

2 tiny nits, code LGTM otherwise. Not tested yet.

And trading - yay! :)

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2227. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/234476861.
Appveyor build 2062. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_warehouse_refactor-2062.

[GunChleoc (gunchleoc)]

Tested now, so you can merge this any time :)

[SirVer (sirver)]

@bunnybot merge

=== modified file 'src/ai/defaultai_warfare.cc'
--- src/ai/defaultai_warfare.cc	2017-01-28 14:53:28 +0000
+++ src/ai/defaultai_warfare.cc	2017-05-21 11:20:39 +0000
@@ -122,7 +122,7 @@
 		// get list of immovable around this our military site
 		std::vector<ImmovableFound> immovables;
 		map.find_immovables(Area<FCoords>(f, (vision + 3 < 13) ? 13 : vision + 3), &immovables,
-		                    FindImmovableAttackable());
+		                    FindImmovableAttackTarget());
 
 		for (uint32_t j = 0; j < immovables.size(); ++j) {
 			if (upcast(MilitarySite const, bld, immovables.at(j).object)) {
@@ -229,7 +229,7 @@
 				defenders_strength = calculate_strength(defenders);
 
 				flag = &bld->base_flag();
-				if (is_visible && bld->can_attack()) {
+				if (is_visible && bld->attack_target()->can_be_attacked()) {
 					is_attackable = true;
 				}
 				owner_number = bld->owner().player_number();
@@ -244,7 +244,7 @@
 
 				flag = &Wh->base_flag();
 				is_warehouse = true;
-				if (is_visible && Wh->can_attack()) {
+				if (is_visible && Wh->attack_target()->can_be_attacked()) {
 					is_attackable = true;
 				}
 				owner_number = Wh->owner().player_number();

=== modified file 'src/economy/route.h'
--- src/economy/route.h	2017-01-25 18:55:59 +0000
+++ src/economy/route.h	2017-05-21 11:20:39 +0000
@@ -23,14 +23,16 @@
 #include <vector>
 
 #include "economy/iroute.h"
+#include "io/fileread.h"
+#include "io/filewrite.h"
 #include "logic/map_objects/map_object.h"
 
 namespace Widelands {
 
+class EditorGameBase;
+class MapObjectLoader;
 struct Flag;
-class EditorGameBase;
 struct MapObjectSaver;
-class MapObjectLoader;
 struct RoutingNode;
 
 /**

=== modified file 'src/logic/CMakeLists.txt'
--- src/logic/CMakeLists.txt	2017-05-14 18:17:16 +0000
+++ src/logic/CMakeLists.txt	2017-05-21 11:20:39 +0000
@@ -127,14 +127,13 @@
     save_handler.h
     widelands_geometry_io.cc
     widelands_geometry_io.h
-    map_objects/draw_text.h
-    map_objects/attackable.h
     map_objects/bob.cc
     map_objects/bob.h
     map_objects/buildcost.cc
     map_objects/buildcost.h
     map_objects/checkstep.cc
     map_objects/checkstep.h
+    map_objects/draw_text.h
     map_objects/immovable.cc
     map_objects/immovable.h
     map_objects/immovable_program.h
@@ -142,6 +141,7 @@
     map_objects/map_object.h
     map_objects/terrain_affinity.cc
     map_objects/terrain_affinity.h
+    map_objects/tribes/attack_target.h
     map_objects/tribes/battle.cc
     map_objects/tribes/battle.h
     map_objects/tribes/bill_of_materials.h

=== modified file 'src/logic/cmd_queue.h'
--- src/logic/cmd_queue.h	2017-01-25 18:55:59 +0000
+++ src/logic/cmd_queue.h	2017-05-21 11:20:39 +0000
@@ -27,7 +27,6 @@
 #include <stdint.h>
 
 #include "logic/queue_cmd_ids.h"
-#include <stdint.h>
 
 class FileRead;
 class FileWrite;

=== modified file 'src/logic/findimmovable.cc'
--- src/logic/findimmovable.cc	2017-01-25 18:55:59 +0000
+++ src/logic/findimmovable.cc	2017-05-21 11:20:39 +0000
@@ -21,7 +21,6 @@
 
 #include "base/macros.h"
 #include "economy/flag.h"
-#include "logic/map_objects/attackable.h"
 #include "logic/map_objects/immovable.h"
 #include "logic/map_objects/tribes/militarysite.h"
 
@@ -61,8 +60,11 @@
 	return false;
 }
 
-bool FindImmovableAttackable::accept(const BaseImmovable& imm) const {
-	return dynamic_cast<Attackable const*>(&imm);
+bool FindImmovableAttackTarget::accept(const BaseImmovable& imm) const {
+	if (upcast(Building const, b, &imm)) {
+		return b->attack_target() != nullptr;
+	}
+	return false;
 }
 
 bool FindImmovableByDescr::accept(const BaseImmovable& baseimm) const {

=== modified file 'src/logic/findimmovable.h'
--- src/logic/findimmovable.h	2017-01-25 18:55:59 +0000
+++ src/logic/findimmovable.h	2017-05-21 11:20:39 +0000
@@ -132,8 +132,8 @@
 
 	const Player& player;
 };
-struct FindImmovableAttackable {
-	FindImmovableAttackable() {
+struct FindImmovableAttackTarget {
+	FindImmovableAttackTarget() {
 	}
 
 	bool accept(const BaseImmovable&) const;

=== modified file 'src/logic/map_objects/map_object.h'
--- src/logic/map_objects/map_object.h	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/map_object.h	2017-05-21 11:20:39 +0000
@@ -491,12 +491,12 @@
 	ObjectPointer() {
 		serial_ = 0;
 	}
-	ObjectPointer(MapObject* const obj) {
+	ObjectPointer(const MapObject* const obj) {
 		serial_ = obj ? obj->serial_ : 0;
 	}
 	// can use standard copy constructor and assignment operator
 
-	ObjectPointer& operator=(MapObject* const obj) {
+	ObjectPointer& operator=(const MapObject* const obj) {
 		serial_ = obj ? obj->serial_ : 0;
 		return *this;
 	}

=== renamed file 'src/logic/map_objects/attackable.h' => 'src/logic/map_objects/tribes/attack_target.h'
--- src/logic/map_objects/attackable.h	2017-01-25 18:55:59 +0000
+++ src/logic/map_objects/tribes/attack_target.h	2017-05-21 11:20:39 +0000
@@ -17,68 +17,55 @@
  *
  */
 
-#ifndef WL_LOGIC_MAP_OBJECTS_ATTACKABLE_H
-#define WL_LOGIC_MAP_OBJECTS_ATTACKABLE_H
+#ifndef WL_LOGIC_MAP_OBJECTS_TRIBES_ATTACK_TARGET_H
+#define WL_LOGIC_MAP_OBJECTS_TRIBES_ATTACK_TARGET_H
+
+#include "base/macros.h"
 
 namespace Widelands {
 
-class Player;
 class Soldier;
 
-enum {
-	/**
-    * This is the maximum radius that a military building can protect
-    * in the sense that an enemy soldier that enters the player's territory
-    * will call \ref Attackable::aggressor if it is that close.
-    */
-	MaxProtectionRadius = 25
-};
-
-/**
- * Buildings can implement this interface to indicate that
- * they can be attacked.
- */
-struct Attackable {
-	/**
-	 * Return the player that owns this attackable.
-	 */
-	virtual Player& owner() const = 0;
-
-	/**
-	 * Determines whether this building can be attacked right now.
-	 *
-	 * This should only return false for military sites that have not
-	 * been occupied yet.
-	 */
-	virtual bool can_attack() = 0;
-
-	/**
-	 * Called by an enemy soldier that enters a node with distance
-	 * less than or equal to \ref MaxProtectionRadius from the building.
-	 *
-	 * This allows the building to send protective forces to intercept
-	 * the soldier.
-	 */
-	virtual void aggressor(Soldier&) = 0;
-
-	/**
-	 * Called by a soldier who is standing on the building's flag
-	 * to attack the building.
-	 *
-	 * The building must send a soldier for defense, and return \c true.
-	 * Otherwise, i.e. if the building cannot defend itself anymore,
-	 * it must destroy itself or turn over to the attacking player,
-	 * and return \c false.
-	 *
-	 * \return \c true if a soldier was launched in defense of the building,
-	 * or \c false if the building cannot defend itself any longer.
-	 */
-	virtual bool attack(Soldier&) = 0;
-
-protected:
-	virtual ~Attackable() {
+// This is the maximum radius that a military building can protect in the sense
+// that an enemy soldier that enters the player's territory will call \ref
+// AttackTarget::enemy_soldier_approaches if it is that close.
+constexpr int kMaxProtectionRadius = 25;
+
+class AttackTarget {
+public:
+	AttackTarget() = default;
+	virtual ~AttackTarget() {
 	}
+
+	// Determines whether this building can be attacked right now.
+	virtual bool can_be_attacked() const = 0;
+
+	// Called by an enemy soldier that enters a node with distance
+	// less than or equal to \ref kMaxProtectionRadius from the building.
+	//
+	// This allows the building to send protective forces to intercept
+	// the soldier.
+	virtual void enemy_soldier_approaches(const Soldier& enemy) const = 0;
+
+	// Called by a soldier who is standing on the building's flag
+	// to attack the building.
+	//
+	// The building must send a soldier for defense, and return \c true.
+	// Otherwise, i.e. if the building cannot defend itself anymore,
+	// it must destroy itself or turn over to the attacking player,
+	// and return \c false.
+	enum class AttackResult {
+		// Returned when a soldier was launched in defense of the building.
+		DefenderLaunched,
+
+		// Returned if the building cannot defend itself any longer.
+		Defenseless
+	};
+	virtual AttackResult attack(Soldier* attacker) const = 0;
+
+private:
+	DISALLOW_COPY_AND_ASSIGN(AttackTarget);
 };
 }
 
-#endif  // end of include guard: WL_LOGIC_MAP_OBJECTS_ATTACKABLE_H
+#endif  // end of include guard: WL_LOGIC_MAP_OBJECTS_TRIBES_ATTACK_TARGET_H

=== modified file 'src/logic/map_objects/tribes/building.cc'
--- src/logic/map_objects/tribes/building.cc	2017-05-09 09:18:14 +0000
+++ src/logic/map_objects/tribes/building.cc	2017-05-21 11:20:39 +0000
@@ -235,7 +235,8 @@
      animstart_(0),
      leave_time_(0),
      defeating_player_(0),
-     seeing_(false) {
+     seeing_(false),
+     attack_target_(nullptr) {
 }
 
 void Building::load_finish(EditorGameBase& egbase) {
@@ -692,6 +693,11 @@
 	Notifications::publish(NoteBuilding(serial(), NoteBuilding::Action::kWorkersChanged));
 }
 
+void Building::set_attack_target(AttackTarget* new_attack_target) {
+	assert(attack_target_ == nullptr);
+	attack_target_ = new_attack_target;
+}
+
 /**
  * Change whether this building sees its vision range based on workers
  * inside the building.

=== modified file 'src/logic/map_objects/tribes/building.h'
--- src/logic/map_objects/tribes/building.h	2017-05-06 19:21:47 +0000
+++ src/logic/map_objects/tribes/building.h	2017-05-21 11:20:39 +0000
@@ -30,6 +30,7 @@
 #include "base/macros.h"
 #include "logic/map_objects/buildcost.h"
 #include "logic/map_objects/immovable.h"
+#include "logic/map_objects/tribes/attack_target.h"
 #include "logic/map_objects/tribes/bill_of_materials.h"
 #include "logic/map_objects/tribes/wareworker.h"
 #include "logic/map_objects/tribes/workarea_info.h"
@@ -293,6 +294,13 @@
 	void add_worker(Worker&) override;
 	void remove_worker(Worker&) override;
 
+	// Returns the AttackTarget object associated with this building. If the
+	// building can never be attacked (for example productionsites) this will be
+	// nullptr.
+	const AttackTarget* attack_target() const {
+		return attack_target_;
+	}
+
 	void send_message(Game& game,
 	                  const Message::Type msgtype,
 	                  const std::string& title,
@@ -324,6 +332,7 @@
 	draw_info(TextToDraw draw_text, const Vector2f& point_on_dst, float scale, RenderTarget* dst);
 
 	void set_seeing(bool see);
+	void set_attack_target(AttackTarget* new_attack_target);
 
 	Coords position_;
 	Flag* flag_;
@@ -350,6 +359,7 @@
 
 private:
 	std::string statistics_string_;
+	AttackTarget* attack_target_;  // owned by the base classes, set by 'set_attack_target'.
 };
 }
 

=== modified file 'src/logic/map_objects/tribes/militarysite.cc'
--- src/logic/map_objects/tribes/militarysite.cc	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/militarysite.cc	2017-05-21 11:20:39 +0000
@@ -43,6 +43,145 @@
 
 namespace Widelands {
 
+bool MilitarySite::AttackTarget::can_be_attacked() const {
+	return military_site_->didconquer_;
+}
+
+void MilitarySite::AttackTarget::enemy_soldier_approaches(const Soldier& enemy) const {
+	auto& owner = military_site_->owner();
+	Game& game = dynamic_cast<Game&>(owner.egbase());
+	Map& map = game.map();
+	if (enemy.get_owner() == &owner || enemy.get_battle() ||
+	    military_site_->descr().get_conquers() <=
+	       map.calc_distance(enemy.get_position(), military_site_->get_position()))
+		return;
+
+	if (map.find_bobs(Area<FCoords>(map.get_fcoords(military_site_->base_flag().get_position()), 2),
+	                  nullptr, FindBobEnemySoldier(&owner)))
+		return;
+
+	// We're dealing with a soldier that we might want to keep busy
+	// Now would be the time to implement some player-definable
+	// policy as to how many soldiers are allowed to leave as defenders
+	std::vector<Soldier*> present = military_site_->present_soldiers();
+
+	if (1 < present.size()) {
+		for (Soldier* temp_soldier : present) {
+			if (!military_site_->has_soldier_job(*temp_soldier)) {
+				SoldierJob sj;
+				sj.soldier = temp_soldier;
+				sj.enemy = &enemy;
+				sj.stayhome = false;
+				military_site_->soldierjobs_.push_back(sj);
+				temp_soldier->update_task_buildingwork(game);
+				return;
+			}
+		}
+	}
+
+	// Inform the player, that we are under attack by adding a new entry to the
+	// message queue - a sound will automatically be played.
+	military_site_->notify_player(game, true);
+}
+
+AttackTarget::AttackResult MilitarySite::AttackTarget::attack(Soldier* enemy) const {
+	Game& game = dynamic_cast<Game&>(military_site_->owner().egbase());
+
+	std::vector<Soldier*> present = military_site_->present_soldiers();
+	Soldier* defender = nullptr;
+
+	if (!present.empty()) {
+		// Find soldier with greatest health
+		uint32_t current_max = 0;
+		for (Soldier* temp_soldier : present) {
+			if (temp_soldier->get_current_health() > current_max) {
+				defender = temp_soldier;
+				current_max = defender->get_current_health();
+			}
+		}
+	} else {
+		// If one of our stationed soldiers is currently walking into the
+		// building, give us another chance.
+		std::vector<Soldier*> stationed = military_site_->stationed_soldiers();
+		for (Soldier* temp_soldier : stationed) {
+			if (temp_soldier->get_position() == military_site_->get_position()) {
+				defender = temp_soldier;
+				break;
+			}
+		}
+	}
+
+	if (defender) {
+		military_site_->pop_soldier_job(defender);  // defense overrides all other jobs
+
+		SoldierJob sj;
+		sj.soldier = defender;
+		sj.enemy = enemy;
+		sj.stayhome = true;
+		military_site_->soldierjobs_.push_back(sj);
+
+		defender->update_task_buildingwork(game);
+
+		// Inform the player, that we are under attack by adding a new entry to
+		// the message queue - a sound will automatically be played.
+		military_site_->notify_player(game);
+
+		return AttackTarget::AttackResult::DefenderLaunched;
+	}
+
+	// The enemy has defeated our forces, we should inform the player
+	const Coords coords = military_site_->get_position();
+	{
+		military_site_->send_message(game, Message::Type::kWarfareSiteLost,
+		                             /** TRANSLATORS: Militarysite lost (taken/destroyed by enemy) */
+		                             pgettext("building", "Lost!"),
+		                             military_site_->descr().icon_filename(), _("Militarysite lost!"),
+		                             military_site_->descr().defeated_enemy_str_, false);
+	}
+
+	// Now let's see whether the enemy conquers our militarysite, or whether
+	// we still hold the bigger military presence in that area (e.g. if there
+	// is a fortress one or two points away from our sentry, the fortress has
+	// a higher presence and thus the enemy can just burn down the sentry.
+	if (military_site_->military_presence_kept(game)) {
+		// Okay we still got the higher military presence, so the attacked
+		// militarysite will be destroyed.
+		military_site_->set_defeating_player(enemy->owner().player_number());
+		military_site_->schedule_destroy(game);
+		return AttackTarget::AttackResult::Defenseless;
+	}
+
+	// The enemy conquers the building
+	// In fact we do not conquer it, but place a new building of same type at
+	// the old location.
+
+	Building::FormerBuildings former_buildings = military_site_->old_buildings_;
+
+	// The enemy conquers the building
+	// In fact we do not conquer it, but place a new building of same type at
+	// the old location.
+	Player* enemyplayer = enemy->get_owner();
+
+	// Now we destroy the old building before we place the new one.
+	military_site_->set_defeating_player(enemyplayer->player_number());
+	military_site_->schedule_destroy(game);
+
+	enemyplayer->force_building(coords, former_buildings);
+	BaseImmovable* const newimm = game.map()[coords].get_immovable();
+	upcast(MilitarySite, newsite, newimm);
+	newsite->reinit_after_conqueration(game);
+
+	// Of course we should inform the victorious player as well
+	newsite->send_message(
+	   game, Message::Type::kWarfareSiteDefeated,
+	   /** TRANSLATORS: Message title. */
+	   /** TRANSLATORS: If you run out of space, you can also translate this as "Success!" */
+	   _("Enemy Defeated!"), newsite->descr().icon_filename(), _("Enemy at site defeated!"),
+	   newsite->descr().defeated_you_str_, true);
+
+	return AttackTarget::AttackResult::Defenseless;
+}
+
 /**
   * The contents of 'table' are documented in
   * /data/tribes/buildings/militarysites/atlanteans/castle/init.lua
@@ -91,6 +230,7 @@
 
 MilitarySite::MilitarySite(const MilitarySiteDescr& ms_descr)
    : Building(ms_descr),
+     attack_target_(this),
      didconquer_(false),
      capacity_(ms_descr.get_max_number_of_soldiers()),
      nexthealtime_(0),
@@ -98,6 +238,7 @@
      soldier_upgrade_try_(false),
      doing_upgrade_request_(false) {
 	next_swap_soldiers_time_ = 0;
+	set_attack_target(&attack_target_);
 }
 
 MilitarySite::~MilitarySite() {
@@ -684,142 +825,6 @@
 	didconquer_ = true;
 }
 
-bool MilitarySite::can_attack() {
-	return didconquer_;
-}
-
-void MilitarySite::aggressor(Soldier& enemy) {
-	Game& game = dynamic_cast<Game&>(owner().egbase());
-	Map& map = game.map();
-	if (enemy.get_owner() == &owner() || enemy.get_battle() ||
-	    descr().get_conquers() <= map.calc_distance(enemy.get_position(), get_position()))
-		return;
-
-	if (map.find_bobs(Area<FCoords>(map.get_fcoords(base_flag().get_position()), 2), nullptr,
-	                  FindBobEnemySoldier(&owner())))
-		return;
-
-	// We're dealing with a soldier that we might want to keep busy
-	// Now would be the time to implement some player-definable
-	// policy as to how many soldiers are allowed to leave as defenders
-	std::vector<Soldier*> present = present_soldiers();
-
-	if (1 < present.size()) {
-		for (Soldier* temp_soldier : present) {
-			if (!has_soldier_job(*temp_soldier)) {
-				SoldierJob sj;
-				sj.soldier = temp_soldier;
-				sj.enemy = &enemy;
-				sj.stayhome = false;
-				soldierjobs_.push_back(sj);
-				temp_soldier->update_task_buildingwork(game);
-				return;
-			}
-		}
-	}
-
-	// Inform the player, that we are under attack by adding a new entry to the
-	// message queue - a sound will automatically be played.
-	notify_player(game, true);
-}
-
-bool MilitarySite::attack(Soldier& enemy) {
-	Game& game = dynamic_cast<Game&>(owner().egbase());
-
-	std::vector<Soldier*> present = present_soldiers();
-	Soldier* defender = nullptr;
-
-	if (!present.empty()) {
-		// Find soldier with greatest health
-		uint32_t current_max = 0;
-		for (Soldier* temp_soldier : present) {
-			if (temp_soldier->get_current_health() > current_max) {
-				defender = temp_soldier;
-				current_max = defender->get_current_health();
-			}
-		}
-	} else {
-		// If one of our stationed soldiers is currently walking into the
-		// building, give us another chance.
-		std::vector<Soldier*> stationed = stationed_soldiers();
-		for (Soldier* temp_soldier : stationed) {
-			if (temp_soldier->get_position() == get_position()) {
-				defender = temp_soldier;
-				break;
-			}
-		}
-	}
-
-	if (defender) {
-		pop_soldier_job(defender);  // defense overrides all other jobs
-
-		SoldierJob sj;
-		sj.soldier = defender;
-		sj.enemy = &enemy;
-		sj.stayhome = true;
-		soldierjobs_.push_back(sj);
-
-		defender->update_task_buildingwork(game);
-
-		// Inform the player, that we are under attack by adding a new entry to
-		// the message queue - a sound will automatically be played.
-		notify_player(game);
-
-		return true;
-	} else {
-		// The enemy has defeated our forces, we should inform the player
-		const Coords coords = get_position();
-		{
-			send_message(game, Message::Type::kWarfareSiteLost,
-			             /** TRANSLATORS: Militarysite lost (taken/destroyed by enemy) */
-			             pgettext("building", "Lost!"), descr().icon_filename(),
-			             _("Militarysite lost!"), descr().defeated_enemy_str_, false);
-		}
-
-		// Now let's see whether the enemy conquers our militarysite, or whether
-		// we still hold the bigger military presence in that area (e.g. if there
-		// is a fortress one or two points away from our sentry, the fortress has
-		// a higher presence and thus the enemy can just burn down the sentry.
-		if (military_presence_kept(game)) {
-			// Okay we still got the higher military presence, so the attacked
-			// militarysite will be destroyed.
-			set_defeating_player(enemy.owner().player_number());
-			schedule_destroy(game);
-			return false;
-		}
-
-		// The enemy conquers the building
-		// In fact we do not conquer it, but place a new building of same type at
-		// the old location.
-
-		Building::FormerBuildings former_buildings = old_buildings_;
-
-		// The enemy conquers the building
-		// In fact we do not conquer it, but place a new building of same type at
-		// the old location.
-		Player* enemyplayer = enemy.get_owner();
-
-		// Now we destroy the old building before we place the new one.
-		set_defeating_player(enemyplayer->player_number());
-		schedule_destroy(game);
-
-		enemyplayer->force_building(coords, former_buildings);
-		BaseImmovable* const newimm = game.map()[coords].get_immovable();
-		upcast(MilitarySite, newsite, newimm);
-		newsite->reinit_after_conqueration(game);
-
-		// Of course we should inform the victorious player as well
-		newsite->send_message(
-		   game, Message::Type::kWarfareSiteDefeated,
-		   /** TRANSLATORS: Message title. */
-		   /** TRANSLATORS: If you run out of space, you can also translate this as "Success!" */
-		   _("Enemy Defeated!"), newsite->descr().icon_filename(), _("Enemy at site defeated!"),
-		   newsite->descr().defeated_you_str_, true);
-
-		return false;
-	}
-}
-
 /// Initialises the militarysite after it was "conquered" (the old was replaced)
 void MilitarySite::reinit_after_conqueration(Game& game) {
 	clear_requirements();

=== modified file 'src/logic/map_objects/tribes/militarysite.h'
--- src/logic/map_objects/tribes/militarysite.h	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/militarysite.h	2017-05-21 11:20:39 +0000
@@ -24,7 +24,6 @@
 
 #include "base/macros.h"
 #include "economy/request.h"
-#include "logic/map_objects/attackable.h"
 #include "logic/map_objects/tribes/building.h"
 #include "logic/map_objects/tribes/requirements.h"
 #include "logic/map_objects/tribes/soldiercontrol.h"
@@ -69,7 +68,7 @@
 	DISALLOW_COPY_AND_ASSIGN(MilitarySiteDescr);
 };
 
-class MilitarySite : public Building, public SoldierControl, public Attackable {
+class MilitarySite : public Building, public SoldierControl {
 	friend class MapBuildingdataPacket;
 	MO_DESCR(MilitarySiteDescr)
 
@@ -102,15 +101,6 @@
 	void drop_soldier(Soldier&) override;
 	int incorporate_soldier(EditorGameBase& game, Soldier& s) override;
 
-	// Begin implementation of Attackable
-	Player& owner() const override {
-		return Building::owner();
-	}
-	bool can_attack() override;
-	void aggressor(Soldier&) override;
-	bool attack(Soldier&) override;
-	// End implementation of Attackable
-
 	/// Launch the given soldier on an attack towards the given
 	/// target building.
 	void send_attacker(Soldier&, Building&);
@@ -153,6 +143,21 @@
 	bool drop_least_suited_soldier(bool new_has_arrived, Soldier* s);
 
 private:
+	// We can be attacked if we have stationed soldiers.
+	class AttackTarget : public Widelands::AttackTarget {
+	public:
+		explicit AttackTarget(MilitarySite* military_site) : military_site_(military_site) {
+		}
+
+		bool can_be_attacked() const override;
+		void enemy_soldier_approaches(const Soldier&) const override;
+		Widelands::AttackTarget::AttackResult attack(Soldier*) const override;
+
+	private:
+		MilitarySite* const military_site_;
+	};
+
+	AttackTarget attack_target_;
 	Requirements soldier_requirements_;  // This is used to grab a bunch of soldiers: Anything goes
 	RequireAttribute soldier_upgrade_requirements_;     // This is used when exchanging soldiers.
 	std::unique_ptr<Request> normal_soldier_request_;   // filling the site

=== modified file 'src/logic/map_objects/tribes/soldier.cc'
--- src/logic/map_objects/tribes/soldier.cc	2017-05-13 18:48:26 +0000
+++ src/logic/map_objects/tribes/soldier.cc	2017-05-21 11:20:39 +0000
@@ -42,7 +42,6 @@
 #include "logic/game.h"
 #include "logic/game_controller.h"
 #include "logic/game_data_error.h"
-#include "logic/map_objects/attackable.h"
 #include "logic/map_objects/checkstep.h"
 #include "logic/map_objects/tribes/battle.h"
 #include "logic/map_objects/tribes/building.h"
@@ -602,7 +601,7 @@
 	return false;
 }
 
-Battle* Soldier::get_battle() {
+Battle* Soldier::get_battle() const {
 	return battle_;
 }
 
@@ -902,12 +901,14 @@
 		}
 	}
 
-	upcast(Attackable, attackable, enemy);
-	assert(attackable);
+	assert(enemy->attack_target() != nullptr);
 
 	molog("[attack] attacking target building\n");
 	//  give the enemy soldier some time to act
-	schedule_act(game, attackable->attack(*this) ? 1000 : 10);
+	schedule_act(
+	   game, enemy->attack_target()->attack(this) == AttackTarget::AttackResult::DefenderLaunched ?
+	            1000 :
+	            10);
 }
 
 void Soldier::attack_pop(Game& game, State&) {
@@ -1520,20 +1521,21 @@
 	PlayerNumber const land_owner = get_position().field->get_owned_by();
 	// First check if the soldier is standing on someone else's territory
 	if (land_owner != owner().player_number()) {
-		// Let's collect all reachable attackable sites in vicinity (militarysites mainly)
-		std::vector<BaseImmovable*> attackables;
+		// Let's collect all reachable attack_target sites in vicinity (militarysites mainly)
+		std::vector<BaseImmovable*> attack_targets;
 		game.map().find_reachable_immovables_unique(
-		   Area<FCoords>(get_position(), MaxProtectionRadius), attackables,
-		   CheckStepWalkOn(descr().movecaps(), false), FindImmovableAttackable());
+		   Area<FCoords>(get_position(), kMaxProtectionRadius), attack_targets,
+		   CheckStepWalkOn(descr().movecaps(), false), FindImmovableAttackTarget());
 
-		for (BaseImmovable* temp_attackable : attackables) {
-			const Player* attackable_player =
-			   dynamic_cast<const PlayerImmovable&>(*temp_attackable).get_owner();
+		for (BaseImmovable* temp_attack_target : attack_targets) {
+			Building* building = dynamic_cast<Building*>(temp_attack_target);
+			assert(building != nullptr && building->attack_target() != nullptr);
+			const Player& attack_target_player = building->owner();
 			// Let's inform the site that this (=enemy) soldier is nearby and within the site's owner's
 			// territory
-			if (attackable_player->player_number() == land_owner &&
-			    attackable_player->is_hostile(*get_owner())) {
-				dynamic_cast<Attackable&>(*temp_attackable).aggressor(*this);
+			if (attack_target_player.player_number() == land_owner &&
+			    attack_target_player.is_hostile(*get_owner())) {
+				building->attack_target()->enemy_soldier_approaches(*this);
 			}
 		}
 	}

=== modified file 'src/logic/map_objects/tribes/soldier.h'
--- src/logic/map_objects/tribes/soldier.h	2017-05-13 13:14:29 +0000
+++ src/logic/map_objects/tribes/soldier.h	2017-05-21 11:20:39 +0000
@@ -262,7 +262,7 @@
 
 	bool is_on_battlefield();
 	bool is_attacking_player(Game&, Player&);
-	Battle* get_battle();
+	Battle* get_battle() const;
 	bool can_be_challenged();
 	bool check_node_blocked(Game&, const FCoords&, bool commit) override;
 

=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
--- src/logic/map_objects/tribes/warehouse.cc	2017-05-03 10:34:46 +0000
+++ src/logic/map_objects/tribes/warehouse.cc	2017-05-21 11:20:39 +0000
@@ -69,6 +69,57 @@
 
 }  // namespace
 
+bool Warehouse::AttackTarget::can_be_attacked() const {
+	return warehouse_->descr().get_conquers() > 0;
+}
+
+void Warehouse::AttackTarget::enemy_soldier_approaches(const Soldier& enemy) const {
+	if (!warehouse_->descr().get_conquers())
+		return;
+
+	Player& owner = warehouse_->owner();
+	Game& game = dynamic_cast<Game&>(owner.egbase());
+	Map& map = game.map();
+	if (enemy.get_owner() == &owner || enemy.get_battle() ||
+	    warehouse_->descr().get_conquers() <=
+	       map.calc_distance(enemy.get_position(), warehouse_->get_position()))
+		return;
+
+	if (game.map().find_bobs(
+	       Area<FCoords>(map.get_fcoords(warehouse_->base_flag().get_position()), 2), nullptr,
+	       FindBobEnemySoldier(&owner)))
+		return;
+
+	DescriptionIndex const soldier_index = owner.tribe().soldier();
+	Requirements noreq;
+
+	if (!warehouse_->count_workers(game, soldier_index, noreq, Match::kCompatible))
+		return;
+
+	Soldier& defender =
+	   dynamic_cast<Soldier&>(warehouse_->launch_worker(game, soldier_index, noreq));
+	defender.start_task_defense(game, false);
+}
+
+AttackTarget::AttackResult Warehouse::AttackTarget::attack(Soldier* enemy) const {
+	Player& owner = warehouse_->owner();
+	Game& game = dynamic_cast<Game&>(owner.egbase());
+	DescriptionIndex const soldier_index = owner.tribe().soldier();
+	Requirements noreq;
+
+	if (warehouse_->count_workers(game, soldier_index, noreq, Match::kCompatible)) {
+		Soldier& defender =
+		   dynamic_cast<Soldier&>(warehouse_->launch_worker(game, soldier_index, noreq));
+		defender.start_task_defense(game, true);
+		enemy->send_signal(game, "sleep");
+		return AttackTarget::AttackResult::DefenderLaunched;
+	}
+
+	warehouse_->set_defeating_player(enemy->owner().player_number());
+	warehouse_->schedule_destroy(game);
+	return AttackTarget::AttackResult::Defenseless;
+}
+
 WarehouseSupply::~WarehouseSupply() {
 	if (economy_) {
 		log("WarehouseSupply::~WarehouseSupply: Warehouse %u still belongs to "
@@ -260,11 +311,13 @@
 
 Warehouse::Warehouse(const WarehouseDescr& warehouse_descr)
    : Building(warehouse_descr),
+     attack_target_(this),
      supply_(new WarehouseSupply(this)),
      next_military_act_(0),
      portdock_(nullptr) {
 	next_stock_remove_act_ = 0;
 	cleanup_in_progress_ = false;
+	set_attack_target(&attack_target_);
 }
 
 Warehouse::~Warehouse() {
@@ -1165,51 +1218,6 @@
 	next_worker_without_cost_spawn_[worker_types_without_cost_index] = never();
 }
 
-bool Warehouse::can_attack() {
-	return descr().get_conquers() > 0;
-}
-
-void Warehouse::aggressor(Soldier& enemy) {
-	if (!descr().get_conquers())
-		return;
-
-	Game& game = dynamic_cast<Game&>(owner().egbase());
-	Map& map = game.map();
-	if (enemy.get_owner() == &owner() || enemy.get_battle() ||
-	    descr().get_conquers() <= map.calc_distance(enemy.get_position(), get_position()))
-		return;
-
-	if (game.map().find_bobs(Area<FCoords>(map.get_fcoords(base_flag().get_position()), 2), nullptr,
-	                         FindBobEnemySoldier(&owner())))
-		return;
-
-	DescriptionIndex const soldier_index = owner().tribe().soldier();
-	Requirements noreq;
-
-	if (!count_workers(game, soldier_index, noreq, Match::kCompatible))
-		return;
-
-	Soldier& defender = dynamic_cast<Soldier&>(launch_worker(game, soldier_index, noreq));
-	defender.start_task_defense(game, false);
-}
-
-bool Warehouse::attack(Soldier& enemy) {
-	Game& game = dynamic_cast<Game&>(owner().egbase());
-	DescriptionIndex const soldier_index = owner().tribe().soldier();
-	Requirements noreq;
-
-	if (count_workers(game, soldier_index, noreq, Match::kCompatible)) {
-		Soldier& defender = dynamic_cast<Soldier&>(launch_worker(game, soldier_index, noreq));
-		defender.start_task_defense(game, true);
-		enemy.send_signal(game, "sleep");
-		return true;
-	}
-
-	set_defeating_player(enemy.owner().player_number());
-	schedule_destroy(game);
-	return false;
-}
-
 void Warehouse::PlannedWorkers::cleanup() {
 	while (!requests.empty()) {
 		delete requests.back();

=== modified file 'src/logic/map_objects/tribes/warehouse.h'
--- src/logic/map_objects/tribes/warehouse.h	2017-04-23 12:11:19 +0000
+++ src/logic/map_objects/tribes/warehouse.h	2017-05-21 11:20:39 +0000
@@ -24,7 +24,6 @@
 #include "base/wexception.h"
 #include "economy/request.h"
 #include "economy/wares_queue.h"
-#include "logic/map_objects/attackable.h"
 #include "logic/map_objects/tribes/building.h"
 #include "logic/map_objects/tribes/soldiercontrol.h"
 #include "logic/map_objects/tribes/wareworker.h"
@@ -71,7 +70,7 @@
 	DISALLOW_COPY_AND_ASSIGN(WarehouseDescr);
 };
 
-class Warehouse : public Building, public Attackable, public SoldierControl {
+class Warehouse : public Building, public SoldierControl {
 	friend class PortDock;
 	friend class MapBuildingdataPacket;
 
@@ -221,15 +220,6 @@
 	void enable_spawn(Game&, uint8_t worker_types_without_cost_index);
 	void disable_spawn(uint8_t worker_types_without_cost_index);
 
-	// Begin Attackable implementation
-	Player& owner() const override {
-		return Building::owner();
-	}
-	bool can_attack() override;
-	void aggressor(Soldier&) override;
-	bool attack(Soldier&) override;
-	// End Attackable implementation
-
 	void receive_ware(Game&, DescriptionIndex ware) override;
 	void receive_worker(Game&, Worker& worker) override;
 
@@ -250,13 +240,26 @@
 
 	void log_general_info(const EditorGameBase&) override;
 
-protected:
+private:
+	// A warehouse that conquers space can also be attacked.
+	class AttackTarget : public Widelands::AttackTarget {
+	public:
+		AttackTarget(Warehouse* warehouse) : warehouse_(warehouse) {
+		}
+
+		bool can_be_attacked() const override;
+		void enemy_soldier_approaches(const Soldier&) const override;
+		Widelands::AttackTarget::AttackResult attack(Soldier*) const override;
+
+	private:
+		Warehouse* const warehouse_;
+	};
+
+	void init_portdock(EditorGameBase& egbase);
+
 	/// Initializes the container sizes for the owner's tribe.
 	void init_containers(Player& owner);
 
-private:
-	void init_portdock(EditorGameBase& egbase);
-
 	/**
 	 * Plan to produce a certain worker type in this warehouse. This means
 	 * requesting all the necessary wares, if multiple different wares types are
@@ -282,6 +285,7 @@
 	void update_planned_workers(Game&, PlannedWorkers& pw);
 	void update_all_planned_workers(Game&);
 
+	AttackTarget attack_target_;
 	WarehouseSupply* supply_;
 
 	std::vector<StockPolicy> ware_policy_;

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2017-04-22 05:35:46 +0000
+++ src/logic/player.cc	2017-05-21 11:20:39 +0000
@@ -452,8 +452,8 @@
 		log("Clearing for road at (%i, %i)\n", c.x, c.y);
 
 		//  Make sure that the player owns the area around.
-		dynamic_cast<Game&>(egbase())
-		   .conquer_area_no_building(PlayerArea<Area<FCoords>>(player_number(), Area<FCoords>(c, 1)));
+		dynamic_cast<Game&>(egbase()).conquer_area_no_building(
+		   PlayerArea<Area<FCoords>>(player_number(), Area<FCoords>(c, 1)));
 
 		if (BaseImmovable* const immovable = c.field->get_immovable()) {
 			assert(immovable != &start);
@@ -869,8 +869,8 @@
 		log("enemyflagaction: count is 0\n");
 	else if (is_hostile(flag.owner())) {
 		if (Building* const building = flag.get_building()) {
-			if (upcast(Attackable, attackable, building)) {
-				if (attackable->can_attack()) {
+			if (const AttackTarget* attack_target = building->attack_target()) {
+				if (attack_target->can_be_attacked()) {
 					std::vector<Soldier*> attackers;
 					find_attack_soldiers(flag, &attackers, count);
 					assert(attackers.size() <= count);

=== modified file 'src/wui/attack_box.h'
--- src/wui/attack_box.h	2017-02-23 17:58:25 +0000
+++ src/wui/attack_box.h	2017-05-21 11:20:39 +0000
@@ -26,7 +26,6 @@
 #include "graphic/font_handler1.h"
 #include "graphic/text/font_set.h"
 #include "graphic/text_constants.h"
-#include "logic/map_objects/attackable.h"
 #include "logic/map_objects/bob.h"
 #include "logic/map_objects/tribes/soldier.h"
 #include "logic/player.h"

=== modified file 'src/wui/fieldaction.cc'
--- src/wui/fieldaction.cc	2017-02-27 19:10:24 +0000
+++ src/wui/fieldaction.cc	2017-05-21 11:20:39 +0000
@@ -26,7 +26,7 @@
 #include "economy/road.h"
 #include "graphic/graphic.h"
 #include "logic/cmd_queue.h"
-#include "logic/map_objects/attackable.h"
+#include "logic/map_objects/tribes/attack_target.h"
 #include "logic/map_objects/tribes/soldier.h"
 #include "logic/map_objects/tribes/tribe_descr.h"
 #include "logic/map_objects/tribes/warehouse.h"
@@ -380,13 +380,16 @@
 void FieldActionWindow::add_buttons_attack() {
 	UI::Box& a_box = *new UI::Box(&tabpanel_, 0, 0, UI::Box::Horizontal);
 
-	if (upcast(Widelands::Attackable, attackable, map_->get_immovable(node_))) {
-		if (player_ && player_->is_hostile(attackable->owner()) && attackable->can_attack()) {
-			attack_box_ = new AttackBox(&a_box, player_, &node_, 0, 0);
-			a_box.add(attack_box_);
+	if (upcast(Widelands::Building, building, map_->get_immovable(node_))) {
+		if (const Widelands::AttackTarget* attack_target = building->attack_target()) {
+			if (player_ && player_->is_hostile(building->owner()) &&
+			    attack_target->can_be_attacked()) {
+				attack_box_ = new AttackBox(&a_box, player_, &node_, 0, 0);
+				a_box.add(attack_box_);
 
-			set_fastclick_panel(&add_button(
-			   &a_box, "attack", pic_attack, &FieldActionWindow::act_attack, _("Start attack")));
+				set_fastclick_panel(&add_button(
+				   &a_box, "attack", pic_attack, &FieldActionWindow::act_attack, _("Start attack")));
+			}
 		}
 	}