[Freeciv-Dev] Re: (PR#3648) Superfluous tiles bug

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Arnstein Lindgard wrote:
> I think this patch here will significantly reduce lag when a player
> reconnects. It should probably be applied to the pubservers.
> 
> I set up a client to study incoming packets in the hope of optimizing
> the network code itself, failed miserably, but found some redundancy
> that seems not to increase the systems robustness.
> 
> When a dead player reconnects, the number of PACKET_TILE_INFOs is
> reduced by 50% because each tile was blessed with to identical packets.
> When a living player reconnects, the gain seems to be ~43%.
> 
> The other patch deals with Apollo. Seems the whole damn map is sent
> every turn, and in duplicate because of the first bug! In function
> do_apollo_program() it seems superfluous to send tile and city info,
> since it's taken care of elsewhere in the code after map_know_all()
> is called. You still get updates when a fogged city changes.
> 
> I tested by having the client check if a packet actually contained
> new information. In the case of Apollo, 4000 out of 4006 packets each
> turn were wasted (default mapsize 80 x 50 = 4000).

> ------------------------------------------------------------------------
> 
> --- cvs/server/maphand.c      Sat Feb 22 10:43:01 2003
> +++ antilag1/server/maphand.c Fri Mar  7 12:23:09 2003
> @@ -324,7 +324,9 @@
>       send_tile_info_always(pplayer, &pconn->self, x, y);
>        } else if (map_get_known(x, y, pplayer)) {
>       send_NODRAW_tiles(pplayer, &pconn->self, x, y, 0);
> -     send_tile_info_always(pplayer, &pconn->self, x, y);
> +    if (!map_get_sent(x, y, pplayer)) {
> +      send_tile_info_always(pplayer, &pconn->self, x, y);
> +    }
>        }
>      } conn_list_iterate_end;
>    } whole_map_iterate_end;

I'm pretty sure this is not safe.  The "sent" information is only used 
and updated for NODRAW tiles.  So any arbitrary tile may be marked as 
sent, even if an update is needed.  It could be fixed by clearing the 
sent field for all tiles before the loop (via clear_unknown_tiles_set or 
whatever) along with this change, or by checking whether the NODRAW tile 
is known rather than if it's sent (thus avoiding duplicates), or simply 
by removing the use of NODRAW tiles entirely.

On the other hand, you are right - the current code means most tiles are 
sent twice.  Once in the NODRAW call (which sends every tile adjacent to 
the current tile, but only once) and another time in the 
send_tile_info_always call.

> ------------------------------------------------------------------------
> 
> --- cvs/server/srv_main.c     Tue Feb 18 15:52:11 2003
> +++ antilag1/server/srv_main.c        Fri Mar  7 13:17:35 2003
> @@ -316,8 +316,6 @@
>        } players_iterate_end;
>      } else {
>        map_know_all(pplayer);
> -      send_all_known_tiles(&pplayer->connections);
> -      send_all_known_cities(&pplayer->connections);
>      }
>    }
>  }

This, on the other hand, seems completely safe.  map_know_all will 
already send the tile and city information for every tile, and checks 
for duplicates (already-sent data) to boot (i.e. it only sends 6 
packets, not 4000).

jason