[Freeciv-Dev] Re: (PR#2650) Overflow in military_amortize

["Per I. Mathisen via RT" <rt@xxxxxxxxxxxxxx>]

On Thu, 26 Dec 2002, Gregory Berkolaiko via RT wrote:
> The expression in military_amortize() in aiunit.c
> fully_amortized = ((value * simply_amortized) * 100
>                    / (MAX(1, value - simply_amortized))
>                    / (build_cost * MORT));
> overflows on a regular basis.
>
> This is rather damaging as can trun attractive objective / build into an
> unattractive (negative) one.

I suppose military_amortize() is meant to balance the use of build cost in
the kill_desire() function, which gives us want.

But if we really want to do this, we should calculate (in aidata.c) the
average city production each turn, divide build cost on that and add the
result to the delay parameter to normal amortize instead. This way we
really know what impact the loss of build_cost shields.

This makes sense for what-do-we-want-to-build uses of this function, while
for find-target uses, the only way it makes a difference is when comparing
a we-need-ferry mission to a mission where we don't (since we add the
build cost of our ferry to the mess).

So military_amortize would be like this:

/**************************************************************************
  Amortize a want modified by the shields (build_cost) we risk losing.
**************************************************************************/
int military_amortize(struct player *pplayer, int value, int delay, int
                      build_cost)
{
  struct ai_data *ai = ai_data_get(pplayer);
  int build_time = ai->stats.average_production / build_cost;

  if (value <= 0) {
    return 0;
  }

  return amortize(value, delay + build_time);
}

Patch attached. Only autogame tested.

The big question is if we need this at all. Simpe amortize might just get
the job done, too.

  - Per

Index: ai/advmilitary.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/advmilitary.c,v
retrieving revision 1.126
diff -u -r1.126 advmilitary.c
--- ai/advmilitary.c    2002/12/24 19:38:50     1.126
+++ ai/advmilitary.c    2002/12/26 19:04:18
@@ -972,7 +972,7 @@
       desire -= move_time * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING
                              : SHIELD_WEIGHTING);
 
-      want = military_amortize(desire, MAX(1, move_time),
+      want = military_amortize(pplayer, desire, MAX(1, move_time),
                                bcost_balanced + needferry);
       
       if (want > 0) {
@@ -1257,7 +1257,8 @@
   }
   want -= move_time * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING 
                      : SHIELD_WEIGHTING);
-  want = military_amortize(want, MAX(1, move_time), bcost_bal + needferry);
+  want = military_amortize(pplayer, want, MAX(1, move_time), 
+                           bcost_bal + needferry);
   
   if (myunit->id != 0) {
     freelog(LOG_ERROR, "Non-virtual unit in kill_something_with");
Index: ai/aiair.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiair.c,v
retrieving revision 1.6
diff -u -r1.6 aiair.c
--- ai/aiair.c  2002/12/22 18:14:43     1.6
+++ ai/aiair.c  2002/12/26 19:04:18
@@ -144,7 +144,8 @@
   profit = kill_desire(victim_cost, unit_attack, unit_cost, victim_defence, 1) 
     - SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING;
   if (profit > 0) {
-    profit = military_amortize(profit, sortie_time, balanced_cost);
+    profit = military_amortize(unit_owner(punit), profit, sortie_time, 
+                               balanced_cost);
     freelog(LOG_DEBUG, 
            "%s at (%d, %d) is a worthy target with profit %d", 
            unit_type(pdefender)->name, dest_x, dest_y, profit);
Index: ai/aidata.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aidata.c,v
retrieving revision 1.5
diff -u -r1.5 aidata.c
--- ai/aidata.c 2002/12/23 18:09:57     1.5
+++ ai/aidata.c 2002/12/26 19:04:18
@@ -178,10 +178,13 @@
 
   ai->stats.workers = fc_calloc(map.num_continents + 1, sizeof(int));
   ai->stats.cities = fc_calloc(map.num_continents + 1, sizeof(int));
+  ai->stats.average_production = 0;
   city_list_iterate(pplayer->cities, pcity) {
     struct tile *ptile = map_get_tile(pcity->x, pcity->y);
     ai->stats.cities[ptile->continent]++;
+    ai->stats.average_production += pcity->shield_surplus;
   } city_list_iterate_end;
+  ai->stats.average_production /= city_list_size(&pplayer->cities);
   unit_list_iterate(pplayer->units, punit) {
     struct tile *ptile = map_get_tile(punit->x, punit->y);
     if (ptile->terrain != T_OCEAN && unit_flag(punit, F_SETTLERS)) {
Index: ai/aidata.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aidata.h,v
retrieving revision 1.4
diff -u -r1.4 aidata.h
--- ai/aidata.h 2002/12/23 18:09:58     1.4
+++ ai/aidata.h 2002/12/26 19:04:18
@@ -47,6 +47,7 @@
   struct {
     int *workers; /* cities to workers on continent*/
     int *cities;  /* number of cities on continent */
+    int average_production;
   } stats;
 
   int num_continents; /* last time we updated our continent data */
Index: ai/aidiplomat.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aidiplomat.c,v
retrieving revision 1.3
diff -u -r1.3 aidiplomat.c
--- ai/aidiplomat.c     2002/12/18 17:36:18     1.3
+++ ai/aidiplomat.c     2002/12/26 19:04:18
@@ -198,7 +198,7 @@
       return;
     }
 
-    want = military_amortize(want, time_to_dest, ut->build_cost);
+    want = military_amortize(pplayer, want, time_to_dest, ut->build_cost);
 
     if (!player_has_embassy(pplayer, city_owner(acity))) {
         freelog(LOG_DIPLOMAT, "A diplomat desired in %s to establish an "
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.245
diff -u -r1.245 aiunit.c
--- ai/aiunit.c 2002/12/25 13:17:44     1.245
+++ ai/aiunit.c 2002/12/26 19:04:19
@@ -775,23 +775,19 @@
 }
 
 /**************************************************************************
-Military "want" estimates are amortized in this complicated way.
-COMMENTME: Why not use simple amortize? -- GB
+  Amortize a want modified by the shields (build_cost) we risk losing.
 **************************************************************************/
-int military_amortize(int value, int delay, int build_cost)
+int military_amortize(struct player *pplayer, int value, int delay, int
+                      build_cost)
 {
-  int simply_amortized, fully_amortized;
+  struct ai_data *ai = ai_data_get(pplayer);
+  int build_time = ai->stats.average_production / build_cost;
 
   if (value <= 0) {
     return 0;
   }
 
-  simply_amortized = amortize(value, delay);
-  fully_amortized = ((value * simply_amortized) * 100
-                     / (MAX(1, value - simply_amortized))
-                     / (build_cost * MORT));
-
-  return fully_amortized;
+  return amortize(value, delay + build_time);
 }
 
 /**************************************************************************
@@ -1863,11 +1859,12 @@
       /* FIXME: build_cost of ferry */
       needferry = 
         (go_by_boat && !ferryboat && is_ground_unit(punit) ? 40 : 0);
-      want = military_amortize(want, MAX(1, move_time), bcost_bal + needferry);
+      want = military_amortize(pplayer, want, MAX(1, move_time), 
+                               bcost_bal + needferry);
 
       /* BEGIN STEAM-ENGINES-ARE-OUR-FRIENDS KLUGE */
       if (want <= 0 && punit->id == 0 && best == 0) {
-        int bk_e = military_amortize(benefit * SHIELD_WEIGHTING, 
+        int bk_e = military_amortize(pplayer, benefit * SHIELD_WEIGHTING, 
                                      MAX(1, move_time), bcost_bal + needferry);
         if (bk_e > bk) {
           *x = acity->x;
@@ -1989,7 +1986,8 @@
          * (costs 2 luxuries to compensate) */
         want -= (unhap ? 2 * move_time * TRADE_WEIGHTING : 0);
       }
-      want = military_amortize(want, MAX(1, move_time), bcost_bal);
+      want = military_amortize(pplayer, want, MAX(1, move_time), 
+                               bcost_bal);
       if (want > best && ai_fuzzy(pplayer, TRUE)) {
         best = want;
         *x = aunit->x;
Index: ai/aiunit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.h,v
retrieving revision 1.38
diff -u -r1.38 aiunit.h
--- ai/aiunit.h 2002/12/25 13:17:44     1.38
+++ ai/aiunit.h 2002/12/26 19:04:19
@@ -64,7 +64,8 @@
                                int x, int y, bool fortified, bool veteran,
                                bool use_alternative_hp, int alternative_hp);
 int kill_desire(int benefit, int attack, int loss, int vuln, int attack_count);
-int military_amortize(int value, int delay, int build_cost);
+int military_amortize(struct player *pplayer, int value, int delay, int 
+                      build_cost);
 
 bool is_on_unit_upgrade_path(Unit_Type_id test, Unit_Type_id base);