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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jan 07, 2002 at 10:57:01PM +0100, Petr Baudis wrote:
> Dear diary, on Sun, Jan 06, 2002 at 11:54:22PM CET, I got a letter,
> where Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> > On Sun, Jan 06, 2002 at 01:59:40PM -0800, Petr Baudis wrote:
> > >  
> > > /**************************************************************************
> > > -Explores unknown territory, finds huts.
> > > +Handle eXplore mode of a unit - explores unknown territory, finds huts.
> >            ^ This is a client-only gui thing
> It helps you to associate its function with something. I can lowercase the X,
> if you want.
> 
> > > +  /* The position of the unit; updated inside the function */
> > > +  int x, y;
> > 
> > Since also unit_id is initialized here x and y can also.
> Can't it change with handle_unit_activity_request()? (no idea)

It can still be set at this point.

> > > +      if (!handle_unit_move_request(punit, best_x, best_y, FALSE, 
> > > FALSE)) {
> > > +        /* This shouldn't happen */
> > 
> > Than add an assert(0);
> There's no "this can't happen" - if it happens, it is still ok, we aren't
> assured anywhere it won't happen. See source of handle_unit_move_request() and
> you will see really many cases when return 0 is issued and most of them aren't
> fatal. I.e.:
> 
>     /* DO NOT Auto-attack.  Findvictim routine will decide if we should. */
>     if (pplayer->ai.control && punit->activity == ACTIVITY_GOTO) {
>       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
>                        _("Game: Aborting GOTO for AI attack procedures."));
>       return 0;
>     }
> 
> When I observe AI, I see this frequently. IMHO we would die really soon with
> assert(0) ;).

Than the comment is wrong.

> > > +    /* XXX: There's some duplicate code here, but it's several tiny 
> > > pieces,
> > > +     * impossible to group together and not worth their own function
> > > +     * separately. --pasky */
> > 
> > ....
> > 
> > > +        /* Number of unknown tiles in vision range around this tile */
> > > +        int unknown = 0;
> > > +        
> > > +        square_iterate(x1, y1, range, x2, y2) {
> > > +          if (!map_get_known(x2, y2, pplayer))
> > > +            unknown++;
> > > +        } square_iterate_end;
> > 
> > This could be made a function. This construct is used above.
> I think it's too small - and we do it only two times and we have a space for
> it. And we would have to pass too many parameters to it (x1, y1, range,
> pplayer, &unknown)... not worth it, IMHO.
> > 
> > > +
> > > +      /* XXX: Why we don't test if the unit is still alive? */
> > 
> > Add an assert which test it.
> Well, when some unit gets killed during goto, we don't want to victimize whole
> civserver for this one poor fellow duteous to its honourable mission of
> exploring of those white (erm, black) areas of the map bedight by whales and
> notes like "hic sunt liones". Sure, maybe for second all the daemon (the guys
> at the sides blowing) processes should stop and for the eternal glory of it 
> all
> buffers should be synced, but IMHO those great fellows serving loyally to 
> Karel
> IV. of Czech wouldn't wish themselves to bury all the world with them, just 
> for
> one assert().
> > 
> > About the "XXX" and "pasky" comments: we need a consistent style for this.
> > For example we have to decide between FIXMEs and unclear/unknown purpose.
> I have consistent (I hope :) style for this ;-). I sign myself near my 
> personal
> opinions/questions. XXX means unclarity/glitch in the code, FIXME means it's
> actually even confirmed as buggy or confirmed that it needs to be replaced.
> Yup, there's no sharp boundary between these two, but hey, we aren't machines.
> 
> Updated patch attached. Compiles ;-).
> 
> I'm now not sure if ai_manage_explore() wouldn't be even more appropriate. 
> What
> you guys think?

> +  /* The position of the unit; updated inside the function */
> +  int x, y;
....
> +  x = punit->x;
> +  y = punit->y;


>    if (!is_barbarian(pplayer)
> -      && is_ground_unit(punit)) { /* boats don't hunt huts */
> +      && is_ground_unit(punit)) {

> +    /* Maximal acceptable move cost to the target */
>      int maxcost = pplayer->ai.control ? 2 * THRESHOLD : 3;
> +    /* Move cost to the best target (=> lower is better) */
>      int bestcost = maxcost * SINGLE_MOVE + 1;

Something which I have a problem since a long time: maxcost and
bestcost have different units IMHO. Unfortunately I don't have good
names for them. warmap.cost (which bestcost is compared to) holds a
"flattened" cost. So because "bestcost = maxcost * SINGLE_MOVE"
maxcost is more like a maxmoves. I would really appreciate if we can
clarify this.

> +      if (unknown > most_unknown) {

if(unknown <= most_unknown) {continue;}

> +        /* We won't travel into cities, unless we are able to do so - 
> diplomats
> +         * and caravans can. */
> +        /* FIXME/TODO: special flag for this */

You want a special case for these two? IMHO not necessary.

> +      /* We can die because easy AI may accidently stumble on huts it fuzzily
> +       * ignored - unit_id is used for the check of this */

Does this mean that non-easy AI doesn't go hut-hunting?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  This customer comes into the computer store. "I'm looking for a mystery
  Adventure Game with lots of graphics. You know, something realy
  challenging". "Well," replied the clerk, "have you tried Windows 98 ?"