[Freeciv-Dev] Re: (PR#4729) cleanup to canvas<->city pos functions

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Worthwhile, but fix the one inconsistency which is actually I think a
future raft of bugs in hiding.

Cheers,
RossW
=====

Jason Short wrote:
> This patch cleans up the canvas_to_city_pos and city_to_canvas_pos 
> functions:
> 
> - They are renamed so that the extra "pos" is removed.  Thus they 
> correspond to the naming system used everywhere else.

good

> - The target coordinate pointers are passed as the first parameters. 
> Thus they correspond to the parameter ordering used everywhere else.

good

> - canvas_to_city_pos returns a boolean.  The users check this value. 
> Thus we don't try to use invalid city positions.  Formerly this value 
> was used, leading to a spurious network packet (this buglet is caught by 
> an assertion in PR#4723).

good

> (Note city_to_canvas_pos could return a boolean, for consistency, but it 
> should always be TRUE.  We should never have to deal with off-canvas 
> tiles at all.)

bad (for three reasons besides the final foolish wishlist sentence)

1)  It is not always TRUE, and/or forcing coders to insure this condition
leads to all sorts of ad hoc ways to check or miss the checks.

2)  All the coordinate functions should return a standard boolean that
confirms that the transformation was successful (even if it seems like it
cannot fail). Thus one never has to worry about which does and doesn't.
and never has to update code when a future update adds something that
changes this. Think about interfaces, that means thinking about the future
and not just the present or past.

3)  It would not hurt to return the condition of tile_visible_map_canvas()
for city_to_canvas_pos(), or the equivalent clip part of map_to_canvas_pos()
since it is perfectly reasonable for one to iterate over city coordinates in
a canvas setting without doing either a checked iteration (to guarantee any
city tile is on the map, and a city can overlap the map edge), or that it
lies within the current gui window. One can always ignore the boolean, but
it would seem like it is more often useful than not and should be a quick
test.
     In fact, I would probably return the clip() condition which is valid in
all contexts of this transformation, and make a second
city_to_canvas_pos_checked() that calls this and adds the map condition.
This is much the same as the city iterators that can run in full city or
map constrained modes.

> - Parameters of canvas_to_city_pos are renamed as city_x,city_y instead 
> of map_x,map_y.

good

> - Minor style fixes.
> 
> Tested under GTK and XAW clients.
> 
> jason
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: client/citydlg_common.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/citydlg_common.c,v
> retrieving revision 1.20
> diff -u -r1.20 citydlg_common.c
> --- client/citydlg_common.c   2003/07/31 21:05:36     1.20
> +++ client/citydlg_common.c   2003/07/31 21:31:14
> @@ -55,8 +55,7 @@
>  This converts a city coordinate position to citymap canvas coordinates
>  (either isometric or overhead).  It should be in cityview.c instead.
>  **************************************************************************/
> -void city_pos_to_canvas_pos(int city_x, int city_y, int *canvas_x,
> -                         int *canvas_y)
> +void city_to_canvas_pos(int *canvas_x, int *canvas_y, int city_x, int city_y)
>  {
>    if (is_isometric) {
>      /*
> @@ -80,7 +79,7 @@
>  This converts a citymap canvas position to a city coordinate position
>  (either isometric or overhead).  It should be in cityview.c instead.
>  **************************************************************************/
> -void canvas_pos_to_city_pos(int canvas_x, int canvas_y, int *map_x, int 
> *map_y)
> +bool canvas_to_city_pos(int *city_x, int *city_y, int canvas_x, int canvas_y)
>  {
>    int orig_canvas_x = canvas_x, orig_canvas_y = canvas_y;
>  
> @@ -93,19 +92,21 @@
>  
>      /* Perform a pi/4 rotation, with scaling.  See canvas_pos_to_map_pos
>         for a full explanation. */
> -    *map_x = DIVIDE(canvas_x * H + canvas_y * W, W * H);
> -    *map_y = DIVIDE(canvas_y * W - canvas_x * H, W * H);
> +    *city_x = DIVIDE(canvas_x * H + canvas_y * W, W * H);
> +    *city_y = DIVIDE(canvas_y * W - canvas_x * H, W * H);
>  
>      /* Add on the offset of the top-left corner to get the final
> -       coordinates (like in canvas_pos_to_map_pos). */
> -    *map_x -= 2;
> -    *map_y += 2;
> +     * coordinates (like in canvas_to_map_pos). */
> +    *city_x -= 2;
> +    *city_y += 2;
>    } else {
> -    *map_x = canvas_x / NORMAL_TILE_WIDTH;
> -    *map_y = canvas_y / NORMAL_TILE_HEIGHT;
> +    *city_x = canvas_x / NORMAL_TILE_WIDTH;
> +    *city_y = canvas_y / NORMAL_TILE_HEIGHT;
>    }
> -  freelog(LOG_DEBUG, "canvas_pos_to_city_pos(pos=(%d,%d))=(%d,%d)",
> -       orig_canvas_x, orig_canvas_y, *map_x, *map_y);
> +  freelog(LOG_DEBUG, "canvas_to_city_pos(pos=(%d,%d))=(%d,%d)",
> +       orig_canvas_x, orig_canvas_y, *city_x, *city_y);
> +
> +  return is_valid_city_coords(*city_x, *city_y);
>  }
>  
>  /**************************************************************************
> Index: client/citydlg_common.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/citydlg_common.h,v
> retrieving revision 1.13
> diff -u -r1.13 citydlg_common.h
> --- client/citydlg_common.h   2003/07/31 21:05:36     1.13
> +++ client/citydlg_common.h   2003/07/31 21:31:14
> @@ -36,8 +36,10 @@
>  int get_citydlg_canvas_width(void);
>  int get_citydlg_canvas_height(void);
>  
> -void city_pos_to_canvas_pos(int city_x, int city_y, int *canvas_x, int 
> *canvas_y);
> -void canvas_pos_to_city_pos(int canvas_x, int canvas_y, int *map_x, int 
> *map_y);
> +void city_to_canvas_pos(int *canvas_x, int *canvas_y,
> +                     int city_x, int city_y);
> +bool canvas_to_city_pos(int *city_x, int *city_y,
> +                     int canvas_x, int canvas_y);
>  
>  void get_city_dialog_production(struct city *pcity,
>                                  char *buffer, size_t buffer_len);
> Index: client/mapview_common.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
> retrieving revision 1.55
> diff -u -r1.55 mapview_common.c
> --- client/mapview_common.c   2003/07/28 04:10:56     1.55
> +++ client/mapview_common.c   2003/07/31 21:31:14
> @@ -190,8 +190,7 @@
>    }
>  
>    if (is_isometric) {
> -    /* For a simpler example of this math, see
> -       city_pos_to_canvas_pos(). */
> +    /* For a simpler example of this math, see city_to_canvas_pos(). */
>      int iso_x, iso_y;
>  
>      /*
> @@ -272,7 +271,7 @@
>       * only use integer math, and C integer division rounds toward zero
>       * instead of rounding down.
>       *
> -     * For another example of this math, see canvas_pos_to_city_pos().
> +     * For another example of this math, see canvas_to_city_pos().
>       */
>      *map_x = DIVIDE(canvas_x * H + canvas_y * W, W * H);
>      *map_y = DIVIDE(canvas_y * W - canvas_x * H, W * H);
> Index: client/gui-gtk/citydlg.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/citydlg.c,v
> retrieving revision 1.167
> diff -u -r1.167 citydlg.c
> --- client/gui-gtk/citydlg.c  2003/07/31 21:05:37     1.167
> +++ client/gui-gtk/citydlg.c  2003/07/31 21:31:14
> @@ -1778,7 +1778,8 @@
>         && city_map_to_map(&map_x, &map_y, pcity, city_x, city_y)) {
>       if (tile_get_known(map_x, map_y)) {
>         int canvas_x, canvas_y;
> -       city_pos_to_canvas_pos(city_x, city_y, &canvas_x, &canvas_y);
> +
> +       city_to_canvas_pos(&canvas_x, &canvas_y, city_x, city_y);
>         put_one_tile_full(pdialog->map_canvas_store, map_x, map_y,
>                           canvas_x, canvas_y, 1);
>       }
> @@ -1789,7 +1790,8 @@
>    city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>      if (tile_get_known(map_x, map_y)) {
>        int canvas_x, canvas_y;
> -      city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +      city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>        if (pcity->city_map[x][y] == C_TILE_WORKER) {
>       put_city_tile_output(pdialog->map_canvas_store,
>                            canvas_x, canvas_y,
> @@ -1808,7 +1810,8 @@
>    city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>      if (tile_get_known(map_x, map_y)) {
>        int canvas_x, canvas_y;
> -      city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +      city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>        if (pcity->city_map[x][y] == C_TILE_UNAVAILABLE) {
>       pixmap_frame_tile_red(pdialog->map_canvas_store,
>                             canvas_x, canvas_y);
> @@ -2793,8 +2796,9 @@
>    if (pcity) {
>      int xtile, ytile;
>  
> -    canvas_pos_to_city_pos(ev->x, ev->y, &xtile, &ytile);
> -    city_toggle_worker(pcity, xtile, ytile);
> +    if (canvas_to_city_pos(&xtile, &ytile, ev->x, ev->y)) {
> +      city_toggle_worker(pcity, xtile, ytile);
> +    }
>    }
>    return TRUE;
>  }
> Index: client/gui-gtk-2.0/citydlg.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk-2.0/citydlg.c,v
> retrieving revision 1.60
> diff -u -r1.60 citydlg.c
> --- client/gui-gtk-2.0/citydlg.c      2003/07/31 21:05:37     1.60
> +++ client/gui-gtk-2.0/citydlg.c      2003/07/31 21:31:14
> @@ -1376,7 +1376,8 @@
>         && city_map_to_map(&map_x, &map_y, pcity, city_x, city_y)) {
>       if (tile_get_known(map_x, map_y)) {
>         int canvas_x, canvas_y;
> -       city_pos_to_canvas_pos(city_x, city_y, &canvas_x, &canvas_y);
> +
> +       city_to_canvas_pos(&canvas_x, &canvas_y, city_x, city_y);
>         put_one_tile_full(pdialog->map_canvas_store, map_x, map_y,
>                           canvas_x, canvas_y, 1);
>       }
> @@ -1387,7 +1388,8 @@
>    city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>      if (tile_get_known(map_x, map_y)) {
>        int canvas_x, canvas_y;
> -      city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +      city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>        if (pcity->city_map[x][y] == C_TILE_WORKER) {
>       put_city_tile_output(pdialog->map_canvas_store,
>                            canvas_x, canvas_y,
> @@ -1406,7 +1408,8 @@
>    city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>      if (tile_get_known(map_x, map_y)) {
>        int canvas_x, canvas_y;
> -      city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +      city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>        if (pcity->city_map[x][y] == C_TILE_UNAVAILABLE) {
>       pixmap_frame_tile_red(pdialog->map_canvas_store,
>                             canvas_x, canvas_y);
> @@ -2328,8 +2331,9 @@
>    if (pcity) {
>      int xtile, ytile;
>  
> -    canvas_pos_to_city_pos(ev->x, ev->y, &xtile, &ytile);
> -    city_toggle_worker(pcity, xtile, ytile);
> +    if (canvas_to_city_pos(&xtile, &ytile, ev->x, ev->y)) {
> +      city_toggle_worker(pcity, xtile, ytile);
> +    }
>    }
>    return TRUE;
>  }
> Index: client/gui-mui/mapclass.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/mapclass.c,v
> retrieving revision 1.94
> diff -u -r1.94 mapclass.c
> --- client/gui-mui/mapclass.c 2003/05/05 12:11:12     1.94
> +++ client/gui-mui/mapclass.c 2003/07/31 21:31:14
> @@ -2143,7 +2143,8 @@
>        city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>       if (tile_get_known(map_x, map_y)) {
>         int canvas_x, canvas_y;
> -       city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +       city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>         put_one_tile_full(_rp(o), map_x, map_y, canvas_x + _mleft(o), 
> canvas_y + _mtop(o), 1);
>       }
>        } city_map_checked_iterate_end;
> @@ -2152,7 +2153,8 @@
>        city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>       if (tile_get_known(map_x, map_y)) {
>         int canvas_x, canvas_y;
> -       city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +       city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>         if (pcity->city_map[x][y]==C_TILE_WORKER) {
>           put_city_output_tile(_rp(o),
>                            city_get_food_tile(x, y, pcity),
> @@ -2171,8 +2173,9 @@
>       if (tile_get_known(map_x, map_y))
>       {
>         int canvas_x, canvas_y;
> -       city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
>  
> +       city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
> +
>            canvas_x += _mleft(o);
>            canvas_y += _mtop(o);
>         if(pcity->city_map[x][y]==C_TILE_UNAVAILABLE)
> @@ -2258,10 +2261,13 @@
>       if (_isinobject(msg->imsg->MouseX, msg->imsg->MouseY))
>       {
>         int x,y;
> -       canvas_pos_to_city_pos(msg->imsg->MouseX - _mleft(o), 
> msg->imsg->MouseY - _mtop(o), &x, &y);
> -       data->click.x = x;
> -       data->click.y = y;
> -       set(o, MUIA_CityMap_Click, &data->click);
> +       if (canvas_to_city_pos(&x, &y,
> +                              msg->imsg->MouseX - _mleft(o),
> +                              msg->imsg->MouseY - _mtop(o))) {
> +         data->click.x = x;
> +         data->click.y = y;
> +         set(o, MUIA_CityMap_Click, &data->click);
> +       }
>       }
>        }
>        break;
> Index: client/gui-win32/citydlg.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/gui-win32/citydlg.c,v
> retrieving revision 1.56
> diff -u -r1.56 citydlg.c
> --- client/gui-win32/citydlg.c        2003/07/31 21:05:37     1.56
> +++ client/gui-win32/citydlg.c        2003/07/31 21:31:14
> @@ -506,7 +506,8 @@
>            && city_map_to_map(&map_x, &map_y, pcity, city_x, city_y)) {
>          if (tile_get_known(map_x, map_y)) {
>            int canvas_x, canvas_y;
> -          city_pos_to_canvas_pos(city_x, city_y, &canvas_x, &canvas_y);
> +
> +          city_to_canvas_pos(&canvas_x, &canvas_y, city_x, city_y);
>            put_one_tile_full(hdc, map_x, map_y,
>                              canvas_x, canvas_y, 1);
>          }
> @@ -517,7 +518,8 @@
>    city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>      if (tile_get_known(map_x, map_y)) {
>        int canvas_x, canvas_y;
> -      city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +      city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>        if (pcity->city_map[x][y]==C_TILE_WORKER) {
>          put_city_tile_output(hdc,
>                               canvas_x, canvas_y,
> @@ -535,7 +537,8 @@
>    city_map_checked_iterate(pcity->x, pcity->y, x, y, map_x, map_y) {
>      if (tile_get_known(map_x, map_y)) {
>        int canvas_x, canvas_y;
> -      city_pos_to_canvas_pos(x, y, &canvas_x, &canvas_y);
> +
> +      city_to_canvas_pos(&canvas_x, &canvas_y, x, y);
>        if (pcity->city_map[x][y]==C_TILE_UNAVAILABLE) {
>          pixmap_frame_tile_red(hdc,
>                             canvas_x, canvas_y);
> @@ -1726,10 +1729,11 @@
>        if ((x>=pdialog->map.x)&&(x<(pdialog->map.x+pdialog->map_w)))
>       {
>         int tile_x,tile_y;
> -       xr=x-pdialog->map.x;
> -       yr=y-pdialog->map.y;
> -       canvas_pos_to_city_pos(xr,yr,&tile_x,&tile_y);
> -       city_toggle_worker(pdialog->pcity, tile_x, tile_y);
> +
> +       if (canvas_to_city_pos(&tile_x, &tile_y,
> +                              pdialog->map.x, pdialog->map.y)) {
> +         city_toggle_worker(pdialog->pcity, tile_x, tile_y);
> +       }
>       }
>      }
>    xr=x-pdialog->pop_x;
> @@ -2240,10 +2244,11 @@
>        if ((x>=pdialog->maph.x)&&(x<(pdialog->maph.x+pdialog->map_w))&&
>         (y>=pdialog->maph.y)&&(y<(pdialog->maph.y+pdialog->map_h))) {
>       int tile_x,tile_y;
> -     canvas_pos_to_city_pos(x-pdialog->maph.x,
> -                            y-pdialog->maph.y,
> -                            &tile_x,&tile_y);
> -     city_toggle_worker(pdialog->pcity, tile_x, tile_y);
> +
> +     if (canvas_to_city_pos(&tile_x, &tile_y,
> +                            x-pdialog->maph.x, y-pdialog->maph.y)) {
> +       city_toggle_worker(pdialog->pcity, tile_x, tile_y);
> +     }
>        }
>        break;
>      case WM_PAINT:
> Index: client/gui-xaw/citydlg.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/citydlg.c,v
> retrieving revision 1.99
> diff -u -r1.99 citydlg.c
> --- client/gui-xaw/citydlg.c  2003/07/31 21:05:37     1.99
> +++ client/gui-xaw/citydlg.c  2003/07/31 21:31:14
> @@ -1885,8 +1885,9 @@
>      if (!cma_is_city_under_agent(pcity, NULL)) {
>        int xtile, ytile;
>  
> -      canvas_pos_to_city_pos(ev->x, ev->y, &xtile, &ytile);
> -      city_toggle_worker(pcity, xtile, ytile);
> +      if (canvas_to_city_pos(&xtile, &ytile, ev->x, ev->y)) {
> +     city_toggle_worker(pcity, xtile, ytile);
> +      }
>      }
>    }
>  }