[Freeciv-Dev] Re: Reproducable core dump (PR#1051)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

Summary:
the patch is a big improvement

 --- jdorje@xxxxxxxxxxxxxxxxxxxxx wrote: 
> Gregory Berkolaiko wrote:
> 
> > 1. I was told that using goto is a very bad style.  You can get rid
> > of them quite simply but you don't.  Why not?
> 
> Goto gets a bad rep because it's often used sloppily.  There were 
> originally 2 goto's in this code, and they were sloppy :-).  I removed 
> one of them, but it seemed more elegant to leave the other.  I'll give 
> it another look.

I don't mind provided you have a good reason to have "goto".  But you
seem to cope fine without it ;)

> > 2. This piece of code is used by human palyers (AI doesn't fly) but
> > it peeks under the FOW absolutely shamelessly.  You don't attempt to
> > fix it.
> > Why not?
> 
> 
> Ouch.  I did not realize this was a problem - I just blindly duplicated
> the original functionality :-(.  The goto should also avoid going
> across unknown tiles, right?

It is a problem, largerly unfixed.  I have a patch fixing it in
find_the_best_path, but it's in the waiting.

As for unknown tiles, there is a comment at
http://www.freeciv.org/lxr/source/server/gotohand.c?v=cvs#L763
with which I agree.  There is also a maximalist's and minimalist's
approach to the problem of avoiding enemy.  You seem to follow
maximalist: if you don't know the tile, you assume it's enemy-occupied. 
I think it is more wise to do the minimalist's "if I don't see the enemy,
I hope it's not there" approach with the planes, otherwise you don't get
anywhere (in particular to some airfield -- it doesn't give you vision). 
So what I had in mind is more like: 
if (!MAPSTEP(nx, ny, x, y, dir) ||
     (map_get_known_and_seen(nx, ny, pplayer) 
        && is_non_allied_unit_tile(map_get_tile(nx, ny), pplayer)
        && !((nx==dest_x) && (ny==dest_y))))  {
  break;
}
Note that we don't care if our destination is occupied !

> BTW, why is this part of the server code instead of client code? 
> Shouldn't the client be picking the goto route?  Then there would be no
> problem with fog-of-war.  (Of course, the server should have similar 
> code for the AI to use planes, so maybe it should go in common/).

True.  It seems the problem is that you can't generate a full gotomap for
a plane because of the refuelling points.  But you can find a route once
you know the destination.  Then, for some reason, the client just reads
the destination from the user and passes it to the server and the server
does all the rest.

> Yes, that's very wise.  I think the code will be slower, but much more 
> structurally sound.

Why the code would be slower?

[..]
> Okay, forget all that.  The new patch gets rid of the goto cleanly, and

> also cleans up the code quite nicely (and I've tried to comment 
> thoroughly).  It includes the quick-search, but it's been trimmed down 

very nice.

> even more.  The full search has been changed to an A*.  I'm a bit
> unsure of the "known" checking, though: the code will refuse to send
the > plane
> through an unknown tile, but will happily send the plane over a known, 
> fogged tile assuming no enemy is there.  But this assumption is bad:
> for 
> instance you shouldn't send the plane over a known city (although it 
> will most likely be okay since you'll realize the mistake once you get 
> in sight of the city, but...).  I'm not quite sure how to handle this.

Yes, city is bad...
please add (is_tile_known() && is_non_allied_city_tile())
condition too.
This is how it's done in find_the_shortest_path.

> jason
> > ? old
> ? topology
> Index: server/gotohand.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
> retrieving revision 1.122
> diff -u -r1.122 gotohand.c
> --- server/gotohand.c 2001/10/08 12:11:17     1.122
> +++ server/gotohand.c 2001/11/26 19:21:59
> @@ -1436,99 +1436,55 @@
>  The function has 3 stages:
>  Try to rule out the possibility in O(1) time              else
>  Try to quickly verify in O(moves) time                    else
> -Create a movemap to decide with certainty in O(moves2) time.
> +Do an A* search using the warmap to completely search for the path.
>  Each step should catch the vast majority of tries.
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
what would that mean?

**************************************************************************/
>  int air_can_move_between(int moves, int src_x, int src_y,
>                        int dest_x, int dest_y, struct player *pplayer)
>  {
> +  int x, y, dist;
> +  int total_distance = real_map_distance(src_x, src_y, dest_x,
> dest_y);
> +
>    freelog(LOG_DEBUG, "naive_air_can_move_between %i,%i -> %i,%i,
> moves: %i",
>         src_x, src_y, dest_x, dest_y, moves);

Raimar had different log message here


> +  /* First we do some very simple O(1) checks. */
> +  if (total_distance > moves) return -1;
> +  if (total_distance == 0) return moves;
> +
> +  /* Then we do a fast O(n) straight-line check.  It'll work so long
> as the straight-line
> +      doesn't cross any unreal tiles, unknown tiles, or
> enemy-controlled tiles.  So, it
> +      should work most of the time. */
> +  x = src_x, y = src_y;
> +  for (dist =  total_distance; dist > 0; dist--)  {
> +    /* FIXME: straightest_direction doesn't actually follow the
> straight line. */

it's not really FIXME, it's just a warning, no?

> +    int dir = straightest_direction(x, y, dest_x, dest_y);
> +    int nx, ny;
> +
> +    if (!MAPSTEP(nx, ny, x, y, dir) ||
> +        !map_get_known(nx, ny, pplayer) ||
> +        (map_get_known_and_seen(nx, ny, pplayer) &&
> is_non_allied_unit_tile(map_get_tile(nx, ny), pplayer)))  {
> +      break;
>      }
> +    x = nx, y = ny;
> +  }
> +  if (dist == 0) {
> +    assert(x == dest_x && y == dest_y);
> +    return moves - total_distance; /* ? */

seems right

>    }
> +
> +  /* Finally, we do a full A* search if nothing else worked.  This is
> copied from
> +      find_the_shortest_path but we use real_map_distance as a minimum
> +      distance estimator for the A* search.  This distance estimator
> is used for
> +      the cost value in the queue, but is not stored in the warmap
> itself.  We could
> +      add yet another optimization by having the warmap store the full
> cost (current
> +      cost + minimum leftover cost). */
> +  /* Note, A* is possible here but not in a normal FreeCiv search
> because planes
> +      always take 1 movement unit to move - which is not true of land
> units. */
>    freelog(LOG_DEBUG, "didn't work. Lets try full");
>  
>      init_warmap(src_x, src_y, AIR_MOVING);
> +    /* The 0 is inaccurate, but it doesn't matter. */
>      add_to_mapqueue(0, src_x, src_y);

why 0 is inaccurate?

>      while (get_from_mapqueue(&x, &y)) {
> @@ -1537,23 +1493,22 @@
>       break;
>  
>        adjc_dir_iterate(x, y, x1, y1, dir) {
>       if (warmap.cost[x1][y1] == MAXCOST) {
> +       struct tile *ptile = map_get_tile(x1, y1);
> +       struct unit *enemy = map_get_known_and_seen(x1, y1, pplayer)  ?
> is_non_allied_unit_tile(ptile, pplayer) : NULL;
> +
> +       /* We allow attack goto's, but otherwise we'll refuse to goto
> +           through unknown territory or an enemy unit. */
> +       if ((map_get_known(x1, y1, pplayer) && !enemy)
>             || (x1 == dest_x && y1 == dest_y)) { /* allow attack goto's */
> +         int cost = warmap.cost[x1][y1] = warmap.cost[x][y] + 1;

Maybe add a comment that using 1 instead of SINGLE_MOVE is ok here
because we only deal with the planes?

> +         add_to_mapqueue(cost + real_map_distance(x1, y1, dest_x, dest_y),
> x1, y1);
>         }
>       }
>        } adjc_dir_iterate_end;
>      }
>  
>      freelog(LOG_DEBUG, "movecost: %i", warmap.cost[dest_x][dest_y]);
>    return (warmap.cost[dest_x][dest_y] <= moves)?
>      moves - warmap.cost[dest_x][dest_y]: -1;
>  }

I think you improved the code _a_lot_.  However we should test the patch
after the final version is reached.

Great work,
G.

__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page from News and Sport to Email and 
Music Charts
http://uk.my.yahoo.com