diff --git a/src/games/file.cc b/src/games/file.cc index 08961017f..ed36d6a27 100644 --- a/src/games/file.cc +++ b/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); } } diff --git a/src/games/game.cc b/src/games/game.cc index a441bb0fb..9a467565b 100644 --- a/src/games/game.cc +++ b/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(this, j, "")); diff --git a/src/games/game.h b/src/games/game.h index 286ddf12b..e7fff307f 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -460,8 +460,11 @@ class GamePlayerRep : public std::enable_shared_from_this { using Strategies = ElementCollection; using Sequences = ElementCollection; - 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 { 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 { /// 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 { 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(); diff --git a/src/games/gameagg.cc b/src/games/gameagg.cc index a702e8b01..af223c993 100644 --- a/src/games/gameagg.cc +++ b/src/games/gameagg.cc @@ -179,8 +179,8 @@ template class AGGMixedStrategyProfileRep; GameAGGRep::GameAGGRep(std::shared_ptr p_aggPtr) : aggPtr(p_aggPtr) { for (int pl = 1; pl <= aggPtr->getNumPlayers(); pl++) { - m_players.push_back(std::make_shared(this, pl, aggPtr->getNumActions(pl - 1))); - m_players.back()->m_label = lexical_cast(pl); + m_players.push_back(std::make_shared(this, pl, lexical_cast(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 &s) mutable { s->m_label = std::to_string(st++); diff --git a/src/games/gameagg.h b/src/games/gameagg.h index 3b0bb979e..609924e6a 100644 --- a/src/games/gameagg.h +++ b/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 diff --git a/src/games/gamebagg.cc b/src/games/gamebagg.cc index fea3bb695..cfca1517f 100644 --- a/src/games/gamebagg.cc +++ b/src/games/gamebagg.cc @@ -213,9 +213,8 @@ GameBAGGRep::GameBAGGRep(std::shared_ptr _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(this, k, baggPtr->getNumActions(pl - 1, j))); - m_players.back()->m_label = std::to_string(k); + m_players.push_back(std::make_shared(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 &s) mutable { diff --git a/src/games/gamebagg.h b/src/games/gamebagg.h index 641a13735..c51a8a661 100644 --- a/src/games/gamebagg.h +++ b/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 diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 078969d1a..8fd04871b 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -376,8 +376,9 @@ GameTableRep::GameTableRep(const std::vector &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(this, m_players.size() + 1, nstrat)); - m_players.back()->m_label = lexical_cast(m_players.size()); + const auto pl = m_players.size() + 1; + m_players.push_back( + std::make_shared(this, pl, lexical_cast(pl), nstrat)); std::for_each(m_players.back()->m_strategies.begin(), m_players.back()->m_strategies.end(), [st = 1](const std::shared_ptr &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(this, m_players.size() + 1, p_label, 1); IncrementVersion(); - auto player = std::make_shared(this, m_players.size() + 1, 1); m_players.push_back(player); for (const auto &outcome : m_outcomes) { outcome->m_payoffs[player.get()] = Number(); diff --git a/src/games/gametable.h b/src/games/gametable.h index 04facee7d..df331c363 100644 --- a/src/games/gametable.h +++ b/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 diff --git a/src/games/gametree.cc b/src/games/gametree.cc index e89c4f0cf..535d1db19 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -718,7 +718,7 @@ GameInfoset GameTreeRep::InsertMove(GameNode p_node, GameInfoset p_infoset) GameTreeRep::GameTreeRep() : m_root(std::make_shared(this, nullptr)), - m_chance(std::make_shared(this, 0)) + m_chance(std::make_shared(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(this, m_players.size() + 1, p_label); IncrementVersion(); - auto player = std::make_shared(this, m_players.size() + 1); m_players.push_back(player); for (const auto &outcome : m_outcomes) { outcome->m_payoffs[player.get()] = Number(); diff --git a/src/games/gametree.h b/src/games/gametree.h index 4cb6e09dd..1b2e15e9e 100644 --- a/src/games/gametree.h +++ b/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 diff --git a/src/gui/dlinsertmove.cc b/src/gui/dlinsertmove.cc index 71c16a9e7..dbfc58eba 100644 --- a/src/gui/dlinsertmove.cc +++ b/src/gui/dlinsertmove.cc @@ -190,9 +190,7 @@ GamePlayer InsertMoveDialog::GetPlayer() const if (playerNumber <= static_cast(m_doc->GetGame()->NumPlayers())) { return m_doc->GetGame()->GetPlayer(playerNumber); } - const GamePlayer player = m_doc->GetGame()->NewPlayer(); - player->SetLabel("Player " + lexical_cast(m_doc->GetGame()->NumPlayers())); - return player; + return m_doc->DoNewPlayer(); } GameInfoset InsertMoveDialog::GetInfoset() const diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index f971996e2..7804e12b7 100644 --- a/src/gui/gamedoc.cc +++ b/src/gui/gamedoc.cc @@ -22,6 +22,7 @@ #include #include +#include #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(player->GetNumber())); + std::set playerLabels; + + for (const auto &player : m_game->GetPlayers()) { + playerLabels.insert(player->GetLabel()); + } + + int number = m_game->NumPlayers() + 1; + while (contains(playerLabels, "Player " + lexical_cast(number))) { + number++; + } + const GamePlayer player = m_game->NewPlayer("Player " + lexical_cast(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) diff --git a/src/gui/gamedoc.h b/src/gui/gamedoc.h index ac384fe22..b9dbb1dd2 100644 --- a/src/gui/gamedoc.h +++ b/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); } diff --git a/src/pygambit/gambit.pxd b/src/pygambit/gambit.pxd index 4d6781edb..a9e25bfb1 100644 --- a/src/pygambit/gambit.pxd +++ b/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 diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index 403d095d8..d166562d8 100644 --- a/src/pygambit/game.pxi +++ b/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: diff --git a/src/pygambit/player.pxi b/src/pygambit/player.pxi index dae64cee8..7cfc4f7de 100644 --- a/src/pygambit/player.pxi +++ b/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 diff --git a/tests/test_extensive.py b/tests/test_extensive.py index a18c24a29..5f5e35e1f 100644 --- a/tests/test_extensive.py +++ b/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), diff --git a/tests/test_players.py b/tests/test_players.py index bcc10b026..b5f13ed51 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -35,6 +35,59 @@ def test_player_label_non_ascii_rejected(label): player.label = label +def test_add_player_requires_label(): + """add_player now requires a label; omitting it is a TypeError.""" + game = gbt.Game.new_tree() + with pytest.raises(TypeError): + game.add_player() + + +def test_add_player_duplicate_label_raises_and_leaves_game_unchanged(): + game = gbt.Game.new_table([2, 2]) + existing = next(iter(game.players)).label + count_before = len(game.players) + with pytest.raises(ValueError): + game.add_player(existing) + assert len(game.players) == count_before + + +def test_add_player_empty_label_raises_and_leaves_game_unchanged(): + game = gbt.Game.new_table([2, 2]) + count_before = len(game.players) + with pytest.raises(ValueError): + game.add_player("") + assert len(game.players) == count_before + + +def test_add_player_reserved_chance_label_raises_and_leaves_game_unchanged(): + game = gbt.Game.new_tree() + count_before = len(game.players) + with pytest.raises(ValueError): + game.add_player("Chance") + assert len(game.players) == count_before + + +def test_chance_player_has_label(): + """The chance player is labeled "Chance" by default.""" + game = gbt.Game.new_tree() + assert game.players.chance.label == "Chance" + + +def test_chance_player_label_cannot_be_changed(): + """The chance player's label is reserved ("Chance") and cannot be changed.""" + game = gbt.Game.new_tree() + with pytest.raises(ValueError): + game.players.chance.label = "Nature" + + +def test_regular_player_cannot_be_relabeled_to_chance(): + game = gbt.Game.new_tree() + game.add_player("Alice") + player = next(iter(game.players)) + with pytest.raises(ValueError): + player.label = "Chance" + + def test_player_index_by_string(): game = gbt.Game.new_table([2, 2]) pl1, pl2 = game.players @@ -56,30 +109,30 @@ def test_player_label_invalid(): _ = game.players["Not a player"] -def test_set_empty_player_futurewarning(): +def test_set_empty_player_raises_valueerror(): game = games.create_stripped_down_poker_efg() player = next(iter(game.players)) - with pytest.warns(FutureWarning): + with pytest.raises(ValueError): player.label = "" -def test_set_duplicate_player_futurewarning(): +def test_set_duplicate_player_raises_valueerror(): game = games.create_stripped_down_poker_efg() pl1, pl2, *_ = game.players - with pytest.warns(FutureWarning): + with pytest.raises(ValueError): pl1.label = pl2.label def test_strategic_game_add_player(): game = gbt.Game.new_table([2, 2]) - new_player = game.add_player() + new_player = game.add_player("Player 3") assert len(game.players) == 3 assert len(new_player.strategies) == 1 def test_extensive_game_add_player(): game = gbt.Game.new_tree() - game.add_player() + game.add_player("Alice") pl1 = next(iter(game.players)) assert len(game.players) == 1 assert len(pl1.infosets) == 0