[Freeciv-Dev] Re: PATCH: borders update

[Stewart Adcock <stewart@xxxxxxxxxxxxxxxxxxxxxx>]

Hi,

> > +Unresolved issues
> > +-----------------
> > +To do or fix:
> > +* Several portions of code are 'less-than-optimal'.  These will be recoded 
> > in due course.
> > +* fog-of-war artifacts.  Maybe fixed?  Needs more playtesting.
> > +* Civ score uses different analysis of territory.  Should be easy to 
> > alter, but is it desirable?
> > +* Define territorial claim radius in ruleset.  i.e. Replace 
> > MAP_CITY_INFL_DISTANCE constant.
> > +* Occasional buggy worker states for tiles.  The sanity checks pick up 
> > occasional problems.  Needs more thought.
> 
> IMHO these should be sorted out before inclusion.
Well, as I stated, even the bits that have been done aren't ready for
inclusion.  I just wanted feedback - which I got, so am happy.

> You may want to look at the patch from "Sat, 22 Sep 2001 16:30:04
> +0200" with subject "Re: [Freeciv-Dev] Re: Grid color". We want to
> maximize the code that is shared between the clients.
Okay, I shall.  I did put my color stuff in the client directory instead
of the client/gui-* directories for that very reason.

> > +    /* the nation colour. */
> > +    colbuf[0] = '#';   /* NOTE: Should replace with a pixmap. */
> > +    colbuf[1] = '\0';
> > +    row[3] = colbuf;
> 
> Will this be fixed?
Yes.  I was too lazy to do it at the time.

> > +  row[10] = (char *) player_addr_hack(&game.players[i]);     /* Fixme */
> 
> What should be fixed?
Oh.  That seems to be something that happened when transfering code
between CVS versions.  I didn't notice that mistake.

> > +  int foundation_date;        /* Date this city was founded -- used only 
> > in server */
> 
> IMHO you should use the turn and not the year.
> 
> >  /***************************************************************
> > +Return Player_no for nation owning this tile, return
> > +-1 for unclaimed tiles.
> > +***************************************************************/
> > +int map_get_owner(int x, int y)
> > +{
> > +  if (!normalize_map_pos(&x, &y))
> > +    return MAP_TILE_OWNER_NULL;
> > +  else
> > +    return (map.tiles + x + y * map.xsize)->owner;
> > +}
> > +
> > +/***************************************************************
> > +Set the owner of a tile.
> > +***************************************************************/
> > +void map_set_owner(int x, int y, const int owner)
> > +{
> > +  assert(is_real_tile(x, y));
> > +  normalize_map_pos(&x, &y);
> > +  (map.tiles + x + y * map.xsize)->owner = owner;
> > +}
> 
> > +  int owner;            /* Player_no owning this tile, or -1 for 
> > unclaimed. */
> 
> Since we want to avoid ids it would be nice if the owner is saved as a
> "struct player *".
> 
> > +#define MAP_TILE_OWNER_NULL   MAX_UINT8
> 
> What is this? Is this -1? Why haven't you used MAP_TILE_OWNER_NULL
> instead of -1?
That is used to indicte unclaimed tiles in the tile info packets.  The
owner is stored as an int in the code, so it wouldn't necessarily be -1.

> > +  iget_uint8(&iter, &packet->owner);
> 
> There should be a "has_capability(...)" call.
As I said, the patch ins't ready for CVS anyway.  I added the capability
"+borders" so a client without that capability can't be used anyway, so
a has_capability() call would be a bit pointless.  Obviously, this would
be required later if people agree that this is a worthwhile patch to
consider.

> > +  cptr=put_uint8(cptr, pinfo->owner);
> 
> Same here.
See above.

> > +/***************************************************************
> > +Return pointer to the oldest adjacent city to this tile.  If
> > +there is a city on the exact tile, that is returned instead.
> > +***************************************************************/
> > +struct city *map_get_adjc_city(int x, int y)
> > +{
> > +  struct city *closest=NULL;   /* Closest city */
> > +  struct tile *ptile;
> > +
> > +  assert(is_real_tile(x, y));
> > +  normalize_map_pos(&x, &y);
> > +
> > +  ptile = map_get_tile(x, y);
> > +  if (ptile->city) return ptile->city;
> > +
> > +  adjc_iterate(x,y,xp,yp) {
> > +    ptile = map_get_tile(xp, yp);
> > +    if ( ptile->city &&
> > +         (!closest || 
> > ptile->city->foundation_date<closest->foundation_date) )
> > +      closest = ptile->city;
> > +  } adjc_iterate_end;
> > +
> > +  return closest;
> > +}
> > +
> > +/***************************************************************
> > +Return pointer to the closest city to this tile on the same
> > +continent.  If two or more cities are equally distant, then
> > +return the oldest (i.e. the one with the lowest id).  This
> > +also correctly works for water bases in SMAC mode.
> > +***************************************************************/
> > +struct city *map_get_closest_city(int x, int y)
> 
> You may want to unifiy this with ai/aitools.c:dist_nearest_city
I'll look at that function.

> > +/***************************************************************
> > +Return the id of the closest city to this tile.  If two cities
> > +are the same distance, then return the oldest.
> > +Return '-1' in event of not finding a city.
> > +***************************************************************/
> > +int map_get_closest_city_id(int x, int y)
> 
> Don't use int but "struct city *"
Actually, this isn't called at any point in the patch so shouldn't have
been included.  It is used in other "experimentation".

> game.borders isn't used.
I agree, but it will be when I've added a ruleset option to control
border behaviour.

Cheers,
Stewart. 
-- 
_______________________________________________________________
Stewart Adcock     saa@xxxxxxxxxxx     www.stewart-adcock.co.uk
11 Centurion Close, Reading, Berkshire. UK.     (0118) 959 4366