[Freeciv-Dev] Re: Military amortize (PR#1196)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

This is a mass reply to your comments.
First of all, thanks for your time!

To Raahul: Renaming variables like "e" should be done in one big renaming
sweep IMHO, by you, or Petr, or myself, or whoever.  I wanted to keep the
patches small :)

To Ross: I thought about making amortize and then military_amortized use
a lookup table.  It can (and should) be done in next patches.  In any
case, amortize is potentially buggy: on one hand it uses predefined MORT
which can be changed elsewhere.  On the other hand it implicitly assumes
that MORT == 24 (in (23/24)^12 == 3/5 speedup).

To Chris (on build_cost_prime): Yes, passing a pointer to unit_type
rather than offset can be easily done.  Do people think it is worth it? 
I think that if need arises, it can be changed later.  I don't see that
much use for build_cost_prime anyway...

To Petr: On formatting: when I view my file everything looks perfectly
formatted.  Can it be that diff chews the tabs up somehow?
Sorry Raahul, I think the crazy formatting in your diff was also because
of this -- I didn't know that :(
On "MAX(1,c) in the function itself": here I had the same reason to leave
it outside as suggested by Chris --- I want to leave freedom of passing
c=0.
More:

 --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> Nothing terribly exciting about the operators in new line, but can you
> please at least indent them into the parenthesis?
> 
> And I would like the original...
> 
>   fully_amortized = ((value * simply_amortized)
>                      / (MAX(1, value - simply_amortized) * 100
>                      / (build_cost * MORT));

no problem.

> > -            if (e > bk) {
> > +     /* FIXME: sheild_cost of ferry */
> shield_cost.. and can you please rather use spaces?

oh yes ;)  I actually indented it to be build_cost (the corresponding
field in unit_types)

> > +          needferry = ((!sanity && !boatid && is_ground_unit(punit))
> ? 
> > +                  40 : 0);
> this deserves an if

I like ternary operators :(
An if will take 5 lines :(

> > +             !unit_flag(punit, F_MARINES) &&
> > +             map_get_continent(acity->x, acity->y) != con) {
> uhm, yes, you just exchange tabs and spaces here (no idea why), but
> when you
> already touch it, what about moving that boolean opeators? ;)

sure I should

> MORT);
> > -          } else e = 0;
> > +     e = military_amortize(b0, MAX(1, c), fprime);
> this indendation looks odd - just tab-hallucination?

I guess... :(

A slightly improved patch is attached.  All seemingly unformated lines
are due to tabulation thing -- do I have to hunt them out?

G.

__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com

? rfile1
? rfile2
? saves
? ai/aiunit.mod.c
Index: ai/advmilitary.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/advmilitary.c,v
retrieving revision 1.80
diff -u -r1.80 advmilitary.c
--- ai/advmilitary.c    2001/12/21 11:17:30     1.80
+++ ai/advmilitary.c    2002/01/04 16:01:34
@@ -543,7 +543,7 @@
 { 
   Unit_Type_id i;
   Tech_Type_id j;
-  int a, c, d, e, a0, b0, f, g, fprime;
+  int a, c, d, e, b0, f, g, fprime;
   int k, l, m, q;
   int shore = is_terrain_near_tile(pcity->x, pcity->y, T_OCEAN);
   struct city *acity = map_get_city(x, y);
@@ -624,10 +624,7 @@
         b0 -= l * SHIELD_WEIGHTING;
         b0 -= c * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
SHIELD_WEIGHTING);
       }
-      if (b0 > 0) {
-        a0 = amortize(b0, MAX(1, c));
-        e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / ((fprime + needferry) * 
MORT);
-      } else e = 0;  
+      e = military_amortize(b0, MAX(1, c), fprime + needferry);
       if (e > 0) {
         if (k) {
           pplayer->ai.tech_want[j] += e;
@@ -659,7 +656,7 @@
 {
   int a, b, c, d, e, f, g; /* variables in the attacker-want equation */
   Unit_Type_id v, n;
-  int m, vet, dist, a0, b0, fprime;
+  int m, vet, dist, b0, fprime;
   int x, y, unhap = 0;
   struct unit *pdef, *aunit, *ferryboat;
   struct city *acity;
@@ -822,10 +819,7 @@
                g * SHIELD_WEIGHTING / (acity->ai.a * acity->ai.a + g * d);
     }
     b0 -= c * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
SHIELD_WEIGHTING);
-    if (b0 > 0) {
-      a0 = amortize(b0, MAX(1, c));
-      e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / ((fprime + needferry) * 
MORT);
-    } else e = 0;  
+    e = military_amortize(b0, MAX(1, c), fprime + needferry);
 
 #ifdef DEBUG
     if (e != fstk && acity) {
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.158
diff -u -r1.158 aiunit.c
--- ai/aiunit.c 2001/11/05 01:59:59     1.158
+++ ai/aiunit.c 2002/01/04 16:01:59
@@ -490,6 +490,26 @@
 }
 
 /**************************************************************************
+Military "want" estimates are amortized in this complicated way.
+COMMENTME: Why not use simple amortize? -- GB
+**************************************************************************/
+int military_amortize(int value, int delay, int build_cost)
+{
+  int simply_amortized, fully_amortized;
+
+  if (value <= 0) {
+    return 0;
+  }
+
+  simply_amortized = amortize(value, delay);
+  fully_amortized = ((value * simply_amortized) 
+                     / (MAX(1, value - simply_amortized)) * 100
+                     / (build_cost * MORT));
+
+  return fully_amortized;
+}
+
+/**************************************************************************
  this is still pretty dopey but better than the original -- Syela
 **************************************************************************/
 static int reinforcements_value(struct unit *punit, int x, int y)
@@ -1162,14 +1182,14 @@
 **************************************************************************/
 int find_something_to_kill(struct player *pplayer, struct unit *punit, int *x, 
int *y)
 {
-  int a=0, b, c, d, e, m, n, v, i, f, a0, b0, ab, g;
+  int a=0, b, c, d, e, m, n, v, i, f, b0, ab, g;
 #ifdef DEBUG
   int aa = 0, bb = 0, cc = 0, dd = 0, bestb0 = 0;
 #endif
   int con = map_get_continent(punit->x, punit->y);
   struct player *aplayer;
   struct unit *pdef;
-  int best = 0, maxd, boatid = 0;
+  int best = 0, maxd, boatid = 0, needferry;
   int harborcity = 0, bx = 0, by = 0;
   int fprime, handicap;
   struct unit *ferryboat = 0;
@@ -1314,35 +1334,31 @@
                       g * SHIELD_WEIGHTING / (acity->ai.a * acity->ai.a + g * 
d);
           }
           b0 -= c * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
SHIELD_WEIGHTING);
-          if (b0 > 0) {
-            a0 = amortize(b0, MAX(1, c));
-            if (!sanity && !boatid && is_ground_unit(punit))
-              e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / ((fprime + 40) * 
MORT);
-            else e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / (fprime * MORT);
- /* BEGIN STEAM-ENGINES-ARE-OUR-FRIENDS KLUGE */
-          } else if (!punit->id && !best) {
-            b0 = b * SHIELD_WEIGHTING;
-            a0 = amortize(b0, MAX(1, c));
-            if (!sanity && !boatid && is_ground_unit(punit))
-              e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / ((fprime + 40) * 
MORT);
-            else e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / (fprime * MORT);
-            if (e > bk) {
-              if (punit->id && is_ground_unit(punit) &&
-                       !unit_flag(punit, F_MARINES) &&
-                       map_get_continent(acity->x, acity->y) != con) {
+         /* FIXME: build_cost of ferry */
+          needferry = ((!sanity && !boatid && is_ground_unit(punit)) ? 
+                      40 : 0);
+         e = military_amortize(b0, MAX(1, c), fprime + needferry);
+         /* BEGIN STEAM-ENGINES-ARE-OUR-FRIENDS KLUGE */
+          if (b0 <= 0 && !punit->id && !best) {
+            int bk_e = military_amortize(b * SHIELD_WEIGHTING, 
+                                        MAX(1, c), fprime + needferry);
+            if (bk_e > bk) {
+              if (punit->id && is_ground_unit(punit) 
+                  && !unit_flag(punit, F_MARINES) 
+                  && map_get_continent(acity->x, acity->y) != con) {
                 if (find_beachhead(punit, acity->x, acity->y, &xx, &yy)) {
                   *x = acity->x;
                   *y = acity->y;
-                  bk = e;
+                  bk = bk_e;
                 } /* else no beachhead */
               } else {
                 *x = acity->x;
                 *y = acity->y;
-                bk = e;
+                bk = bk_e;
               }
             }
-            e = 0; /* END STEAM-ENGINES KLUGE */
-          } else e = 0;
+         }
+         /* END STEAM-ENGINES KLUGE */
 
          if (punit->id && ferryboat && is_ground_unit(punit)) {
            freelog(LOG_DEBUG, "%s@(%d, %d) -> %s@(%d, %d) -> %s@(%d, %d)"
@@ -1410,10 +1426,7 @@
           else if (c > THRESHOLD) b0 = 0;
           else b0 = ((b * a - f * d) * SHIELD_WEIGHTING / (a + d)) - 
             c * (unhap ? SHIELD_WEIGHTING + 2 * TRADE_WEIGHTING : 
SHIELD_WEIGHTING);
-          if (b0 > 0) {
-            a0 = amortize(b0, MAX(1, c));
-            e = ((a0 * b0) / (MAX(1, b0 - a0))) * 100 / (fprime * MORT);
-          } else e = 0;
+          e = military_amortize(b0, MAX(1, c), fprime);
           if (e > best && ai_fuzzy(pplayer,1)) {
 #ifdef DEBUG
             aa = a; bb = b; cc = c; dd = d; bestb0 = b0;
Index: ai/aiunit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.h,v
retrieving revision 1.24
diff -u -r1.24 aiunit.h
--- ai/aiunit.h 2001/01/10 18:50:45     1.24
+++ ai/aiunit.h 2002/01/04 16:02:27
@@ -34,6 +34,7 @@
 int unit_vulnerability_basic(struct unit *punit, struct unit *pdef);
 int unit_vulnerability_virtual(struct unit *punit);
 int unit_vulnerability(struct unit *punit, struct unit *pdef);
+int military_amortize(int value, int delay, int build_cost);
 
 int is_on_unit_upgrade_path(Unit_Type_id test, Unit_Type_id base);
 int is_ai_simple_military(Unit_Type_id type);