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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:

Background: I replaced the code

  x = map_adjust_x(x);
  y = map_adjust_y(y);

with

  assert(is_real_tile(x, y));
  if (!normalize_map_pos(&x, &y))
    return;

> 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.

I still hold that rather than introducing bugs, what my patch may do is
bring existing bugs to light.

> 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.

Correct.

> 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.

This is not one of those locations.  The current code is equivalent to a
nearest_real_pos().

> 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.

We certainly could call nearest_real_pos() here, however that is most
definitely not the intended behavior.  The code I've proposed will catch
any bad coordinates passed with an assert() while continuing to behave
sanely when in non-debug mode.

> 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.

Unfortunately, much of the behavior is not commented and often the use
of map_adjust_* is wrong.  I've seen a number of cases where
x=map_adjust(x),y=map_adjust(y) is used in place of
normalize_map_pos(&x, &y).  The code is assuming that real coordinates
are being passed in, but the writer decided to go ahead and call
map_adjust_y anyway (perhaps for safety, perhaps out of
misunderstanding).  Having an explicit check is definitely safer, not
less safe.

Other places in the code just called x=map_adjust_x(x) while leaving y
untouched.  They did this so that the tile could be checked for realness
separately, or sometimes because they assumed that a real coordinate set
was being passed.  Again, having an explicit check is better.

> 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.

My goal is to make code that will work correctly under the new system,
continue to work sanely under the current system, and allow us to easily
catch any bugs.

I believe this code does so.  Any time unreal coordinates are passed to
refresh_map_canvas, that is clearly a bug.  There is no way the
normalization code in refresh_map_canvas is intended to be a
nearest_real_pos.  There is some chance that unreal coordinates are
being passed in to draw a black tile, but that's not happening anyway
right now so that's OK.

If bad coordinates are passed in, the function will handle them sanely:
asserting if in debug mode, just returning otherwise.  If there are any
problems, the assertion will let us easily find them.

jason