[Freeciv-Dev] Re: (PR#14551) client crash when moving unit

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=14551 >

Mateusz Stefek wrote:
> <URL: http://bugs.freeciv.org/Ticket/Display.html?id=14551 >
> 
> Dnia 2005-11-06 07:39:39, Jason Short napisał(a):
> 
>><URL: http://bugs.freeciv.org/Ticket/Display.html?id=14551 >
>>
>>jdorje@devon:~/src/freeciv/freeciv$ ./civ
>>1: 0x8e0ff18 Settlers at (65,40) Perak
>>civclient: packhand.c:1993: handle_tile_info: Assertion
>>`unit_list_size(ptile->units) == 0' failed.
>>Aborted (core dumped)
>>
>>It happened when I moved my Legion.  There is an enemy city (just
>>founded) right next to the old position.
> 
> 
> I guess this is the immortal bug with vision of units in foreign  
> cities. Please check your vision code if it correctly handles:
> - Enemy unit entering a city when your unit is next to it.
> - Creation of enemy unit.
> - _Not_ sending info about units in a city if you are not allied with  
> the owner. (I guess this is the case since the client practically  
> always crashes when I autoexplore)

Since all such checks are now done with can_player_see_unit_at, I don't 
see how the core vision code can be at fault here - it is never checked 
in the case of enemy cities.

However there is some slight oddness in can_player_see_unit_at, which is 
fixed by the first patch.

Looking a little closer, I see that this bug is actually caused by a 
bugfix I made.  In server_remove_unit, there was code like

   players_iterate(pplayer) {
     if (map_is_known_and_seen(unit_tile, pplayer)) {
       dlsend_packet_unit_remove(&pplayer->connections, punit->id);
     }
   } players_iterate_end;

which I changed to

   players_iterate(pplayer) {
     if (can_player_see_unit(pplayer, punit)) {
       dlsend_packet_unit_remove(pplayer->connections, punit->id);
     }
   } players_iterate_end;

because I thought it was just the same and prevents a potential way to 
cheat (by knowing when other people lose units).  However this does not 
work because when a settler founds a city, the city is first built and 
then the settler is removed...thus the player can no longer see the 
settler and the unit-remove packet is not sent.  The second patch simply 
reverts this change, although I'm not sure this is really the best thing 
to do.

I think this may actually be a symptom of a deeper bug, but that will 
take some checking.  The settler needs to be hidden from the client when 
the city is founded, not as a byproduct of the settler dying.  Otherwise 
other units that happen to be on the same tile will not be hidden.  I 
suspect this is what is happening now (and in 2.0) and allows potential 
(minor) cheating.

It's also possible that we could wipe the settler _before_ creating the 
city, but this would require some reworking of the code and probably 
isn't a good idea.

It might be a good idea to store unit 'seen' status just like we store 
unit vision range.  Then each unit would have a bv_player storing 
whether each player knows about the unit.  When hiding a unit we should 
only hide it if it is currently seen.  And we can have sanity calls like

   void refresh_unit_seen(struct unit *punit)
   {
     players_iterate(pplayer) {
       bool seen1 = can_player_see_unit(punit);
       bool seen0 = BV_ISSET(punit->seen, pplayer->player_no);

       if (seen1 && !seen0) {
         send_unit_info(pplayer->connection, punit);
       } else if (seen0 && !seen1) {
         unit_goes_out_of_sight(pplayer, punit);
       }
     } players_iterate_end;
   }

that work similarly to the current refresh_unit_vision.  This would 
allow effects to change whether units can be seen, without the 
possibility of desynchronization and ghost units.

-jason

Index: common/player.c
===================================================================
--- common/player.c     (revision 11231)
+++ common/player.c     (working copy)
@@ -292,8 +292,6 @@
 
   /* Hiding units are only seen by the V_INVIS fog layer. */
   return BV_ISSET(ptile->tile_seen[V_INVIS], pplayer->player_no);
-
-  return FALSE;
 }
 
 

Index: server/unittools.c
===================================================================
--- server/unittools.c  (revision 11231)
+++ server/unittools.c  (working copy)
@@ -1374,7 +1374,7 @@
   }
 
   players_iterate(pplayer) {
-    if (can_player_see_unit(pplayer, punit)) {
+    if (map_is_known_and_seen(unit->tile, pplayer)) {
       dlsend_packet_unit_remove(pplayer->connections, punit->id);
     }
   } players_iterate_end;