[Freeciv-Dev] Re: remove map_adjust_[xy] invocations (PR#1130)

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

Most comments to the map_adjust_canvas_pos() email also apply here
with appropriate context substitution.

At 05:46 PM 01/12/13 +0100, Raimar Falke wrote:
>On Thu, Dec 13, 2001 at 02:31:07AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>> The attached patch removes most of the remaining uses of map_adjust_x 
>> and map_adjust_y.
>> 
>> 
>> Many of the changes follow this form:
>> 
>>    x = map_adjust_x(x);
>>    y = map_adjust_y(y);
>> 
>> -- becomes --
>> 
>>    is_real = normalize_map_pos(&x, &y);
>>    assert(is_real);
>> 
>> These should be self-explanatory.  The only question, really, is whether 
>> a nearest_real_map_pos is intended instead of the normalize_map_pos.  In 
>> all of these cases I think it is not.
>> 
>> 
>> Some of the changes are of this form:
>> 
>>    for (y_itr = y; y_itr < y + height; y_itr++)
>>      for (x_itr = x; x_itr < x + width; x_itr++) {
>>        int map_x = map_adjust_x(x_itr);
>>        int map_y - y_itr;
>>        get_canvas_xy(map_x, map_y, &canvas_x, &canvas_y);
>>        if (tile_visible_mapcanvas(map_x, map_y))
>>          put_tile(..., map_x, map_y, canvas_x, canvas_y);
>>      }
>> 
>> -- becomes --
>> 
>>    for (map_y = y; map_y < y + height; map_y++)
>>      for (map_x = x; map_x < x + width; map_x++)
>>        if (get_canvas_xy(map_x, map_y, &canvas_x, &canvas_y);
>>          put_tile(..., map_x, map_y, canvas_x, canvas_y);

Absolutely correct. I moved the routines around so they all sit next
to each other to make the equivalences glaringly obvious. This means
that those adding fixes can make the duplicate changes, until a rename
patch or macro patch consolidates things..
 
>> 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)

>> Third, put_tile calls normalize_map_pos on its 
>> own, so it's quite safe to pass in the non-normal position.  I'm sure 
>> Ross would point out that this code mixes together steps that should be 
>> kept separate (and he's right), but doing a full cleanup of the code is 
>> really a whole separate issue.

Actually, put_tile() is fine. It has (and needs) both sets of coordinates
map and canvas to do its job, so one should be able to iterate over
either set. In general, put_tile() needs to call both normalize_map_pos()
to access the map values in building the tile, and map_to_canvas_pos() to 
know where to put the resulting pixmap.

It is map_to_canvas_pos() where you sell your pot-au-feu as an artistic
expression of haute cuisine. 

>> Finally, there are a few straggler changes that don't fit either of 
>> these molds.  Make of them what you will - I've done my best to 
>> interpret the original intention of the code, but of course any advice 
>> or criticism on this is welcome.
>> 
>> 
>> For the record, the only map_adjust_x uses left behind this patch are in 
>> map_canvas_adjust_x() and mapview_common.c, both of which will be 
>> handled separately.

Not needed, get rid of them as part of the housecleaning.

>> It really needs testing, especially under the win32 and (if such a thing 
>> even compiles...) mui platforms.  If I've misjudged the intent of the 
>> code, a failed assertion will result and I'll probably need to switch in 
>> nearest_real_map_pos for normalize_map_pos.
>
>Since it loooks like we can't test the mui code what do you think
>about giving the mui code its own map_adjust_[xy] and remove it from
>the rest of the code (which is your goal)?

Do the generic changes there. They are tested elsewhere for concept and
if you are going to change something, this is about as failsafe as you
can get. Using the old flavours may badly break given the other changes
that have been made to remove all normalize_map_pos() fixes.

Use (normalize_map_pos() || nearest_real_pos()) if you want to propagate
the void_tile-ish solution for map_adjust_y() under failure conditions.

>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
> "From what I am reading Win98 and NT5.0 will be getting rid of all that
>  crap anyway. Seems that Microsoft has invented something called TCP/IP and
>  another really revolutionary concept called DNS that eliminates the
>  netbios crap too. All that arping from browsers is going to go away.
>  I also hear rumors that they are on the verge of breakthrough discoveries
>  called NFS, and LPD too. Given enough time and money, they might
>  eventually invent Unix."
>    -- George Bonser in linux-kernel