[Freeciv-Dev] Re: [PATCH RFC] borders-5

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Stewart Adcock wrote:

> Hi,
>
> I've just updated my borders patch to the latest cvs version.  There 
> have been some significant enhancements since the last version I posted.
> I will be extremely busy for the next couple of weeks so am showing you 
> all what I've got now.
> I haven't had a chance to extensively test this patch since today's 
> update, but the earlier version was perfectly stable during a couple of 
> complete games against AI opponents.  I think it is now very close to 
> being finished.  The patch contains some notes regarding what still 
> needs to be done; see README.borders_patch.
> The main thing that needs to be added is support by clients other than 
> GTK with the isometric view.
> I had to move the civ_score() code into the server from common to make 
> it use the new territory claims.  Does this cause problems for anyone? 
> As far as I can see, it is only ever used from the server anyway.
> Okay, so what you all think?
There is a bug in pixmap_put_tile_iso; you ignore the return value of 
normalize_map_pos().  This should never be done: it's an accident 
waiting to happen at best and an impending segfault at worst (as in this 
case).
The following code:

  if (draw_borders) {
    thisowner = map_get_owner(x,y);
    leftx = x-1;
    lefty = y;
    normalize_map_pos(&leftx, &lefty);
    rightx = x;
    righty = y-1;
    normalize_map_pos(&rightx, &righty);
    leftowner = map_get_owner(leftx, lefty);
    rightowner = map_get_owner(rightx, righty);

    if ( draw & D_M_R && thisowner != rightowner &&
         tile_get_known(rightx, righty) ) {
      if (rightowner != MAP_TILE_OWNER_NULL) {
        gdk_gc_set_foreground(border_line_gc,
               colors_standard[nation_get_color(rightowner)]);
        gdk_draw_line(pm, border_line_gc,
                   canvas_x + NORMAL_TILE_WIDTH / 2,
                    canvas_y - 1,
                   canvas_x + NORMAL_TILE_WIDTH,
                   canvas_y + NORMAL_TILE_HEIGHT / 2 - 1);
      }

should either become

  if (draw_borders) {
    thisowner = map_get_owner(x,y);

    rightx = x;
    righty = y-1;
    if (normalize_map_pos(&right_x, &right_y) {
      /* the same code to draw the border */
    }

or could use MAPSTEP as

  if (draw_borders) {
    thisowner = map_get_owner(x,y);

    if (MAPSTEP(right_x, right_y, x, y, DIR8_NORTH) {
      /* the same code to draw the border  */
    }

or (perhaps best of all) you could turn this into a loop to avoid 
duplicating code.  Take a look at adjc_iterate and friends in map.h.
To trigger the segfault, just build a polar city.


Also, I checked out the GTK flat-view, and it looked like there was a 
minor problem with the updating but things were otherwise OK.  However, 
it suffers from the above problem as well (in pixmap_put_tile()).
jason