[Freeciv-Dev] Re: review of corecleanup_06

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

Ok, I see the last draw_tile_pixmap() line now. And for this example
you are correct that the presumed action may not be what is being
executed in one case or the other.

But the executed action may actually be what is desired in case 1. I'd
need to understand whether the GUI was looping over virtual coordinates
it wanted drawn to a real screen. Or whether it was looping over its
screen coordinates for which it wanted real data tiles. Both are not
unreasonable for callers of draw_tile given the limited context here.

Note, in a Flat-Earth or Donut-World, the GUI doesn't crash with the 
patches applied, but neither are its scrolling operations particularly
sensible. I think a lot of what is does is hardcoded to MAP_WRAP_X, and 
generalizing it to handle the 4 rectangular topologies will turn up and 
hopefully result in the correct fixes for a lot of these cases.

Cheers,
RossW
=====

At 02:10 AM 01/08/24 -0400, Jason Dorje Short wrote:
>"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).

You mean both to an unreal value, not 0,0. I think you are pushing
"indeterminate" too far if not.

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

I don't think there is an absolute right/wrong here. I think one needs to 
look a little more closely at each case that is being fixed.

But I don't want to get drawn into cleanuping the GUI stuff myself :-).

Cheers,
RossW
=====