[Freeciv-Dev] (PR#7451) remove player_can_see_unit

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

This patch removes player_can_see_unit and player_can_see_unit_at_location.

Originally these two functions were used to check if a player could see 
a unit.  Note that these functions don't check the tile's known status. 
  This was in the bad old days when map_get_known didn't work the same 
way at server and client (I think), and when all units were sent to the 
client and it was left to the client not to cheat.

In PR#5071 Greg figured out a phantom units bug.  The solution was to 
add another check in a few places so that units in cities were not 
"seen".  So he and I introduced can_player_see_unit_at to do these extra 
checks.  The original functions were still used as-is in the server. 
can_player_see_unit_at was a wrapper for 
player_can_see_unit_at_location.  Greg also requested that 
player_can_see_unit[_at_location] be renamed, but we never did this.

Later in the delta patch Raimar introduced can_player_see_unit_at2.  I 
have no idea why.  In any case this was recently folded back into 
can_player_see_unit.

So with this patch player_can_see_unit and 
player_can_see_unit_at_location are removed (folded into 
can_player_see_unit_at).  A new function can_player_see_unit is 
introduced (a simple wrapper for can_player_see_unit_at).  The existing 
users of player_can_see_unit are replaced with one of these remaining 
functions.

There are only a few such users.
- The client doesn't need to check if we can see units at all.  If the 
client knows about the unit, then it's seen.  So these checks are just 
removed.
- One check is replaced in autoattack.c.  This is the only questionable 
one since it changes the meaning of H_TARGETS slightly.  The old code 
was probably buggy since it didn't check for units in cities or transports.
- One check is replaced in wakeup_neighbor_sentries.  Only sentries that 
can see the unit are woken.  This change is straightforward and fixes a 
potential bug.  (However, I don't think the bug would ever apply since 
all moving units can be seen.)
- One check is replaced in maybe_cancel_patrol_due_to_enemy.  Here we 
have to check for occupied cities as well, since we can't see the units 
in an enemy occupied city.  Note this function may still be buggy if 
is_non_allied_unit_tile returns the wrong unit.

One FIXME comment is added.  We can see an invisible unit only if we 
have an adjacent allied unit (this includes our own units).  Of course a 
check to see if the tile is known has already been done, so we're not 
going to see units off in the fog.  But it looks like we could have a 
destroyer 2 spaces away, with an allied but non-shared-vision unit 1 
space away from the hidden unit, and we'd see it.  This seems erronous.

jason

Index: client/control.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/control.c,v
retrieving revision 1.128
diff -u -r1.128 control.c
--- client/control.c    2004/02/16 00:24:53     1.128
+++ client/control.c    2004/02/21 00:38:58
@@ -351,8 +351,7 @@
          panyowned = punit;
         }
       }
-    } else if (!ptptother && !punit->transported_by &&
-              player_can_see_unit(game.player_ptr, punit)) {
+    } else if (!ptptother && !punit->transported_by) {
       if (get_transporter_capacity(punit) > 0) {
        ptptother = punit;
       } else if (!panyother) {
Index: client/mapview_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
retrieving revision 1.78
diff -u -r1.78 mapview_common.c
--- client/mapview_common.c     2004/02/18 02:20:51     1.78
+++ client/mapview_common.c     2004/02/21 00:38:59
@@ -1321,9 +1321,8 @@
 
   flush_dirty();
 
-  if (player_can_see_unit(game.player_ptr, punit) &&
-      (tile_visible_mapcanvas(map_x, map_y) ||
-       tile_visible_mapcanvas(dest_x, dest_y))) {
+  if (tile_visible_mapcanvas(map_x, map_y)
+      || tile_visible_mapcanvas(dest_x, dest_y)) {
     int i, steps;
     int start_x, start_y;
     int this_x, this_y;
Index: common/player.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.c,v
retrieving revision 1.135
diff -u -r1.135 player.c
--- common/player.c     2004/02/19 18:18:17     1.135
+++ common/player.c     2004/02/21 00:38:59
@@ -217,23 +217,47 @@
   return NULL;
 }
 
-/***************************************************************
-  Returns TRUE iff the player can see the unit at (x,y), i.e. if 
-  the unit is not invisible or if it is adjacent to one of our 
-  cities or units.  No map visibility check here! The unit does 
-  not have to be at (x,y)! 
-  Allied units and cities can be used for sub hunting. 
-***************************************************************/
-bool player_can_see_unit_at_location(struct player *pplayer, 
-                                     struct unit *punit, 
-                                     int x, int y)
+/****************************************************************************
+  Checks if a unit can be seen by pplayer at (x,y).
+  A player can see a unit if he:
+  (a) can see the tile AND
+  (b) can see the unit at the tile (i.e. unit not invisible at this tile) AND
+  (c) the unit is outside a city OR in an allied city AND
+  (d) the unit isn't in a transporter, or we are allied AND
+  (e) the unit isn't in a transporter, or we can see the transporter
+****************************************************************************/
+bool can_player_see_unit_at(struct player *pplayer, struct unit *punit,
+                           int x, int y)
 {
+  struct city *pcity;
+
+  /* If the player can't even see the tile... */
+  if (!map_get_known(x, y, pplayer) == TILE_KNOWN) {
+    return FALSE;
+  }
+
+  /* Don't show non-allied units that are in transports.  This is logical
+   * because allied transports can also contain our units.  Shared vision
+   * isn't taken into account. */
+  if (punit->transported_by != -1 && unit_owner(punit) != pplayer
+      && !pplayers_allied(pplayer, unit_owner(punit))) {
+    return FALSE;
+  }
+
+  /* Units in cities may be hidden. */
+  pcity = map_get_city(x, y);
+  if (pcity && !can_player_see_units_in_city(pplayer, pcity)) {
+    return FALSE;
+  }
+
+  /* Allied or non-hiding units are always seen. */
   if (pplayers_allied(unit_owner(punit), pplayer)
       || !is_hiding_unit(punit)) {
     return TRUE;
   }
 
-  /* Search for units/cities that might be able to see the sub/missile */
+  /* Hiding units may only be seen by adjacent allied units or cities. */
+  /* FIXME: shouldn't a check for shared vision be done here? */
   adjc_iterate(x, y, x1, y1) {
     struct city *pcity = map_get_city(x1, y1);
     if (pcity && pplayers_allied(city_owner(pcity), pplayer)) {
@@ -249,44 +273,15 @@
   return FALSE;
 }
 
-/***************************************************************
-  Same thing as above only the location is the unit's current one.
-***************************************************************/
-bool player_can_see_unit(struct player *pplayer, struct unit *punit)
-{
-  return player_can_see_unit_at_location(pplayer, punit, punit->x, punit->y);
-}
 
 /****************************************************************************
-  Checks if a unit can be seen by pplayer at (x,y).
-  A player can see a unit if he:
-  (a) can see the tile AND
-  (b) can see the unit at the tile (i.e. unit not invisible at this tile) AND
-  (c) the unit is outside a city OR in an allied city AND
-  (d) the unit isn't in a transporter, or we are allied AND
-  (e) the unit isn't in a transporter, or we can see the transporter
-  
-  TODO: the name is confusingly similar to player_can_see_unit_at_location
-  But we need to rename p_c_s_u_a_t because it is really 
-  is_unit_visible_to_player_at or player_ignores_unit_invisibility_at.
+  Checks if a unit can be seen by pplayer at its current location.
+
+  See can_player_see_unit_at.
 ****************************************************************************/
-bool can_player_see_unit_at(struct player *pplayer, struct unit *punit,
-                           int x, int y)
+bool can_player_see_unit(struct player *pplayer, struct unit *punit)
 {
-  struct city *pcity;
-
-  /* Don't show non-allied units that are in transports.  This is logical
-   * because allied transports can also contain our units.  Shared vision
-   * isn't taken into account. */
-  if (punit->transported_by != -1 && unit_owner(punit) != pplayer
-      && !pplayers_allied(pplayer, unit_owner(punit))) {
-    return FALSE;
-  }
-
-  pcity = map_get_city(x, y);
-  return ((map_get_known(x, y, pplayer) == TILE_KNOWN)
-         && player_can_see_unit_at_location(pplayer, punit, x, y)
-         && (!pcity || can_player_see_units_in_city(pplayer, pcity)));
+  return can_player_see_unit_at(pplayer, punit, punit->x, punit->y);
 }
 
 /****************************************************************************
Index: common/player.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.h,v
retrieving revision 1.113
diff -u -r1.113 player.h
--- common/player.h     2004/02/19 18:18:17     1.113
+++ common/player.h     2004/02/21 00:38:59
@@ -225,10 +225,7 @@
 void player_set_unit_focus_status(struct player *pplayer);
 bool player_has_embassy(struct player *pplayer, struct player *pplayer2);
 
-bool player_can_see_unit(struct player *pplayer, struct unit *punit);
-bool player_can_see_unit_at_location(struct player *pplayer, 
-                                     struct unit *punit, 
-                                     int x, int y);
+bool can_player_see_unit(struct player *pplayer, struct unit *punit);
 bool can_player_see_unit_at(struct player *pplayer, struct unit *punit,
                            int x, int y);
 
Index: server/autoattack.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/autoattack.c,v
retrieving revision 1.49
diff -u -r1.49 autoattack.c
--- server/autoattack.c 2004/01/11 17:45:05     1.49
+++ server/autoattack.c 2004/02/21 00:39:00
@@ -120,9 +120,8 @@
      * Note, cheating AI may attack units it cannot see unless it has
      * H_TARGETS handicap, but currently AI never uses auto-attack.
      */
-    if ((ai_handicap(pplayer, H_TARGETS)
-         && !map_is_known_and_seen(x, y, pplayer))
-       || !player_can_see_unit_at_location(pplayer, enemy, x, y)) {
+    if (ai_handicap(pplayer, H_TARGETS)
+       && !can_player_see_unit(pplayer, enemy)) {
       freelog(LOG_DEBUG, "can't see %s at (%d,%d)", unit_name(enemy->type),
               x, y);
       continue;
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.280
diff -u -r1.280 unittools.c
--- server/unittools.c  2004/02/19 20:01:07     1.280
+++ server/unittools.c  2004/02/21 00:39:01
@@ -2745,9 +2745,8 @@
       
       if (!pplayers_allied(unit_owner(punit), unit_owner(penemy))
          && penemy->activity == ACTIVITY_SENTRY
-         && map_is_known_and_seen(punit->x, punit->y, unit_owner(penemy))
          && range >= real_map_distance(punit->x, punit->y, x, y)
-         && player_can_see_unit(unit_owner(penemy), punit)
+         && can_player_see_unit(unit_owner(penemy), punit)
          /* on board transport; don't awaken */
          && !(move_type == LAND_MOVING && is_ocean(terrain))) {
        set_unit_activity(penemy, ACTIVITY_IDLE);
@@ -3134,6 +3133,7 @@
 {
   bool cancel = FALSE;
   int range;
+  struct player *pplayer = unit_owner(punit);
 
   if (map_has_special(punit->x, punit->y, S_FORTRESS)
       && unit_profits_of_watchtower(punit))
@@ -3143,9 +3143,14 @@
   
   square_iterate(punit->x, punit->y, range, x, y) {
     struct unit *penemy =
-       is_non_allied_unit_tile(map_get_tile(x, y), unit_owner(punit));
-    if (penemy && player_can_see_unit(unit_owner(punit), penemy)) {
+       is_non_allied_unit_tile(map_get_tile(x, y), pplayer);
+    struct dumb_city *pdcity = map_get_player_tile(x, y, pplayer)->city;
+
+    if ((penemy && can_player_see_unit(pplayer, penemy))
+       || (pdcity && !pplayers_allied(pplayer, get_player(pdcity->owner))
+           && pdcity->occupied)) {
       cancel = TRUE;
+      break;
     }
   } square_iterate_end;