[Freeciv-Dev] Re: GUI PATCH: refresh_tile_mapcanvas cleanup

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

I agree that the map_adjust_y() here is bad. Your code is slightly
better in not doing a bogus translation. But may still be introducing
bugs.

Many of the locations in the GUI specifically warn that map_adjust_y()
is *not* being called. They specifically want to be called with unreal
map coordinates, detect it and draw black tiles to fill out the GUI
window beyond the current gameboard.

In this case it may be intentional or not, and may or may not be the
original code. I haven't checked back in CVS here. 

If you look into overview_update_tile, there are a number of other 
coordinate transformations to get to screen coordinates which may 
handle clips just fine. And the tile_is_visible() definitely checks 
for a lot of bogus things. So your early exit may not be required.

But someone should check this out before they change the behaviour
and in this case I things several opinions are needed if you can't
find anyone that wrote or understood the original GUI code.

The original updates to normalize_map_pos() were careful to continue
the commented behaviour, in not using bogus, nearest_real_pos()
equivalents, but instead returned the unreal coordinates untouched.
More recent ones in CVS are doing strange things, so you may need to
fix things properly, or add hacks in places like this.

Note, you are quite possibly correct, my hunch lies this way. 

Just check it out ... and in the broader context of all the other
cases that are explicitly commented to work differently from this one.

Cheers,
RossW
=====

At 12:38 AM 01/10/13 -0400, Jason Dorje Short wrote:
>"Ross W. Wetmore" wrote:
>> 
>> You may have introduced a few bugs, see explanation below :-).
>> >+ GUI Independent (with new access functions).  However, this should go
>> >+ in client/mapview.c instead of client/climisc.c.
>>
>+**************************************************************************/
>> >+void refresh_tile_mapcanvas(int x, int y, int write_to_screen)
>> >+{
>> >+  assert(is_real_tile(x, y));
>> >+  if (!normalize_map_pos(&x, &y))
>> >+    return;
>> >+
>> >+  if (tile_visible_mapcanvas(x, y)) {
>> >+    update_map_canvas(x,y, 1, 1, write_to_screen);
>> >+  }
>> >+  overview_update_tile(x, y);
>> >+}
>> 
>> I don't think this is a correct interpretation of the original code.
>> 
>> If you remove the assert(is_real_tile() and if condition around
>> normalize_map_pos() then I am much happier with it.
>> 
>> But I don't understand the GUI, so my belief that the code checking
>> in tile_visible_mapcanvas() and overview_update_tile() that expects
>> to draw unreal tiles as black tiles in the GUI window may be in
>> error.
>> 
>> But I would get someone to confirm this before you really change the
>> existing code flow by stopping the GUI from ever drawing unreal tiles
>> where it expects (and maybe needs) to.
>
>Look again.  My proposed change is not identical to current code, but
>I'm pretty positive it's the intended behavior.  The current code will
>never draw an unreal or non-normal tile; if passed an unreal position it
>will convert it to a real, normal tile (using the equivalent of
>nearest_real_pos).  This is surely not intended; the only reason
>map_adjust_[xy] is called is to wrap a non-normal real map position.  So
>the current code, if it is bogusly called with an unreal position, will
>simply redraw some random nearby real tile.
>
>> [...]
>> >Index: client/gui-gtk/mapview.c
>> >===================================================================
>> >RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v

>> >retrieving revision 1.103
>> >diff -u -r1.103 mapview.c
>> >--- client/gui-gtk/mapview.c   2001/10/05 09:47:41     1.103
>> >+++ client/gui-gtk/mapview.c   2001/10/10 15:46:23
>> >@@ -640,21 +640,6 @@
>> >
/**************************************************************************
>> > ...
>> >
**************************************************************************/
>> >-void refresh_tile_mapcanvas(int x, int y, int write_to_screen)
>> >-{
>> >-  x = map_adjust_x(x);
>> >-  y = map_adjust_y(y);
>> >-
>> >-  if (tile_visible_mapcanvas(x, y)) {
>> >-    update_map_canvas(x, y, 1, 1, write_to_screen);
>> >-  }
>> >-  overview_update_tile(x, y);
>> >-}
>
>jason