[Freeciv-Dev] Re: (PR#1870) FreecivAC: borders patch

["Ben Webb" <ben@xxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Jul 13, 2003 at 04:46:50PM -0700, Jason Short wrote:
> - How come owners are sent across the network as a Uint16, but
> MAP_TILE_OWNER_NULL is MAX_UINT8?

Hmm, I guess MAX_UINT16 (or MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS) should
be used instead.

> - map_get_adjc_city should probably use map_get_city not map_get_tile.

True.

> - The comment "(N.B. will probably need modifications to deal with other
> topologies)." isn't true; since this code just uses adjc_iterate no
> modifications will be needed. (This is in is_claimed_ocean.)

I was thinking of maps where adjc_iterate can return a different number
of adjacent tiles than 8 (e.g. for non-square tiles). Does the general
topologies patch not include such map types?

> - is_claimed_ocean is buggy; what if there is continent A with 1
> adjacent tile and continent B with 5 adjacent tiles to the ocean, but
> continent A happens to be found first?  We'll see that numland==1 and
> the tile will be unclaimed.  The rule could be changed so that that if
> two different continents touch a tile it cannot be claimed by either
> (making for a simple code change instead of a harder one, and probably
> better results).

Hmm, good point (although this is a rather rare occurrence, surely).

> - Why is map_get_adjc_city required for map_get_closest_city?  Is this
> just to speed things up?

It's so that cities claim all immediately adjacent tiles. I suppose you
could do the same thing with a distance calculation though...

> - Recalculating borders is hideously expensive.  A better algorithm
> should be possible, but using square distance makes things tricky.

True, although this should only happen on reloading a savegame or
landmass change.

> - map_update_borders_landmass_change is problematic.  Both because it
> recalculates the whole world and because it has to resend the whole
> world (I think).
> 
> - map_calculate_territory looks like it is O(n*n*m*m) in time (n=# of
> cities, m=map.borders).  This can definitely be improved on.

In both cases it was felt that this should happen so infrequently that
optimization of the algorithm wasn't necessary. If the whole world is
resent though, this should be fixed.

> - Why is civ_score completely rewritten, again?  Surely this could be
> avoided?

Sure - it just seemed odd to use the old landarea calculation when
border information is available.

> - If map.borders==0, the "hack" to use MAP_DEFAULT_BORDERS to calculate
> landarea is too ugly, IMO.  Just use an approximation, calculate the
> area by hand, or use the old landarea calculation.

Fair enough; I guess the old calculation would be suitable.

> - Moving civ_score and associated functions into the server is a task in
> its own IMO.  We might as well have a separate patch.

Yes, I suggested that about two years ago; nobody was interested. I can
split the patch if there is demand. Will have to wait until next week
though.

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx         http://bellatrix.pcl.ox.ac.uk/~ben/
"I know the truth is in between the 1st and 40th drink"