[Freeciv-Dev] Re: [Patch] Fortress with enhanced vision (watchtower-like)

[Bert Buchholz <bertbuchholz@xxxxxx>]

On Sun, Aug 26, 2001 at 03:20:56PM +0100, Gregory Berkolaiko wrote:
> Hi Bert,
>  --- Bert Buchholz <bertbuchholz@xxxxxx> wrote: 
> > Hi,
> > 
> > Patch against cvs from Fri, Aug-24, somewhen in the evening :-) 
> > This is now the watchtower "included" in a fortress, because most
> Here is my promised report on your patch.  I detected several problems
> with it:
> 
> 1. Falk is right: you should check for is_ground_unit more
> systematically.
> AFAIR, there was something about ships going into rivers so the fact that
> there is fortress on a square does not guarantee that there are no ships
> there.

Yes, I found one spot in the code where I forgot this, but the rest
looks okay, I think. I've partially replaced the is_ground_unit() checks
with unit_profits_of_watchtower() as Falk suggested. Regarding this
ships-on-river issue: This isn't implemented yet, right? Or did I miss
something? I'm not sure what you mean, ships are not treated as ground
units and they wouldn't get enhanced view by a fortress, would they?
 
> 2. Consider the situation: players A and B are allied and player A builds
> a fortress on a square occupied by Bs unit.  Your patch would not handle
> this situation properly.

Hmm, what do you mean? Player A builds a fortress on a tile on which
player B's units are? How's this possible? I must admit that I've never
had an alliance where this would have happened, so I'm not sure about
this. If this were the case, I would of course take a closer look into
the necessary code...

> 3. Thue was very right when he said that fog system is very touchy.  If
> you calculate it incorrectly once, it will stay this way.  So please
> check for all cases of potential vision changes!!  You can do using
> freeciv's source browsing system.  For example 
>   http://www.freeciv.org/lxr/ident?v=cvs&i=fog_area
> reveals that you forgot to upgrade unittools.c around line 1541 (unit
> upgrading).

Yes, I understand. I'm looking for further flaws in my implementation.

> > So, if anything's missing, or other failures (which are of course
> > intentional ;-), please tell me and I'll try to kick them out, or you
> You must be absolutely sure that your patch updates all the relevant bits
> of code.  Otherwise we will have many fog bugs in seemingly random
> places.

Yes, I'm very aware of that. For debug-purposes, I'm thinking of putting
in some function into the client's tile-draw-methods, that prints the
value of player_tile.seen onto the map. This way it would be easy to
find errors made while moving units through fortresses. Even though I'm
not 100% sure about how player_tile.own_seen and player_tile.seen
differ, but I'll find out. :-)

> All in all, I would rate your patch as "good development with quite a lot
> of problems" (it's 2 in Raimar's system init?).
> 
> Comments on other topics:
> 1. Things like watchtower_vision and watchtower_extra_vision should
> definitely go into the rulesets, but not necessarily in this patch.

Up to now, I put them together into get_watchtower_vision(). Then, later
changes will be easier, because only this function needs to be changed.

> 2. The shape of vision area is a topic completely independent of
> watchtower business and should be discussed as such (i.e. please change
> subject of the email when writing about vision areas).

Well, this was not really my idea, to put anything about the vision AREA
into the watchtower patch, even though I think, that Raimar is right,
that it would be good to have something like this, but of course, this
has nothing to do with the watchtower-patch, this is a general change,
that might make lots of other changes necessary. But I'm not that much
involved into freeciv-code that I could possibly make myself a picture
of this.

Bert