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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Mon, Jan 14, 2002 at 12:09:17PM CET, I got a letter,
where Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
>  --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> [..]
> > >
> > >  a. determines unit's vision range at (x,y) -- if the player has no
> > >  info on
> > >  (x,y), assume there is no watchtower
> > > 
> > >  b. count the number of unknown location within the vision range. 
> > >  This
> > >  function would be used at least in 2 places, so it's worth it.
> > And it's worth separate patch. I don't want to get my patch too big.
> > Are you willing to make it?
> 
> I am willing but incapable right now.  So maybe some time...
> I would prefer if you removed the watchtower code or at least added a
> FIXME, because it is a bug.
FIXME is fine for me.

> > > 2. Comments like
> > > > +  /* The unit's owner */
> > > >    struct player *pplayer = unit_owner(punit);
> > > are completely useles IMO, as the function name says the same thing.
> > There were some notes about commenting etc ;p. I wanted to see how many
> > people will catch this and throw it on me - you were the only, quite
> > funny ;).
> 
> I don't think many people read your patch :(
I believe the important ones did. And we don't have so many people in active
development (of server or even AI).

> > > 3. Splitting of long if (...) condition into many if() continue;
> > > is nice.  There is another place that you can do the same.
> > Which one?
> 
> I think I mant this one:
> >        if (map_get_special(x1, y1) & S_HUT
> > -     && warmap.cost[x1][y1] < bestcost
> > -     && (!ai_handicap(pplayer, H_HUTS) || map_get_known(x1, y1,
> pplayer))
> > -     && tile_is_accessible(punit, x1, y1)
> > -     && ai_fuzzy(pplayer, 1)) {
> 
> But it's all non-critical.
That's not yet so terrible and pretty clear from the condition what it does.

> > Improved patch attached. I also added symbolic constant for the '9' in part
> > 3 move cost applying to unknown. I would still like to get some more
> > explanation of this, if anyone has an idea.
> 
> not likely.  the underlying idea is surely to say "if we go a tad further but
> can uncover lot's of uncharted land, go for it".  But then 9 is too large a
> value for this, I would have gone for 9/SINGLE_MOVE at most...
Then each operant would be in different units (one in 'costunit' and second in
'moveunit' ;). Let it be :).

> Again, I think that changing function name is noise.
The 'explorer' is just plain wrong from the pedantic point of view. And
confusing IMHO.

Fixed patch attached. I call for commit now, if noone has anything
urgent/serious.

-- 

                                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/

exploring-cleanup.patch
Description: Text document