Enforce unique, nonempty labels for players

src/games/file.cc

@@ -531,7 +531,7 @@ void ReadPlayers(GameFileLexer &p_state, Game &p_game, TreeData &p_treeData)
   p_state.ExpectCurrentToken(TOKEN_RBRACE, "'}'");
   NormalizeLabelStrings(player_labels);
   for (const auto &label : player_labels) {
-    p_game->NewPlayer()->SetLabel(label);
+    p_game->NewPlayer(label);
   }
 }
 

src/games/game.cc

@@ -67,8 +67,8 @@ GameAction GameStrategyRep::GetAction(const GameInfoset &p_infoset) const
 //                       class GamePlayerRep
 //========================================================================
 
-GamePlayerRep::GamePlayerRep(GameRep *p_game, int p_id, int p_strats)
-  : m_game(p_game), m_number(p_id)
+GamePlayerRep::GamePlayerRep(GameRep *p_game, int p_id, const std::string &p_label, int p_strats)
+  : m_game(p_game), m_number(p_id), m_label(p_label)
 {
   for (int j = 1; j <= p_strats; j++) {
     m_strategies.push_back(std::make_shared<GameStrategyRep>(this, j, ""));

src/games/game.h

@@ -460,8 +460,11 @@ class GamePlayerRep : public std::enable_shared_from_this<GamePlayerRep> {
   using Strategies = ElementCollection<GamePlayer, GameStrategyRep>;
   using Sequences = ElementCollection<GamePlayer, GameSequenceRep>;
 
-  GamePlayerRep(GameRep *p_game, int p_id) : m_game(p_game), m_number(p_id) {}
-  GamePlayerRep(GameRep *p_game, int p_id, int m_strats);
+  GamePlayerRep(GameRep *p_game, int p_id, const std::string &p_label)
+    : m_game(p_game), m_number(p_id), m_label(p_label)
+  {
+  }
+  GamePlayerRep(GameRep *p_game, int p_id, const std::string &p_label, int p_strats);
   ~GamePlayerRep();
 
   bool IsValid() const { return m_valid; }
@@ -471,11 +474,7 @@ class GamePlayerRep : public std::enable_shared_from_this<GamePlayerRep> {
   Game GetGame() const;
 
   const std::string &GetLabel() const { return m_label; }
-  void SetLabel(const std::string &p_label)
-  {
-    CheckLabel(p_label);
-    m_label = p_label;
-  }
+  void SetLabel(const std::string &p_label);
 
   bool IsChance() const { return (m_number == 0); }
 
@@ -764,6 +763,8 @@ class GameRep : public std::enable_shared_from_this<GameRep> {
   /// Mark that the content of the game has changed
   void IncrementVersion() { m_version++; }
   void IndexStrategies() const;
+  /// Validate that p_label is a nonempty, valid, unique label for a player of this game,
+  void CheckPlayerLabel(const std::string &p_label) const;
   //@}
 
   /// Hooks for derived classes to update lazily-computed orderings if required
@@ -1162,7 +1163,7 @@ class GameRep : public std::enable_shared_from_this<GameRep> {
   virtual GamePlayer GetChance() const = 0;
   auto GetPlayersWithChance() const { return prepend_value(GetChance(), GetPlayers()); }
   /// Creates a new player in the game, with no moves
-  virtual GamePlayer NewPlayer() = 0;
+  virtual GamePlayer NewPlayer(const std::string &p_label) = 0;
   //@}
 
   /// @name Dimensions of the game
@@ -1336,9 +1337,35 @@ inline void GameInfosetRep::SetLabel(const std::string &p_label)
   }
   m_label = p_label;
 }
+inline void GameRep::CheckPlayerLabel(const std::string &p_label) const
+{
+  if (p_label.empty()) {
+    throw ValueException("Player label must not be empty");
+  }
+  CheckLabel(p_label);
+  if (IsTree() && p_label == GetChance()->GetLabel()) {
+    throw ValueException("Player label must not be the reserved chance player label");
+  }
+  for (const auto &player : m_players) {
+    if (player->GetLabel() == p_label) {
+      throw ValueException("Player label must be unique within the game");
+    }
+  }
+}
 inline bool GameInfosetRep::IsChanceInfoset() const { return m_player->IsChance(); }
 
 inline Game GamePlayerRep::GetGame() const { return m_game->shared_from_this(); }
+inline void GamePlayerRep::SetLabel(const std::string &p_label)
+{
+  if (IsChance()) {
+    throw ValueException("The chance player's label cannot be changed");
+  }
+  if (p_label == m_label) {
+    return;
+  }
+  GetGame()->CheckPlayerLabel(p_label);
+  m_label = p_label;
+}
 inline GameStrategy GamePlayerRep::GetStrategy(int st) const
 {
   m_game->BuildComputedValues();

src/games/gameagg.cc

@@ -179,8 +179,8 @@ template class AGGMixedStrategyProfileRep<Rational>;
 GameAGGRep::GameAGGRep(std::shared_ptr<agg::AGG> p_aggPtr) : aggPtr(p_aggPtr)
 {
   for (int pl = 1; pl <= aggPtr->getNumPlayers(); pl++) {
-    m_players.push_back(std::make_shared<GamePlayerRep>(this, pl, aggPtr->getNumActions(pl - 1)));
-    m_players.back()->m_label = lexical_cast<std::string>(pl);
+    m_players.push_back(std::make_shared<GamePlayerRep>(this, pl, lexical_cast<std::string>(pl),
+                                                        aggPtr->getNumActions(pl - 1)));
     std::for_each(m_players.back()->m_strategies.begin(), m_players.back()->m_strategies.end(),
                   [st = 1](const std::shared_ptr<GameStrategyRep> &s) mutable {
                     s->m_label = std::to_string(st++);

src/games/gameagg.h

@@ -63,7 +63,7 @@ class GameAGGRep : public GameRep {
   /// Returns the chance (nature) player
   GamePlayer GetChance() const override { throw UndefinedException(); }
   /// Creates a new player in the game, with no moves
-  GamePlayer NewPlayer() override { throw UndefinedException(); }
+  GamePlayer NewPlayer(const std::string &) override { throw UndefinedException(); }
   //@}
 
   /// @name Nodes

src/games/gamebagg.cc

@@ -213,9 +213,8 @@ GameBAGGRep::GameBAGGRep(std::shared_ptr<agg::BAGG> _baggPtr)
   int k = 1;
   for (int pl = 1; pl <= baggPtr->getNumPlayers(); pl++) {
     for (int j = 0; j < baggPtr->getNumTypes(pl - 1); j++, k++) {
-      m_players.push_back(
-          std::make_shared<GamePlayerRep>(this, k, baggPtr->getNumActions(pl - 1, j)));
-      m_players.back()->m_label = std::to_string(k);
+      m_players.push_back(std::make_shared<GamePlayerRep>(this, k, std::to_string(k),
+                                                          baggPtr->getNumActions(pl - 1, j)));
       agent2baggPlayer[k] = pl;
       std::for_each(m_players.back()->m_strategies.begin(), m_players.back()->m_strategies.end(),
                     [st = 1](const std::shared_ptr<GameStrategyRep> &s) mutable {

src/games/gamebagg.h

@@ -70,7 +70,7 @@ class GameBAGGRep : public GameRep {
   /// Returns the chance (nature) player
   GamePlayer GetChance() const override { throw UndefinedException(); }
   /// Creates a new player in the game, with no moves
-  GamePlayer NewPlayer() override { throw UndefinedException(); }
+  GamePlayer NewPlayer(const std::string &) override { throw UndefinedException(); }
   //@}
 
   /// @name Nodes

src/games/gametable.cc

@@ -376,8 +376,9 @@ GameTableRep::GameTableRep(const std::vector<int> &dim, bool p_sparseOutcomes /*
   : m_results(std::accumulate(dim.begin(), dim.end(), 1, std::multiplies<>()))
 {
   for (const auto &nstrat : dim) {
-    m_players.push_back(std::make_shared<GamePlayerRep>(this, m_players.size() + 1, nstrat));
-    m_players.back()->m_label = lexical_cast<std::string>(m_players.size());
+    const auto pl = m_players.size() + 1;
+    m_players.push_back(
+        std::make_shared<GamePlayerRep>(this, pl, lexical_cast<std::string>(pl), nstrat));
     std::for_each(m_players.back()->m_strategies.begin(), m_players.back()->m_strategies.end(),
                   [st = 1](const std::shared_ptr<GameStrategyRep> &s) mutable {
                     s->m_label = std::to_string(st++);
@@ -494,10 +495,11 @@ void GameTableRep::WriteNfgFile(std::ostream &p_file) const
 //                       GameTableRep: Players
 //------------------------------------------------------------------------
 
-GamePlayer GameTableRep::NewPlayer()
+GamePlayer GameTableRep::NewPlayer(const std::string &p_label)
 {
+  CheckPlayerLabel(p_label);
+  auto player = std::make_shared<GamePlayerRep>(this, m_players.size() + 1, p_label, 1);
   IncrementVersion();
-  auto player = std::make_shared<GamePlayerRep>(this, m_players.size() + 1, 1);
   m_players.push_back(player);
   for (const auto &outcome : m_outcomes) {
     outcome->m_payoffs[player.get()] = Number();

src/games/gametable.h

@@ -76,7 +76,7 @@ class GameTableRep : public GameExplicitRep {
   /// Returns the chance (nature) player
   GamePlayer GetChance() const override { throw UndefinedException(); }
   /// Creates a new player in the game, with no moves
-  GamePlayer NewPlayer() override;
+  GamePlayer NewPlayer(const std::string &p_label) override;
   //@}
 
   /// @name Nodes

src/games/gametree.cc

@@ -718,7 +718,7 @@ GameInfoset GameTreeRep::InsertMove(GameNode p_node, GameInfoset p_infoset)
 
 GameTreeRep::GameTreeRep()
   : m_root(std::make_shared<GameNodeRep>(this, nullptr)),
-    m_chance(std::make_shared<GamePlayerRep>(this, 0))
+    m_chance(std::make_shared<GamePlayerRep>(this, 0, "Chance"))
 {
 }
 
@@ -1524,10 +1524,11 @@ int GameTreeRep::BehavProfileLength() const
 //                        GameTreeRep: Players
 //------------------------------------------------------------------------
 
-GamePlayer GameTreeRep::NewPlayer()
+GamePlayer GameTreeRep::NewPlayer(const std::string &p_label)
 {
+  CheckPlayerLabel(p_label);
+  auto player = std::make_shared<GamePlayerRep>(this, m_players.size() + 1, p_label);
   IncrementVersion();
-  auto player = std::make_shared<GamePlayerRep>(this, m_players.size() + 1);
   m_players.push_back(player);
   for (const auto &outcome : m_outcomes) {
     outcome->m_payoffs[player.get()] = Number();

src/games/gametree.h

@@ -136,7 +136,7 @@ class GameTreeRep final : public GameExplicitRep {
   /// Returns the chance (nature) player
   GamePlayer GetChance() const override { return m_chance->shared_from_this(); }
   /// Creates a new player in the game, with no moves
-  GamePlayer NewPlayer() override;
+  GamePlayer NewPlayer(const std::string &p_label) override;
   //@}
 
   /// @name Nodes

src/gui/dlinsertmove.cc

@@ -190,9 +190,7 @@ GamePlayer InsertMoveDialog::GetPlayer() const
   if (playerNumber <= static_cast<int>(m_doc->GetGame()->NumPlayers())) {
     return m_doc->GetGame()->GetPlayer(playerNumber);
   }
-  const GamePlayer player = m_doc->GetGame()->NewPlayer();
-  player->SetLabel("Player " + lexical_cast<std::string>(m_doc->GetGame()->NumPlayers()));
-  return player;
+  return m_doc->DoNewPlayer();
 }
 
 GameInfoset InsertMoveDialog::GetInfoset() const

src/gui/gamedoc.cc

@@ -22,6 +22,7 @@
 
 #include <sstream>
 #include <fstream>
+#include <set>
 
 #include "gambit.h"
 #include "core/tinyxml.h" // for XML parser for LoadDocument()
@@ -479,14 +480,24 @@ void GameDocument::DoSetTitle(const wxString &p_title, const wxString &p_comment
   NotifyChanged(GameModificationType::GameLabels);
 }
 
-void GameDocument::DoNewPlayer()
+GamePlayer GameDocument::DoNewPlayer()
 {
-  const GamePlayer player = m_game->NewPlayer();
-  player->SetLabel("Player " + lexical_cast<std::string>(player->GetNumber()));
+  std::set<std::string> playerLabels;
+
+  for (const auto &player : m_game->GetPlayers()) {
+    playerLabels.insert(player->GetLabel());
+  }
+
+  int number = m_game->NumPlayers() + 1;
+  while (contains(playerLabels, "Player " + lexical_cast<std::string>(number))) {
+    number++;
+  }
+  const GamePlayer player = m_game->NewPlayer("Player " + lexical_cast<std::string>(number));
   if (!m_game->IsTree()) {
     player->GetStrategy(1)->SetLabel("1");
   }
   NotifyChanged(GameModificationType::GameForm);
+  return player;
 }
 
 void GameDocument::DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label)

src/gui/gamedoc.h

@@ -295,7 +295,7 @@ class GameDocument {
   }
   void DoSave(const wxString &p_filename, GameSaveFormat p_format);
   void DoSetTitle(const wxString &p_title, const wxString &p_comment);
-  void DoNewPlayer();
+  GamePlayer DoNewPlayer();
   void DoSetPlayerLabel(GamePlayer p_player, const wxString &p_label);
   void DoNewStrategy(GamePlayer p_player);
   void DoDeleteStrategy(GameStrategy p_strategy);
@@ -333,8 +333,8 @@ inline GameDocument *NewTreeDocument()
 {
   const Game efg = NewTree();
   efg->SetTitle("Untitled Extensive Game");
-  efg->NewPlayer()->SetLabel("Player 1");
-  efg->NewPlayer()->SetLabel("Player 2");
+  efg->NewPlayer("Player 1");
+  efg->NewPlayer("Player 2");
   return new GameDocument(efg);
 }
 

src/pygambit/gambit.pxd

@@ -319,7 +319,7 @@ cdef extern from "games/game.h":
         c_GamePlayer GetPlayer(int) except +IndexError
         Players GetPlayers() except +
         c_GamePlayer GetChance() except +
-        c_GamePlayer NewPlayer() except +
+        c_GamePlayer NewPlayer(string) except +ValueError
 
         int NumOutcomes() except +
         c_GameOutcome GetOutcome(int) except +IndexError

src/pygambit/game.pxi

@@ -563,7 +563,7 @@ class Game:
         g = Game.wrap(NewTree())
         g.title = title
         for player in (players or []):
-            Player.wrap(g.game.deref().NewPlayer()).label = str(player)
+            g.game.deref().NewPlayer(str(player).encode("ascii"))
         return g
 
     @classmethod
@@ -2053,23 +2053,32 @@ class Game:
                 "Operation only defined for games with a tree representation"
             )
 
-    def add_player(self, label: str = "") -> Player:
+    def add_player(self, label: str) -> Player:
         """Add a new player to the game.
 
+        .. versionchanged:: 16.7.0
+            A label is now required and must be nonempty and unique among the game's players.
+            In extensive games, the label cannot be ``"Chance"``, which is reserved for the
+            chance player.
+
         Parameters
         ----------
-        label : str, default ""
-            The label for the player.
+        label : str
+            The label for the new player.  Must be nonempty and not the same as the label
+            of an existing player in the game.
 
         Returns
         -------
         Player
             A reference to the newly-created player.
+
+        Raises
+        ------
+        ValueError
+            If `label` is empty, is already the label of another player, or (in an
+            extensive game) is ``"Chance"``, the reserved label of the chance player.
         """
-        p = Player.wrap(self.game.deref().NewPlayer())
-        if str(label) != "":
-            p.label = str(label)
-        return p
+        return Player.wrap(self.game.deref().NewPlayer(label.encode("ascii")))
 
     def set_player(self, infoset: Infoset | str,
                    player: Player | str) -> None:

src/pygambit/player.pxi

@@ -256,11 +256,6 @@ class Player:
 
     @label.setter
     def label(self, value: str) -> None:
-        if value == self.label:
-            return
-        if value == "" or value in (player.label for player in self.game.players):
-            warnings.warn("In a future version, players must have unique labels",
-                          FutureWarning)
         self.player.deref().SetLabel(value.encode("ascii"))
 
     @property

tests/test_extensive.py

@@ -42,11 +42,6 @@ def test_game_add_players_label(players: list):
         assert player.label == label
 
 
-def test_game_add_players_nolabel():
-    game = gbt.Game.new_tree()
-    game.add_player()
-
-
 @pytest.mark.parametrize("game_input,expected_result", [
     # Games with perfect recall from files (game_input is a string)
     (gbt.catalog.load("journals/ijgt/selten1975/fig2"), True),