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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Tue, Jan 08, 2002 at 11:57:30AM CET, I got a letter,
where Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> told me, that...
> 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.
Ok.

> > > > +      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.
It isn't. The comment doesn't say anything about possibility, but about
proximity.  And I say frequently because it's relatively frequent to me, like
once per several turns - but if you will look how much times this function is
in reality called, it's not so frequent from that view at all.

> >    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.
maxmoves is good.

> > +      if (unknown > most_unknown) {
> 
> if(unknown <= most_unknown) {continue;}
No. "if (unknown > most_unknown)" is common condition in the search of the best
solution in all the code and it's worthless changing that and it just confuses
IMHO. I choose consistency here.

> > +        /* 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.
I want a special flag for these. And IMHO it's worth it, altough it's not
urgent.  Just a piece for future generations, when more important
generalization stuff will find its way to the code; I don't think we have to
decide this now, I would rather like to see discussion about more important
topics nows and I would be happy to have this in the code as a little reminder
for our children ;-).

> > +      /* 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?
Good catch, in fact this comment is wrong :). Didn't noticed that.

No updated patch attached, I'm going to attach it to the reply to the last
comment pending :).

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
The advantage of GUI is that you can see everything you can change.
The disadvantage of GUI is that you can change only what you can see.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/