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

[Nicolai Hähnle]

I'm going to be super-consistent about this now and just put up a merge proposal even for a small change like this. I'm curious how that plays out as a process?

[SirVer (sirver)]

:). I do not really know myself where to draw the line - it costs the reviewer very little to read a few lines but having the delay in the push process is sometimes annoying for the coder. I pushed some bug fixes yesterday also without reviews... so, I just don't know.

if (Building * building = flag->get_building()) -> I once was bitten by a refactoring bug which didn't handle assignements in if correctly Since them I prefer to split this out into two lines - just to be safe. Your call.
FindImmovable finder_; -> Can be const I think.
CheckStepWalkOn is now CheckStepDefault -> Can you elaborate? I think this changes the semantics of when you can attack, or not?

otherwise lgtm.

review: Approve

[Nicolai Hähnle (nha)]

Well, this time certainly helped make the code a little neater, if only in a small detail. And it might also encourage people to read each other's code more, which could increase the bus count ;-)

I am going to change the const thing.

As for the variable declaration + assignment in the if clause: I actually like this pattern because it neatly puts the scope exactly where it makes the most sense. Can you explain what went wrong with refactoring?

CheckStepWalkOn used to be necessary because the code searched for buildings. Fields with buildings don't have MOVECAPS_WALK, so the only way the old code could ever have found any buildings at all is by allowing a special case exception on the last step of the search. Now that the code searches for flags, which are walkable by definition, this is no longer necessary.

Now that I think about it, this might also fix some weirdness in those already buggy cases when a military sites seals a narrow passage off completely: The findAttackSoldiers code may have decided that an attack in northwestern direction is possible (because it searches from the attackees flag and hits one of the fields of the military sites that seals passage), but when the attack is launched, the soldiers would fail to find a way. The new code guarantees that there is a path for the soldiers to follow.

=== modified file 'src/logic/findimmovable.cc'
--- src/logic/findimmovable.cc	2013-02-10 19:36:24 +0000
+++ src/logic/findimmovable.cc	2013-02-12 10:17:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2009 by the Widelands Development Team
+ * Copyright (C) 2008-2013 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
@@ -24,6 +24,8 @@
 #include "militarysite.h"
 #include "upcast.h"
 
+#include "economy/flag.h"
+
 namespace Widelands {
 
 struct FindImmovableAlwaysTrueImpl {
@@ -71,4 +73,14 @@
 	return false;
 }
 
+bool FindFlagOf::accept(const BaseImmovable & baseimm) const {
+	if (upcast(const Flag, flag, &baseimm)) {
+		if (Building * building = flag->get_building()) {
+			if (finder_.accept(*building))
+				return true;
+		}
+	}
+	return false;
 }
+
+} // namespace Widelands

=== modified file 'src/logic/findimmovable.h'
--- src/logic/findimmovable.h	2013-02-10 19:36:24 +0000
+++ src/logic/findimmovable.h	2013-02-12 10:17:23 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2009 by the Widelands Development Team
+ * Copyright (C) 2008-2013 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
@@ -131,8 +131,15 @@
 
 	const Immovable_Descr & descr;
 };
-
-
-}
+struct FindFlagOf {
+	FindFlagOf(const FindImmovable & finder) : finder_(finder) {}
+
+	bool accept(const BaseImmovable &) const;
+
+	const FindImmovable finder_;
+};
+
+
+} // namespace Widelands
 
 #endif

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2013-02-10 19:36:24 +0000
+++ src/logic/player.cc	2013-02-12 10:17:23 +0000
@@ -797,22 +797,22 @@
 		soldiers->clear();
 
 	Map & map = egbase().map();
-	std::vector<BaseImmovable *> immovables;
+	std::vector<BaseImmovable *> flags;
 
 	map.find_reachable_immovables_unique
 		(Area<FCoords>(map.get_fcoords(flag.get_position()), 25),
-		 immovables,
-		 CheckStepWalkOn(MOVECAPS_WALK, false),
-		 FindImmovablePlayerMilitarySite(*this));
+		 flags,
+		 CheckStepDefault(MOVECAPS_WALK),
+		 FindFlagOf(FindImmovablePlayerMilitarySite(*this)));
 
-	if (immovables.empty())
+	if (flags.empty())
 		return 0;
 
-	container_iterate_const(std::vector<BaseImmovable *>, immovables, i) {
-		const MilitarySite & ms =
-			ref_cast<MilitarySite, BaseImmovable>(**i.current);
-		std::vector<Soldier *> const present = ms.presentSoldiers();
-		uint32_t const nr_staying = ms.minSoldierCapacity();
+	container_iterate_const(std::vector<BaseImmovable *>, flags, i) {
+		const Flag * attackerflag = static_cast<Flag *>(*i.current);
+		const MilitarySite * ms = static_cast<MilitarySite *>(attackerflag->get_building());
+		std::vector<Soldier *> const present = ms->presentSoldiers();
+		uint32_t const nr_staying = ms->minSoldierCapacity();
 		uint32_t const nr_present = present.size();
 		if (nr_staying < nr_present) {
 			uint32_t const nr_taken =