[Freeciv-Dev] Re: (PR#9006) player_in_territory() function

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9006 >

James Canete wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=9006 >
> 
> This patch implements a function called player_in_territory(), which
> returns the number of visible units pplayer2 has in pplayer's territory.
> 
> Along with this I have included a separate example patch which makes AI
> diplomacy use this function to calculate the amount of love lost per
> turn during wartime.
> 
> Issues:
> -Should this function be in player.c?
> -There's probably a more efficient way to do this.
> -Compiler warning because can_player_see_unit() doesn't use "const
> struct player *".
> 
> TODO:
> -Use this as a base to introduce right of passage treaties and forced
> expulsion of another civ's units.

Why do you return 0 if the players are equal?  I think you're assuming 
this function will be used as a calculation of threat.  But in this case 
why wouldn't you return 0 if the players are allied as well?  I think 
the answer is that probably the function should never be called with 
equal players.  Maybe an assertion would be good.  But I don't think 
it's a good idea to return 0 since the function name doesn't indicate this.


The function is inefficient.  It would be possible to iterate over tiles 
instead of over units:

   whole_map_iterate(...) {
     struct tile *ptile = map_get_tile(x, y);
     if (ptile->owner == pplayer
         && can_player_see_tile(x, y)) {
       unit_list_iterate(ptile->units, punit) {
         if (can_player_see_unit(pplayer, punit)) {
           in_territory++;
         }
       } unit_list_iterate_end;
     }
   } whole_map_iterate;

but there's no real reason to think this would be faster.


Also, can_player_see_unit should be changed to use a const player right? 
  Probably other functions too.  I don't think we need a "const struct 
player const *" like Erik added to some functions, though.  Just a 
"const struct player *" will do.  But rather than do it one function at 
a time (as Erik did) we should audit and change all functions in a 
particular file (IMO).


The function doesn't work particularly well for transported units.  The 
player knows if a unit is occupied but can't actually see the unit(s) 
inside the transporter.  So the result is that the player knows there's 
a unit there but it's not counted in your function.  Maybe for occupied 
transporters an extra unit should be counted?  (Currently the player 
only knows if the unit is occupied, not how many occupy it.  Perhaps 
this should be changed?  But there's no good way to show that at the 
client.)


Finally, if you put this in player.c you're implying the client might 
want to use it.  Is that ever going to be the case?

jason