[Freeciv-Dev] Re: [IAMBACK] [PATCH] [1.3] cleanup of proccess_*_want() (PR#1295)

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

Please excuse me for not indenting the patch with ">"...  Can't figure out 
how to do it.

===================================================================
[...]
+        /* FIXME? We've bigger desire for land units when city walls can
+         * protect them (?). */
+        if (walls && move_type == LAND_MOVING) {
+          desire *= pcity->ai.wallvalue;
+          /* TODO: More use of POWER_FACTOR ! */
+          desire /= POWER_FACTOR;
+        }

Well, it's better to build defenders which will go well with existing 
structures, isn't it?  I think there is nothing to fix here.


The below comment is nonsense.  The clause does not apply to battleships.

+  /* I was getting four-figure desire for battleships otherwise. -- Syela */
+  if (!walls && unit_types[best_unit_type].move_type == LAND_MOVING) {
     best *= pcity->ai.wallvalue;
+    best /= POWER_FACTOR;
+  }


 /************************************************************************** 
+This function decides, what unit would be best for erasing enemy. It is called,
+when we just want to kill something, we've found it but we don't have the unit
+for killing that built yet - here we'll choose the type of that unit.

This function does something really strange, not quite what you say.
But I guess your explanation is as good as any.

+static void process_attacker_want(struct player *pplayer, struct city *pcity,
+                                  int value, Unit_Type_id victim_unit_type,
+                                  bool veteran, int x, int y, bool unhap,
+                                  int *best_value, int *best_choice,
+                                  int boatx, int boaty, int boatspeed,
+                                  int needferry)
+{ 


I sincerely hope this function will never be trusted to evaluate the need 
for bombers.  I appreciate Raahul remembering me ;)  but I think we can 
safely remove the comment.

+      /* TODO: Case for B_AIRPORT. -- Raahul */
+      bool will_be_veteran = (move_type == LAND_MOVING ||
+                              player_knows_improvement_tech(pplayer, B_PORT));

Remove "not quite right" below.

+      if (unit_type_flag(unit_type, F_IGTER)) {
+        /* Not quite right... */
+        /* TODO: Use something like IGTER_MOVE_COST. -- Raahul */
+        move_rate *= SINGLE_MOVE;
+      }

+      if (unit_types[unit_type].move_type == LAND_MOVING) {
+        if (boatspeed > 0) {
+          /* It's either city or too far away, so don't bother with
+           * victim_move_rate. */
+          move_time = (warmap.cost[boatx][boaty] + move_rate - 1) / move_rate
+                      + 1 + warmap.seacost[x][y] / boatspeed; /* kludge */
+          if (unit_type_flag(unit_type, F_MARINES)) move_time -= 1;
+          
+        } else if (warmap.cost[x][y] <= move_rate) {
+          /* It's adjacent. */
+          move_time = 1;
+
+        } else {

I don't like these spaces before "} else {" above.
They are unsightly :(


+        /* Citywalls give a defensive bonus of 300%. So for units that lack the
+         * ability to ignore city walls, the lack of a city wall makes them 3
+         * times as dangerous. Yet in this check we multiply by 9. WHY????????
+         * (Pulls out hair and screams). -- Raahul */
+        /* FIXME: Use acity->ai.wallvalue? --pasky */
+        vuln *= 9;
+      }

Raahul, please stop ruining your hairstyle!
City walls give bonus of 200% (that's multiplying by 3)
vulnerability is quadratic, so we perform mental calculation
3*3 = 9 and fill in the result


All of my complaints are about your comments which is non-essential.
I haven't checked line-by-line correspondence but if you run a few 
autogames, I think the patch can be safely committed and human resources 
directed to pastures new (like the dreaded and bugged f_s_t_k).

G.