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

[Petr Baudis <pasky@xxxxxxxxxxx>]

First, I feel very sorry for the long delay. But late is better than not at all
;).

Dear diary, on Thu, Jan 24, 2002 at 04:25:52AM CET, I got a letter,
where Mike Kaufman <mkaufman@xxxxxxxxxxxxxx> told me, that...
> I am reviewing your patch.
> 
> Before I get headlong into this thing, your original post mentioned that
> this was only a cleanup? But I get different savegames... this needs to
> be resolved.

Resolved. I accidentally fixed one bug when doing cleanup (I didn't even notice
that :) (the boolean expression for avoiding enemy cities unless diplomat or
caravan is just wrong or I don't get the code meaning right; it will appear as
an one-liner patch after this patch will get applied).

> Other things:
> 
> /**************************************************************************
> Handle eXplore mode of a unit - explores unknown territory, finds huts.
> Returns whether there is any more territory to be explored.
> **************************************************************************/
> int ai_manage_exploring(struct unit *punit)
> {
>   struct player *pplayer = unit_owner(punit);
>   /* The position of the unit; updated inside the function */
>   int x = punit->x, y = punit->y;
>   /* Continent the unit is on */
>   int continent;
>   /* Unit's speed */
>   int move_rate = unit_move_rate(punit);
>   /* Range of unit's vision */
>   int range;
> 
> >> please review the style guide. It's hard to read the declarations
> >> when interspersed with comments. 

I happen to think it's better than have the comments on the side (it's not so
clear here, but look bellow at

    /* Maximal acceptable _number_ of moves to the target */
    int maxmoves = pplayer->ai.control ? 2 * THRESHOLD : 3;

I think stocking it in the thin column at the right side is just impersonated
pure evil; and I believe consistence at least in the smaller scale is a good
thing). Also, in my preview patches I commented the declarations in the same
way and noone stood up - and I want to be consistent. There can always appear
someone other who will move the comments done by me to correct locations - this
will be trivial, but I don't want to do things in my patches which are so
straightly against my nature. I believe this is non-critical issue.

>   /* Get the range */
>   /* FIXME: The vision range should NOT take into account watchtower benefit.
>    * Now it is done in a wrong way: the watchtower bonus is computed locally,
>    * at the point where the unit is, and then it is applied at a faraway
>    * location, as if there is a watchtower there too. --gb */
> 
>   if (unit_profits_of_watchtower(punit)
>       && map_get_tile(punit->x, punit->y)->special & S_FORTRESS)
>     range = get_watchtower_vision(punit);
>   else
>     range = unit_type(punit)->vision_range;
> 
> >> put brackets around this if-else.

Yes sir.

>   /* Idle unit */
> 
>   if (punit->activity != ACTIVITY_IDLE) {
>     handle_unit_activity_request(punit, ACTIVITY_IDLE);
>     /* XXX: Can x,y change? --pasky */
> 
> >> what does the XXX supposedly tell me? Also, this is a good question.
> >> We reset these a couple times. Any clue? 

After glancing at the code now I see the handle_unit_activity_request() in the
original code as well before part 4, without any coords syncing done.  I
believe Syela was right, I also can't very well imagine that the coords changed
while changing the activity to idle :). However, any clues welcomed.

>     x = punit->x;
>     y = punit->y;
>   }
> 
>   /* Localize the unit */
>   
>   if (is_ground_unit(punit)) {
>     continent = map_get_continent(x, y);
>   } else {
>     continent = 0;
>   }
> 
>   /*
>    * PART 1: Look for huts
>    * Non-Barbarian Ground units ONLY.
>    */
>   
>   if (!is_barbarian(pplayer)
>       && is_ground_unit(punit)) {
>     /* Maximal acceptable _number_ of moves to the target */
>     int maxmoves = pplayer->ai.control ? 2 * THRESHOLD : 3;
>     /* Move _cost_ to the best target (=> lower is better) */
>     int bestcost = maxmoves * SINGLE_MOVE + 1;
>     /* Desired destination */
>     int best_x = -1, best_y = -1;
> 
>     /* CPU-expensive but worth it -- Syela */
>     generate_warmap(map_get_city(x, y), punit);
> 
> >> Syela puts this outside the if...

Why I didn't? ;-) Fixed, sir.

>     /* We're iterating outward so that with two tiles with the same movecost
>      * the nearest is used. */
>     iterate_outward(x, y, maxmoves, x1, y1) {
>       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)) {
>         best_x = x1;
>         best_y = y1;
>         bestcost = warmap.cost[best_x][best_y];
>       }
>     } iterate_outward_end;
>     
>     if (bestcost <= maxmoves * SINGLE_MOVE) {
>   
> ~~~~~
> ~~~~~
> 
>   /* 
>    * PART 4: Go home
>    * If we are AI controlled _military_ unit (so Explorers don't count, why?
>    * --pasky), we will return to our homecity, maybe even to another 
> continent.
>    */
>   
>   if (pplayer->ai.control && is_military_unit(punit)) {
>     /* Unit's homecity */
>     struct city *pcity = find_city_by_id(punit->homecity);
> 
> >> this doesn't take into account a homeless unit... shouldn't he find a
> >> boat?

And where the unit should move to?

>     if (pcity) {
>       if (map_get_continent(pcity->x, pcity->y) == continent) {
>         ai_military_gohome(pplayer, punit);
>       } else {
>         /* Sea travel */
>         if (!find_boat(pplayer, &x, &y, 0)) {
>           punit->ai.ferryboat = -1;
>         } else {
>           punit->goto_dest_x = x;
>           punit->goto_dest_y = y;
>           do_unit_goto(punit, GOTO_MOVE_ANY, 0);
>         }
>       }
>     }
>   }
> 
>   return 0;
> }

The improved patch attached. I renamed the function back to ai_manage_explorer
and changed some comments. And I also synced the patch with the latest CVS.

Happy hacking,

-- 

                                Petr "Pasky" Baudis

* UN*X programmer && admin         * IPv6 guy (XS26 co-coordinator)
* elinks maintainer                * FreeCiv AI hacker
* IRCnet operator
.
I love deadlines.
I love the whooshing sound they make as they fly by.
-- Douglas Adams.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/

exploring-cleanup.patch
Description: Text document