[Freeciv-Dev] Re: (PR#5789) Don't send info about transported units to clients without shared vision.

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Wed, 3 Sep 2003, Rafa³ Bursig wrote:
> This code add restriction in sending transported units info only to
> owner player (plus all players that owner player give shared vision)
> and transport owner (plus all players that transport owner player give
> shared vision) for rest players this unit is hidden in transport unit
> (remove unit package is send in enter transport phase)
>
> Additionaly this patch allow sending to client transported_by info and
> unload code search in units loaded on transport , not in units with
> sentry activity.

There are several big problems with this patch:
  - It hides transported units in the client AND in the server. The client
should NOT do this.
  - It has assumptions about punit->transported_by which are IMHO
unwarranted. I do not trust this field to be set correctly in all cases.
  - It seems to send a lot of extra unit info packets from the server. I
do not see why this is necessary.
  - It adds no comments in the code. Document changes and assumptions!

Also:
  - Missing capability (capstr.c).


+++ fc3/client/control.c        2003-09-01 20:30:44.000000000 +0200
@@ -313,21 +313,21 @@
   /* If a unit is attacking we should show that on top */
   if (punit_attacking && map_get_tile(punit_attacking->x,punit_attacking->y) 
== ptile) {
     unit_list_iterate(ptile->units, punit)
-      if(punit == punit_attacking) return punit;
+      if(punit == punit_attacking && punit->transported_by == -1) return punit;
     unit_list_iterate_end;
   }


Here, for example, it attempts to hide a unit that _is attacking_ if its
transported_by field is set. Why? You should always be able to see
attacking units in the first place, and the client is no place to make
decisions about such things.


-  square_iterate(punit->x, punit->y, 2, x, y) {
-    bool refresh = FALSE;
-    unit_list_iterate(map_get_tile(x, y)->units, pu) {
-      if (unit_flag(pu, F_PARTIAL_INVIS)) {
-       refresh = TRUE;
-       goto out;
+  /* transported units can't search subs */
+  if (!pinfo->carried) {
+    square_iterate(punit->x, punit->y, 2, x, y) {
+      bool refresh = FALSE;
+      if (x == punit->x && y == punit->y) {
+       continue;


Another example. This is totally wrong. The above code refreshes stacks
where _the server_ has told us there is a unit. We should _not_ second
guess the server in the client.

The client core code should never need to check punit->transported_by.


server/unithand.c:
   unit_list_iterate(map_get_tile(punit->x, punit->y)->units, punit2) {
-    if (punit != punit2 && punit2->activity == ACTIVITY_SENTRY) {
+    if (punit != punit2 && punit2->transported_by == punit->id) {
       bool wakeup = FALSE;


It only needs to wake up if it is sentried. If sentried, it should also
wake up if not transported but on a transport. Old code is correct, change
is wrong.


+++ fc3/server/unittools.c      2003-09-01 22:31:22.000000000 +0200
   conn_list_iterate(*dest, pconn) {
     struct player *pplayer = pconn->player;
+    bool see_unit = (!carried || (carried && ((unit_owner(punit) == pplayer)
+                    || gives_shared_vision(unit_owner(punit), pplayer))));
     bool see_pos =
-       can_player_see_unit_at(pplayer, punit, punit->x, punit->y);
+       see_unit && can_player_see_unit_at(pplayer, punit, punit->x, punit->y);
     bool see_xy = see_pos;

     if (!same_pos(x, y, punit->x, punit->y)) {
-      see_xy = can_player_see_unit_at(pplayer, punit, x, y);
+      see_xy = see_unit && can_player_see_unit_at(pplayer, punit, x, y);
     }
     if ((!pplayer && pconn->observer) || see_pos || see_xy) {
       send_packet_unit_info(pconn, &info);


Yes, this is the place where you should be changing code. I can't tell if
the changes are correct, though.


@@ -1868,7 +1871,8 @@
 {
   struct conn_list *conn_dest = (dest ? &dest->connections
                                 : &game.game_connections);
-  send_unit_info_to_onlookers(conn_dest, punit, punit->x, punit->y, FALSE);
+  send_unit_info_to_onlookers(conn_dest, punit, punit->x, punit->y,
+                                               (punit->transported_by != -1));
 }


Instead of doing this check in every call to
send_unit_info_to_onlookers(), it should be done there.


 /**************************************************************************
+  This happend when unit enter to transport and for all players
+  (without shared vision) this unit should be hiddend in transport unit.
+**************************************************************************/
+static void put_unit_to_transport(struct unit *punit, struct unit *ptrans)
+{
+  struct packet_generic_integer packet;
+  struct player *punit_owner = unit_owner(punit);
+  struct player *ptrans_owner = unit_owner(ptrans);
+  struct packet_unit_info info;
+
+  /* set to transport */
+  punit->transported_by = ptrans->id;
+  set_unit_activity(punit, ACTIVITY_SENTRY);
+
+  package_unit(punit, &info, TRUE,
+              UNIT_INFO_IDENTITY, FALSE, FALSE);
+  packet.value = punit->id;
+
+  /* update clients info */
+  conn_list_iterate(game.game_connections, pconn) {
+    struct player *pplayer = pconn->player;
+    if ((!pplayer && pconn->observer) || (pplayer
+        && (punit_owner == pplayer || ptrans_owner == pplayer
+        || gives_shared_vision(punit_owner, pplayer)
+        || gives_shared_vision(ptrans_owner, pplayer)))) {
+      send_packet_unit_info(pconn, &info);
+    } else {
+      if (pplayer
+        && can_player_see_unit_at(pplayer, punit, punit->x, punit->y)) {
+        send_packet_generic_integer(pconn, PACKET_REMOVE_UNIT, &packet);
+      }
+    }
+  } conn_list_iterate_end;
+}
+
+/**************************************************************************
   Assigns units on ptrans' tile to ptrans if they should be. This is done
   by setting their transported_by fields to the id of ptrans.

@@ -2380,7 +2415,7 @@
              && (pcargo->owner == playerid
                   || pplayers_allied(unit_owner(pcargo), unit_owner(ptrans)))) 
{
            capacity--;
-           pcargo->transported_by = ptrans->id;
+           put_unit_to_transport(pcargo, ptrans);
          }
        } unit_list_iterate_end;
       }


Can you explain why this change is necessary, and if it is, why you can't
just call send_unit_info_to_onlookers()?


 /**************************************************************************
+  Find freandly transport and assign unit to them.
+ **************************************************************************/
+bool handle_unit_enter_transport(struct unit *punit, struct tile *pdst_tile)
+{
+  if (!pdst_tile->city && is_ocean(pdst_tile->terrain) && is_ground_unit(punit)
+     && punit->transported_by == -1) {
+    struct player *pplayer = unit_owner(punit);
+    unit_list_iterate(pdst_tile->units, ptrans) {
+      if (punit->id != ptrans->id && is_ground_units_transport(ptrans)
+        && pplayers_allied(pplayer, unit_owner(ptrans))
+        && get_transporter_capacity(ptrans)) {
+       put_unit_to_transport(punit, ptrans);
+        return TRUE;
+      }
+    } unit_list_iterate_end;
+  }
+  return FALSE;
+}
+
+/**************************************************************************
 Does: 1) updates  the units homecity and the city it enters/leaves (the
          cities happiness varies). This also takes into account if the
         unit enters/leaves a fortress.
@@ -2637,18 +2692,22 @@
                                          int dest_x, int dest_y)
 {
   struct city *fromcity = map_get_city(src_x, src_y);
-  struct city *tocity = map_get_city(dest_x, dest_y);
+  struct tile *dst_tile = map_get_tile(dest_x, dest_y);
+  struct city *tocity = dst_tile->city;
   struct city *homecity = NULL;
   struct player *pplayer = unit_owner(punit);
   /*  struct government *g = get_gov_pplayer(pplayer);*/
   bool senthome = FALSE;

+  if (tocity) {
+    handle_unit_enter_city(punit, tocity);
+  } else {
+    handle_unit_enter_transport(punit, dst_tile);
+  }
+


If we are not entering a city, we are entering a transport??


  - Per