[Freeciv-Dev] (PR#13429) Move player_research to team

["Mateusz Stefek" <mstefek@xxxxxxxxx>]

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

> > 2.  get_player_research should return NULL if the player has no team.
> > Callers must be audited for teamless players or NULL return values.
> 
> Why can't we assume that each players has a team?
> An autogame with barbarians and civil war seems to work.
> 
>  
> > -jason
> Updated patch attached. With assertion in get_player_research()
> 

This patch makes sure that each player has a team. If pteam is NULL in
team_add_player(), an empty team will be choosen.

Some parts of code on the client side use get_tech_name() before players
are created.
The solution to this problem is to create a new function
get_normal_tech_name(Tech_type_id) which should be called only on
existing techs - This always was bugging me - I don't need a valid
player pointer to get normal tech's name.
--
mateusz

? civgame.log
? civscore.log
? regression
? server/civgame-1000.sav.gz
? server/civgame-1500.sav.gz
? server/civgame-2000.sav.gz
? server/civgame-2500.sav.gz
? server/civgame-3000.sav.gz
? server/civgame-3500.sav.gz
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.528
diff -u -r1.528 packhand.c
--- client/packhand.c   9 Jul 2005 17:46:07 -0000       1.528
+++ client/packhand.c   9 Jul 2005 20:17:24 -0000
@@ -1416,7 +1416,7 @@
   bool poptechup, new_tech = FALSE;
   char msg[MAX_LEN_MSG];
   struct player *pplayer = &game.players[pinfo->playerno];
-  struct player_research* research = get_player_research(pplayer);
+  struct player_research* research;
 
   sz_strlcpy(pplayer->name, pinfo->name);
 
@@ -1470,6 +1470,8 @@
    * only send the player info out at appropriate times - e.g., while the
    * game is running. */
   new_tech = read_player_info_techs(pplayer, pinfo->inventions);
+  
+  research = get_player_research(pplayer);
 
   poptechup = (research->researching != pinfo->researching
                || research->tech_goal != pinfo->tech_goal);
@@ -2172,7 +2174,7 @@
       if (tech_exists(b->obsolete_by)) {
        freelog(LOG_DEBUG, "  obsolete_by %2d/%s",
                b->obsolete_by,
-               get_tech_name(game.player_ptr, b->obsolete_by));
+               get_normal_tech_name(b->obsolete_by));
       } else {
        freelog(LOG_DEBUG, "  obsolete_by %2d/Never", b->obsolete_by);
       }
Index: client/tilespec.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/tilespec.c,v
retrieving revision 1.313
diff -u -r1.313 tilespec.c
--- client/tilespec.c   9 Jul 2005 17:46:07 -0000       1.313
+++ client/tilespec.c   9 Jul 2005 20:17:35 -0000
@@ -2378,7 +2378,7 @@
       = lookup_sprite_tag_alt(t, advances[id].graphic_str,
                              advances[id].graphic_alt,
                              FALSE, "tech_type",
-                             get_tech_name(game.player_ptr, id));
+                             get_normal_tech_name(id));
 
     /* should maybe do something if NULL, eg generic default? */
   } else {
Index: common/game.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/game.c,v
retrieving revision 1.226
diff -u -r1.226 game.c
--- common/game.c       9 Jul 2005 17:46:08 -0000       1.226
+++ common/game.c       9 Jul 2005 20:17:39 -0000
@@ -331,7 +331,6 @@
 ***************************************************************/
 void game_free(void)
 {
-  clean_players_research();
   game_remove_all_players();
   map_free();
   idex_free();
@@ -466,9 +465,6 @@
   city_list_free(pplayer->cities);
   pplayer->cities = NULL;
 
-  free(pplayer->research);
-  pplayer->research = NULL;
-
   if (is_barbarian(pplayer)) game.info.nbarbarians--;
 }
 
Index: common/player.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.c,v
retrieving revision 1.187
diff -u -r1.187 player.c
--- common/player.c     4 Jul 2005 17:48:37 -0000       1.187
+++ common/player.c     9 Jul 2005 20:17:41 -0000
@@ -79,16 +79,6 @@
 }
 
 /***************************************************************
- Fill the structure with some sane values
-***************************************************************/
-void player_research_init(struct player_research* research)
-{
-  memset(research, 0, sizeof(struct player_research));
-  research->tech_goal = A_UNSET;
-  research->changed_from = -1;
-}
-
-/***************************************************************
   In the server you must use server_player_init.  Note that
   this function is matched by game_remove_player() in game.c,
   there is no corresponding player_free() in this file.
@@ -133,8 +123,7 @@
   plr->economic.tax=PLAYER_DEFAULT_TAX_RATE;
   plr->economic.science=PLAYER_DEFAULT_SCIENCE_RATE;
   plr->economic.luxury=PLAYER_DEFAULT_LUXURY_RATE;
-  plr->research = fc_malloc(sizeof(struct player_research));
-  player_research_init(plr->research);
+
   player_limit_to_government_rates(plr);
   spaceship_init(&plr->spaceship);
 
@@ -725,51 +714,11 @@
 }
 
 /****************************************************************************
-  Merges research of two players. This is used by teams
-****************************************************************************/
-void merge_players_research(struct player* p1, struct player* p2)
-{
-  struct player_research* old_research;
-  if (p1->research == p2->research) {
-    return;
-  }
-  old_research = p1->research;
-  players_iterate(aplayer) {
-    if (aplayer->research == old_research) {
-      aplayer->research = p2->research;
-    }
-  } players_iterate_end;
-  free(old_research);
-}
-
-/****************************************************************************
-****************************************************************************/
-void clean_players_research()
-{
-  players_iterate(aplayer) {
-    if (aplayer->research) {
-      players_iterate(bplayer) {
-        if (aplayer == bplayer || aplayer->research != bplayer->research) {
-         continue;
-       }
-       bplayer->research = NULL;
-      } players_iterate_end;
-      free(aplayer->research);
-      aplayer->research = NULL;
-    }
-  } players_iterate_end;
-  
-  players_iterate(aplayer) {
-    aplayer->research = fc_malloc(sizeof (struct player_research));
-    player_research_init(aplayer->research);
-  } players_iterate_end;
-}
-
-/****************************************************************************
   Returns player_research struct of the given player. Note that team
   members share research
 ****************************************************************************/
-struct player_research *get_player_research(const struct player *p1)
+struct player_research *get_player_research(const struct player *plr)
 {
-  return p1->research;
+  assert(plr && plr->team);
+  return &(plr->team->research);
 }
Index: common/player.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.h,v
retrieving revision 1.158
diff -u -r1.158 player.h
--- common/player.h     7 Jul 2005 08:34:38 -0000       1.158
+++ common/player.h     9 Jul 2005 20:17:41 -0000
@@ -68,55 +68,6 @@
   int luxury;
 };
 
-struct player_research {
-  /* The number of techs and future techs the player has
-   * researched/acquired. */
-  int techs_researched, future_tech;
-
-  /* Invention being researched in. Valid values for researching are:
-   *  - any existing tech but not A_NONE or
-   *  - A_FUTURE.
-   * In addition A_NOINFO is allowed at the client for enemies.
-   *
-   * bulbs_researched tracks how many bulbs have been accumulated toward
-   * this research target. */
-  Tech_type_id researching;        
-  int bulbs_researched;
-
-  /* If the player changes his research target in a turn, he loses some or
-   * all of the bulbs he's accumulated toward that target.  We save the
-   * original info from the start of the turn so that if he changes back
-   * he will get the bulbs back. */
-  Tech_type_id changed_from;
-  int bulbs_researched_before;
-
-  /* If the player completed a research this turn, this value is turned on
-   * and changing targets may be done without penalty. */
-  bool got_tech;
-
-  struct {
-    /* One of TECH_UNKNOWN, TECH_KNOWN or TECH_REACHABLE. */
-    enum tech_state state;
-
-    /* 
-     * required_techs, num_required_techs and bulbs_required are
-     * cached values. Updated from build_required_techs (which is
-     * called by update_research).
-     */
-    tech_vector required_techs;
-    int num_required_techs, bulbs_required;
-  } inventions[A_LAST];
-
-  /* Tech goal (similar to worklists; when one tech is researched the next
-   * tech toward the goal will be chosen).  May be A_NONE. */
-  Tech_type_id tech_goal;
-
-  /*
-   * Cached values. Updated by update_research.
-   */
-  int num_known_tech_with_flag[TF_LAST];
-};
-
 struct player_score {
   int happy;
   int content;
@@ -225,7 +176,7 @@
   struct city_list *cities;
   struct player_score score;
   struct player_economic economic;
-  struct player_research* research;
+
   int bulbs_last_turn;    /* # bulbs researched last turn only */
   struct player_spaceship spaceship;
   struct player_ai ai;
@@ -247,7 +198,6 @@
   bv_debug debug;
 };
 
-void player_research_init(struct player_research* research);
 void player_init(struct player *plr);
 struct player *find_player_by_name(const char *name);
 struct player *find_player_by_name_prefix(const char *name,
@@ -320,8 +270,6 @@
 
 struct player_research *get_player_research(const struct player *p1);
 
-void merge_players_research(struct player* p1, struct player* p2);
-void clean_players_research(void);
 #define players_iterate(PI_player)                                            \
 {                                                                             \
   struct player *PI_player;                                                   \
Index: common/team.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/team.c,v
retrieving revision 1.5
diff -u -r1.5 team.c
--- common/team.c       24 Jun 2005 04:34:03 -0000      1.5
+++ common/team.c       9 Jul 2005 20:17:42 -0000
@@ -86,12 +86,16 @@
    * situation. */
   team_remove_player(pplayer);
 
+
+  if (!pteam) {
+    pteam = find_empty_team();
+  }
+
   /* Put the player on the new team. */
   pplayer->team = pteam;
-  if (pteam) {
-    pteam->players++;
-    assert(pteam->players <= MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS);
-  }
+  
+  pteam->players++;
+  assert(pteam->players <= MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS);
 }
 
 /****************************************************************************
@@ -187,5 +191,6 @@
     sz_strlcpy(teams[i].name, names[i]);
 
     teams[i].players = 0;
+    player_research_init(&(teams[i].research));
   }
 }
Index: common/team.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/team.h,v
retrieving revision 1.2
diff -u -r1.2 team.h
--- common/team.h       21 May 2005 19:40:24 -0000      1.2
+++ common/team.h       9 Jul 2005 20:17:42 -0000
@@ -16,11 +16,15 @@
 
 #include "fc_types.h"
 
+#include "tech.h"
+
 #define MAX_NUM_TEAMS (MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS)
 
 struct team {
   Team_type_id index;
   char name[MAX_LEN_NAME];
+  
+  struct player_research research;
 
   int players; /* # of players on the team */
 };
Index: common/tech.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/tech.c,v
retrieving revision 1.98
diff -u -r1.98 tech.c
--- common/tech.c       8 Jul 2005 03:31:18 -0000       1.98
+++ common/tech.c       9 Jul 2005 20:17:47 -0000
@@ -629,6 +629,16 @@
 }
 
 /**************************************************************************
+ Return the name of the given tech. You don't have to free the return
+ pointer.
+**************************************************************************/
+const char *get_normal_tech_name(Tech_type_id tech)
+{
+  assert(tech_exists(tech));
+  return advances[tech].name;
+}
+
+/**************************************************************************
  Returns true if the costs for the given technology will stay constant
  during the game. False otherwise.
 **************************************************************************/
@@ -678,3 +688,13 @@
     tech_free(i);
   }
 }
+
+/***************************************************************
+ Fill the structure with some sane values
+***************************************************************/
+void player_research_init(struct player_research* research)
+{
+  memset(research, 0, sizeof(struct player_research));
+  research->tech_goal = A_UNSET;
+  research->changed_from = -1;
+}
Index: common/tech.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/tech.h,v
retrieving revision 1.57
diff -u -r1.57 tech.h
--- common/tech.h       8 Jul 2005 03:31:18 -0000       1.57
+++ common/tech.h       9 Jul 2005 20:17:47 -0000
@@ -109,6 +109,57 @@
 
 BV_DEFINE(tech_vector, A_LAST);
 
+struct player_research {
+  /* The number of techs and future techs the player has
+   * researched/acquired. */
+  int techs_researched, future_tech;
+
+  /* Invention being researched in. Valid values for researching are:
+   *  - any existing tech but not A_NONE or
+   *  - A_FUTURE.
+   * In addition A_NOINFO is allowed at the client for enemies.
+   *
+   * bulbs_researched tracks how many bulbs have been accumulated toward
+   * this research target. */
+  Tech_type_id researching;        
+  int bulbs_researched;
+
+  /* If the player changes his research target in a turn, he loses some or
+   * all of the bulbs he's accumulated toward that target.  We save the
+   * original info from the start of the turn so that if he changes back
+   * he will get the bulbs back. */
+  Tech_type_id changed_from;
+  int bulbs_researched_before;
+
+  /* If the player completed a research this turn, this value is turned on
+   * and changing targets may be done without penalty. */
+  bool got_tech;
+
+  struct {
+    /* One of TECH_UNKNOWN, TECH_KNOWN or TECH_REACHABLE. */
+    enum tech_state state;
+
+    /* 
+     * required_techs, num_required_techs and bulbs_required are
+     * cached values. Updated from build_required_techs (which is
+     * called by update_research).
+     */
+    tech_vector required_techs;
+    int num_required_techs, bulbs_required;
+  } inventions[A_LAST];
+
+  /* Tech goal (similar to worklists; when one tech is researched the next
+   * tech toward the goal will be chosen).  May be A_NONE. */
+  Tech_type_id tech_goal;
+
+  /*
+   * Cached values. Updated by update_research.
+   */
+  int num_known_tech_with_flag[TF_LAST];
+};
+
+void player_research_init(struct player_research* research);
+
 enum tech_state get_invention(const struct player *pplayer,
                              Tech_type_id tech);
 void set_invention(struct player *pplayer, Tech_type_id tech,
@@ -138,6 +189,7 @@
                            Tech_type_id goal);
 bool is_future_tech(Tech_type_id tech);
 const char *get_tech_name(const struct player *pplayer, Tech_type_id tech);
+const char *get_normal_tech_name(Tech_type_id tech);
 
 void precalc_tech_data(void);
 
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.260
diff -u -r1.260 savegame.c
--- server/savegame.c   9 Jul 2005 17:46:09 -0000       1.260
+++ server/savegame.c   9 Jul 2005 20:18:02 -0000
@@ -1738,7 +1738,14 @@
   int id;
   int target_no;
   struct team *pteam;
-  struct player_research *research = get_player_research(plr);
+  struct player_research *research;
+
+  /* not all players have teams */
+  id = secfile_lookup_int_default(file, -1, "player%d.team_no", plrno);
+  pteam = team_get_by_id(id);
+  team_add_player(plr, pteam);
+  
+  research = get_player_research(plr);
 
   server_player_init(plr, TRUE, FALSE);
   ai = ai_data_get(plr);
@@ -1791,14 +1798,6 @@
   /* Add techs from game and nation, but ignore game.info.tech. */
   init_tech(plr, 0);
 
-  /* not all players have teams */
-  id = secfile_lookup_int_default(file, -1, "player%d.team_no", plrno);
-  pteam = team_get_by_id(id);
-  if (pteam) {
-    /* Players with no team will be assigned to an empty team later. */
-    team_add_player(plr, pteam);
-  }
-
   if (is_barbarian(plr)) {
     plr->nation = game.control.nation_count - 1;
   }
@@ -3398,7 +3397,7 @@
 
     game.info.min_players   = secfile_lookup_int(file, "game.min_players");
     game.info.max_players   = secfile_lookup_int(file, "game.max_players");
-    game.info.nplayers      = secfile_lookup_int(file, "game.nplayers");
+
     game.info.heating = secfile_lookup_int_default(file, 0, "game.heating");
     game.info.globalwarming = secfile_lookup_int(file, "game.globalwarming");
     game.info.warminglevel  = secfile_lookup_int(file, "game.warminglevel");
@@ -3490,6 +3489,7 @@
        game.info.diplchance = 100 - (10 * (game.info.diplchance - 1));
       }
     }
+
     game.info.aqueductloss = secfile_lookup_int_default(file, 
game.info.aqueductloss,
                                                   "game.aqueductloss");
     game.info.killcitizen = secfile_lookup_int_default(file, 
game.info.killcitizen,
@@ -3578,6 +3578,9 @@
     load_rulesets();
   }
 
+  game.info.nplayers      = secfile_lookup_int(file, "game.nplayers");
+
+
   script_state_load(file);
 
   {
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.276
diff -u -r1.276 srv_main.c
--- server/srv_main.c   29 Jun 2005 02:17:53 -0000      1.276
+++ server/srv_main.c   9 Jul 2005 20:18:07 -0000
@@ -1909,14 +1909,6 @@
    } players_iterate_end;
   }
   
-  players_iterate(pplayer) {
-    players_iterate(pdest) {
-      if (players_on_same_team(pplayer, pdest)
-          && pplayer->player_no != pdest->player_no) {
-       merge_players_research(pplayer, pdest);
-      }
-    } players_iterate_end;
-  } players_iterate_end;
   /* tell the gamelog about the players */
   players_iterate(pplayer) {
     gamelog(GAMELOG_PLAYER, pplayer);