[Freeciv-Dev] Re: review of corecleanup_06

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> At 11:34 PM 01/08/23 -0400, Jason Dorje Short wrote:

Sorry; the additional use of is_real_tile in the first set of code is
confusing.  The correct code should be

void draw_tile(int x, int y)
{
       ...
       if (normalize_map_pos(&x, &y))
               draw_black_unreal_tile(x, y);
       else {
               tile *ptile = get_map_pos(x, y);
               pixmap *tile_pixmap = get_pixmap_for_tile(ptile);
               draw_tile_pixmap(x, y, pixmap);
       }
}

void draw_tile(int x, int y)
{
       int mapx = x, mapy = y;
       ...
       if (normalize_map_pos(&mapx, &mapy))
               draw_black_unreal_tile(x, y);
       else {
               tile *ptile = get_map_pos(mapx, mapy);
               pixmap *tile_pixmap = get_pixmap_for_tile(ptile);
               draw_tile_pixmap(x, y, pixmap);
       }
}

Now do you see why the first set of code won't work?  It draws the
pixmap not to the *absolute* coordinates that it was given but to the
*normalized* coordinates.  In the case of unreal tiles it just happens
to get things right because normalize_map_pos does not change the values
of x and y.  In the case of real tiles it will never draw outside of the
"normal" map rectangle, so you can never look at a full wrap-around on
one screen.  Oddly enough, this is almost exactly the same problem that
the GUI has now.  If you remove mapx/mapy from the second function, it
will stop working.

> >Counting on normalize_map_pos to leave X and Y values of unreal
> >coordinates alone is both unsafe (since it certainly does not guarantee
> >this condition) and a sign of problems elsewhere (as described above).
> 
> Wrong on both counts.
> 
> It does currently guarantee safeness. There is an optimization if you
> aren't worried about safeness, but this has carefully not been implemented
> in all flavours of normalize+map_pos to date.

The code in normalize_map_pos guarantees safety, but the definition of
normalize_map_pos does not (AFAIK) guarantee this.  If normalize_map_pos
set x and y to both be zero if the realness check is failed, then
everything should continue to work (should, but won't).

> >Calling code should treat x and y as undefined values after a call to
> >normalize_map_pos(&x, &y) fails.  (I believe this is the most "correct"
> >solution, but it's easier just to leave the values as-is and hope
> >everything works out.)
> 
> That is the safest association for (new) code to make. But I don't think
> it is required, and existing code should not be "corrected" to this
> extreme position without careful checking.

The problem is that this cavalier treatment of the values is exactly the
same thing that can lead to the quite-wrong pseudo-code above, and what
appears to lead in the client (I'm looking at pixmap_put_tile() and
get_canvas_xy()) to the exact same problem!

Maybe a patch to fix it will convince you?

jason