[Freeciv-Dev] Re: [PATCH] cleanup to canvas_pos_to_map_pos (PR#1183)

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Ross W. Wetmore wrote:

> At 10:39 PM 01/12/31 -0500, Jason Short wrote:
>
> > Ross W. Wetmore wrote:
[snip: off-by-one error in canvas_pos_to_[city|map]_pos]


> Thus the divide isssue is really a red herring and not affecting anything,
> except that you probably introduce a bug with yours when it does kick in,
> and I suspect that you really haven't got a (0,0) tile center/origin 
> roundoff in your case, but that depends on how you defined map_view_x0,y0 
> to some extent.
No, think about this again.


Canvas coordinates are always positive for both the city and map 
conversions.  Semantics aside, the transformation we're both doing is then:
  /* Initial scaling */
  canvas_y *= 2;

  /* Rotation */
  rotated_canvas_x = canvas_x + canvas_y;
  rotated_canvas_y = canvas_y - canvas_x;

  /* Final scaling */
  map_x = rotated_canvas_x / NORMAL_TILE_WIDTH;
  map_y = rotated_canvas_y / NORMAL_TILE_WIDTH;

the divide error most certainly comes into play, because the value we're 
dividing is not the canvas position but the rotated canvas position. 
The rotated_canvas_y value can be negative if canvas_x > canvas_y, and 
it is this case that I got the off-by-one error.  Your code is slightly 
different because you add a "roundoff" value to the rotated_canvas_* 
values before dividing, so you'll see the off-by-one error in a 
different place.  When I try out the corecleanups, I see the off-by-one 
in the upper right of the mapview and the lower left of the cityview.

Now, you say that each tile isn't quite 
NORMAL_TILE_WIDTHxNORMAL_TILE_HEIGHT in size, and so we have to fudge 
things with a roundoff factor to get things to work out.  I'm pretty 
sure this is incorrect as well.  First of all, I don't see this problem 
at all; my code seems to work fine :-).  But more technically, if this 
were the case it would be necessary for the same effect to be present in 
map_pos_to_canvas_pos.  These functions 
(map_pos_to_canvas_pos/canvas_pos_to_map_pos) use algorithms that are 
exact inverses, so any effect in one must be present in the other.  I 
really think you're making things more complicated than they are here 
(and perhaps confusing the off-by-one error for a scaling problem).

jason