[Freeciv-Dev] Re: (PR#7304) iso-map support for mapgen

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7304 >


Jason Short wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7304 >
> 
>>[rwetmore@xxxxxxxxxxxx - Tue Jan 27 03:15:14 2004]:
> 
>>In all the above cases these are property lookups where the lookup is made
>>at a particular location (aka tile).
>>
>>There should be one API for returning each property from a tile.
>>
>>There may be many ways to find a tile given various coordinates, thus one
>>would always write something like
>>
>>   nat_get_tile(xn, yn)->get_terrain()
>>   map_get_tile(xn, yn)->get_terrain()
>>
>>If there are 10 properties like terrain, unit_list, printable
> 
> location, ...
> 
>>for each tile and 5 different coordinate systems for finding a tile,
>>then rather than 5 x 10 = 50 APIs, you have 5 + 10 = 15 to worry about.
>>
>>If you add a new property, you don't add any new coordinate APIs, and
>>if you add a new coordinate system, you don't add any property lookups.
> 
> 
> A reasonable argument.
> 
> Unfortunately the current API is all in map coordinates.  To do this
> properly the API would have to be in tile or perhaps index terms, since
> they are at a lower level - and, more to the point, 1-dimensional.

The current API makes extensive use of functions/macros to access almost
all operations including the new macros you were defining.

It is a rather trivial patch to rework all these macros in the few header
files to insert the tile API layer and will probably get 90% of the use.
Direct references to memory using coordinates are being replaced everywhere
anyway, so make sure there is a correct way to do it from now on and in
no time at all you will have reached a point where one last curry patch
will finish the job.


>  We
> can't do
> 
>   map_set_special(native_to_map_pos(nx, ny), S_RIVER);
> 
> but we could do
> 
>   index_set_special(native_pos_to_index(xn, yn), S_RIVER);
> 
> or
> 
>   tile_set_special(nat_get_tile(xn, yn), S_RIVER);

You didn't really absorb the message. Your arguiments about one coordinate
system or another are totally off on the wrong track and thus whether they are
1-D or 2-D is thus a spurious argument.

The correct API listed above says the final access is through a "location",
which can be a reference to tile or tile index if you want, but *not* a
coordinate given all the excess baggage that coordinates have.

#define map_set_special(map_x, map_y, spe) \
(
         _FC_TILE = map_get_tile(map_x, map_y),
         tile_set_special( _FC_TILE, spe)
}

With an appropriate definition for tile_set_special as the only way to
access and manipulate special setting works just fine.

The above fixes map_<functions> which are somewhat special as this is the
common coordinate system that everyone should use for argument passing and
such. The fact that map_* is special is historical, but there is no need
to change it, and certainly those arguments are orthogonal to this.

Any code like your new nat_<functions> should follow the style. You may
want similar functions in local code, but I would advise against it. Instead
I would write your new code with the understanding that computing and caching
the location in the outermost code block and using ptile->set_special()
constructs rather than coordinate based ones will make this code both more
robust and more efficient.

In any event, after this one has only to change the single location based
code/function to update operations. And if there are specific checks for
one type of coordinate lookups over another they can just be built into the
coordinate to location lookup function.

[...]
> So I would see this as a long-term design goal rather than something to
> target in this patch.  But I'd be happy to code it the other way if
> others disagree.
[...]
> jason

I'd do it as a long term process, but fix the (relatively trivial) first step
now so all changes from here on in follow the appropriate style and coding
practices, i.e. have a goal to converge to.

Cheers,
RossW
=====