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

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

On Sun, 7 Apr 2002, Petr Baudis wrote:

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

Err, don't try to confuse me.
We _already_ have citywalls => if we build a land he will benefit from it, 
and a ship won't, so we have to take that into account, right?

> FIXME removed.

good :)

> > 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?

Well, nobody seems to understand this comment => it's not serving it's 
purpose (to comment the code).  Remove it methinks

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

No, your explanation is perfect, but I'm not sure if the code lives up to 
it ;)

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

This is an idiotic routine called from another messy and idiotic routine 
which in turn is called from yet another even more messy and idiotic 
routine.  All this and should be done in three functions 
ai_eval_land_offensive
ai_eval_sea_offensive
ai_eval_air_offensive  
(which could share some common functions) nicely and clearly.  The above 
unit types are quite different and trying to unite their evaluation brings 
the code into the state it is now: AWFUL.

Sorry for shouting...  I just spent an hour trying to trace evaluation of 
city walls...

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


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

it's up to you, but I don't like them :(

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

yes :)

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

they are both worthy targets

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

oh well
another time then ;)

G.