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

["Rafa³ Bursig" <bursig@xxxxxxxxx>]

Dnia 2003.09.03 20:59, Per I. Mathisen napisa³(a):

Before start I must say short history of this code...

At the begining I want fix ugly draw code bug in sdl clinent with move 
loaded transport procedure. When you move transport with units you can 
for short period of time see drawed sentry transported units insteed 
transport unit and this meen that this unit is redrawed before 
transport unit. I search this bug many time and can't find any problem 
in client then this bug must be in server. Next I look on server move 
unit code and with some investigation I found it. The problem is that 
client know anything about transported unit, for client this unit is 
simply "sentry". But lets see on it :

1) Server
When you move transport server first search units (land units on sea) 
that are unassignet to any transport and then assign then. (units is 
loaded when transport move from this tile, NOT when unit entry on this 
tile (transport) !!!!!!!). Next transporter "unlink" all carrying 
(unit->transported_by == transport->id) units from src tile and "link" 
those units with dst tile (simple move those units), next sent unit 
info about those units to clients. Now clients know that on dst tile 
are all transported/carrying units but NOT transporter. Next server 
make move transporter unit ( "unlink" him from src tile and "link" with 
dst tile) to dst tile and sent unit info (new position) to clients.

2) Client
Client get transporter unit info packade and compare old unit posiotion 
with new position with package, when are different then start move 
procedure :
a) unlink transp unit from old/src tile
b) draw old/src tile (without transport unit)
c) draw move unit animation
d) draw old/src tile (without transport unit)
e) link transp unit with new/dst tile
f) draw new/dst tile (with transprort unit)

BUG : when you redrawing b), d) you redraw part of all neighbour tiles, 
new/dst tile too and there are all transported/carrying units. Client 
simple call get_drawable_unit(...) and draw part of sentry transported 
unit.

even you "link" trasport unit with dst tile before undraw src tile you 
can't be sure that this solve all cases. Client must know that unit is 
transported !. Server know about this by "transpoted_by" field then I 
make code that send this info to client. Now get_drawable_unit(...) 
don't return transported units.

This allow me add code to update menu part and only enable "unload" 
order button when transporter realy carry some units, and draw real 
payload on unit.

Next problem is that current all clients have info about all 
transported units (own and enemy) and simple block (or not) some info 
about it. This code try solve this by adding restrictions on server, to 
send this info only to owner.

> 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.
Yes that is true curent transport code is unwarranted.

>   - It seems to send a lot of extra unit info packets from the 
> server. I do not see why this is necessary.
Client need know when you load unit on transport 
>   - It adds no comments in the code. Document changes and assumptions!
> 
> Also:
>   - Missing capability (capstr.c).
I made this code to show you and others the problem and I want get 
yours opinon.


> +++ 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.
Here you got me :) I forget about marines and attack from transports.
But this is get_drawable_unit(...) and this is only case when it could 
return transported unit.

> -  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.
No, this isn't true the same check is make for all transported unit 
plus the ransport unit. When you have transport with 30 tanks then you 
don't need check this 31 times, but this is bigger question what can 
you get form transported units ? Only transporter should make this 
check.

When your transporter unit have only 1 tile range of seen and 
transported spy have 2 tiles then should you see 1 or 2 tiles ?
IMHO only 1 becouse main units here is transporter.
The same problem is when you have land transporter and loaded with 
units . When somebody attack this tile then transporter sould fight not 
cargo.

> The client core code should never need to check 
> punit-> >transported_by.
This is plain wrong becouse this give you draw bugs and you never know 
how many units is loaded on transporter.

> 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.
This is not unsentry function only unload call, and you want unload 
units from sellected transport unit not firts on list sentry unit.
Current all transport unit can allways sent unload order then when you 
are in port city and there is some sentry units and give unload orders 
then you simple activates those units not unload from transport.

This code was made as dirty hack to fix problems with transport code.

> +++ 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.
Per this is simple change you can sent info to player when unit is not 
transported or is transported by player or is transported by player2 
who give you shared vision.

> 
> @@ -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 check must be done somewhere ! I prefer make it here and don't 
change function call (remove bool carried) in code.

> 
>  /
> **************************************************************************
> +  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()?
Becouse this help us track all load moments, set sentry for those units 
and allow us inform about it clients. We can't use 
send_unit_info_to_onlookers() for this becous this send only unit info 
package. In this situation info package is send only to unit owner (+ 
players with his shared vision) and transport owner (+ players with his 
shared vision) for rest is send remove unit package to force remove 
this unit (hide).

> 
> 
>  /
> **************************************************************************
> +  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??
Revers situations when we entering city/"sea city" we don't need check 
entering transport.
I want have this part clean then I move all checks to 
handle_unit_enter_transport(...) function :
if (!tocity && is_ocean(pdst_tile->terrain) &&
      is_ground_unit(punit)  && punit->transported_by == -1) {
   handle_unit_enter_transport(...)
}

Current load transport code work nice when you have in city some sentry 
units and your transport moving away take some of them (auto load) but 
problem is when your sea transport is on sea and your land unit enter 
to him. This unit isn't loaded untill transporter move to next tile.
Unsentry as unloading only try fix this situation.

I try solve it by adding this here after unit move animation, for other 
players unit should be removed and owner should get info about 
"transported_by". I still don't like this code and IMHO we need some 
design work here.

We need code that allow : load rocket land transport (with rockets) to 
sea transport (can't carry rockets) , move to next contient and unload 
land transport with cargo by unloading only transporter. 
Rafal