[Freeciv-Dev] Re: (PR#3924) Bugfix for map/canvas coordination functions

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

a-l@xxxxxxx wrote:
> This patch fixes two bugs:
> one in canvas_to_map_pos(), and one in map_to_canvas_pos().
> Both are relevant only for non-isometric mode.
> 
> If this is not formalized already, I would propose that the
> following should be a defined property of the two functions
> in question:
> 
> That this verification procedure should hold true:
> {
>   map_to_canvas_pos(&x, &y, tile_x1, tile_y1);
> 
>   x += NORMAL_TILE_WIDTH / 2;
>   y += NORMAL_TILE_HEIGHT / 2;
> 
>   canvas_to_map_pos(&tile_x2, &tile_y2, x, y);
> 
>   assert(tile_x1 == tile_x2);
>   assert(tile_y1 == tile_y2);
> }
> For all tiles at all times.

If you define the tile origin as the upper left corner, then I
think you should make sure that any value from [0,NORMAL_TILEWIDTH)
and [0,NORMAL_TILE_HEIGHT) can be added to the initial coordinate
transformation and the result will still be true.

If you define the center of the tile as the tile origin the range
is [-NORMAL_TILE_WIDTH/2,NORMAL_TILE_WIDTH - NORMAL_TILE_WIDTH/2).

> We must add half a tile because of PR#3800, the bounding
> draw box thing. Ie nobody should "fix" PR#3800.

Fix PR#3800 by defining the tile origin precisely and making the
transformation conform to the definition.

> It is currently only valid for the tiles that are
> within the visible mapview canvas at any given time.

This is a bug. The transformation should work on any coordinates
and give a "correct" result. The function should return "FALSE" if
the coordinate is not a valid canvas coordinate like all coordinate
operations and not return a bogus transformation as its result.

The way to think of this is that canvas coordinates have a "FLAT_EARTH"
normal set (no wrapping), and are masked by a given GUI rectangle/shaped
window. Real canvas coordinates are within the allowed window and unreal
ones are outside it. Thus the transformation returns a true/false value
equivalent to normalize_canvas_pos() in addition to an algebraically
correct result for the transformation.

> That makes precise map/canvas coordination impossible for, say, a
> topology independent selection rectangle that is bigger than mapview.
> You could possibly also run into problems when you try to make
> efficient pixel-smooth scrolling by establishing a pixel buffer
> outside the mapview (or so I imagine).
> 
> I don't think any current cvs code trips over these bugs, because none
> of it concerns itself with out-of-sight canvas coordinates.
> 
> 
> More technical description of the bugfixes for the interested:
> 
> Bug 1:  map_to_canvas_pos()
> 
> In this situation:
> 
>   When the wrapping axis (0, tile_y) is within the visible mapview,
>   and not the leftmost column, ie mapview_canvas.map_x0 != 0.
> 
> The problem is:
> 
>   map_to_canvas_pos() only outputs meaningful canvas coordinates for
>   those tiles that are currently within the mapview. All other tiles
>   are assigned a canvas X coordinate of -1 * NORMAL_TILE_WIDTH.
> 
> However, when axis (0, tile_y) is out of sight, map_to_canvas_pos()
> gives meaningful, consistent, canvas coordinates for all tiles,
> sometimes negative.
> 
> Bug 2:  canvas_to_map_pos()
> 
>   The leftmost tile (mapview_canvas.map_x0) is defined by a sum of
>   pixels that is twice the size of the sums that define other tiles.
> 
> Example of correct behaviour:
> 
> Say mapsize x is 80 and tile width is 30 pixels.
> (0, y) is the leftmost tile in the visible mapview:
> 
> TILE      Valid X canvas
> ...
> (79, y) = [-30, -1]
> (0, y)  = [0, 29]
> (1, y)  = [30, 59]
> ...
> 
> Before the bugfix, (0, y) was defined by [-29, 29] which is incorrect.
> 
> 
> Arnstein
> 
> 
> 
> ------------------------------------------------------------------------
> 
> --- cvs-Apr-02/client/mapview_common.c        Thu Apr  3 15:38:37 2003
> +++ bugfix-map-canvas/client/mapview_common.c Thu Apr  3 15:41:04 2003
> @@ -214,7 +214,7 @@
>                                   + mapview_canvas.tile_width)) {
>        *canvas_x = map_x + map.xsize - mapview_canvas.map_x0;
>      } else {
> -      *canvas_x = -1;
> +      *canvas_x = map_x - mapview_canvas.map_x0;
>      }
>  
>      *canvas_y = map_y - mapview_canvas.map_y0;
> @@ -235,6 +235,8 @@
>  **************************************************************************/
>  bool canvas_to_map_pos(int *map_x, int *map_y, int canvas_x, int canvas_y)
>  {
> +  const int W = NORMAL_TILE_WIDTH, H = NORMAL_TILE_HEIGHT;
> +
>    if (is_isometric) {
>      /* The basic operation here is a simple pi/4 rotation; however, we
>       * have to first scale because the tiles have different width and
> @@ -261,13 +263,13 @@
>       *
>       * For another example of this math, see canvas_pos_to_city_pos().
>       */
> -    const int W = NORMAL_TILE_WIDTH, H = NORMAL_TILE_HEIGHT;
> -
>      *map_x = DIVIDE(canvas_x * H + canvas_y * W, W * H);
>      *map_y = DIVIDE(canvas_y * W - canvas_x * H, W * H);
>    } else {                   /* is_isometric */
> -    *map_x = canvas_x / NORMAL_TILE_WIDTH;
> -    *map_y = canvas_y / NORMAL_TILE_HEIGHT;
> +    *map_x = canvas_x / W;
> +    *map_y = canvas_y / H;
> +    if (canvas_x < 0 && canvas_x % W)   *map_x -= 1;
> +    if (canvas_y < 0 && canvas_y % H)   *map_y -= 1;
>    }
>  
>    *map_x += mapview_canvas.map_x0;