From cf4adb1ff41a8a45fe5bcebebf35e0aa78cbbe27 Mon Sep 17 00:00:00 2001 From: drdkad Date: Wed, 8 Jul 2026 13:46:29 +0100 Subject: [PATCH 01/10] Enforce unique, nonempty labels for players --- src/games/file.cc | 2 +- src/games/game.h | 32 +++++++++++++++++++++++++------- src/games/gameagg.h | 2 +- src/games/gamebagg.h | 2 +- src/games/gametable.cc | 3 ++- src/games/gametable.h | 2 +- src/games/gametree.cc | 6 +++--- src/games/gametree.h | 2 +- src/pygambit/gambit.pxd | 2 +- src/pygambit/game.pxi | 22 ++++++++++++++-------- src/pygambit/player.pxi | 5 ----- tests/test_extensive.py | 2 +- tests/test_players.py | 32 ++++++++++++++++++++++++++------ 13 files changed, 77 insertions(+), 37 deletions(-) diff --git a/src/games/file.cc b/src/games/file.cc index 08961017f9..ed36d6a270 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.h b/src/games/game.h index 286ddf12bb..8cf051b5cb 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -460,7 +460,10 @@ 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, const std::string &p_label) + : m_game(p_game), m_number(p_id), m_label(p_label) + { + } GamePlayerRep(GameRep *p_game, int p_id, int m_strats); ~GamePlayerRep(); @@ -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); } @@ -1162,7 +1161,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 @@ -1339,6 +1338,25 @@ inline void GameInfosetRep::SetLabel(const std::string &p_label) 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.empty()) { + throw ValueException("Player label must not be empty"); + } + if (p_label == m_label) { + return; + } + CheckLabel(p_label); + for (const auto &player : GetGame()->GetPlayers()) { + if (player.get() != this && player->GetLabel() == p_label) { + throw ValueException("Player label must be unique within the game"); + } + } + m_label = p_label; +} inline GameStrategy GamePlayerRep::GetStrategy(int st) const { m_game->BuildComputedValues(); diff --git a/src/games/gameagg.h b/src/games/gameagg.h index 3b0bb979e5..609924e6ad 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.h b/src/games/gamebagg.h index 641a13735e..c51a8a661f 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 078969d1a9..64bdf01902 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -494,10 +494,11 @@ void GameTableRep::WriteNfgFile(std::ostream &p_file) const // GameTableRep: Players //------------------------------------------------------------------------ -GamePlayer GameTableRep::NewPlayer() +GamePlayer GameTableRep::NewPlayer(const std::string &p_label) { IncrementVersion(); auto player = std::make_shared(this, m_players.size() + 1, 1); + player->SetLabel(p_label); 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 04facee7d9..df331c363a 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 e89c4f0cf9..6103ea4d25 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,10 @@ int GameTreeRep::BehavProfileLength() const // GameTreeRep: Players //------------------------------------------------------------------------ -GamePlayer GameTreeRep::NewPlayer() +GamePlayer GameTreeRep::NewPlayer(const std::string &p_label) { IncrementVersion(); - auto player = std::make_shared(this, m_players.size() + 1); + auto player = std::make_shared(this, m_players.size() + 1, p_label); 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 4cb6e09dd2..1b2e15e9e5 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/pygambit/gambit.pxd b/src/pygambit/gambit.pxd index 4d6781edba..a9e25bfb1f 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 403d095d89..68ed0570c3 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,29 @@ 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. + Parameters ---------- - label : str, default "" - The label for the player. + label : str + The label for the new player. Must be nonempty and not already in use + by another player in the game. Returns ------- Player A reference to the newly-created player. + + Raises + ------ + ValueError + If `label` is empty or is already the label of another player. """ - p = Player.wrap(self.game.deref().NewPlayer()) - if str(label) != "": - p.label = str(label) - return p + return Player.wrap(self.game.deref().NewPlayer(str(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 dae64cee87..7cfc4f7de8 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 a18c24a290..a3dddca605 100644 --- a/tests/test_extensive.py +++ b/tests/test_extensive.py @@ -44,7 +44,7 @@ def test_game_add_players_label(players: list): def test_game_add_players_nolabel(): game = gbt.Game.new_tree() - game.add_player() + game.add_player("Player") @pytest.mark.parametrize("game_input,expected_result", [ diff --git a/tests/test_players.py b/tests/test_players.py index bcc10b0267..63f3096939 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -35,6 +35,26 @@ 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_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_player_index_by_string(): game = gbt.Game.new_table([2, 2]) pl1, pl2 = game.players @@ -56,30 +76,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 From 9615b57148b4db06dded7757c1fbb90cdc813551 Mon Sep 17 00:00:00 2001 From: drdkad Date: Wed, 8 Jul 2026 14:10:05 +0100 Subject: [PATCH 02/10] Fix GUI NewPlayer call sites for required label --- src/gui/dlinsertmove.cc | 4 ++-- src/gui/gamedoc.cc | 4 ++-- src/gui/gamedoc.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/gui/dlinsertmove.cc b/src/gui/dlinsertmove.cc index 71c16a9e76..96ee42a556 100644 --- a/src/gui/dlinsertmove.cc +++ b/src/gui/dlinsertmove.cc @@ -190,8 +190,8 @@ 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())); + const GamePlayer player = m_doc->GetGame()->NewPlayer( + "Player " + lexical_cast(m_doc->GetGame()->NumPlayers() + 1)); return player; } diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index f971996e26..ba2caaac1e 100644 --- a/src/gui/gamedoc.cc +++ b/src/gui/gamedoc.cc @@ -481,8 +481,8 @@ void GameDocument::DoSetTitle(const wxString &p_title, const wxString &p_comment void GameDocument::DoNewPlayer() { - const GamePlayer player = m_game->NewPlayer(); - player->SetLabel("Player " + lexical_cast(player->GetNumber())); + const GamePlayer player = + m_game->NewPlayer("Player " + lexical_cast(m_game->NumPlayers() + 1)); if (!m_game->IsTree()) { player->GetStrategy(1)->SetLabel("1"); } diff --git a/src/gui/gamedoc.h b/src/gui/gamedoc.h index ac384fe22c..7c9f2d25f7 100644 --- a/src/gui/gamedoc.h +++ b/src/gui/gamedoc.h @@ -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); } From 070c3d1b731320ee2e3af02b4b0609c47543dba6 Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Wed, 8 Jul 2026 15:22:28 +0100 Subject: [PATCH 03/10] In GUI ensure generation of unique player label, in a single site. --- src/gui/dlinsertmove.cc | 4 +--- src/gui/gamedoc.cc | 17 ++++++++++++++--- src/gui/gamedoc.h | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/gui/dlinsertmove.cc b/src/gui/dlinsertmove.cc index 96ee42a556..dbfc58ebae 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 " + lexical_cast(m_doc->GetGame()->NumPlayers() + 1)); - return player; + return m_doc->DoNewPlayer(); } GameInfoset InsertMoveDialog::GetInfoset() const diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index ba2caaac1e..7804e12b7e 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 " + lexical_cast(m_game->NumPlayers() + 1)); + 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 7c9f2d25f7..b9dbb1dd2f 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); From d35f04525efb6be6f0ae4cf07ebb6c7934a13e09 Mon Sep 17 00:00:00 2001 From: drdkad Date: Wed, 8 Jul 2026 17:17:30 +0100 Subject: [PATCH 04/10] Reject duplicate and empty labels when creating players, leaving game unchanged on failure --- src/pygambit/game.pxi | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index 68ed0570c3..dad90ebfaa 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -2075,7 +2075,12 @@ class Game: ValueError If `label` is empty or is already the label of another player. """ - return Player.wrap(self.game.deref().NewPlayer(str(label).encode("ascii"))) + label_str = str(label) + if not label_str: + raise ValueError("add_player(): label must not be empty") + if label_str in (player.label for player in self.players): + raise ValueError(f"add_player(): label '{label_str}' is already in use") + return Player.wrap(self.game.deref().NewPlayer(label_str.encode("ascii"))) def set_player(self, infoset: Infoset | str, player: Player | str) -> None: From c22f092d4500b6dba5b6831048bffbe8a7d37244 Mon Sep 17 00:00:00 2001 From: drdkad Date: Wed, 8 Jul 2026 17:39:45 +0100 Subject: [PATCH 05/10] Reject the reserved Chance label on player creation; add tests --- src/pygambit/game.pxi | 10 ++++++++-- tests/test_players.py | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index dad90ebfaa..da96166511 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -2058,6 +2058,8 @@ class 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 ---------- @@ -2073,12 +2075,16 @@ class Game: Raises ------ ValueError - If `label` is empty or is already the label of another player. + 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. """ label_str = str(label) if not label_str: raise ValueError("add_player(): label must not be empty") - if label_str in (player.label for player in self.players): + existing = [player.label for player in self.players] + if self.is_tree: + existing.append(self.players.chance.label) + if label_str in existing: raise ValueError(f"add_player(): label '{label_str}' is already in use") return Player.wrap(self.game.deref().NewPlayer(label_str.encode("ascii"))) diff --git a/tests/test_players.py b/tests/test_players.py index 63f3096939..8ffdad6eca 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -42,6 +42,31 @@ def test_add_player_requires_label(): 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() From c545412e68b40247b03851a8678e100c5ce4845d Mon Sep 17 00:00:00 2001 From: drdkad Date: Wed, 8 Jul 2026 18:19:39 +0100 Subject: [PATCH 06/10] Reject relabeling a player to the reserved chance label in extensive games --- src/games/game.h | 3 +++ tests/test_players.py | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/src/games/game.h b/src/games/game.h index 8cf051b5cb..493a267edf 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -1350,6 +1350,9 @@ inline void GamePlayerRep::SetLabel(const std::string &p_label) return; } CheckLabel(p_label); + if (GetGame()->IsTree() && p_label == GetGame()->GetChance()->GetLabel()) { + throw ValueException("Player label must not be the reserved chance player label"); + } for (const auto &player : GetGame()->GetPlayers()) { if (player.get() != this && player->GetLabel() == p_label) { throw ValueException("Player label must be unique within the game"); diff --git a/tests/test_players.py b/tests/test_players.py index 8ffdad6eca..b5f13ed511 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -80,6 +80,14 @@ def test_chance_player_label_cannot_be_changed(): 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 From ad2e7893fd56d04ac6129261b5916f710b557b64 Mon Sep 17 00:00:00 2001 From: drdkad Date: Thu, 9 Jul 2026 12:42:20 +0100 Subject: [PATCH 07/10] Move player label enforcement into NewPlayer; require label in both constructors --- src/games/game.cc | 4 ++-- src/games/game.h | 2 +- src/games/gameagg.cc | 4 ++-- src/games/gamebagg.cc | 5 ++--- src/games/gametable.cc | 9 +++++---- src/games/gametree.cc | 3 ++- src/pygambit/game.pxi | 10 +--------- 7 files changed, 15 insertions(+), 22 deletions(-) diff --git a/src/games/game.cc b/src/games/game.cc index a441bb0fb5..9a467565b4 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 493a267edf..37867aa063 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -464,7 +464,7 @@ class GamePlayerRep : public std::enable_shared_from_this { : m_game(p_game), m_number(p_id), m_label(p_label) { } - GamePlayerRep(GameRep *p_game, int p_id, int m_strats); + GamePlayerRep(GameRep *p_game, int p_id, const std::string &p_label, int p_strats); ~GamePlayerRep(); bool IsValid() const { return m_valid; } diff --git a/src/games/gameagg.cc b/src/games/gameagg.cc index a702e8b011..af223c993b 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/gamebagg.cc b/src/games/gamebagg.cc index fea3bb695d..cfca1517fd 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/gametable.cc b/src/games/gametable.cc index 64bdf01902..5bc9179f1c 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++); @@ -496,9 +497,9 @@ void GameTableRep::WriteNfgFile(std::ostream &p_file) const GamePlayer GameTableRep::NewPlayer(const std::string &p_label) { - IncrementVersion(); - auto player = std::make_shared(this, m_players.size() + 1, 1); + auto player = std::make_shared(this, m_players.size() + 1, "", 1); player->SetLabel(p_label); + IncrementVersion(); m_players.push_back(player); for (const auto &outcome : m_outcomes) { outcome->m_payoffs[player.get()] = Number(); diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 6103ea4d25..021ab73d40 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -1526,8 +1526,9 @@ int GameTreeRep::BehavProfileLength() const GamePlayer GameTreeRep::NewPlayer(const std::string &p_label) { + auto player = std::make_shared(this, m_players.size() + 1, ""); + player->SetLabel(p_label); IncrementVersion(); - auto player = std::make_shared(this, m_players.size() + 1, p_label); m_players.push_back(player); for (const auto &outcome : m_outcomes) { outcome->m_payoffs[player.get()] = Number(); diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index da96166511..b396c37042 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -2078,15 +2078,7 @@ class Game: 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. """ - label_str = str(label) - if not label_str: - raise ValueError("add_player(): label must not be empty") - existing = [player.label for player in self.players] - if self.is_tree: - existing.append(self.players.chance.label) - if label_str in existing: - raise ValueError(f"add_player(): label '{label_str}' is already in use") - return Player.wrap(self.game.deref().NewPlayer(label_str.encode("ascii"))) + return Player.wrap(self.game.deref().NewPlayer(label.encode("ascii"))) def set_player(self, infoset: Infoset | str, player: Player | str) -> None: From a502e9bf88b6707d96db9b01b006d0a49e98c4ea Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Thu, 9 Jul 2026 14:31:30 +0100 Subject: [PATCH 08/10] Assign normal form player label on object creation --- src/games/gametable.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 5bc9179f1c..54867d8e7e 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -497,8 +497,7 @@ void GameTableRep::WriteNfgFile(std::ostream &p_file) const GamePlayer GameTableRep::NewPlayer(const std::string &p_label) { - auto player = std::make_shared(this, m_players.size() + 1, "", 1); - player->SetLabel(p_label); + auto player = std::make_shared(this, m_players.size() + 1, p_label, 1); IncrementVersion(); m_players.push_back(player); for (const auto &outcome : m_outcomes) { From 59d3b7197e152331e4601731913c27f5631798de Mon Sep 17 00:00:00 2001 From: Theodore Turocy Date: Thu, 9 Jul 2026 14:35:24 +0100 Subject: [PATCH 09/10] Remove redundant test --- src/pygambit/game.pxi | 4 ++-- tests/test_extensive.py | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index b396c37042..d166562d85 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -2064,8 +2064,8 @@ class Game: Parameters ---------- label : str - The label for the new player. Must be nonempty and not already in use - by another player in the game. + The label for the new player. Must be nonempty and not the same as the label + of an existing player in the game. Returns ------- diff --git a/tests/test_extensive.py b/tests/test_extensive.py index a3dddca605..5f5e35e1f5 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("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), From 660ac49dd03dfa5ddae73a872c15ffd3b7a1de1a Mon Sep 17 00:00:00 2001 From: drdkad Date: Thu, 9 Jul 2026 15:41:41 +0100 Subject: [PATCH 10/10] Extract player label validation into GameRep::CheckPlayerLabel --- src/games/game.h | 30 ++++++++++++++++++------------ src/games/gametable.cc | 1 + src/games/gametree.cc | 4 ++-- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/games/game.h b/src/games/game.h index 37867aa063..e7fff307fd 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -763,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 @@ -1335,6 +1337,21 @@ 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(); } @@ -1343,21 +1360,10 @@ inline void GamePlayerRep::SetLabel(const std::string &p_label) if (IsChance()) { throw ValueException("The chance player's label cannot be changed"); } - if (p_label.empty()) { - throw ValueException("Player label must not be empty"); - } if (p_label == m_label) { return; } - CheckLabel(p_label); - if (GetGame()->IsTree() && p_label == GetGame()->GetChance()->GetLabel()) { - throw ValueException("Player label must not be the reserved chance player label"); - } - for (const auto &player : GetGame()->GetPlayers()) { - if (player.get() != this && player->GetLabel() == p_label) { - throw ValueException("Player label must be unique within the game"); - } - } + GetGame()->CheckPlayerLabel(p_label); m_label = p_label; } inline GameStrategy GamePlayerRep::GetStrategy(int st) const diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 54867d8e7e..8fd04871ba 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -497,6 +497,7 @@ void GameTableRep::WriteNfgFile(std::ostream &p_file) const 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(); m_players.push_back(player); diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 021ab73d40..535d1db19c 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -1526,8 +1526,8 @@ int GameTreeRep::BehavProfileLength() const GamePlayer GameTreeRep::NewPlayer(const std::string &p_label) { - auto player = std::make_shared(this, m_players.size() + 1, ""); - player->SetLabel(p_label); + CheckPlayerLabel(p_label); + auto player = std::make_shared(this, m_players.size() + 1, p_label); IncrementVersion(); m_players.push_back(player); for (const auto &outcome : m_outcomes) {