[Freeciv-Dev] Re: (PR#4559) citymap iterators are misnamed

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

Jason Short wrote:
> The citymap iterators are horribly misnamed:

Not really ... but some are aging for those with shorter memories.

> city_map_iterate(x, y) => iterates over (city_x, city_y)
> city_map_iterate_outwards(x, y) => iterates over (city_x, city_y)
> 
>   These two are the same, just a different order of coordinates
>   encountered.  This is a hack that allows some iterations to work
>   slightly better.  And "iterate" should come last; it should swap
>   with outwards.

The order is critical for some operations. The outwards on the end is
the original flavour before standardization got as far along as it is,
but there is absolutely no reason to always have iterate on the end.
The change here is not warranted vs the code impact on its own.

> city_map_checked_iterate(x0, y0, cx, cy, mx, my)
> 
>   What does "checked" mean?  Basically nothing.  This is the core
>   iterator.

Shame on you ...

City_map coordinates have fixed boundaries, they are a 5x5 grid with
the corners missing. Only when you introduce map coordinates into a
city_map iteration do you get the *extra* concept of an invalid
coordinate that must be checked, and only for some types of map
topologies.

The checked form is *not* the core iterator and should *never* be. It
has extra functionality and penalties arising from the map linkage.
But it does happen to be the most commonly used since many operations
are actually done in map coordinates with a city filter. This macro
should be moved to map.h.

It should be the case that city values are well-defined without a map
exclusion. Thus summarizing or handling worker placements can use the
unchecked simple form. This is what the UNAVAILABLE vs EMPTY tags are
to be used for. There may be other examples of the difference between
checked and unchecked iteration apart from simple initialization.

> map_city_radius_iterate(city_x, city_y, map_x, map_y)
> 
>   This is just a wrapper for city_map_checked_iterate.  "Radius" has
>   no meaning here.  The variables are the same as in the other, but
>   with different names.  This is by far the worst...

This is an original macro that was pretty much replaced by the checked
form once map checking became a critical aspect in most of the places
where this was used. It went away a long time ago in the corecleanups.

> The basic problem is a confusion of terms.  I suggest:
> 
>   citymap: the city's map area; i.e. set of tiles usable by
>            the city.  Or maybe city_map?

I like city_map for consistency, but see below for the confusion of
coordinate names vs type/target of coordinates.

>   city_pos: The "city position" for a tile; i.e. its coordinates
>             relative to the city.
>   map_pos: Global coordinates representing a tile.

But I really don't think the distinctions above are anything more than
worse confusion. The "pos" needs to distinguish between a pure citymap
and a city_on_the_map and you are making the two indistinguishable,
likely because you missed the fundamental differences above with the
checked.

> So our iterators then become:
> 
>   citymap_city_iterate
>   citymap_city_outwards_iterate
>   citymap_map_city_iterate
>   citymap_map_iterate
> 
> or something like that.  citymap_city_iterate could probably be dropped.

You have city on the brain ... :-).

Normal iteration should be outwards from the center for most operations
and can be done efficiently with indexed lookups. Only gui tiling has
additional order dependent needs.

city coordinates:  2 args - city pos variables + no map checking
   city_array_iterate        - iterates in gui/array order (for tiling)
   city_iterate              - iterates in an outwards spiral or circle
   city_iterate1             - iterates in an outwards spiral or circle
                               excluding the center
city+map coordinates: 6 args - map origin + map pos + city pos variables
   city_checked_iterate      - iterates in city and map checked "pos"
   city_checked_iterate1     - iterates in city and map checked "pos"
                               excluding the center

This would match the form in the recent iterator cleanup patch in Dec
with the "_map" confusion removed. We don't have map_map_iterators, so
probably shouldn't have city_map_iterators.

Note, the underlying core iterator is just wrapped with a hardcoded
offset to handle the center omission as a convenience, but could be done
with a single iterator and explicit "0" or "1" starting arg value.

> Failing that, I would be happy with a simple rename (not removal!) of
> map_city_radius_iterate.
> 
> See also PR#1233 and PR#4558.

Removal of map_city_radius_iterate is long overdue.

Cheers,
RossW
=====