[Freeciv-Dev] what's broken and what's not (was: Re: remove map_adjust_[xy] invocations (PR#1130))

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Ross W. Wetmore wrote:

Ross - I'll take a look at the new corecleanups tomorrow with regard to 
this.  I don't doubt there's a lot I can learn from it.

> > > This change is a little confusing, but correct for several reasons. 
> > > First off, the use of tile_visible_mapcanvas is completely spurious in 
> > > the first piece of code - it has the same return value as get_canvas_xy 
> > > (in fact for iso-view it is just a wrapper for get_canvas_xy).  Second, 
> > > note that if we want to draw black tiles, we can't (or shouldn't) call 
> > > normalize_map_pos to wrap, since it may change the x and y coordinates 
> > > of an unreal position. 
> Only because you broke normalize_map_pos(). Fix that function and you 
> won't have this problem, as well as improving its performance.
> In general, put_tile() functions should take any coordinates and do the 
> normalization themselves. They are called enough, that duplicating the 
> normalization and realness checks everywhere is a mess of code bloat.
> It is also the case that you should let put_tile() functions put black
> tiles for unreal positions that lie in the gui window, for this you 
> need to give it unreal coordinates. Having separate code to handle the
> unreal cases is foolish duplication. (See corecleanup_10)
First off, I haven't changed normalize_map_pos at all.  But note, 
though, that it doesn't specify what values x and y become if it returns 
0.  It is undefined.  So if any code uses the values after it has 
returned 0, that code is wrong.  There's a good reason for this, too: if 
you _were_ to specify what the x and y values were to become, how would 
you define them?  (1) unchanged, (2) x value wrapped, y unchanged, or 
(3) x value wrapped, y value clipped?  All three of these have valid 
arguments in favor of them.  The only conclusion is that all should be 
avoided.
Fortunately, I don't think the code has this problem anywhere.  The 
drawing code in put_tile() is something like this:
void put_tile(abs_x, abs_y, canvas_x, canvas_y)
{
  int map_x = abs_x, map_y = abs_y;
  if (normalize_map_pos(&map_x, &map_y))
    put_real_tile(map_x, map_y, canvas_x, canvas_y);
  else
    put_black_tile(canvas_x, canvas_y);
}


so AFAIK there is no code that is "bad" in this regard.


--------------------------

But here's how the drawing code is broken, though, and I think you'll 
agree with me on this.  There is currently one function, get_canvas_xy, 
that converts a map position to a canvas position.  It is intended to 
take into account everything (wrapping, realness, etc.) and return the 
"correct" canvas position.  Unfortunately, what the correct canvas 
position is depends on how we're using the function.
There are two ways the GUI draws to the screen.  The first is when it 
loops over "absolute" GUI coordinates, drawing a tile for each position 
to update a whole segment of the screen.  For each position, it should 
take the gui coordinates (gui_x, gui_y), convert them to map coordinates 
(abs_x, abs_y), and convert them to canvas coordinates (canvas_x, 
canvas_y).  Since we're talking about a conversion from _gui_ 
coordinates to canvas coordinates, no wrapping should be necessary, and 
unreal tiles should be drawn correctly.  We only do a normalize_map_pos 
at the very end to figure out what it is we're looking at.  If we want 
to limit ourselves to drawing a tile only once, we should (but don't) do 
it by limiting the bounds for the loop.
The second way is that we have a map position (map_x, map_y) that we 
wish to update.  In this case we need to convert this to a gui position, 
using an appropriate wrapping function 
(find_representative_map_pos/unnormalize_map_pos), then do a straight 
conversion from this to canvas coordinates.  Here we don't even need to 
worry about unreal coordinates.
Now, these two cases are very different and should be handled by 
entirely different code.  The problem (and it's a big one) is that 
get_canvas_xy is supposed to handle both cases.  AFAICT this was 
originally written this way as an "easy" way to avoid drawing a tile 
multiple times: since get_canvas_xy will wrap coordinates, we don't even 
have to worry about bounding the loop in the first-case drawing 
scenario.  But the result is that get_canvas_xy has to both (1) check if 
the coordinate is real, and if not then just treat it as-is and (2) wrap 
it into the appropriate representative set (using an appropriate 
wrapping function).
The result is that writing get_canvas_xy using topology functions 
(namely, find_representative_map_pos) is very ugly and probably very 
slow as well.  This will become apparent as soon as I propose a patch to 
make this change.
jason