<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40113 >
Madeline Book wrote:
> ... In the case where the client's player is then
> created (attach_connection_to_player(pconn, NULL) returns
> TRUE, server/connecthand.c +137) the server does not
> notify the client that its game.info.player_idx has
> changed (hence the client keeps its client.playing equal
> to NULL even though it should now have a player, and
> indeed receives conn and player info packets to that
> effect). ...
>
Good analysis. This kind of inconsistency is always a problem whenever
there are multiple copies of the same data (in this case the player
pointer in several data structures).
> Solution:
> The server sends another game info packet after the
> client's player has been created. This ensures that
> the client receives an updated player_idx field and
> accordingly sets its client.playing to a valid pointer.
>
Ahem. That's not exactly a solution, that's generally called a "hack" --
from the old days of applying a hacksaw to model railroads.
I've been trying to reduce *_info spam over the past 6 months, see PR#40000
and its predecessors.
A better (temporary) solution is to remove the validity checking, as the
player_idx is not always valid, despite the comments in the code.
I'm not a supporter of temporary solutions, when there are obvious
permanent solutions. (See below.)
> .... Hence when running a forked server (started
> by the client using "Start New Game"), which sends
> hardcoded commands right upon connection (i.e. "/set
> aifill 5"), the client.playing variable in the client
> is made consistent (i.e. not NULL) before the user can
> notice any discrepancies.
>
Excellent catch! This is how the problem was missed in the past. Only by
pretending to be a second player in a multi-player game (when you are
actually the first), you bypassed the usual command processing.
Sending those hardcoded commands is a bad idea, too. There are several
existing bug reports.
> It is strongly recommended that the game.info.player_idx
> field be completely removed as soon as convenient, and the
> client rely on a better designed and more logical mecha-
> nism to update its client.playing global variable (the
> conn info packet handler comes to mind).
>
I concur. Almost all uses were removed in PR#39872.
However, a deeper analysis shows that game.info.player_idx is based upon:
server/gamehand.c:370:
/* ? fixme: check for non-players: */
ginfo.player_idx = (pconn->player ? player_number(pconn->player) : -1);
That is, a duplicate of the connection player! Duplication be bad!
That fixme has been sitting there like a land-mine since before 1.14.2
(the earliest version that I have on hand).
As you suggest, the comprehensive solution is to replace player_idx with an
indication of the connection! In turn, the connection has (or soon will
have) the player and observer status in it.
There is *no* need for the client.playing (nee game.player_ptr) variable.
This could also eliminate the duplicate aconnection.player (and other
duplicate data).
The connection number is learned during the login sequence. It is never
changed or renumbered, unlike the player number.
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
