[Freeciv-Dev] (PR#4196) Enemies in allied transport assert.

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

If there are enemies in a transport of your ally and you attempt to move 
onto the tile, you get an assert from get_defender.

Even if we decide that ally of an ally cannot be enemy, this is still a 
bug, as the lacks robustness and the error message comes from a wrong 
place.

I therefore set off to remove the assert.  Instead, I went through 
get_defender hitting C-k in a seemingly random fashion.  Here is the 
result.

The function will now return the best defender available on the tile 
against given unit.  It might be an enemy, might be an ally, might even be 
one of the attacker's relatives.  I decided that get_defender is no place 
to make such checks.

The patch is not tested yet.  I also plan to retire unit_can_defend_here 
which is not used anywhere else and being such a short function itself, it 
only obscures the only check that get_defender makes.

The step after that is a thorough cleaning of handle_unit_move_request, to 
ensure that it issues right warnings.

Please comment on the patch.  Also it would be helpful if you can tell me 
what happens when something (not Fighter) attacks a tile with a Bomber and 
fortified tank.

Thanks,
G.

? ttt.gz
? ai/aisettler.c
? ai/aisettler.h
? common/aicore/citymap.c
? common/aicore/citymap.h
Index: common/combat.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/combat.c,v
retrieving revision 1.30
diff -u -r1.30 combat.c
--- common/combat.c     2003/05/06 08:13:21     1.30
+++ common/combat.c     2003/05/08 16:39:57
@@ -19,7 +19,6 @@
 #include <math.h>
 #include <stdio.h>
 
-#include "log.h"
 #include "map.h"
 #include "unit.h"
 
@@ -452,66 +451,51 @@
 Finds the best defender on the square, given an attacker.
 
 This is simply done by calling win_chance with all the possible defenders
-in turn.
-This functions could be improved to take the value of the unit into
-account. It currently uses build cost as a modifier in case the chances of
-2 units are identical, but this is crude as build cost does not neccesarily
-have anything to do with the value of a unit.
-It would be nice if the function was a bit more fuzzy about prioritizing,
-making it able to fx choose a 1a/9d unit over a 10a/10d unit. It should
-also be able to spare units without full hp's to some extend, as these
-could be more valuable later.
+in turn.  It uses build_cost as a tie breaker and then defense_rating as 
+the last tie breaker.
+
+If there is noone on the tile, it returns NULL.  NB: If it does return a 
+unit, it doesn't mean we can attack it, it might be a neutral, an ally or
+even one of ours!
 **************************************************************************/
 struct unit *get_defender(struct unit *attacker, int x, int y)
 {
   struct unit *bestdef = NULL;
-  int bestvalue = -1, count = 0, best_cost = 0, rating_of_best = 0;
-  struct player *att_owner = unit_owner(attacker);
+  int best_chance = -1, best_cost = 0, best_rating = 0;
+  struct tile *ptile = map_get_tile(x, y);
 
-  unit_list_iterate(map_get_tile(x, y)->units, defender) {
-    if (pplayers_allied(att_owner, unit_owner(defender)))
-      continue;
-    count++;
+  unit_list_iterate(ptile->units, defender) {
     if (unit_can_defend_here(defender)) {
-      bool change = FALSE;
-      int build_cost = unit_type(defender)->build_cost;
-      int defense_rating = get_defense_rating(attacker, defender);
-
+      int unit_cost = unit_type(defender)->build_cost;
+      int unit_rating = get_defense_rating(attacker, defender);
       /* This will make units roughly evenly good defenders look alike. */
-      int unit_def = (int) (100000 * (1 - unit_win_chance(attacker, 
defender)));
-      assert(unit_def >= 0);
+      int unit_chance = 
+          (int) (100000 * (1 - unit_win_chance(attacker, defender)));
 
-      if (unit_def > bestvalue) {
-       change = TRUE;
-      } else if (unit_def == bestvalue) {
-       if (build_cost < best_cost) {
-         change = TRUE;
-       } else if (build_cost == best_cost) {
-         if (rating_of_best < defense_rating) {        
-           change = TRUE;
+      assert(unit_chance >= 0);
+
+      if (unit_chance < best_chance) {
+        continue;
+      } else if (unit_chance == best_chance) {
+       if (unit_cost > best_cost) {
+          continue;
+       } else if (unit_cost == best_cost) {
+         if (unit_rating < best_rating) {      
+           continue;
          }
        }
       }
 
-      if (change) {
-       bestvalue = unit_def;
-       bestdef = defender;
-       best_cost = build_cost;
-       rating_of_best = defense_rating;
-      }
+      bestdef = defender;
+      best_chance = unit_chance;
+      best_cost = unit_cost;
+      best_rating = unit_rating;
     }
   } unit_list_iterate_end;
 
-  if (count > 0 && !bestdef) {
-    struct tile *ptile = map_get_tile(x, y);
-    struct unit *punit = unit_list_get(&ptile->units, 0);
-
-    freelog(LOG_ERROR, "get_defender bug: %s's %s vs %s's %s (total %d"
-            " units) on %s at (%d,%d). ", unit_owner(attacker)->name,
-            unit_type(attacker)->name, unit_owner(punit)->name,
-            unit_type(punit)->name, unit_list_size(&ptile->units), 
-            get_terrain_name(ptile->terrain), x, y);
-    assert(FALSE);
+  if (unit_list_size(&ptile->units) > 0 && !bestdef) {
+    die("From %d units nobody can defend at (%d, %d)", 
+        unit_list_size(&ptile->units), x, y);
   }
 
   return bestdef;