[Freeciv-Dev] Re: [Patch] Reduce superfluous tile_info packets version 2

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Oct 25, 2001 at 03:49:17PM -0400, Jason Dorje Short wrote:
> Jason Dorje Short wrote:
> > 
> > Raimar Falke wrote:
> > >
> > > Ok here the cleaned up patch for official submission. This patch does:
> > >  - removes the sent field of struct tile (AFAI see this was intended
> > >  to solve a similar problem and it now obsolete)
> > >  - adds a new server-only bit vector in struct tile: changed
> > >  - if the server tries to sent a tile_info packet it is checked if
> > >  this tile has changed since last sent for this player
> > >  - if ther server has sent a tile_info packet the corresponding bit in
> > >  the changed field is cleared
> > >  - settler.c changes the special field calculates some stuff and
> > >  reverts these changes. Such changes aren't user/client visible and
> > >  have from now on to be done with the base_* methods.
> > 
> > Everything looks OK.  However:
> > 
> > - In savegame.c when the terrain is loaded from the savegame, an initial
> > glance would make one think that map_set_terrain should be used here instead
> > of manually setting the terrain and marking the tile as changed.  Of course,
> > this would not work - a comment to this effect would be nice IMO.
> > 
> > - It is difficult to verify that you've picked up every place that changes
> > the tile information.  Thus although the patch itself looks good, it is
> > difficult to verify that it is complete/correct.  I'll take a deeper look at
> > this later today.
> 
> What about the following code in unit_enter_hut() in unittools.c?
> 
>   map_get_tile(punit->x, punit->y)->special^=S_HUT;
>   send_tile_info(0, punit->x, punit->y);
> 
> Although a unit entering a hut seems to be drawn properly, I do not think
> the above code is technially coorect.  Seems to me it should be
> 
>   map_clear_special(punit->x, punit->y, S_HUT);
> 

Ack.

> In fact I'd say any code that doesn't use the map functions to set tile
> information should have an explicit comment as to why.

I didn't some grepping but only for "=" (and without "=="). You found
the only one with "^=". Grepping for "|=" reveals:

./server/settlers.c:702:    ptile->special|=S_ROAD; /* have to do this to avoid 
reset_move_costs -- Syela */
./server/settlers.c:727:    ptile->special|=(S_ROAD | S_RAILROAD);

But this is also a non-problem but should be changed to
base_map_set_special.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "I was dead ... but I'm better now."
    -- Capitain Sheridan in Babylon 5