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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Mike Kaufman wrote:
> 
> 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.

Hmmm.  I was going off the assumption that it was there because someone
wanted it to be used.  I now see my mistake.

> > 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.

Ahh, I understand now.  Since the server shouldn't use tile_is_known,
moving it to the client code seems wise (perhaps even if the other
changes you suggest are made).  It will prevent people like me from
making the same mistake in future.

> > > 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.

Yes.  Using map_get_tile is better.  But please change this code:

+enum known_type tile_is_known(int x, int y)
+{
+  if (!normalize_map_pos(&x, &y))
+    return TILE_UNKNOWN;
+  else
+    return (enum known_type) map_get_tile(x, y)->known;
+}

to

{
  return map_get_tile(x, y)->known;
}

or, if you don't feel safe with this,

{
  struct tile * ptile = map_get_tile(x, y);
  assert(ptile);
  return ptile->known;
}

This will make things simpler for the check_map_pos changes.  Speaking
of which, since civworld uses this same code you should verify that
civworld works with the check_map_pos patch (i.e. doesn't pass around
non-normal coordinates anywhere).

> > > 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.

Yes, I see.  I also see you're in favor of it :-)

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

tile_is_known uses check_map_pos, so they conflict.  But it's a small
matter, and easily resolved as above.

jason