[Freeciv-Dev] Re: semi-quick tile_is_known patch (and dicussion of memory bloat)

[Mike Kaufman <mkaufman@xxxxxxxxxxxxxx>]

I figured that this would be a hard sell...

Yes, I realize that without my particular goal,
tile_is_known would be a completely redundant function and ought to be
removed.

> 
> As I said, tile_is_known *should* be used by server and common code too.

Without civworld, tile_is_known probably *shouldn't* be used anywhere.

BUT
vvv

[code locations snipped]

> So, I'm somewhat opposed to this movement.  Can you point us again to
> the thread where it was discussed before?

http://arch.freeciv.org/freeciv-dev-200108/msg01425.html

after reviewing what I said, I realize that I didn't explain clearly
what was going on:

ptile->known is interesting, because it is used by the server and client
in different ways. 
The server uses it to store a bitvector that stores information for
_all_ players whether a tile is TILE_KNOWN or TILE_UNKOWN.
The client uses it to store the condition of a tile TILE_KNOWN,
TILE_UNKOWN, or TILE_KNOWN_FOGGED, and then uses this information to
draw the tiles correctly.

for reference, see: maphand.c:send_tile_info_always(),
maphand.c:map_get_known(), client/tilespec.c, and client/gui-*/mapview.c

The Problem: civworld uses both---at the same time. Since it is now a
scenario editor, you can create multiple players each with different
knowledge of the map. so you need ptile->known as a bit vector, but it
also draws to a gui, so you need ptile->known as an enum known_type.

Solutions: 
1. "clone" all the relevant files in client/ and change the code in the
   relevant places to extract the needed data the way the server does. 

2. add a field to struct tile. call it "enum known_type cknown" that only the
   client would access.

3. In the files needed to be "cloned" by civworld in 1, instead
   encapsulate the ptile->known calls in a function which could then be
   hijacked to do what 1 does, but in one place. My current proposal.

4. do what Reiner suggests in the thread above. create 
   struct client_tile and struct server_tile and put shared fields in a 
   struct tile.

Discussion:
I believe that I put the solutions in order of elegance. For 1, no
client code would need to be changed, but I would have to add, for
example, tilespec.c which is 1800 lines to the civworld patch to change
5 or 6 lines where it is necessary.

For 2, this would be just fine, but for a 200x100 map, this solution
would use 80kB additional memory. For that reason alone, I don't think
it's a good solution. (though see 4)

For 3, my proposed method, I could tilespec.o as is in the already
compiled client, so I wouldn't have to "clone" tilespec.c in the
civworld/ directory. I would change tile_is_known to one like the one
below, and I'm ready to go.

/**************************************************************************
 this function is for mapview.c and tilespec.c. It allows proper drawing of
 a player's private map.
***************************************************************************/
enum known_type ed_tile_is_known(int x, int y)
{
  if(!draw_private_map) return TILE_KNOWN;
  if(map_get_known(x, y, game.player_ptr)){
    if(map_get_player_tile(x, y, game.player_ptr)->seen)
      return TILE_KNOWN;
    else
      return TILE_KNOWN_FOGGED;
  }else
    return TILE_UNKNOWN;
}

For 4, this is probably the best in the long run, as it would save
memory and all the aformentioned problems. I notice now as I'm looking
and the tile struct that "unsigned sent" and "int assigned" are not used
in the client, _and_ ptile->worked is allocated as a struct city when it
seems that all the clients do is check if it's NULL? (That can't be
right) so already, the client is allocating a lot more memory
than it really needs to. This solution would---on initial
reflection---require a lot of changes, but perhaps this would be the
best.

> 
> > one caveat: the macro MAP_TILE in the body of tile_is_known() has been
> > removed. This is because MAP_TILE is defined in map.c rather than map.h
> > I did not want to move it until I got feedback from the list. yes, this
> > kind of direct access to map.tiles is not good. So respond with a
> > suggestion.
> 
> You should use map_get_tile.  But won't you then have the same problem
> with that function?

Note the next patch, but I'm sure you already have.

> 
> > This patch is needed to remove ~60k from the civworld patch.
> 
> It seems like a better solution would be to put civworld into freeciv's
> CVS, so that source sharing would be much easier.  But I'm sure this has
> been discussed before...

this won't change anything, as civworld still uses both server and
client specific contructs. see above.

> 
> Finally, I'd rather see the check_map_pos patch go in before this one. 
> It makes this fix simpler...

how?

-mike