[Freeciv-Dev] (PR#4171) Re: AI FIXES : advmilitary.c

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Mon, 5 May 2003, Olivier DAVY wrote:
> Hi everybody,
>
> Here are some fixes of ai/advmilitary.c and others
>
> You will find in this mail :
> - the diff against freeciv-cvs-May-01
>
> - the modified files (in modified.tar.gz)
>
> Please, read carefully my changes since it is the first time I have 
> developping
> for freeciv.
>
> Tell me to what are the correct e-mail addresses to send fixes.

Hi there. Welcome to AI hacking.

You only need to send it to rt@xxxxxxxxxxx, it gets forwarded to the
freeciv-dev list automatically. Also please subscribe to this list and
the freeciv-ai list, otherwise the list moderator will have to approve
your mails every time.

You don't need to send modified files, only the diff. Please describe in
your mail what your changes consist of.

Now, some comments on your patch:

+#include <math.h>              /* Added by Olivier DAVY, using ceil */
...
+  FIXED by Olivier DAVY <olivier.davy@xxxxxxx>, 03 May 2003

These comments should either be omitted or go into the changelog (which is
automatically generated from cvs commit messages), not be in the source
code. They have very little information value and a lot of bloat.

-static Unit_Type_id ai_choose_attacker(struct city *pcity,
+static Unit_Type_id simple_ai_choose_attacker(struct city *pcity,
                                        enum unit_move_type which)

AFAIK, there is no need for a "simple" version of each of these functions.
Just fix the original function and use that everywhere. Please put the
necessary changes to other parts in the code in your patch, so that we can
see the consequences these changes have and so that they can be tested.

       if (can_build_unit(pcity, i)
-          && (cur > best
-              || (cur == best
-                  && get_unit_type(i)->build_cost
-                     <= get_unit_type(bestid)->build_cost))) {
-        best = cur;
+          && (
+             (bestid == -1)
+             ||
+             (current > best)
+              ||
+               (
+                 (current == best)
+                  && ( (get_unit_type(i)->build_cost)
+                       <= (get_unit_type(bestid)->build_cost)
+                     )
+               )
+             )
+         )
+                        {

Please read doc/CodingStyle. We try to keep the freeciv code in a uniform
style for easier readability.

The bestid == -1 test is unnecessary. I don't actually understand what the
point of the above changes are, apart from renaming 'cur' to 'current'.

+/**************************************************************************
+  Choose best attacker based on movement type. It chooses based on unit
+  desirability without regard to cost, unless costs are equal.
+  It amortizes on time to build.
+  FIXED by Olivier DAVY <olivier.davy@xxxxxxx>, 03 May 2003
+  The result can be -1 if no attacker was found.
+**************************************************************************/
+static Unit_Type_id ai_choose_attacker(struct city *pcity,
+                                       enum unit_move_type which)

The comment is wrong. Desirability is not "without regard to cost" here.

+       time_to_build =  ceil((float)get_unit_type(i)->build_cost) / 
((float)surplus);

I don't see why you need to use float and ceil here. It is a perfectly
simple calculation.

-  if (acity) {
+  if (!is_stack_vulnerable(x,y)){
     /* If it is a city, we may have to whack it many times */
     /* FIXME: Also valid for fortresses! */
+    /* odavy : "and also for air bases. */
+    /* Using is_stack_vulnerable of aitools instead." */

Ah, yes. Good catch.

  - Per