[Freeciv-Dev] Re: [PATCH] aiunit.c ai_military_findvictim() cleanup (PR#1264)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Feb 22, 2002 at 03:34:10PM +0100, Petr Baudis wrote:
>  1. Undefended cities
>  2. Huts
>  3. Enemy units weaker than the unit
>  4. Land barbarians also like unfrastructure tiles (for later pillage)

> +99999   means empty city
> +99998   means hut
> +sane number means ordinary value of the victim
> +1       means barbarians wanting to pillage
> +0       means nothing found or error
> +-2*MORT*TRADE_WEIGHTING means nothing found and punit causing unhappiness

Can't these two blocks be merged?

> +      struct unit *patt = get_attacker(punit, x1, y1);
> +

> +      if (!can_unit_attack_tile(punit, x1, y1))
> +        continue;

{}

> +      /* If we are in the city, let's deeply consider defending it - however,
> +       * horsemen in city refused to attack phalanx just outside that was
> +       * bodyguarding catapult; thus, we get the best attacker on the tile as

s/on the tile/on the tile (x1, y1)/

> +       * well, for the case when there are multiple different units on one
> +       * title. Thus we force punit to attack a stack of units if they are

s/on one title/on the tile (x1, y1)/

> +       * endangering punit seriously, even if they aren't that weak. */
> +      /* FIXME: The get_total_defense_power(pdef, punit) should probably use
> +       * patt rather than pdef. There also ought to be a better metric for
> +       * determining this. */
> +      if (map_get_city(x, y)
> +          && get_total_defense_power(pdef, punit) *

> +             get_total_defense_power(punit, pdef) >= /* didn't like > 
> --Syela */

Who doesn't like it? I would say: remove it.

> +             get_total_attack_power(patt, punit) *
> +             get_total_attack_power(punit, pdef)

Can't this test be replaced with win_chance?

> +          && unit_list_size(&(map_get_tile(punit->x, punit->y)->units)) < 2

Constant. Should be moved outside the loop.

> +        if (map_get_city(x1, y1)) {
> +          /* 40 is something like value of a city, as used in many other 
> parts
> +           * of code (?). --pasky */
> +          benefit = (benefit + 40) * punit->hp / unit_type(punit)->hp;

s/40/unit_value(get_role_unit(F_CITIES, 0))/

> +          && could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) != 0

Are you sure that you don't mean can_unit_move_to_tile here?

> +          && !is_barbarian(unit_owner(punit))
> +          && punit->ai.ai_role != AIUNIT_ESCORT

> +          && punit->ai.charge == 0 /* Above line doesn't seem to work. :( */

Please explain.

> +          && get_tile_infrastructure_set(map_get_tile(x1, y1)) != 0

Although the return type of get_tile_infrastructure_set is an int it
really returns enum tile_special_type. So please use S_NO_SPECIAL.

>  /*************************************************************************
> Index: aiunit.h
> ===================================================================
> RCS file: /home/cvs/aiciv/freeciv-a2/ai/aiunit.h,v
> retrieving revision 1.1.1.4
> retrieving revision 1.6
> diff -u -r1.1.1.4 -r1.6

Something missing?

Side note: it would be nice if you put a serial number on the patch
file/subject.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  reality.sys corrupt. Reboot Universe? (y,n,q)