Merge lp:~widelands-dev/widelands/bug-1451078-fix into lp:widelands

[Teppo Mäenpää]

Fixed a simple bug (changed a break-statement into a continue-statement in the loop where trainingsite checks which soldiers may stay further).

[GunChleoc (gunchleoc)]

This fix is very weird. The check is there to drop the soldier if its level is higher than the maximum level that it can get trained to. Removing this check might mean that soldiers trained to max won't be kicked out.

So, my guess is that the real bug lies with setting the max value. I think this needs some more digging.

On another note, since "it" now isn't an iterator anymore, we should rename it to "upgrade".

[Teppo Mäenpää (kxq)]

I did not remove the check.

Previously, if the soldier was at a too high level, it was assumed that he/she should be let go. In the linked bug, the problem was that soldier was full trained in defense and health but not evade.

After this change, the code merely proceeds to next check, which was the original intention. Please note that the routine takes a default-unsafe approach, where soldiers are whitelisted if a reason to keep them in is found.

I still think that this fixes the problem. Soldiers will be kicked out after this change, too. I tested that much, though of course it is possible that I accidentally tested a wrong binary, sans this fix :-(

Regarding the other note.

I do not mind variable names, and gladly change it to something else. Changing to range loop was not necessary; the old style just hurt my eyes, and I did not see clear reasons why not to do it.

[Teppo Mäenpää (kxq)]

I am not sure if I was speaking clearly.

here, both break (old way) and continue (new way) skip the rest of the test. As a result of this, the soldier is not declared trainable in either case. However, the old way caused the code to skip subsequent arts, which was bad if those trainings were not complete yet.

Old way: Health training complete, evade training not complete: soldier is not declared trainable and might be kicked out, if the status of evade is considered after health.

New way: Soldier stays in.

Anyway, thanks for the quick comments.

[GunChleoc (gunchleoc)]

I get it now - we need to check for all attributes before we break, not just for one of them. Maybe set an additional boolean "trainable_atrribute_found"? Then at the bottom, if (!this_soldier_is_safe || !trainable_atrribute_found) -> drop soldier.

Removing the iterator is great, we want to do that everywhere eventually - it makes the code so much easier to read :)

[Teppo Mäenpää (kxq)]

I am really sorry, but do not see the benefit of adding more booleans. In my opinion the bug is now fixed, unless you or somebody else explain the remaining problems in so simple words that even I understand. Hopefully all the extra typing won't give you joint pains.

[GunChleoc (gunchleoc)]

I was worried that fully trained soldiers would get stuck in the trainingsite now. I have tested and dug around in the code, and there is a separate function "drop_unupgradable_soldiers" that takes care of that.

So, please ignore my last suggestion. Instead, we can remove the "if (level > upgrade.max)" check completely, because it doesn't do anything any more.

[GunChleoc (gunchleoc)]

I don't know where I have my brain - of course the continue does do something useful. Go ahead and merge. Thanks for the fix :)

review: Approve

=== modified file 'src/logic/trainingsite.cc'
--- src/logic/trainingsite.cc	2014-11-30 18:49:38 +0000
+++ src/logic/trainingsite.cc	2015-05-04 14:55:22 +0000
@@ -483,28 +483,27 @@
 		}
 		else
 		{
-			std::vector<Upgrade>::iterator it = m_upgrades.begin();
-			for (; it != m_upgrades.end(); ++it)
+			for (const Upgrade& upgrade: m_upgrades)
 			if  (! this_soldier_is_safe)
 			{
 				// Soldier is safe, if he:
 				//  - is below maximum, and
 				//  - is not in a stalled state
 				// Check done separately for each art.
-				int32_t level = m_soldiers[i]->get_level(it->attribute);
+				int32_t level = m_soldiers[i]->get_level(upgrade.attribute);
 
 				 // Below maximum -check
-				if (level > it->max)
+				if (level > upgrade.max)
 				{
-					break;
+					continue;
 				}
 
-				TypeAndLevel train_tl(it->attribute, level);
+				TypeAndLevel train_tl(upgrade.attribute, level);
 				TrainFailCount::iterator tstep = training_failure_count.find(train_tl);
 				if (tstep ==  training_failure_count.end())
 					{
 						log("\nTrainingSite::drop_stalled_soldiers: ");
-						log("training step %d,%d not found in this school!\n", it->attribute, level);
+						log("training step %d,%d not found in this school!\n", upgrade.attribute, level);
 						break;
 					}