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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sat, Apr 06, 2002 at 11:19:08PM CEST, I got a letter,
where Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx> told me, that...
> Please excuse me for not indenting the patch with ">"...  Can't figure out 
> how to do it.

No problem.

> ===================================================================
> [...]
> +        /* 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.

IMHO we should rather increase desire for city walls when building land
defenders.. but maybe it wouldn't work so well in the grand scheme of things..
I can't see what Raahul thought that was wrong with that anymore.. ;)) He
ellegantly skipped this when replying.. :^)

FIXME removed.

> 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;
> +  }

Well, maybe this way battleships won't get larger desire than land units.. I
can't understand it fully as well, but I've no idea where else I should place
that comment.. Or should I remove it completely?

>  /************************************************************************** 
> +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.

What it does then? I can't see what's wrong with my explanation, please
elaborate :).

> +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.

Why it shouldn't evaluate need for bombers? I'd like much more cleaning the
current evaluation routines so that we can add support for flying units there
nicely than gluing the air evaluation routines to the face of the current eval
routines, thus only dramatically increasing the current mess in evaluation
routines.

> +      /* 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.

Oh, yes :).

> +      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 :(

Why? I like them, they IMHO help to separate the cases. If they would be only
one-liners, I'd not make them, obviously, but..

> +        /* 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

I'm a bit confused from this all, thus current comment is:

        /* Bonus of Citywalls can be expressed as * 3, and vulnerability is
         * quadratic, thus we multiply it by 9. */

I hope it's correct finally :).

> 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).

I'm not sure yet if I should first clean assess_danger() or f_s_t_k().

Can't test if autogames are still same - any attempt to autogame ends up with
crash really soon, see my another bugreport.

-- 
 
                                Petr "Pasky" Baudis
 
* ELinks maintainer                * IPv6 guy (XS26 co-coordinator)
* IRCnet operator                  * FreeCiv AI hacker
.
Teamwork is essential -- it allows you to blame someone else.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/

want.patch
Description: Text document