[Freeciv-Dev] Re: get_canvas_xy unification (PR#1054)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

"Ross W. Wetmore" wrote:
> 
> This function is better split into two functions, one a map to gui/canvas
> coordinates, and the second a scaling/clipping of the canvas coordinates.

Perhaps.  But a first step toward such a split would be unifying the
code among different platforms, don't you agree?

> There is a minor bug in the map (pre-)normalization step.

See below.

> At 07:14 PM 01/11/04 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> >The attached patch makes a new common function, base_get_canvas_xy,
> >which is called by get_canvas_xy for each GUI platform.
> >
> >There are several advantages to this: it reduces overall code,
> >consolidates places where changes will have to be made in the GUI, and
> >provides isometric-view capabilities to this function for all GUI's.
> >
> >get_canvas_xy is kept around because the variables needed by
> >base_get_canvas_xy are not common.  But they should be.  I would propose
> >a naming system that is a cross between the win32 and gtk current
> >systems: map_view_x0/map_view_y0 for the top-left window corner,
> >map_win_width/map_win_height for the canvas width and height.  Currently
> >map_canvas_store_twidth/map_canvas_store_theight are used for the canvas
> >width in tiles; these are misnamed but I'm not sure what would be better
> >(map_win_twidth/map_win_theight, perhaps?).  This patch is a big step
> >toward an eventual total unification.
> >
> >Platforms tested: gtk
> >Untested: xaw (I'll test shortly), win32, mui
> >Unaffected: beos
> >
> >jason? rc
> >? diff
> >? old
> >? topology
> >Index: client/mapview_common.c
> >===================================================================
> >RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
> >retrieving revision 1.1
> >diff -u -r1.1 mapview_common.c
> >--- client/mapview_common.c    2001/10/30 12:11:43     1.1
> >+++ client/mapview_common.c    2001/11/05 03:04:10
> >@@ -21,6 +21,7 @@
> > #include "mapview_g.h"
> >
> > #include "mapview_common.h"
> >+#include "tilespec.h"
> >
> > /**************************************************************************
> >  Refreshes a single tile on the map canvas.
> >@@ -77,5 +78,67 @@
> >     return COLOR_STD_RED;
> >   } else {
> >     return COLOR_STD_WHITE;
> >+  }
> >+}
> >+
> >+int base_get_canvas_xy(int map_x, int map_y,
> >+                     int *canvas_x, int *canvas_y,
> >+                     int map_view_x0, int map_view_y0,
> >+                     int canvas_width, int canvas_height)
> >+{
> >+  if (is_isometric) {
> >+    /* canvas_x,canvas_y is the top corner of the actual tile, not the
> pixmap.
> >+       This function also handles non-adjusted tile coords (like -1, -2)
> as if
> >+       they were adjusted.  You might want to first take a look at the
> simpler
> >+       city_get_canvas() for basic understanding. */
> >+    int diff_xy;
> >+    int diff_x, diff_y;
> >+
> >+    map_x %= map.xsize;
> 
> I believe you want to insure this value is in the range [0:xsize-1].
> 
> If you are passing normal coordinates, you don't need this. If you are
> not it should be corrected.

Hmmm, you may be correct.  Note that this code is copied directly from
current code, for which no bugs have been reported.  The line
immediately below (which you have clipped) says

  if (map_x < map_view_x0)
    map_x += map.xsize;

which at first glance seems to take care of the less-than-zero case. 
But really, it doesn't: if map_view_x0==map.xsize-2 and
map_x%map.xsize==-map.xsize+1 then you'll have to add on map.xsize twice
to get the desired results.

The obvious inference is that the function is never passed negative
coordinates (although it may be passed large positive coordinates, which
should be fine).  At the minimum, though, an assert(map_x>=0) should be
added.

jason