Merge lp:~widelands-dev/widelands/bug-1388028 into lp:widelands

[GunChleoc]

Fixed division by zero in AI that can come up when loading a savegame.

@Tibor: could you please make sure that this is still intended behaviour for the AI and remove the NOCOM comments when you're done?

[TiborB (tiborb95)]

GunChleoc - this is not about zero division but about out-of-range - I think.

Do you think that problem is that genstats is not populated some very short time after loading and AI tries to access members that are not there yet? So that genstats.size()==0?

If this is the case I would use other aproach then try-catch...

Also see comments in diff, default decision for AI if it can not get data on military strength of enemy should be 'do not attack'

[GunChleoc (gunchleoc)]

I added the try/catch because I was suspecting holes in the genstats
array, but the hole is in miltary_strength. So, the important point here
is to check for military_strength.empty() - if miltary_strength.back()
gives us nothing, we do get a division by 0. The try/catch isn't that
important really, so if you think it's not necessary, we can revert to
using genstats[j - 1] rather than player_attackable.at(j - 1). I just
thought it can't hurt to leave it in.

01/11/2014 20:09, sgrìobh TiborB:
> GunChleoc - this is not about zero division but about out-of-range - I think.
>
> Do you think that problem is that genstats is not populated some very short time after loading and AI tries to access members that are not there yet? So that genstats.size()==0?
>
> If this is the case I would use other aproach then try-catch...
>
> Also see comments in diff, default decision for AI if it can not get data on military strength of enemy should be 'do not attack'
>
>
>
>
> Diff comments:
>
>> === modified file 'src/ai/defaultai.cc'
>> --- src/ai/defaultai.cc 2014-10-31 20:49:02 +0000
>> +++ src/ai/defaultai.cc 2014-11-01 14:10:33 +0000
>> @@ -698,7 +698,7 @@
>> }
>> }
>>
>> - // folowing is done allways (regardless of military or not)
>> + // the following is done always (regardless of military or not)
>>
>> // we get immovables with higher radius
>> immovables.clear();
>> @@ -710,6 +710,7 @@
>> field.military_presence_ = 0;
>>
>> for (uint32_t i = 0; i < immovables.size(); ++i) {
>> +
>> const BaseImmovable& base_immovable = *immovables.at(i).object;
>>
>> // testing if it is enemy-owned field
>> @@ -3089,20 +3090,27 @@
>> const Game::GeneralStatsVector& genstats = game().get_general_statistics();
>> for (uint8_t j = 1; j <= plr_in_game; ++j) {
>> if (pn == j) {
>> - player_attackable[j - 1] = false;
>> + player_attackable.at(j - 1) = false;
>> continue;
>> }
>>
>> - if (genstats[j - 1].miltary_strength.back() == 0) {
>> - // to avoid improbable zero division
>> - player_attackable[j - 1] = true;
>> - any_attackable = true;
>> - } else if ((genstats[pn - 1].miltary_strength.back() * 100 /
>> - genstats[j - 1].miltary_strength.back()) > treshold_ratio) {
>> - player_attackable[j - 1] = true;
>> - any_attackable = true;
>> - } else {
>> - player_attackable[j - 1] = false;
>> + try {
>> + // Avoid division by zero
>> + if (genstats.at(j - 1).miltary_strength.empty() ||
>> + genstats.at(j - 1).miltary_strength.back() == 0) {
>> + player_attackable.at(j - 1) = true;
>> + any_attackable = true;
>> + // Check threshold
>> + } else if ((genstats.at(pn - 1).miltary_strength.back() * 100 /
>> + genstats.at(j - 1).miltary_strength.back()) > treshold_ratio) {
>> + player_attackable.at(j - 1) = true;
>> + any_attackable = true;
>> + } else {
>> + player_attackable.at(j - 1) = false;
>> + }
>> + } catch (const std::out_of_range&) {
>> + player_attackable.at(j - 1) = true;
>
> change to false
>
>> + any_attackable = true;
>
> remove the line
>
>> }
>> }
>>
>>
>
>

[TiborB (tiborb95)]

from http://www.cplusplus.com/reference/vector/vector/back/

Calling this function on an empty container causes undefined behavior.

But what is more interesting is that if really the genstats or miltary_strength can be empty during a small period of time after loading.

Also, it can be empty during first 60 seconds of new game...

I somehow dont like that 'try' just because it can hide other problems

I will think about it and rework it...

[TiborB (tiborb95)]

What do you think?

[TiborB (tiborb95)]

Also, I tested the savegame, I got:

ComputerPlayer(5): initializing (2)
ComputerPlayer(5): miltary_strength empty
ComputerPlayer(5): miltary_strength empty
ComputerPlayer(5): miltary_strength empty
ComputerPlayer(6): initializing (2)
ComputerPlayer(6): miltary_strength empty
ComputerPlayer(6): miltary_strength empty
ComputerPlayer(6): miltary_strength empty
ComputerPlayer(7): initializing (2)

but the game keeps running :)

[GunChleoc (gunchleoc)]

Do we really need the

if (genstats.size()<j) {

As I said, the try/catch was just something I added while searching, and the real fix for this particular crash was the check for

genstats.at(j - 1).miltary_strength.empty())

I don't mind either way though. I expect that these cases will mostly come up when loading a savegame, so if the AI decided to attack a bit later because initialization isn't ready, no harm done.

[TiborB (tiborb95)]

I have no idea if a case genstats.size()==0 can ever happen, if samebody guarantees that it cannot happen ever, we can omit it. But this way we will be on a safe side.

Can AI know that saving or loading of a game is in progress?

[GunChleoc (gunchleoc)]

I don't know. Let's stay on the safe side and use the exception in combination with your log output and defaults.

=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc	2014-10-31 20:49:02 +0000
+++ src/ai/defaultai.cc	2014-11-02 09:15:33 +0000
@@ -698,7 +698,7 @@
 		}
 	}
 
-	// folowing is done allways (regardless of military or not)
+	// the following is done always (regardless of military or not)
 
 	// we get immovables with higher radius
 	immovables.clear();
@@ -710,6 +710,7 @@
 	field.military_presence_ = 0;
 
 	for (uint32_t i = 0; i < immovables.size(); ++i) {
+
 		const BaseImmovable& base_immovable = *immovables.at(i).object;
 
 		// testing if it is enemy-owned field
@@ -3089,20 +3090,31 @@
 	const Game::GeneralStatsVector& genstats = game().get_general_statistics();
 	for (uint8_t j = 1; j <= plr_in_game; ++j) {
 		if (pn == j) {
-			player_attackable[j - 1] = false;
+			player_attackable.at(j - 1) = false;
 			continue;
 		}
 
-		if (genstats[j - 1].miltary_strength.back() == 0) {
-			// to avoid improbable zero division
-			player_attackable[j - 1] = true;
-			any_attackable = true;
-		} else if ((genstats[pn - 1].miltary_strength.back() * 100 /
-		            genstats[j - 1].miltary_strength.back()) > treshold_ratio) {
-			player_attackable[j - 1] = true;
-			any_attackable = true;
-		} else {
-			player_attackable[j - 1] = false;
+		try {
+			// It seems that under some circumstances genstats can be empty.
+			// So, to avoid crash, the AI tests its content first.
+			if (genstats.at(j - 1).miltary_strength.empty()) {
+				log("ComputerPlayer(%d): miltary_strength is empty\n", player_number());
+				player_attackable.at(j - 1) = false;
+			// Avoid division by zero
+			} else if ( genstats.at(j - 1).miltary_strength.back() == 0) {
+				player_attackable.at(j - 1) = true;
+				any_attackable = true;
+			// Check threshold
+			} else if ((genstats.at(pn - 1).miltary_strength.back() * 100 /
+							genstats.at(j - 1).miltary_strength.back()) > treshold_ratio) {
+				player_attackable.at(j - 1) = true;
+				any_attackable = true;
+			} else {
+				player_attackable.at(j - 1) = false;
+			}
+		} catch (const std::out_of_range&) {
+			log("ComputerPlayer(%d): genstats entry missing - size :%d\n", player_number(), genstats.size());
+			player_attackable.at(j - 1) = false;
 		}
 	}