Re: [Freeciv-Dev] (PR#40113) connection missing client.playing during nation selection
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40113 On Mon, Feb 25, 2008 at 11:15 PM, William Allen Simpson [EMAIL PROTECTED] wrote: It would seem to make more sense to change aconnection .player to client.playing in the above (or the converse ;)). It looks like some incomplete conversion when both occur in the code like this. And yet Per chose to split the functionality. I do not recall doing any such thing. It is not the code that I have typically worked on. Generally I did not touch the client if I could avoid it. I agree this is a curious change. What is the intended difference in use of aconnection.player and game.player_ptr? I have no idea. Book wrote: Hi Per, can you help us to understand? (I know you are reading this :. Sorry, I do not read most threads on this list anymore. If you want my comments on something, you may have to notify me privately. - Per ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40113) multi-player connection missing client.playing during nation selection
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#4 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
Re: [Freeciv-Dev] (PR#40113) connection missing client.playing during nation selection
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40113
I indicated earlier in the discussion thread where you added the revised
test that is now failing, because the two pointers are updated by
different packets and apparently aren't always equal
I assumed you had a reason for making the change. Since you don't, I'll
remove the change. Thanks for letting us know.
Per I. Mathisen wrote:
I do not recall doing any such thing. It is not the code that I have
typically worked on. Generally I did not touch the client if I could
avoid it.
This aconnection.player change appeared in PR#16459:
static void pick_nation_callback(GtkWidget *w, gpointer data)
{
- if (game.player_ptr) {
+ if (aconnection.player) {
popup_races_dialog(game.player_ptr);
+ } else if (game.info.is_new_game) {
+send_chat(/take -);
}
}
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] (PR#40115) vestigial game.info.player_idx removal
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40115
Almost all references to game.info.player_idx were eliminated in PR#39872.
This removes the remaining 9 instances in 6 files.
While investigating, another bug was discovered! When players are
renumbered during pre-game, aconnection.player was not updated.
After considerable discussion in PR#40113, there are no intended differences
between client.playing (nee game.info.player_ptr) and aconnection.player.
This codifies the duplication by always setting them at the same time.
client.playing: 910 instances in 91 files.
aconnection.player: 8 instances in 4 files.
Both are duplicates of the actual connection[i].player.
pc-player: 2 instances in 2 files.
conn([.-]+)player: 82 instances in 20 files.
Index: server/gamehand.c
===
--- server/gamehand.c (revision 14424)
+++ server/gamehand.c (working copy)
@@ -367,8 +367,6 @@
}
conn_list_iterate(dest, pconn) {
-/* ? fixme: check for non-players: */
-ginfo.player_idx = (pconn-player ? player_number(pconn-player) : -1);
send_packet_game_info(pconn, ginfo);
}
conn_list_iterate_end;
Index: server/savegame.c
===
--- server/savegame.c (revision 14424)
+++ server/savegame.c (working copy)
@@ -4407,8 +4407,6 @@
shuffle_players();
}
- game.info.player_idx = 0;
-
/* Fix ferrying sanity */
players_iterate(pplayer) {
unit_list_iterate_safe(pplayer-units, punit) {
Index: common/packets.def
===
--- common/packets.def (revision 14424)
+++ common/packets.def (working copy)
@@ -380,7 +380,6 @@
PLAYER min_players;
PLAYER max_players;
PLAYER nplayers;
- PLAYER player_idx;
UINT32 globalwarming, heating, warminglevel;
UINT32 nuclearwinter, cooling, coolinglevel;
Index: common/game.c
===
--- common/game.c (revision 14424)
+++ common/game.c (working copy)
@@ -382,7 +382,7 @@
game.info.global_advances[i]=FALSE;
for (i=0; iB_LAST; i++) /* game.num_impr_types = 0 here */
game.info.great_wonders[i]=0;
- game.info.player_idx = 0;
+
terrain_control.river_help_text[0] = '\0';
game.meta_info.user_message_set = FALSE;
@@ -608,11 +608,6 @@
assert(city_list_size(game.players[i].cities) == 0);
}
- /* has no effect in server, but adjusts in client */
- if(game.info.player_idx plrno) {
-game.info.player_idx--;
- }
-
game.info.nplayers--;
player_init(game.players[game.info.nplayers]);
Index: client/packhand.c
===
--- client/packhand.c (revision 14424)
+++ client/packhand.c (working copy)
@@ -218,9 +218,9 @@
freelog(LOG_VERBOSE, join game accept:%s, message);
aconnection.established = TRUE;
aconnection.id = conn_id;
+
agents_game_joined();
update_menus();
-
set_server_busy(FALSE);
if (get_client_page() == PAGE_MAIN
@@ -235,11 +235,13 @@
my_snprintf(msg, sizeof(msg),
_(You were rejected from the game: %s), message);
append_output_window(msg);
-aconnection.id = 0;
+aconnection.id = -1; /* not in range of conn_info id */
+
if (auto_connect) {
freelog(LOG_NORMAL, %s, msg);
}
gui_server_connect();
+
if (!with_ggz) {
set_client_page(in_ggz ? PAGE_MAIN : PAGE_GGZ);
}
@@ -1540,7 +1542,7 @@
return;
}
- if (packet-owner == game.info.player_idx) {
+ if (valid_player_by_number(packet-owner) == client.playing) {
freelog(LOG_ERROR, handle_unit_short_info() for own unit.);
}
@@ -1612,7 +1614,6 @@
game.government_when_anarchy =
government_by_number(game.info.government_when_anarchy_id);
- client.playing = valid_player_by_number(game.info.player_idx);
if (C_S_PREPARING == client_state()) {
/* FIXME: only for change in nations */
@@ -1954,6 +1955,8 @@
aconnection.established = pconn-established;
aconnection.observer = pconn-observer;
aconnection.access_level = pconn-access_level;
+ /* FIXME: duplication */
+ client.playing =
aconnection.player = pplayer;
}
}
Index: client/climisc.c
===
--- client/climisc.c(revision 14424)
+++ client/climisc.c(working copy)
@@ -62,11 +62,15 @@
**/
void client_remove_player(int plrno)
{
+ struct connection *pc = find_conn_by_id(aconnection.id);
+
game_remove_player(player_by_number(plrno));
game_renumber_players(plrno);
- /* ensure our pointer is valid */
- client.playing = valid_player_by_number(game.info.player_idx);
+ /* ensure our duplicate pointers are valid */
+
Re: [Freeciv-Dev] (PR#40115) vestigial game.info.player_idx removal
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40115 Committed trunk revision 14425. Committed S2_2 revision 14426. ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
Re: [Freeciv-Dev] (PR#40113) multi-player connection missing client.playing during nation selection
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40113 It was too hard to do it all in one patch. Please check whether the current revision fixes your problem? I'll finish removing the duplicates later ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] (PR#40113) multi-player connection missing client.playing during nation selection
URL: http://bugs.freeciv.org/Ticket/Display.html?id=40113 [wsimpson - Wed Feb 27 14:23:41 2008]: It was too hard to do it all in one patch. Please check whether the current revision fixes your problem? All trivial tests succeed, which is adequate for my purposes. ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
