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

["Olivier DAVY" <olivier.davy@xxxxxxx>]

Olivier DAVY

Thank you for your answer,

let me some time and I will send you a more correct patch,
regarding to your comments : 
- no more comments about my changes, assuming that what I do is ok (more or 
less)
- clear old deprecated code, since I changed it (cf. simple_ai_...)
- optimizing test (cf. bestId)
- New comments on function (clearing the old ones)

I am very please to help Freeciv pave the way for free games !


olive


En réponse à "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
> 
> 



---------------------------------------
 Olivier DAVY
 ENSIMAG engineer - HEC alumnus
 E-mail : olivier.davy@xxxxxxx
 Phone  : +33/(0)1.42.67.19.85
---------------------------------------