Merge lp:~janosch-peters-deactivatedaccount/widelands/bugfix_1395238 into lp:widelands

[xxx-deleted]

Fix for Bug #1395238: Soldiers now recognize when they are stuck. If they are, they walk along their path without honoring blocked nodes (e.g. they just walk through other soldiers).

This is also a fix for Bug #1522290 (as this bug is caused by #1395238).

To test this fix load the savegame attached in #1522290 and verify that the soldiers return home (they are not stuck anymore).

[GunChleoc (gunchleoc)]

I couldn't test if this works, but the code seems OK. Just one code style nit: We are moving away from using defines and using constexpr instead. So, instead of

    #define BLOCK_COUNT_IS_STUCK 10

it would be better to use something like:

    constexpr int kBlockCountIsStuck = 10;

The type should match state.ivar3.

[TiborB (tiborb95)]

I tested it. I believe the problem is more frequent than it seems - I watched output in console. But generally I think it is good design. Unless there will be no complains in near time I would approve it.

[bunnybot (widelandsofficial)]

Hi, I am bunnybot (https://github.com/widelands/bunnybot).

I am keeping the source branch lp:~janosch-peters/widelands/bugfix_1395238 mirrored to
  https://github.com/widelands/widelands/tree/_janosch_peters_widelands_bugfix_1395238

The latest continuous integration build can always be found here:
  https://travis-ci.org/widelands/widelands/branches
Please do not merge without making sure that it passes.

You can give me commands by starting a line with @bunnybot <command>. I understand:
 merge: Merges the source branch into the target branch, closing the pull request.

[TiborB (tiborb95)]

OK, I looked at it again and do think it can go

review: Approve

[xxx-deleted (janosch-peters-deactivatedaccount)]

1. Changed following defines to static constexpr:

#define SOLDIER_HP_BAR_WIDTH 13
#define BLOCK_COUNT_IS_STUCK 10

2. Removed unused function remove_spaces

[Tino (tino79)]

LGTM, nice work.

I would suggest to merge trunk, then we get a successful build on travis and we can merge it into trunk.

review: Approve

[bunnybot (widelandsofficial)]

Travis build 129 has changed state to: failed. Details: https://travis-ci.org/widelands/widelands/builds/99815629.

[bunnybot (widelandsofficial)]

Travis build 129 has changed state to: failed. Details: https://travis-ci.org/widelands/widelands/builds/99815629.

[TiborB (tiborb95)]

Why has this not been merged yet? And why travis fails - especially with error:

 error: ‘UBLOCK_ARABIC_MATHEMATICAL_ALPHABETIC_SYMBOLS’ is not a member of ‘UBlockCode’
    UBlockCode::UBLOCK_ARABIC_MATHEMATICAL_ALPHABETIC_SYMBOLS,

that is completely unrelated?

What now? Can we use that bunny stuff to automatically merge or do we (Janosh) need to merge trunk to get rid of compilation bug?

[Tino (tino79)]

Janosch has to merge trunk into his own branch, because trunk contains an updated .travis.yml.
This should fix building by travis.

When the travis build is ok, Janosch (or anyone) can issue the bunnybot command to merge to trunk.

[xxx-deleted (janosch-peters-deactivatedaccount)]

I allready did the merge ony my pc. However, I get an exception if I load the savegame I attached to the bug report. I dont think it has something to do with my changes, but I have to verify that. Ia m doing that right now.

[Tino (tino79)]

This only(!) applies to the fact that travis fails to build this branch:
The online branch lp:~janosch-peters/widelands/bugfix_1395238 does not contain recent trunk.
In particular this revision is missing: https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/revision/7675

As soon you merge trunk and push your local branch to launchpad travis build will succeed.

This won't change the diff of your branch to trunk.

[xxx-deleted (janosch-peters-deactivatedaccount)]

I pushed a new revision where I merged trunk into. However, I get an exception because of a missing animation "crop" (for an imperial ranger) when I load the savegame attached to the bug report. I could not figure out what went wrong. I do not get an exception if I start a new game.

As I am pretty sure it has nothing to do with my changes, I pushed it anyway. It seems that some push one lp:widelands broke backwards-compatibility. It would be great if someone could look into that.

[TiborB (tiborb95)]

I think there was a change/renaming of actions from crop to plant lately somewhere. So you are probably right and this will be an broken savegame compatibility in trunk....

[SirVer (sirver)]

Yes, we are not caring about backwards compatibility between b18 and b19 - for the first time in Wideland's history. That allowed us to clean out some of the cruft we had in the code base. But that has bitten you here.

The code lgtm. I am a bit concerned that now many soldiers can rush through tight spots and overwhelm an opponent instead of going through one by one - that might attacks stronger. But if that is a real concern must be seen through in-game experience.

@bunnybot merge

review: Approve

[TiborB (tiborb95)]

> that might attacks stronger

By my understanding the exemption is used only for soldiers heading home....

[bunnybot (widelandsofficial)]

Travis build 177 has changed state to: passed. Details: https://travis-ci.org/widelands/widelands/builds/100087243.

[GunChleoc (gunchleoc)]

 error: ‘UBLOCK_ARABIC_MATHEMATICAL_ALPHABETIC_SYMBOLS’ is not a member of ‘UBlockCode’
    UBlockCode::UBLOCK_ARABIC_MATHEMATICAL_ALPHABETIC_SYMBOLS,

When you see this error, it means that the version of the ICU library is too old.

[GunChleoc (gunchleoc)]

review: Approve

=== modified file 'src/logic/bob.cc'
--- src/logic/bob.cc	2015-12-05 10:59:02 +0000
+++ src/logic/bob.cc	2016-01-04 10:23:26 +0000
@@ -568,7 +568,8 @@
 	 int32_t         const persist,
 	 const DirAnimations & anims,
 	 bool            const forceonlast,
-	 int32_t         const only_step)
+	 int32_t         const only_step,
+	 bool            const forceall)
 {
 	Path path;
 	BlockedTracker tracker(game, *this, dest);
@@ -580,6 +581,9 @@
 		cstep.add(CheckStepDefault(descr().movecaps()));
 	cstep.add(CheckStepBlocked(tracker));
 
+	if (forceall)
+		tracker.disabled = true;
+
 	Map & map = game.map();
 	if (map.findpath(m_position, dest, persist, path, cstep) < 0) {
 		if (!tracker.nrblocked)
@@ -600,7 +604,7 @@
 	State & state  = top_state();
 	state.path     = new Path(path);
 	state.ivar1    = 0; // step #
-	state.ivar2    = forceonlast ? 1 : 0;
+	state.ivar2    = forceonlast ? 1 : (forceall ? 2 : 0);
 	state.ivar3    = only_step;
 	state.diranims = anims;
 	return true;
@@ -676,7 +680,6 @@
 	return false;
 }
 
-
 void Bob::movepath_update(Game & game, State & state)
 {
 	if (get_signal().size()) {
@@ -718,12 +721,11 @@
 	}
 
 	++state.ivar1;
-	return start_task_move(game, dir, state.diranims, forcemove);
+	return start_task_move(game, dir, state.diranims, state.ivar2 == 2 ? true : forcemove);
 	// Note: state pointer is invalid beyond this point
 }
 
-
-/**
+	/**
  * Move into one direction for one step.
  * \li ivar1: direction
  * \li ivar2: non-zero if the move should be forced

=== modified file 'src/logic/bob.h'
--- src/logic/bob.h	2016-01-02 12:36:38 +0000
+++ src/logic/bob.h	2016-01-04 10:23:26 +0000
@@ -271,7 +271,8 @@
 		 const int32_t         persist,
 		 const DirAnimations &,
 		 const bool            forceonlast = false,
-		 const int32_t         only_step = -1)
+		 const int32_t         only_step = -1,
+		 const bool            forceall = false)
 		__attribute__((warn_unused_result));
 
 	/// This can fail (and return false). Therefore the caller must check the

=== modified file 'src/logic/soldier.cc'
--- src/logic/soldier.cc	2015-12-30 23:07:55 +0000
+++ src/logic/soldier.cc	2016-01-04 10:23:26 +0000
@@ -514,7 +514,7 @@
 {
 	// Gather information to determine coordinates
 	uint32_t w;
-	w = SOLDIER_HP_BAR_WIDTH;
+	w = kSoldierHpBarWidth;
 
 	const Image* hppic = get_hp_level_pic();
 	const Image* attackpic = get_attack_level_pic();
@@ -592,7 +592,7 @@
 	uint16_t evh = evadepic->height();
 
 	uint16_t animw;
-	animw = SOLDIER_HP_BAR_WIDTH;
+	animw = kSoldierHpBarWidth;
 
 	w = std::max(std::max(atw + dew, hpw + evw), 2 * animw);
 	h = 5 + std::max(hph + ath, evh + deh);
@@ -729,7 +729,8 @@
 	State & state  = top_state();
 	state.objvar1  = &building;
 	state.coords   = building.get_position();
-	state.ivar2    = 0; // Thre return state 1=go home 2=go back in known land
+	state.ivar2    = 0; // The return state 1=go home 2=go back in known land
+	state.ivar3    = 0; // Counts how often the soldier is blocked in a row
 
 	state.ivar1    |= CF_RETREAT_WHEN_INJURED;
 	state.ui32var3 = kRetreatWhenHealthDropsBelowThisPercentage * get_max_hitpoints() / 100;
@@ -746,11 +747,15 @@
 	uint32_t    defenders = 0;
 
 	if (signal.size()) {
-		if
-			(signal == "blocked" || signal == "battle" || signal == "wakeup" ||
-			 signal == "sleep")
-			signal_handled();
+		if (signal == "battle" || signal == "wakeup" || signal == "sleep") {
+			state.ivar3 = 0;
+			signal_handled();
+		} else if (signal == "blocked") {
+			state.ivar3++;
+			signal_handled();
+		}
 		else if (signal == "fail") {
+			state.ivar3 = 0;
 			signal_handled();
 			if (state.objvar1.get(game)) {
 				molog("[attack] failed to reach enemy\n");
@@ -761,6 +766,7 @@
 			}
 		} else if (signal == "location") {
 			molog("[attack] Location destroyed\n");
+			state.ivar3 = 0;
 			signal_handled();
 			if (state.ivar2 == 0) {
 				state.ivar2 = 1;
@@ -770,6 +776,9 @@
 				("[attack] cancelled by unexpected signal '%s'\n", signal.c_str());
 			return pop_task(game);
 		}
+	} else {
+		// no signals means no consecutive block -> we're not stuck anymore
+		state.ivar3 = 0;
 	}
 
 	//  We are at enemy building flag, and a defender is coming, sleep until he
@@ -810,12 +819,15 @@
 				return start_task_leavebuilding(game, false);
 			}
 			// Head to home
-			if
-				(start_task_movepath
+			if (state.ivar3 > kBockCountIsStuck)
+				molog("[attack] soldier is stuck, blocked nodes will be ignored\n");
+
+			if (start_task_movepath
 					(game,
 						baseflag.get_position(),
 						4, // use larger persist when returning home
-						descr().get_right_walk_anims(does_carry_ware())))
+						descr().get_right_walk_anims(does_carry_ware()),
+						false, -1, state.ivar3 > kBockCountIsStuck))
 				return;
 			else {
 				molog("[attack] failed to return home\n");

=== modified file 'src/logic/soldier.h'
--- src/logic/soldier.h	2015-11-25 08:42:28 +0000
+++ src/logic/soldier.h	2016-01-04 10:23:26 +0000
@@ -26,8 +26,6 @@
 #include "logic/training_attribute.h"
 #include "logic/worker.h"
 
-#define SOLDIER_HP_BAR_WIDTH 13
-
 struct RGBColor;
 
 namespace Widelands {
@@ -287,6 +285,11 @@
 	 */
 	Battle * m_battle;
 
+	static constexpr uint8_t kSoldierHpBarWidth = 13;
+
+	/// Number of consecutive blocked signals until the soldiers are considered permanently stuck
+	static constexpr uint8_t kBockCountIsStuck = 10;
+
 	// saving and loading
 protected:
 	struct Loader : public Worker::Loader {