diff --git a/ChangeLog b/ChangeLog index c269adff0..166279723 100644 --- a/ChangeLog +++ b/ChangeLog @@ -40,6 +40,7 @@ - Added `Game.relabel_players`, which simultaneously reassigns the labels of the game's players. (#1058) - Added `max_rectangles` to `enumpoly_solve` (and `-r` to `gambit-enumpoly`), bounding the number of cells examined when searching for equilibria on a single support. (#1055) +- Added `Game.set_players`, which declares the ordered list of the game's players, matching by label. (#1059) ### Fixed - `MixedStrategy.__eq__` raised `AttributeError` when comparing two `MixedStrategy` instances @@ -185,6 +186,8 @@ equivalent `SetStrategies` call directly. (#1056) - Assigning to `Player.label` has been removed; use `Game.relabel_players`, which enforces nonempty, unique labels. (#1058) + - `Game.add_player` has been removed; use `Game.set_players`, specifying the labels + of the players. (#1059) ## [17.0.0-alpha.1] - 2026-08-13 diff --git a/Makefile.am b/Makefile.am index c6ea8b5ca..31e1e787b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -47,7 +47,6 @@ EXTRA_DIST = \ src/gui/bitmaps/layout.xpm \ src/gui/bitmaps/move.xpm \ src/gui/bitmaps/newcol.xpm \ - src/gui/bitmaps/newplayer.xpm \ src/gui/bitmaps/newrow.xpm \ src/gui/bitmaps/newtable.xpm \ src/gui/bitmaps/newtree.xpm \ diff --git a/doc/gui.general.rst b/doc/gui.general.rst index 1a516cc9e..f34b50b6f 100644 --- a/doc/gui.general.rst +++ b/doc/gui.general.rst @@ -14,13 +14,17 @@ The frame presenting a game consists of a single main panel, which displays the game graphically; in this case, showing the game tree of a simple one-card poker game. Note that where applicable, information is color-coded to match the colors assigned to the players: Fred's moves and payoffs are -presented in red, and Alice's in blue. Player names and colors are set on -the :guilabel:`Players` page of the :guilabel:`Game properties` dialog -(:menuselection:`Edit --> Game`): each player is listed with a text field -for its name and a color swatch beside it, which opens a color picker when -clicked. A new player is added to the game using -:menuselection:`Edit --> Add player`; the Players page itself does not yet -support adding, removing, or reordering players. +presented in red, and Alice's in blue. Players are added, removed, reordered, +renamed, and recolored on the :guilabel:`Players` page of the +:guilabel:`Game properties` dialog (:menuselection:`Edit --> Game`), in the +same way as a player's strategies are edited on the :guilabel:`Edit +strategies` dialog (see :ref:`Adding, removing, and reordering strategies +`): each player has a text field for its name, a color +swatch beside it that opens a color picker when clicked, and +:guilabel:`↑`/:guilabel:`↓`/:guilabel:`✕` buttons to reorder or remove it. +A player can't be removed if it has decisions in the game, or more than one +strategy in the game's strategic representation; its :guilabel:`✕` button is +disabled, with a tooltip explaining why. Hovering the mouse pointer over a node in the tree briefly displays a small window showing, for each player, that player's expected payoff from that diff --git a/doc/gui.nfg.rst b/doc/gui.nfg.rst index b9f8880e8..ac2c2a0bc 100644 --- a/doc/gui.nfg.rst +++ b/doc/gui.nfg.rst @@ -141,10 +141,10 @@ continues to identify the player to whom it belongs. Adding players -------------- -To add an additional player to the game, use the menu item -:menuselection:`Edit --> Add player`, -or the corresponding toolbar icon . The newly created player -has one strategy, by default labeled with the number :guilabel:`1`. +Players are added, removed, and reordered on the :guilabel:`Players` page of +the :guilabel:`Game properties` dialog (:menuselection:`Edit --> Game`); see +:doc:`gui.general`. A newly added player has one strategy, by default +labeled with the number :guilabel:`1`. Editing strategies @@ -157,6 +157,8 @@ strategies` dialog for that player, titled with the player's own label, listing a row for each of the player's strategies and showing its label. +.. _editing-strategies: + Adding, removing, and reordering strategies ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/doc/pygambit.api.rst b/doc/pygambit.api.rst index 72724bb2a..fce915191 100644 --- a/doc/pygambit.api.rst +++ b/doc/pygambit.api.rst @@ -84,8 +84,8 @@ Transforming game components .. autosummary:: :toctree: api/ - Game.add_player Game.relabel_players + Game.set_players Game.add_outcome Game.delete_outcome Game.set_outcome diff --git a/src/games/file.cc b/src/games/file.cc index b0d48c61c..4f5a53292 100644 --- a/src/games/file.cc +++ b/src/games/file.cc @@ -589,8 +589,8 @@ 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(label); + if (!player_labels.empty()) { + p_game->SetPlayers(player_labels); } } diff --git a/src/games/game.h b/src/games/game.h index a0931bac3..fe5efa78b 100644 --- a/src/games/game.h +++ b/src/games/game.h @@ -1356,10 +1356,10 @@ class GameRep : public std::enable_shared_from_this { /// Returns the chance (nature) player 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(const std::string &p_label) = 0; /// Reassign player labels. Keys of p_labels are current labels; values are their replacements. void RelabelPlayers(const std::map &p_labels); + /// Declare the ordered list of players of the game. + virtual void SetPlayers(const std::vector &) { throw UndefinedException(); } //@} /// @name Dimensions of the game @@ -1707,8 +1707,8 @@ inline Game GameSubgameRep::GetGame() const { return m_game->shared_from_this(); //======================================================================= -/// Factory function to create new game tree -[[nodiscard]] Game NewTree(); +/// Factory function to create new game tree, +[[nodiscard]] Game NewTree(const std::vector &p_players = {}); /// Factory function to create new game table [[nodiscard]] Game NewTable(const std::vector &p_dim, bool p_sparseOutcomes = false); diff --git a/src/games/gameagg.h b/src/games/gameagg.h index 4e4340695..f3a3eb314 100644 --- a/src/games/gameagg.h +++ b/src/games/gameagg.h @@ -63,8 +63,6 @@ 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(const std::string &) override { throw UndefinedException(); } //@} /// @name Nodes diff --git a/src/games/gamebagg.h b/src/games/gamebagg.h index d862374d7..67611c98a 100644 --- a/src/games/gamebagg.h +++ b/src/games/gamebagg.h @@ -70,8 +70,6 @@ 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(const std::string &) override { throw UndefinedException(); } //@} /// @name Nodes diff --git a/src/games/gametable.cc b/src/games/gametable.cc index 805fd1469..c749b01f7 100644 --- a/src/games/gametable.cc +++ b/src/games/gametable.cc @@ -493,20 +493,6 @@ void GameTableRep::WriteNfgFile(std::ostream &p_file) const // GameTableRep: Players //------------------------------------------------------------------------ -GamePlayer GameTableRep::NewPlayer(const std::string &p_label) -{ - CheckPlayerLabel(p_label); - auto player = std::make_shared(this, m_players.size() + 1, p_label, 1); - player->m_strategies.front()->m_label = "1"; - IncrementVersion(); - m_players.push_back(player); - for (const auto &outcome : m_outcomes) { - outcome->m_payoffs[player.get()] = Number(); - } - IndexStrategies(); - return player; -} - //------------------------------------------------------------------------ // GameTableRep: Outcomes //------------------------------------------------------------------------ @@ -630,6 +616,100 @@ void GameTableRep::SetStrategies(const GamePlayer &p_player, RebuildTable(old_radices, p_player->GetNumber() - 1, old_to_new); } +void GameTableRep::SetPlayers(const std::vector &p_labels) +{ + if (p_labels.empty()) { + throw ValueException("At least one player must be specified"); + } + std::map current; + for (const auto &player : m_players) { + if (!current.emplace(player->GetLabel(), player->GetNumber() - 1).second) { + throw ValueException("Player label '" + player->GetLabel() + "' is ambiguous in this game"); + } + } + std::set declared; + for (const auto &label : p_labels) { + if (!declared.insert(label).second) { + throw ValueException("Player label '" + label + "' appears more than once"); + } + if (current.count(label) == 0) { + CheckPlayerLabel(label); + } + } + std::vector old_radices; + old_radices.reserve(m_players.size()); + for (const auto &player : m_players) { + if (declared.count(player->GetLabel()) == 0 && player->m_strategies.size() != 1) { + throw UndefinedException("A player with more than one strategy cannot be deleted"); + } + old_radices.push_back(player->m_strategies.size()); + } + std::vector source; + source.reserve(p_labels.size()); + for (const auto &label : p_labels) { + const auto it = current.find(label); + source.push_back((it != current.end()) ? it->second : -1); + } + + IncrementVersion(); + std::vector> newPlayers; + newPlayers.reserve(p_labels.size()); + for (size_t j = 0; j < p_labels.size(); ++j) { + if (source[j] >= 0) { + newPlayers.push_back(m_players[source[j]]); + continue; + } + auto player = std::make_shared(this, static_cast(j) + 1, p_labels[j], 1); + player->m_strategies.front()->m_label = "1"; + for (const auto &outcome : m_outcomes) { + outcome->m_payoffs[player.get()] = Number(); + } + newPlayers.push_back(player); + } + for (const auto &player : m_players) { + if (declared.count(player->GetLabel()) == 0) { + for (const auto &outcome : m_outcomes) { + outcome->m_payoffs.erase(player.get()); + } + player->Invalidate(); + } + } + m_players = std::move(newPlayers); + for (size_t j = 0; j < m_players.size(); ++j) { + m_players[j]->m_number = static_cast(j) + 1; + } + // Permute the outcome table into the new player order. + std::vector old_strides(old_radices.size()); + long stride = 1; + for (size_t i = 0; i < old_radices.size(); ++i) { + old_strides[i] = stride; + stride *= old_radices[i]; + } + const long old_size = stride; + std::vector new_strides(m_players.size()); + long new_size = 1; + for (size_t j = 0; j < m_players.size(); ++j) { + new_strides[j] = new_size; + new_size *= m_players[j]->m_strategies.size(); + } + std::vector newResults(new_size, nullptr); + for (long old_index = 0; old_index < old_size; ++old_index) { + if (m_results[old_index] == nullptr) { + continue; + } + long new_index = 0; + for (size_t j = 0; j < m_players.size(); ++j) { + if (source[j] >= 0) { + new_index += + ((old_index / old_strides[source[j]]) % old_radices[source[j]]) * new_strides[j]; + } + } + newResults[new_index] = m_results[old_index]; + } + m_results.swap(newResults); + IndexStrategies(); +} + //------------------------------------------------------------------------ // GameTableRep: Factory functions //------------------------------------------------------------------------ diff --git a/src/games/gametable.h b/src/games/gametable.h index 5a20d19d0..01e88f868 100644 --- a/src/games/gametable.h +++ b/src/games/gametable.h @@ -78,8 +78,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(const std::string &p_label) override; + void SetPlayers(const std::vector &) override; //@} /// @name Nodes diff --git a/src/games/gametree.cc b/src/games/gametree.cc index 4ad5813de..4e01fc69a 100644 --- a/src/games/gametree.cc +++ b/src/games/gametree.cc @@ -927,7 +927,14 @@ Game GameTreeRep::Copy() const return ReadGame(is); } -Game NewTree() { return std::make_shared(); } +Game NewTree(const std::vector &p_players) +{ + auto game = std::make_shared(); + if (!p_players.empty()) { + game->SetPlayers(p_players); + } + return game; +} //------------------------------------------------------------------------ // GameTreeRep: General data access @@ -1706,17 +1713,65 @@ int GameTreeRep::BehavProfileLength() const // GameTreeRep: Players //------------------------------------------------------------------------ -GamePlayer GameTreeRep::NewPlayer(const std::string &p_label) +void GameTreeRep::SetPlayers(const std::vector &p_labels) { - CheckPlayerLabel(p_label); - auto player = std::make_shared(this, m_players.size() + 1, p_label); + if (p_labels.empty()) { + throw ValueException("At least one player must be specified"); + } + std::map current; + for (const auto &player : m_players) { + if (!current.emplace(player->GetLabel(), player->GetNumber() - 1).second) { + throw ValueException("Player label '" + player->GetLabel() + "' is ambiguous in this game"); + } + } + std::set declared; + for (const auto &label : p_labels) { + if (!declared.insert(label).second) { + throw ValueException("Player label '" + label + "' appears more than once"); + } + if (current.count(label) == 0) { + CheckPlayerLabel(label); + } + } + for (const auto &player : m_players) { + if (declared.count(player->GetLabel()) == 0 && !player->m_infosets.empty()) { + throw UndefinedException("A player who has decisions in the game cannot be deleted"); + } + } + std::vector source; + source.reserve(p_labels.size()); + for (const auto &label : p_labels) { + const auto it = current.find(label); + source.push_back((it != current.end()) ? it->second : -1); + } + IncrementVersion(); - m_players.push_back(player); - for (const auto &outcome : m_outcomes) { - outcome->m_payoffs[player.get()] = Number(); + std::vector> newPlayers; + newPlayers.reserve(p_labels.size()); + for (size_t j = 0; j < p_labels.size(); ++j) { + if (source[j] >= 0) { + newPlayers.push_back(m_players[source[j]]); + continue; + } + auto player = std::make_shared(this, static_cast(j) + 1, p_labels[j]); + for (const auto &outcome : m_outcomes) { + outcome->m_payoffs[player.get()] = Number(); + } + newPlayers.push_back(player); + } + for (const auto &player : m_players) { + if (declared.count(player->GetLabel()) == 0) { + for (const auto &outcome : m_outcomes) { + outcome->m_payoffs.erase(player.get()); + } + player->Invalidate(); + } + } + m_players = std::move(newPlayers); + for (size_t j = 0; j < m_players.size(); ++j) { + m_players[j]->m_number = static_cast(j) + 1; } ClearComputedValues(); - return player; } //------------------------------------------------------------------------ diff --git a/src/games/gametree.h b/src/games/gametree.h index ed92d4f8f..7e3906a77 100644 --- a/src/games/gametree.h +++ b/src/games/gametree.h @@ -123,8 +123,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(const std::string &p_label) override; + void SetPlayers(const std::vector &) override; //@} /// @name Nodes diff --git a/src/gui/bitmaps/newplayer.xpm b/src/gui/bitmaps/newplayer.xpm deleted file mode 100644 index f4b57e6f4..000000000 --- a/src/gui/bitmaps/newplayer.xpm +++ /dev/null @@ -1,231 +0,0 @@ -/* XPM */ -static const char * newplayer_xpm[] = { -"24 24 204 2", -" c None", -". c #E5D087", -"+ c #000000", -"@ c #8E7F4F", -"# c #DBCB9A", -"$ c #F2EACC", -"% c #DCCD9A", -"& c #111110", -"* c #4B4844", -"= c #0A0500", -"- c #CEC394", -"; c #F0E9CE", -"> c #FBFAF6", -", c #EFE7CA", -"' c #DCC986", -") c #1D1D1D", -"! c #060606", -"~ c #444444", -"{ c #252525", -"] c #AFA87E", -"^ c #F0E8CD", -"/ c #F0E8CB", -"( c #D7C482", -"_ c #181817", -": c #130A00", -"< c #4C3A29", -"[ c #150B01", -"} c #0E0700", -"| c #1D1003", -"1 c #261504", -"2 c #361F05", -"3 c #8C8468", -"4 c #F1E4B1", -"5 c #F7EFD1", -"6 c #DCCE9A", -"7 c #1B0F01", -"8 c #412405", -"9 c #C5751C", -"0 c #C5741D", -"a c #C1731D", -"b c #C2731C", -"c c #C8741A", -"d c #7C450C", -"e c #080400", -"f c #EFE1B0", -"g c #EADCA4", -"h c #E4D69E", -"i c #8E804F", -"j c #2C1804", -"k c #5B3107", -"l c #D17A1D", -"m c #D17B1E", -"n c #D37D1F", -"o c #D07A1D", -"p c #CD761A", -"q c #79430C", -"r c #2D1905", -"s c #311A04", -"t c #A95F13", -"u c #CC7519", -"v c #CC761A", -"w c #CC751A", -"x c #CB7519", -"y c #CA7218", -"z c #B66614", -"A c #2F1904", -"B c #3A1F05", -"C c #9B580F", -"D c #BE6A15", -"E c #83490F", -"F c #261402", -"G c #2E1803", -"H c #8A4C0E", -"I c #B56413", -"J c #422305", -"K c #472605", -"L c #934F0D", -"M c #321B03", -"N c #AC6011", -"O c #A55C10", -"P c #653607", -"Q c #351C02", -"R c #0D1112", -"S c #292626", -"T c #593615", -"U c #341B04", -"V c #351D04", -"W c #341C04", -"X c #291704", -"Y c #452F1A", -"Z c #181E25", -"` c #090B0D", -" . c #25282C", -".. c #91999F", -"+. c #899DB0", -"@. c #6F7A84", -"#. c #241E1A", -"$. c #180E04", -"%. c #1A120C", -"&. c #222223", -"*. c #646F7A", -"=. c #455666", -"-. c #3D4D5E", -";. c #090D0F", -">. c #282C31", -",. c #A0ABB7", -"'. c #738EAA", -"). c #7591AC", -"!. c #506375", -"~. c #899097", -"{. c #CCCCCC", -"]. c #C5C6C8", -"^. c #8D99A5", -"/. c #6F7F91", -"(. c #556C83", -"_. c #586E84", -":. c #45586B", -"<. c #0A0D10", -"[. c #121415", -"}. c #90989E", -"|. c #7C95AD", -"1. c #6D89A5", -"2. c #6F8BA6", -"3. c #5C748B", -"4. c #7C8A99", -"5. c #F2F3F4", -"6. c #E7EAED", -"7. c #A7B8C7", -"8. c #7E95AC", -"9. c #66809B", -"0. c #5D7995", -"a. c #4E667D", -"b. c #374656", -"c. c #0A0C10", -"d. c #131516", -"e. c #96A1AD", -"f. c #66829E", -"g. c #67839E", -"h. c #6884A0", -"i. c #597088", -"j. c #607285", -"k. c #E6E8EB", -"l. c #C9D3DB", -"m. c #9AACBD", -"n. c #7990A8", -"o. c #BBC6D2", -"p. c #6C859E", -"q. c #526E8B", -"r. c #4E6883", -"s. c #0C1015", -"t. c #191D20", -"u. c #8A9EB0", -"v. c #5C7895", -"w. c #5E7A97", -"x. c #607C98", -"y. c #617D99", -"z. c #52677C", -"A. c #CFD5DB", -"B. c #B8C3D0", -"C. c #68829C", -"D. c #BBC7D1", -"E. c #D5DBE1", -"F. c #CAD3DB", -"G. c #4D6A87", -"H. c #45617E", -"I. c #0C1016", -"J. c #1D2227", -"K. c #8296AA", -"L. c #56728F", -"M. c #587491", -"N. c #597692", -"O. c #5A7692", -"P. c #405267", -"Q. c #B8C3CC", -"R. c #A9B8C6", -"S. c #587490", -"T. c #4E6A87", -"U. c #4A6684", -"V. c #44617E", -"W. c #3F5A78", -"X. c #0D131A", -"Y. c #171E27", -"Z. c #748A9E", -"`. c #4F6B88", -" + c #536F8C", -".+ c #53708C", -"++ c #3A4E63", -"@+ c #6F8498", -"#+ c #8FA0B2", -"$+ c #4E6B88", -"%+ c #4C6886", -"&+ c #486482", -"*+ c #3C5774", -"=+ c #354F69", -"-+ c #040608", -";+ c #182330", -">+ c #3E5773", -",+ c #3D5671", -"'+ c #3E5672", -")+ c #3F5874", -"!+ c #405A74", -"~+ c #3C5670", -"{+ c #3B526C", -"]+ c #293B50", -" ", -" . . ", -" + + + + + @ # $ $ % ", -" & + * * = + + - ; > > , ' ", -" ) ! * ~ { + + + ] ^ > > / ( ", -" _ : < [ } | 1 2 3 4 5 5 6 ", -" 7 8 9 0 a b c d e f g h i ", -" j k l m n o p q r ", -" s t u v w x y z A ", -" B C D E F G H I J ", -" K L M N O K P Q ", -" R S T U V W X Y Z ` ", -" ...+.@.#.$.%.&.*.=.-.;. ", -" >.,.'.).!.~.{.].^./.(._.:.<. ", -" [.}.|.1.2.3.4.5.6.7.8.9.0.a.b.c. ", -" d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s. ", -" t.u.v.w.x.y.z.A.B.C.D.E.F.G.H.I. ", -" J.K.L.M.N.O.P.Q.R.S.q.T.U.V.W.X. ", -" Y.Z.`.q. +.+++@+#+$+%+&+H.*+=+-+ ", -" + ;+>+,+'+)+)+!+>+>+>+~+{+{+]++ ", -" + + + + + + + + + + + + + + ", -" ", -" ", -" "}; diff --git a/src/gui/dlgameprop.cc b/src/gui/dlgameprop.cc index 40b168646..240ad3e88 100644 --- a/src/gui/dlgameprop.cc +++ b/src/gui/dlgameprop.cc @@ -20,7 +20,9 @@ // Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. // +#include #include +#include #include #include @@ -28,6 +30,7 @@ #include #endif // WX_PRECOMP #include +#include #include #ifdef __WXMAC__ #include @@ -35,6 +38,7 @@ #include #endif +#include "gambit.h" #include "gamedoc.h" #include "dlgameprop.h" #include "editlabel.h" @@ -43,6 +47,11 @@ namespace Gambit::GUI { namespace { const wxColour kInvalidLabelBg(255, 220, 220); +const wxColour kNewLabelColour(0, 102, 0); // new player: bold, dark green +const wxColour kRenamedLabelColour(0, 51, 204); // renamed player: italic, strong blue +// A neutral placeholder swatch for a player that doesn't exist yet; freely overridable via +// the row's own colour button before the dialog is committed. +const wxColour kNewPlayerColor(200, 200, 200); // wxGenericColourDialog, not the platform's native wxColourDialog, on macOS/Cocoa only: // its native colour panel has no real Cancel -- any interaction with it commits live, @@ -58,108 +67,432 @@ using PlatformColourDialog = wxColourDialog; } // namespace //======================================================================== -// class PlayerLabelPanel +// class PlayerPanel //======================================================================== -// A fixed list of the game's players (plus chance), each with a color swatch -// button that opens a color picker; every player but chance also gets an -// editable label (chance's label is reserved and immutable). Adding, deleting, -// and reordering players is not supported here yet. -class PlayerLabelPanel final : public wxPanel { +// An editable, variable-length list of the game's players (not including chance), colour-coded +// like a source-control diff (added/renamed/deleted), mirroring StrategyPanel +// (dleditstrategies.cc) with an added per-row colour swatch. +// +// Each row's *stable label* (its label before this dialog touched it, or a placeholder for a +// newly added row) never changes; it's what `Game::SetPlayers()` uses to match the row to its +// underlying player regardless of what the user has typed into its *current* label text. +// Deleting an existing row marks it deleted and disables it, with its delete button becoming +// a restore button, rather than removing it outright -- a newly added row has nothing to +// preserve, so deleting one of those does remove it. +class PlayerPanel final : public wxScrolledWindow { public: struct Row { - GamePlayer player; + GamePlayer player; // null for a row added in this dialog, with no prior player at all + std::string stableLabel; + bool isNew = false; // true for a row added in this dialog + bool isDeleted = false; // true for an existing row marked for deletion (kept, disabled) + wxString currentLabelText; wxColour color; // staged: not written to the document until the dialog's OK is committed - LabelTextCtrl *labelCtrl = nullptr; // null for the chance row -- its label isn't editable + LabelTextCtrl *labelCtrl = nullptr; wxBitmapButton *colorButton = nullptr; + wxButton *upButton = nullptr; + wxButton *downButton = nullptr; + wxButton *deleteButton = nullptr; // doubles as the restore button when isDeleted }; private: std::shared_ptr m_doc; std::vector m_rows; + wxBoxSizer *m_topSizer; wxColour m_defaultBg; std::function m_onChanged; + // False until the constructor completes, so Rebuild()'s initial call can't notify the + // owning dialog before it's finished being constructed. + bool m_ready = false; + // True from the moment a row mutation starts until its (deferred) Rebuild() finishes; see + // StrategyPanel (dleditstrategies.cc) for why this guard is needed. + bool m_rebuilding = false; + + std::string NextPlaceholderLabel() const; + int ActiveCount() const + { + return static_cast( + std::count_if(m_rows.begin(), m_rows.end(), [](const Row &r) { return !r.isDeleted; })); + } + // The reason `row` can't be deleted because of the game's own rules -- it has decisions (in + // an extensive game) or more than one strategy (in a strategic game) -- or an empty string + // if there's no such structural obstruction. Independent of the separate "at least one + // player must remain" rule enforced where this is used. + wxString BlockedDeleteReason(const Row &row) const; + void Rebuild(); + // Colours a (non-deleted) row's label to reflect whether it's new, renamed from its stable + // label, or unchanged, and sets/clears the "renamed from" tooltip to match. + static void UpdateRowStyle(Row &row); + void NotifyChanged() const + { + if (m_ready) { + m_onChanged(); + } + } void OnSetColor(int p_index); - // Builds a fresh color-swatch button for a row and swaps it in for the old one, rather - // than mutating the existing button's bitmap in place -- see OnSetColor() for why. + // Builds a fresh color-swatch button for a row and swaps it in for the old one, rather than + // mutating the existing button's bitmap in place -- see PlayerPanel::OnSetColor() for why. void RecreateColorButton(int p_index); wxBitmapButton *MakeColorButton(int p_index); public: - PlayerLabelPanel(wxWindow *p_parent, const std::shared_ptr &p_doc, - const std::function &p_onChanged); + PlayerPanel(wxWindow *p_parent, const std::shared_ptr &p_doc, + const std::function &p_onChanged); - int NumRows() const { return static_cast(m_rows.size()); } - GamePlayer GetPlayer(int p_index) const { return m_rows.at(p_index).player; } + int NumPlayers() const { return static_cast(m_rows.size()); } + bool IsDeleted(int p_index) const { return m_rows.at(p_index).isDeleted; } + std::string GetStableLabel(int p_index) const { return m_rows.at(p_index).stableLabel; } wxString GetPlayerLabel(int p_index) const { - const Row &row = m_rows.at(p_index); - return row.labelCtrl ? row.labelCtrl->GetNormalizedValue() - : wxString::FromUTF8(row.player->GetLabel()); + return m_rows.at(p_index).labelCtrl->GetNormalizedValue(); } wxColour GetPlayerColor(int p_index) const { return m_rows.at(p_index).color; } - // Highlights any empty or duplicate player labels, and returns a description - // of the first problem found, or an empty string if all are valid. Chance's - // (uneditable) row is never a source of invalidity. + void AddPlayer(); + // Marks an existing row deleted (or restores an already-deleted one); removes a row added + // in this dialog outright. A no-op if this would leave no active (non-deleted) rows, or if + // the row is structurally blocked from deletion (see BlockedDeleteReason()). + void ToggleDeleted(int p_index); + void MovePlayerUp(int p_index); + void MovePlayerDown(int p_index); + + // Highlights any empty or duplicate player labels among active (non-deleted) rows, and + // returns a description of the first problem found, or an empty string if all are valid. wxString ValidateLabels(); }; -PlayerLabelPanel::PlayerLabelPanel(wxWindow *p_parent, const std::shared_ptr &p_doc, - const std::function &p_onChanged) - : wxPanel(p_parent, wxID_ANY), m_doc(p_doc), m_onChanged(p_onChanged) +PlayerPanel::PlayerPanel(wxWindow *p_parent, const std::shared_ptr &p_doc, + const std::function &p_onChanged) + : wxScrolledWindow(p_parent, wxID_ANY, wxDefaultPosition, wxDefaultSize, + wxVSCROLL | wxTAB_TRAVERSAL), + m_doc(p_doc), m_defaultBg(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOW)), + m_onChanged(p_onChanged) { - auto *gridSizer = new wxFlexGridSizer(3, FromDIP(5), FromDIP(10)); - gridSizer->AddGrowableCol(2, 1); - - for (const auto &player : m_doc->GetGame()->GetPlayersWithChance()) { + for (const auto &player : m_doc->GetGame()->GetPlayers()) { Row row; row.player = player; + row.stableLabel = player->GetLabel(); + row.currentLabelText = wxString::FromUTF8(row.stableLabel); row.color = m_doc->GetStyle().GetPlayerColor(player); - m_rows.push_back(row); + m_rows.push_back(std::move(row)); + } - const int index = static_cast(m_rows.size()) - 1; - Row &stored = m_rows[index]; + m_topSizer = new wxBoxSizer(wxVERTICAL); + SetSizer(m_topSizer); + SetScrollRate(0, FromDIP(10)); - stored.colorButton = MakeColorButton(index); - // The border keeps the swatch's spacing from the panel edge sizer-owned rather than - // relying on the button's own native chrome, which can shift after the button is - // recreated (see RecreateColorButton()) if left to the platform to decide. - gridSizer->Add(stored.colorButton, 0, wxALIGN_CENTER_VERTICAL | wxLEFT, FromDIP(5)); + Rebuild(); - wxString number; - if (!player->IsChance()) { - number << player->GetNumber(); + const wxSize bestSize = m_topSizer->CalcMin(); + SetMinSize(wxSize(FromDIP(400), std::min(bestSize.GetHeight(), FromDIP(250)))); + + if (!m_rows.empty()) { + m_defaultBg = m_rows.front().labelCtrl->GetBackgroundColour(); + } + + m_ready = true; +} + +std::string PlayerPanel::NextPlaceholderLabel() const +{ + std::set used; + for (const auto &row : m_rows) { + used.insert(row.stableLabel); + used.insert(row.currentLabelText.ToStdString(wxConvUTF8)); + } + int number = static_cast(m_rows.size()) + 1; + while (contains(used, "Player " + std::to_string(number))) { + number++; + } + return "Player " + std::to_string(number); +} + +void PlayerPanel::UpdateRowStyle(Row &row) +{ + // Colour alone can be hard to tell apart (or perceive at all); weight/slant give the same + // information a second way, so the state still reads even if the colours don't. + wxFont font = row.labelCtrl->GetFont(); + font.SetWeight(wxFONTWEIGHT_NORMAL); + font.SetStyle(wxFONTSTYLE_NORMAL); + + if (row.isNew) { + font.SetWeight(wxFONTWEIGHT_BOLD); + row.labelCtrl->SetForegroundColour(kNewLabelColour); + row.labelCtrl->UnsetToolTip(); + } + else if (row.currentLabelText.ToStdString(wxConvUTF8) != row.stableLabel) { + font.SetStyle(wxFONTSTYLE_ITALIC); + row.labelCtrl->SetForegroundColour(kRenamedLabelColour); + row.labelCtrl->SetToolTip( + wxString::Format(_("Renamed from \"%s\""), wxString::FromUTF8(row.stableLabel))); + } + else { + row.labelCtrl->SetForegroundColour(wxSystemSettings::GetColour(wxSYS_COLOUR_WINDOWTEXT)); + row.labelCtrl->UnsetToolTip(); + } + row.labelCtrl->SetFont(font); + row.labelCtrl->Refresh(); +} + +wxString PlayerPanel::BlockedDeleteReason(const Row &row) const +{ + if (row.isNew) { + return {}; // nothing has been committed yet -- freely removable + } + if (m_doc->GetGame()->IsTree()) { + if (row.player->GetInfosets().size() > 0) { + return _("This player has decisions in the game and cannot be deleted."); } + } + else if (row.player->GetStrategies().size() != 1) { + return _("This player has more than one strategy and cannot be deleted."); + } + return {}; +} + +void PlayerPanel::Rebuild() +{ + m_rebuilding = true; + m_topSizer->Clear(true); // destroys the previous row controls; each Row's own state isn't + // owned by them + + auto *gridSizer = new wxFlexGridSizer(4, FromDIP(5), FromDIP(10)); + gridSizer->AddGrowableCol(2, 1); + + gridSizer->AddSpacer(1); + gridSizer->AddSpacer(1); + gridSizer->Add(new wxStaticText(this, wxID_STATIC, _("Label")), 0, wxALIGN_CENTER_VERTICAL); + gridSizer->AddSpacer(1); + + const int activeCount = ActiveCount(); + const wxSize buttonSize(FromDIP(28), -1); + + for (size_t i = 0; i < m_rows.size(); i++) { + Row &row = m_rows[i]; + + row.colorButton = MakeColorButton(static_cast(i)); + gridSizer->Add(row.colorButton, 0, wxALIGN_CENTER_VERTICAL | wxLEFT, FromDIP(5)); + + wxString number; + number << (i + 1); gridSizer->Add(new wxStaticText(this, wxID_STATIC, number), 0, wxALIGN_CENTER_VERTICAL | wxALIGN_RIGHT); - if (player->IsChance()) { - // Chance's label is reserved and can't be changed (GameRep::RelabelPlayers rejects it - // as a key) -- just display it. - gridSizer->Add(new wxStaticText(this, wxID_STATIC, wxString::FromUTF8(player->GetLabel())), - 1, wxALIGN_CENTER_VERTICAL); + row.labelCtrl = new LabelTextCtrl(this, wxID_ANY, row.currentLabelText); + if (row.isDeleted) { + wxFont strikeFont = row.labelCtrl->GetFont(); + strikeFont.SetStrikethrough(true); + row.labelCtrl->SetFont(strikeFont); + row.labelCtrl->Disable(); + } + else { + UpdateRowStyle(row); + } + row.labelCtrl->Bind(wxEVT_TEXT, [this, i](wxCommandEvent &p_event) { + if (!m_rebuilding && i < m_rows.size()) { + Row &changedRow = m_rows.at(i); + changedRow.currentLabelText = changedRow.labelCtrl->GetValue(); + UpdateRowStyle(changedRow); + NotifyChanged(); + } + p_event.Skip(); + }); + row.labelCtrl->Bind(wxEVT_KILL_FOCUS, [this, i](wxFocusEvent &p_event) { + if (!m_rebuilding && i < m_rows.size()) { + Row &changedRow = m_rows.at(i); + changedRow.currentLabelText = changedRow.labelCtrl->GetNormalizedValue(); + UpdateRowStyle(changedRow); + NotifyChanged(); + } + p_event.Skip(); + }); + gridSizer->Add(row.labelCtrl, 1, wxEXPAND); + + auto *buttonSizer = new wxBoxSizer(wxHORIZONTAL); + + if (!row.isDeleted) { + row.upButton = + new wxButton(this, wxID_ANY, wxUniChar(0x2191), wxDefaultPosition, buttonSize); + row.upButton->Enable(i > 0); + row.upButton->Bind(wxEVT_BUTTON, + [this, i](wxCommandEvent &) { MovePlayerUp(static_cast(i)); }); + buttonSizer->Add(row.upButton, 0, wxLEFT, 2); + + row.downButton = + new wxButton(this, wxID_ANY, wxUniChar(0x2193), wxDefaultPosition, buttonSize); + row.downButton->Enable(i + 1 < m_rows.size()); + row.downButton->Bind(wxEVT_BUTTON, + [this, i](wxCommandEvent &) { MovePlayerDown(static_cast(i)); }); + buttonSizer->Add(row.downButton, 0, wxLEFT, 2); + } + else { + // Keep the delete/restore button aligned under its counterpart in other rows, in place + // of the Up/Down buttons a deleted row no longer has. Clear() already destroyed the + // previous Up/Down controls these pointers referred to; null them out rather than leave + // them dangling. + row.upButton = nullptr; + row.downButton = nullptr; + buttonSizer->AddSpacer(buttonSize.GetWidth() + 2); + buttonSizer->AddSpacer(buttonSize.GetWidth() + 2); + } + + const wxString blockedReason = row.isDeleted ? wxString() : BlockedDeleteReason(row); + const bool canDelete = row.isDeleted || (blockedReason.empty() && activeCount > 1); + + row.deleteButton = + new wxButton(this, wxID_ANY, row.isDeleted ? wxUniChar(0x21BA) : wxUniChar(0x2715), + wxDefaultPosition, buttonSize); + if (row.isDeleted) { + row.deleteButton->SetToolTip(_("Restore player")); + } + else if (!blockedReason.empty()) { + row.deleteButton->SetToolTip(blockedReason); + } + else if (activeCount <= 1) { + row.deleteButton->SetToolTip(_("At least one player must remain.")); } else { - stored.labelCtrl = new LabelTextCtrl(this, wxID_ANY, wxString::FromUTF8(player->GetLabel())); - m_defaultBg = stored.labelCtrl->GetBackgroundColour(); - stored.labelCtrl->Bind(wxEVT_TEXT, [this](wxCommandEvent &p_event) { - m_onChanged(); - p_event.Skip(); - }); - stored.labelCtrl->Bind(wxEVT_KILL_FOCUS, [this](wxFocusEvent &p_event) { - m_onChanged(); - p_event.Skip(); - }); - gridSizer->Add(stored.labelCtrl, 1, wxEXPAND); + row.deleteButton->SetToolTip(_("Delete player")); } + row.deleteButton->Enable(canDelete); + row.deleteButton->Bind(wxEVT_BUTTON, + [this, i](wxCommandEvent &) { ToggleDeleted(static_cast(i)); }); + buttonSizer->Add(row.deleteButton, 0, wxLEFT, 2); + + gridSizer->Add(buttonSizer, 0, wxALIGN_CENTER_VERTICAL); } - SetSizer(gridSizer); + // A trailing row with only its button-column cell populated, so the "+" control lands + // directly below the last row's Up/Down/Delete buttons rather than as a separate control + // elsewhere in the dialog. + gridSizer->AddSpacer(1); + gridSizer->AddSpacer(1); + gridSizer->AddSpacer(1); + auto *addButton = new wxButton(this, wxID_ANY, "+", wxDefaultPosition, buttonSize); + addButton->SetToolTip(_("Add player")); + addButton->Bind(wxEVT_BUTTON, [this](wxCommandEvent &) { AddPlayer(); }); + auto *addSizer = new wxBoxSizer(wxHORIZONTAL); + addSizer->Add(addButton, 0, wxLEFT, 2); + gridSizer->Add(addSizer, 0, wxALIGN_CENTER_VERTICAL); + + m_topSizer->Add(gridSizer, 1, wxALL | wxEXPAND, 5); + FitInside(); + Layout(); + m_rebuilding = false; + // No NotifyChanged() call here -- see m_ready's comment. Callers that mutate m_rows are + // responsible for notifying once Rebuild() returns. } -wxBitmapButton *PlayerLabelPanel::MakeColorButton(int p_index) +void PlayerPanel::AddPlayer() +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + Row row; + row.stableLabel = NextPlaceholderLabel(); + row.isNew = true; + row.currentLabelText = wxString::FromUTF8(row.stableLabel); + row.color = kNewPlayerColor; + m_rows.push_back(std::move(row)); + // See StrategyPanel::AddStrategy() (dleditstrategies.cc) for why this defers Rebuild() and + // raises m_rebuilding immediately, rather than just once Rebuild() starts. + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +void PlayerPanel::ToggleDeleted(int p_index) +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + Row &row = m_rows.at(p_index); + if (!row.isDeleted) { + if (!BlockedDeleteReason(row).empty()) { + return; // structurally blocked; the disabled button shouldn't have fired regardless + } + if (ActiveCount() <= 1) { + return; // refuse to drop below one active player + } + } + if (row.isNew) { + m_rows.erase(m_rows.begin() + p_index); + } + else { + row.isDeleted = !row.isDeleted; + } + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +void PlayerPanel::MovePlayerUp(int p_index) +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + if (p_index <= 0) { + return; + } + std::swap(m_rows[p_index - 1], m_rows[p_index]); + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +void PlayerPanel::MovePlayerDown(int p_index) +{ + if (m_rebuilding) { + return; // a rebuild from an earlier click is still pending; ignore this stale one + } + if (p_index + 1 >= static_cast(m_rows.size())) { + return; + } + std::swap(m_rows[p_index], m_rows[p_index + 1]); + m_rebuilding = true; + CallAfter([this]() { + Rebuild(); + NotifyChanged(); + }); +} + +wxString PlayerPanel::ValidateLabels() +{ + const int numPlayers = NumPlayers(); + wxString message; + + for (int i = 0; i < numPlayers; i++) { + if (m_rows[i].isDeleted) { + continue; + } + const wxString value = m_rows[i].labelCtrl->GetValue(); + bool invalid = value.empty(); + for (int other = 0; !invalid && other < numPlayers; other++) { + if (other != i && !m_rows[other].isDeleted && m_rows[other].labelCtrl->GetValue() == value) { + invalid = true; + } + } + + m_rows[i].labelCtrl->SetBackgroundColour(invalid ? kInvalidLabelBg : m_defaultBg); + m_rows[i].labelCtrl->Refresh(); + + if (invalid && message.empty()) { + message = + value.empty() ? _("Player labels cannot be empty.") : _("Player labels must be unique."); + } + } + return message; +} + +wxBitmapButton *PlayerPanel::MakeColorButton(int p_index) { Row &row = m_rows.at(p_index); auto *button = new wxBitmapButton(this, wxID_ANY, MakeColorSwatch(row.color), wxDefaultPosition, @@ -169,80 +502,47 @@ wxBitmapButton *PlayerLabelPanel::MakeColorButton(int p_index) return button; } -void PlayerLabelPanel::OnSetColor(int p_index) +void PlayerPanel::OnSetColor(int p_index) { Row &row = m_rows.at(p_index); wxColourData data; data.SetColour(row.color); PlatformColourDialog dialog(this, &data); - wxString title; - if (row.player->IsChance()) { - title = _("Choose color for chance"); - } - else { - title << _("Choose color for player ") << row.player->GetNumber(); - } - dialog.SetTitle(title); + dialog.SetTitle(wxString::Format(_("Choose color for %s"), row.currentLabelText)); if (dialog.ShowModal() != wxID_OK) { return; } - // Staged only -- not written to the document until the dialog's OK is committed - // (by GameFrame::OnEditGame), so canceling the dialog leaves colors untouched. + // Staged only -- not written to the document until the dialog's OK is committed (by + // GameFrame::OnEditGame), so canceling the dialog leaves colors untouched. row.color = dialog.GetColourData().GetColour(); // Recreated (rather than mutated in place via SetBitmap()) because SetBitmap() on an - // existing wxBitmapButton visibly shifted its rendered content on this platform. - // Deferred via CallAfter(): this destroys the very button whose click invoked this - // handler, which is still on the call stack -- see ActionPanel::AddAction() in - // dleditmove.cc for the same hazard. + // existing wxBitmapButton visibly shifted its rendered content on this platform. Deferred + // via CallAfter(): this destroys the very button whose click invoked this handler, which is + // still on the call stack -- see ActionPanel::AddAction() in dleditmove.cc for the same + // hazard. CallAfter([this, p_index]() { RecreateColorButton(p_index); }); } -void PlayerLabelPanel::RecreateColorButton(int p_index) +void PlayerPanel::RecreateColorButton(int p_index) { Row &row = m_rows.at(p_index); wxBitmapButton *oldButton = row.colorButton; wxBitmapButton *newButton = MakeColorButton(p_index); - GetSizer()->Replace(oldButton, newButton); + // The button lives in a gridSizer nested inside m_topSizer, not directly in m_topSizer + // itself, so the search needs to recurse into child sizers to find it. + m_topSizer->Replace(oldButton, newButton, true); oldButton->Destroy(); row.colorButton = newButton; Layout(); } -wxString PlayerLabelPanel::ValidateLabels() -{ - const int numRows = NumRows(); - wxString message; - - for (int i = 0; i < numRows; i++) { - if (!m_rows[i].labelCtrl) { - continue; // chance's row has no editable label to validate - } - const wxString value = m_rows[i].labelCtrl->GetValue(); - bool invalid = value.empty(); - for (int other = 0; !invalid && other < numRows; other++) { - if (other != i && m_rows[other].labelCtrl && m_rows[other].labelCtrl->GetValue() == value) { - invalid = true; - } - } - - m_rows[i].labelCtrl->SetBackgroundColour(invalid ? kInvalidLabelBg : m_defaultBg); - m_rows[i].labelCtrl->Refresh(); - - if (invalid && message.empty()) { - message = - value.empty() ? _("Player labels cannot be empty.") : _("Player labels must be unique."); - } - } - return message; -} - //======================================================================== // class GamePropertiesDialog //======================================================================== @@ -313,8 +613,25 @@ GamePropertiesDialog::GamePropertiesDialog(wxWindow *p_parent, generalPanel->SetSizer(generalSizer); notebook->AddPage(generalPanel, _("General")); - m_playerPanel = new PlayerLabelPanel(notebook, m_doc, [this]() { UpdateValidation(); }); - notebook->AddPage(m_playerPanel, _("Players")); + auto *playersPanel = new wxPanel(notebook); + auto *playersSizer = new wxBoxSizer(wxVERTICAL); + + if (game->IsTree()) { + m_chanceColor = m_doc->GetStyle().ChanceColor(); + auto *chanceSizer = new wxBoxSizer(wxHORIZONTAL); + m_chanceColorButton = MakeChanceColorButton(playersPanel); + chanceSizer->Add(m_chanceColorButton, 0, wxALIGN_CENTER_VERTICAL | wxRIGHT, FromDIP(5)); + chanceSizer->Add(new wxStaticText(playersPanel, wxID_STATIC, + wxString::FromUTF8(game->GetChance()->GetLabel())), + 0, wxALIGN_CENTER_VERTICAL); + playersSizer->Add(chanceSizer, 0, wxALL, FromDIP(5)); + } + + m_playerPanel = new PlayerPanel(playersPanel, m_doc, [this]() { UpdateValidation(); }); + playersSizer->Add(m_playerPanel, 1, wxALL | wxEXPAND, FromDIP(5)); + + playersPanel->SetSizer(playersSizer); + notebook->AddPage(playersPanel, _("Players")); topSizer->Add(notebook, 1, wxALL | wxEXPAND, 5); @@ -349,11 +666,16 @@ void GamePropertiesDialog::UpdateValidation() } } -int GamePropertiesDialog::NumRows() const { return m_playerPanel->NumRows(); } +int GamePropertiesDialog::NumPlayerRows() const { return m_playerPanel->NumPlayers(); } -GamePlayer GamePropertiesDialog::GetPlayer(int p_index) const +bool GamePropertiesDialog::IsPlayerDeleted(int p_index) const { - return m_playerPanel->GetPlayer(p_index); + return m_playerPanel->IsDeleted(p_index); +} + +std::string GamePropertiesDialog::GetPlayerStableLabel(int p_index) const +{ + return m_playerPanel->GetStableLabel(p_index); } wxString GamePropertiesDialog::GetPlayerLabel(int p_index) const @@ -366,4 +688,41 @@ wxColour GamePropertiesDialog::GetPlayerColor(int p_index) const return m_playerPanel->GetPlayerColor(p_index); } +wxBitmapButton *GamePropertiesDialog::MakeChanceColorButton(wxWindow *p_parent) +{ + auto *button = new wxBitmapButton(p_parent, wxID_ANY, MakeColorSwatch(m_chanceColor), + wxDefaultPosition, wxDefaultSize, wxNO_BORDER); + button->SetToolTip(_("Change the color for chance")); + button->Bind(wxEVT_BUTTON, [this](wxCommandEvent &) { OnSetChanceColor(); }); + return button; +} + +void GamePropertiesDialog::OnSetChanceColor() +{ + wxColourData data; + data.SetColour(m_chanceColor); + PlatformColourDialog dialog(this, &data); + dialog.SetTitle(_("Choose color for chance")); + + if (dialog.ShowModal() != wxID_OK) { + return; + } + + m_chanceColor = dialog.GetColourData().GetColour(); + CallAfter([this]() { RecreateChanceColorButton(); }); +} + +void GamePropertiesDialog::RecreateChanceColorButton() +{ + wxWindow *parent = m_chanceColorButton->GetParent(); + wxBitmapButton *oldButton = m_chanceColorButton; + wxBitmapButton *newButton = MakeChanceColorButton(parent); + + parent->GetSizer()->Replace(oldButton, newButton, true); + oldButton->Destroy(); + m_chanceColorButton = newButton; + + parent->Layout(); +} + } // namespace Gambit::GUI diff --git a/src/gui/dlgameprop.h b/src/gui/dlgameprop.h index 92db9f679..e396f7f90 100644 --- a/src/gui/dlgameprop.h +++ b/src/gui/dlgameprop.h @@ -25,15 +25,23 @@ namespace Gambit::GUI { -class PlayerLabelPanel; +class PlayerPanel; class GamePropertiesDialog final : public wxDialog { std::shared_ptr m_doc; wxTextCtrl *m_title, *m_comment; - PlayerLabelPanel *m_playerPanel; + PlayerPanel *m_playerPanel; wxStaticText *m_errorText; + // Chance's colour, staged like everything on the Players page; unused if the game isn't a + // tree. Chance itself is never added, deleted, or reordered, so it's kept out of + // PlayerPanel entirely rather than as a fixed, non-editable row there. + wxColour m_chanceColor; + wxBitmapButton *m_chanceColorButton = nullptr; void UpdateValidation(); + void OnSetChanceColor(); + void RecreateChanceColorButton(); + wxBitmapButton *MakeChanceColorButton(wxWindow *p_parent); public: // Lifecycle @@ -43,12 +51,20 @@ class GamePropertiesDialog final : public wxDialog { wxString GetTitle() const override { return m_title->GetValue(); } wxString GetDescription() const { return m_comment->GetValue(); } - // Player (and chance) labels and colors, as edited on the Players page. Row 0 - // is chance; the rest are the game's personal players. Both labels and - // colors are staged only within the dialog and are not applied to the - // document until ShowModal() returns wxID_OK and the caller commits them. - int NumRows() const; - GamePlayer GetPlayer(int p_index) const; + // Chance's colour as edited on the Players page; meaningful only if the game is a tree. + wxColour GetChanceColor() const { return m_chanceColor; } + + // The game's players (excluding chance) as edited on the Players page: adding, deleting, + // reordering, relabeling, and recoloring are all staged only within the dialog and are not + // applied to the document until ShowModal() returns wxID_OK and the caller commits them. + // Row `p_index` may be a current player, kept or marked for deletion, or a brand-new one + // added in this dialog; a deleted row should be excluded from the operation committed. + int NumPlayerRows() const; + bool IsPlayerDeleted(int p_index) const; + // The label the player at position `p_index` had before this edit, or the placeholder + // assigned if the row was newly added -- identifies the player for `Game::SetPlayers`, + // independent of whatever's currently typed into its label field. + std::string GetPlayerStableLabel(int p_index) const; wxString GetPlayerLabel(int p_index) const; wxColour GetPlayerColor(int p_index) const; }; diff --git a/src/gui/dlinsertmove.cc b/src/gui/dlinsertmove.cc index d8e0d3aed..93b20866f 100644 --- a/src/gui/dlinsertmove.cc +++ b/src/gui/dlinsertmove.cc @@ -190,7 +190,7 @@ GamePlayer InsertMoveDialog::GetPlayer() const if (playerNumber <= static_cast(m_doc->GetGame()->NumPlayers())) { return m_doc->GetGame()->GetPlayer(playerNumber); } - return m_doc->DoNewPlayer(); + return nullptr; // "Insert move for a new player" -- see the declaration's comment } GameInfoset InsertMoveDialog::GetInfoset() const diff --git a/src/gui/dlinsertmove.h b/src/gui/dlinsertmove.h index d9c8dbd7b..e7309bdaa 100644 --- a/src/gui/dlinsertmove.h +++ b/src/gui/dlinsertmove.h @@ -42,6 +42,9 @@ class InsertMoveDialog final : public wxDialog { InsertMoveDialog(wxWindow *, const std::shared_ptr &); // Data access (only valid if ShowModal() returns wxID_OK. + // If GetPlayer() returns null, the user selected "Insert move for a new player" -- no such + // player exists yet, so the caller must add one (e.g. via GameDocument::DoAddPlayer()) + // before it can be used. // If GetInfoset() returns null, user selected "new infoset" GamePlayer GetPlayer() const; GameInfoset GetInfoset() const; diff --git a/src/gui/efgnodemenu.cc b/src/gui/efgnodemenu.cc index eb686e074..85b8b7a91 100644 --- a/src/gui/efgnodemenu.cc +++ b/src/gui/efgnodemenu.cc @@ -227,7 +227,12 @@ void EfgDisplay::OnEditInsertMove(wxCommandEvent &) m_doc->DoInsertMove(m_contextNode, dialog.GetInfoset()); } else { - m_doc->DoInsertMove(m_contextNode, dialog.GetPlayer(), dialog.GetActions()); + GamePlayer player = dialog.GetPlayer(); + if (!player) { + // "Insert move for a new player" was selected: no such player exists yet. + player = m_doc->DoAddPlayer(); + } + m_doc->DoInsertMove(m_contextNode, player, dialog.GetActions()); } } catch (std::exception &ex) { diff --git a/src/gui/gamedoc.cc b/src/gui/gamedoc.cc index 18ae079a2..86fb8f9e9 100644 --- a/src/gui/gamedoc.cc +++ b/src/gui/gamedoc.cc @@ -457,21 +457,24 @@ void GameDocument::DoSetTitle(const wxString &p_title, const wxString &p_comment NotifyChanged(GameModificationType::GameLabels); } -GamePlayer GameDocument::DoNewPlayer() +GamePlayer GameDocument::DoAddPlayer() { std::set playerLabels; - + std::vector labels; for (const auto &player : m_game->GetPlayers()) { playerLabels.insert(player->GetLabel()); + labels.push_back(player->GetLabel()); } int number = m_game->NumPlayers() + 1; while (playerLabels.contains("Player " + lexical_cast(number))) { number++; } - const GamePlayer player = m_game->NewPlayer("Player " + lexical_cast(number)); + labels.push_back("Player " + lexical_cast(number)); + + m_game->SetPlayers(labels); NotifyChanged(GameModificationType::GameForm); - return player; + return m_game->GetPlayers().back(); } void GameDocument::DoRelabelPlayers(const std::map &p_labels) @@ -480,6 +483,27 @@ void GameDocument::DoRelabelPlayers(const std::map &p_ NotifyChanged(GameModificationType::GameLabels); } +void GameDocument::DoSetPlayers(const std::vector &p_stableLabels, + const std::vector &p_labels) +{ + // Phase 1: structure (which players exist, and in what order), resolved purely from + // p_stableLabels -- untouched by any pending rename in p_labels. + m_game->SetPlayers(p_stableLabels); + + // Phase 2: relabeling, applied once the structure has settled, so a label freed up by + // a deletion in phase 1 is available for reuse here. + std::map relabels; + for (size_t i = 0; i < p_stableLabels.size(); i++) { + if (p_stableLabels[i] != p_labels[i]) { + relabels[p_stableLabels[i]] = p_labels[i]; + } + } + if (!relabels.empty()) { + m_game->RelabelPlayers(relabels); + } + NotifyChanged(GameModificationType::GameForm); +} + void GameDocument::DoSetStrategies(GamePlayer p_player, const std::vector &p_stableLabels, const std::vector &p_labels) diff --git a/src/gui/gamedoc.h b/src/gui/gamedoc.h index 1827634b2..cf0be054d 100644 --- a/src/gui/gamedoc.h +++ b/src/gui/gamedoc.h @@ -316,9 +316,24 @@ class GameDocument { } void DoSave(const wxString &p_filename, GameSaveFormat p_format); void DoSetTitle(const wxString &p_title, const wxString &p_comment); - GamePlayer DoNewPlayer(); + /// Adds one player to the game, via `Game::SetPlayers`, labeled with the first unused + /// "Player N". + GamePlayer DoAddPlayer(); /// Reassign player labels in a single operation; see `Game::RelabelPlayers`. void DoRelabelPlayers(const std::map &p_labels); + /// Declare the players of the game in a single operation, covering any combination of + /// adding, deleting, reordering, and relabeling players. Chance is not part of either list; + /// it is not affected by this operation. + /// + /// `p_stableLabels` identifies each player as it was before this edit (an existing player's + /// current label, or a placeholder for one newly created); `p_labels` is what that same + /// player, by position, is to be labeled after the edit. Structure (which players exist, + /// and in what order) is resolved first, purely from `p_stableLabels`; labels are then + /// reassigned from `p_stableLabels` to `p_labels`. Doing so in this order means a rename + /// that reuses a label freed up by a simultaneous deletion never collides with the + /// not-yet-renamed original. + void DoSetPlayers(const std::vector &p_stableLabels, + const std::vector &p_labels); /// Declare the strategies of `p_player` in a single operation, covering any combination /// of adding, deleting, reordering, and relabeling strategies. /// @@ -371,10 +386,8 @@ class GameDocument { inline std::shared_ptr NewTreeDocument() { - const Game efg = NewTree(); + const Game efg = NewTree({"Player 1", "Player 2"}); efg->SetTitle("Untitled Extensive Game"); - efg->NewPlayer("Player 1"); - efg->NewPlayer("Player 2"); return std::make_shared(efg); } diff --git a/src/gui/gameframe.cc b/src/gui/gameframe.cc index ddf32340d..423ce4be5 100644 --- a/src/gui/gameframe.cc +++ b/src/gui/gameframe.cc @@ -199,7 +199,6 @@ EVT_MENU_RANGE(wxID_FILE1, wxID_FILE9, GameFrame::OnFileMRUFile) EVT_MENU(wxID_UNDO, GameFrame::OnEditUndo) EVT_MENU(wxID_REDO, GameFrame::OnEditRedo) EVT_MENU(GBT_MENU_EDIT_GAME, GameFrame::OnEditGame) -EVT_MENU(GBT_MENU_EDIT_NEWPLAYER, GameFrame::OnEditNewPlayer) EVT_MENU(GBT_MENU_VIEW_PROFILES, GameFrame::OnViewProfiles) EVT_MENU(GBT_MENU_VIEW_ZOOMIN, GameFrame::OnViewZoom) EVT_MENU(GBT_MENU_VIEW_ZOOMOUT, GameFrame::OnViewZoom) @@ -320,8 +319,6 @@ void GameFrame::OnUpdate() GetToolBar()->EnableTool(wxID_UNDO, m_doc->CanUndo()); GetToolBar()->EnableTool(wxID_REDO, m_doc->CanRedo()); - GetToolBar()->EnableTool(GBT_MENU_EDIT_NEWPLAYER, !m_efgPanel || m_efgPanel->IsShown()); - menuBar->Enable(GBT_MENU_VIEW_PROFILES, m_doc->GetWorkspace().NumProfileLists() > 0); GetToolBar()->EnableTool(GBT_MENU_VIEW_PROFILES, m_doc->GetWorkspace().NumProfileLists() > 0); GetToolBar()->EnableTool(GBT_MENU_FORMAT_DECIMALS_DELETE, m_doc->GetStyle().NumDecimals() > 1); @@ -356,7 +353,6 @@ void GameFrame::OnUpdate() #include "bitmaps/font.xpm" #include "bitmaps/label.xpm" #include "bitmaps/layout.xpm" -#include "bitmaps/newplayer.xpm" #include "bitmaps/newtable.xpm" #include "bitmaps/newtree.xpm" #include "bitmaps/open.xpm" @@ -446,10 +442,6 @@ void GameFrame::MakeMenus() AppendBitmapItem(editMenu, wxID_REDO, _("&Redo\tShift-Ctrl-Z"), _("Redo the last undone change"), wxBitmap(redo_xpm)); editMenu->AppendSeparator(); - AppendBitmapItem(editMenu, GBT_MENU_EDIT_NEWPLAYER, _("Add p&layer"), - _("Add a new player to the game"), wxBitmap(newplayer_xpm)); - - editMenu->AppendSeparator(); editMenu->Append(GBT_MENU_EDIT_GAME, _("&Game"), _("Edit properties of the game")); auto *viewMenu = new wxMenu; @@ -546,11 +538,6 @@ void GameFrame::MakeToolbar() toolBar->AddTool(wxID_PREVIEW, wxEmptyString, wxBitmap(preview_xpm), wxNullBitmap, wxITEM_NORMAL, _("Print preview"), _("View a preview of the game printout"), nullptr); - toolBar->AddSeparator(); - - toolBar->AddTool(GBT_MENU_EDIT_NEWPLAYER, wxEmptyString, wxBitmap(newplayer_xpm), wxNullBitmap, - wxITEM_NORMAL, _("Add a new player"), _("Add a new player to the game"), - nullptr); if (m_doc->GetGame()->IsTree()) { toolBar->AddTool(GBT_MENU_VIEW_ZOOMIN, wxEmptyString, wxBitmap(zoomin_xpm), wxNullBitmap, wxITEM_NORMAL, _("Zoom in"), _("Increase magnification"), nullptr); @@ -908,25 +895,54 @@ void GameFrame::OnEditGame(wxCommandEvent &) try { m_doc->DoSetTitle(dialog.GetTitle(), dialog.GetDescription()); - TreeRenderConfig style = m_doc->GetStyle(); - std::map labels; - for (int i = 0; i < dialog.NumRows(); i++) { - const GamePlayer player = dialog.GetPlayer(i); - if (player->IsChance()) { - style.SetChanceColor(dialog.GetPlayerColor(i)); - continue; // chance's label is reserved and can't be changed + std::vector stableLabels, labels; + std::vector colors; + for (int i = 0; i < dialog.NumPlayerRows(); i++) { + if (dialog.IsPlayerDeleted(i)) { + continue; } - const std::string newLabel = dialog.GetPlayerLabel(i).ToStdString(wxConvUTF8); - if (newLabel != player->GetLabel()) { - labels[player->GetLabel()] = newLabel; + stableLabels.push_back(dialog.GetPlayerStableLabel(i)); + labels.push_back(dialog.GetPlayerLabel(i).ToStdString(wxConvUTF8)); + colors.push_back(dialog.GetPlayerColor(i)); + } + + std::vector currentLabels; + for (const auto &player : m_doc->GetGame()->GetPlayers()) { + currentLabels.push_back(player->GetLabel()); + } + + if (stableLabels != currentLabels) { + // The set, order, or count of players changed: add, delete, and/or reorder in a + // single operation. + m_doc->DoSetPlayers(stableLabels, labels); + } + else { + // No structural change -- relabeling all players in one call, rather than one at a + // time, lets two players' labels be swapped directly without tripping the + // duplicate-label check on an intermediate state that a per-player rename would pass + // through. (Also keeps this working for a game whose `Game::SetPlayers` is + // unsupported, e.g. one loaded from an action graph game file, as long as no + // structural edit was actually made.) + std::map relabels; + for (size_t i = 0; i < stableLabels.size(); i++) { + if (stableLabels[i] != labels[i]) { + relabels[stableLabels[i]] = labels[i]; + } + } + if (!relabels.empty()) { + m_doc->DoRelabelPlayers(relabels); } - style.SetPlayerColor(player->GetNumber(), dialog.GetPlayerColor(i)); } - // Relabeling all players in one call, rather than one at a time, lets two players' - // labels be swapped directly without tripping the duplicate-label check on an - // intermediate state that a per-player rename would pass through. - if (!labels.empty()) { - m_doc->DoRelabelPlayers(labels); + + TreeRenderConfig style = m_doc->GetStyle(); + if (m_doc->GetGame()->IsTree()) { + style.SetChanceColor(dialog.GetChanceColor()); + } + // Colors are assigned by position among the surviving rows, which matches the players' + // resulting numbers whether or not the block above just changed them. + int number = 1; + for (const auto &color : colors) { + style.SetPlayerColor(number++, color); } m_doc->SetStyle(style); } @@ -936,16 +952,6 @@ void GameFrame::OnEditGame(wxCommandEvent &) } } -void GameFrame::OnEditNewPlayer(wxCommandEvent &) -{ - try { - m_doc->DoNewPlayer(); - } - catch (std::exception &ex) { - ExceptionDialog(this, ex.what()).ShowModal(); - } -} - //---------------------------------------------------------------------- // GameFrame: Menu handlers - View menu //---------------------------------------------------------------------- diff --git a/src/gui/gameframe.h b/src/gui/gameframe.h index 2a150dc74..8679802a1 100644 --- a/src/gui/gameframe.h +++ b/src/gui/gameframe.h @@ -71,8 +71,6 @@ class GameFrame final : public wxFrame, public GameView { void OnEditGame(wxCommandEvent &); - void OnEditNewPlayer(wxCommandEvent &); - void OnViewProfiles(wxCommandEvent &); void OnViewZoom(wxCommandEvent &); void OnViewStrategic(wxCommandEvent &); diff --git a/src/gui/menuconst.h b/src/gui/menuconst.h index 1923f4c99..cac8fa0ff 100644 --- a/src/gui/menuconst.h +++ b/src/gui/menuconst.h @@ -47,8 +47,6 @@ enum MenuItems { // context menu, one per player (plus chance); see EfgDisplay::UpdateSetPlayerMenu. GBT_MENU_EDIT_SET_PLAYER_BASE = 1430, - GBT_MENU_EDIT_NEWPLAYER = 1500, - GBT_MENU_VIEW_PROFILES = 1850, GBT_MENU_VIEW_ZOOMIN = 1601, GBT_MENU_VIEW_ZOOMOUT = 1602, diff --git a/src/pygambit/gambit.pxd b/src/pygambit/gambit.pxd index 29ece38f0..4c69e1427 100644 --- a/src/pygambit/gambit.pxd +++ b/src/pygambit/gambit.pxd @@ -317,8 +317,8 @@ cdef extern from "games/game.h": c_GamePlayer GetPlayer(int) except +IndexError Players GetPlayers() except + c_GamePlayer GetChance() except + - c_GamePlayer NewPlayer(string) except +ValueError void RelabelPlayers(stdmap[string, string]) except +ValueError + void SetPlayers(stdvector[string]) except +ValueError int NumOutcomes() except + c_GameOutcome GetOutcome(int) except +IndexError @@ -382,7 +382,7 @@ cdef extern from "games/game.h": c_GameSubgame GetMinimalSubgame(c_GameInfoset) except + stdvector[c_GameSubgame] GetSubgames() except + - c_Game NewTree() except + + c_Game NewTree(stdvector[string]) except +ValueError c_Game NewTable(stdvector[int]) except + diff --git a/src/pygambit/game.pxi b/src/pygambit/game.pxi index fde488d57..402308b0e 100644 --- a/src/pygambit/game.pxi +++ b/src/pygambit/game.pxi @@ -560,10 +560,11 @@ class Game: Game The newly-created extensive game. """ - g = Game.wrap(NewTree()) - g.title = title + c_labels = stdvector[string]() for player in (players or []): - g.game.deref().NewPlayer(str(player).encode("utf-8")) + c_labels.push_back(str(player).encode("utf-8")) + g = Game.wrap(NewTree(c_labels)) + g.title = title return g @classmethod @@ -2536,36 +2537,94 @@ class Game: ) self.game.deref().Reveal(resolved_infoset.infoset, resolved_player.player) - def add_player(self, label: str) -> Player: - """Add a new player to the game. + def set_players(self, + players: list[str], + drop: bool = False, + add: bool = True) -> None: + """Set the players of the game to be `players`, matching by label. - .. 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. + An entry of `players` matching the label of a current player refers to that + player, which keeps its moves or strategies and its payoffs at every outcome; + an entry matching no current player creates a new player there, with no + decisions in an extensive game, or a single strategy labeled ``"1"`` in a + strategic game. A current player whose label is not in `players` is deleted. + Listing the current labels in a new order reorders the players. - .. versionchanged:: 17.0.0 - In a game with a strategic representation, the new player's sole strategy is - labeled ``"1"``. + A player can only be deleted if it has no decisions in the game (in an + extensive game) or exactly one strategy (in a strategic game); otherwise the + operation raises. + + The defaults permit creation and forbid deletion: adding a player -- inserting + its label into the current list -- is the common, non-destructive edit, while + deletion discards the player's payoffs at every outcome, so it must be + confirmed. + + .. versionadded:: 17.0.0 + Subsumes and replaces `Game.add_player`. Parameters ---------- - 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. + players : list of str + The labels of the players the game is to have, in order. Must be nonempty + and without duplicates; each label must be a valid, nonempty label, and in + an extensive game must not be the reserved chance player label. + drop : bool, default False + Deleting players is destructive, so it must be explicitly confirmed: if any + current player is missing from `players` and `drop` is `False`, the + operation raises without modifying the game. + add : bool, default True + If `False`, entries of `players` matching no current player raise. Raises ------ + TypeError + If `players` is a string, or not an iterable of strings. + UndefinedOperationError + If `players` is empty; or if a player to be deleted has decisions in the + game, or more than one strategy. 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. + If a label in `players` is repeated, empty, is not a valid label, or (in an + extensive game) is the reserved label of the chance player. """ - return Player.wrap(self.game.deref().NewPlayer(label.encode("utf-8"))) + if isinstance(players, str) or not hasattr(players, "__iter__"): + raise TypeError("set_players(): players must be an iterable of str") + labels = list(players) + for label in labels: + if not isinstance(label, str): + raise TypeError("set_players(): players must be an iterable of str") + if not labels: + raise UndefinedOperationError("set_players(): `players` must be a nonempty list") + current = [player.label for player in self.players] + if len(set(current)) != len(current): + raise ValueError( + "set_players(): the game has duplicate player labels, " + "so matching by label is not well-defined" + ) + added = [label for label in labels if label not in current] + if added and not add: + raise ValueError(f"set_players(): would create new players {added}") + missing = [label for label in current if label not in labels] + if missing and not drop: + raise ValueError( + f"set_players(): would delete players {missing} and their payoffs at " + f"every outcome; pass drop=True to confirm" + ) + for label in missing: + resolved = self.players[label] + if self.is_tree and len(resolved.infosets) > 0: + raise UndefinedOperationError( + f"set_players(): player '{label}' has decisions in the game " + f"and cannot be deleted" + ) + if not self.is_tree and len(resolved.strategies) != 1: + raise UndefinedOperationError( + f"set_players(): player '{label}' has more than one strategy " + f"and cannot be deleted" + ) + c_labels = stdvector[string]() + for label in labels: + c_labels.push_back(label.encode("utf-8")) + self.game.deref().SetPlayers(c_labels) def add_outcome(self, label: str, diff --git a/tests/test_extensive.py b/tests/test_extensive.py index b43e39f0a..3165c0248 100644 --- a/tests/test_extensive.py +++ b/tests/test_extensive.py @@ -52,10 +52,9 @@ def test_game_title_accepts_text_invalid_for_a_label(text: str): @pytest.mark.parametrize("players", [["Alice"], ["Oscar", "Felix"]]) -def test_game_add_players_label(players: list): +def test_game_set_players_label(players: list): game = gbt.Game.new_tree() - for player in players: - game.add_player(player) + game.set_players(players) for player, label in zip(game.players, players, strict=True): assert player.label == label diff --git a/tests/test_game.py b/tests/test_game.py index 566341469..6c01fc42b 100644 --- a/tests/test_game.py +++ b/tests/test_game.py @@ -162,7 +162,8 @@ def test_game_get_outcome_with_bad_strategies(): def test_game_dereference_invalid(): game = gbt.Game.new_tree() - player = game.add_player("One") + game.set_players(["One"]) + player = game.players["One"] strategy = next(iter(player.strategies)) game.append_move(game.root, player, ["a", "b"]) with pytest.raises(RuntimeError): diff --git a/tests/test_node.py b/tests/test_node.py index 4f17767ee..4618f06cc 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -755,7 +755,7 @@ def test_node_move_across_games(): def test_append_move_creates_single_infoset_list_of_nodes(): """Test that appending a list of nodes creates a single infoset.""" game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) nodes = [game.root.children["2"].children["1"], game.root.children["1"].children["1"], game.root.children["1"].children["2"]] @@ -766,7 +766,7 @@ def test_append_move_creates_single_infoset_list_of_nodes(): def test_append_move_same_infoset_list_of_nodes(): """Test that nodes from a list of nodes are resolved in the same infoset.""" game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) node1 = game.root.children["2"].children["1"] node2 = game.root.children["1"].children["1"] game.append_move([node1, node2], "Player 3", ["B", "F"]) @@ -778,7 +778,7 @@ def test_append_move_actions_list_of_nodes(): have the same actions. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) node1 = game.root.children["2"].children["1"] node2 = game.root.children["1"].children["1"] game.append_move([node1, node2], "Player 3", ["B", "F", "S"]) @@ -788,7 +788,7 @@ def test_append_move_actions_list_of_nodes(): def test_append_move_actions_list_of_node_labels(): """Test that nodes from a list of node labels are resolved correctly.""" game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) node1 = game.root.children["2"].children["1"] node2 = game.root.children["1"].children["1"] node1.label = "0" @@ -806,7 +806,7 @@ def test_append_move_actions_list_of_mixed_node_references(): are resolved correctly. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) node1 = game.root.children["2"].children["1"] node2 = game.root.children["1"].children["1"] @@ -824,7 +824,7 @@ def test_append_move_labels_list_of_nodes(): have the same labels per action. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) node1 = game.root.children["2"].children["1"] node2 = game.root.children["1"].children["1"] game.append_move([node1, node2], "Player 3", ["B", "F", "S"]) @@ -838,7 +838,7 @@ def test_append_move_node_list_with_non_terminal_node(): of nodes that has a non-terminal node. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) with pytest.raises(gbt.UndefinedOperationError): game.append_move( [game.root.children["2"], game.root.children["1"].children["2"]], @@ -852,7 +852,7 @@ def test_append_move_node_list_with_duplicate_node_references(): nodes with non-unique node references. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) node = game.root.children["1"].children["2"] node.label = "00" with pytest.raises(ValueError): @@ -868,7 +868,7 @@ def test_append_move_node_list_is_empty(): empty list of nodes. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) with pytest.raises(ValueError): game.append_move([], "Player 3", ["B", "F"]) @@ -878,7 +878,7 @@ def test_append_infoset_node_list_with_non_terminal_node(): a list of nodes that has a non-terminal node. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) seed_node = game.root.children["1"].children["1"] game.append_move(seed_node, "Player 3", ["B", "F"]) with pytest.raises(gbt.UndefinedOperationError): @@ -893,7 +893,7 @@ def test_append_infoset_node_list_with_duplicate_node(): with non-unique elements. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) seed_node = game.root.children["1"].children["1"] game.append_move(seed_node, "Player 3", ["B", "F"]) with pytest.raises(ValueError): @@ -910,7 +910,7 @@ def test_append_infoset_node_list_is_empty(): empty list of nodes. """ game = games.read_from_file("sample_extensive_game.efg") - game.add_player("Player 3") + game.set_players([player.label for player in game.players] + ["Player 3"]) seed_node = game.root.children["1"].children["1"] game.append_move(seed_node, "Player 3", ["B", "F"]) with pytest.raises(ValueError): diff --git a/tests/test_players.py b/tests/test_players.py index a865aaa90..4339f04fc 100644 --- a/tests/test_players.py +++ b/tests/test_players.py @@ -35,36 +35,35 @@ def test_player_label_unicode_accepted(label): assert 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() +def test_set_players_requires_iterable_of_str(): + game = gbt.Game.new_table([2, 2]) + with pytest.raises(TypeError): + game.set_players("12") with pytest.raises(TypeError): - game.add_player() + game.set_players([1, 2]) -def test_add_player_duplicate_label_raises_and_leaves_game_unchanged(): +def test_set_players_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) + labels = [player.label for player in game.players] with pytest.raises(ValueError): - game.add_player(existing) - assert len(game.players) == count_before + game.set_players(labels + [labels[0]]) + assert [player.label for player in game.players] == labels -def test_add_player_empty_label_raises_and_leaves_game_unchanged(): +def test_set_players_empty_label_raises_and_leaves_game_unchanged(): game = gbt.Game.new_table([2, 2]) - count_before = len(game.players) + labels = [player.label for player in game.players] with pytest.raises(ValueError): - game.add_player("") - assert len(game.players) == count_before + game.set_players(labels + [""]) + assert [player.label for player in game.players] == labels -def test_add_player_reserved_chance_label_raises_and_leaves_game_unchanged(): +def test_set_players_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 + game.set_players(["Chance"]) + assert len(game.players) == 0 def test_chance_player_has_label(): @@ -82,7 +81,7 @@ def test_chance_player_label_cannot_be_changed(): def test_regular_player_cannot_be_relabeled_to_chance(): game = gbt.Game.new_tree() - game.add_player("Alice") + game.set_players(["Alice"]) player = next(iter(game.players)) with pytest.raises(ValueError): game.relabel_players({player.label: "Chance"}) @@ -171,17 +170,19 @@ def test_relabel_players_chance_key_raises_even_when_not_strict(): game.relabel_players({"Chance": "Nature"}, strict=False) -def test_strategic_game_add_player(): +def test_strategic_game_set_players_add(): game = gbt.Game.new_table([2, 2]) - new_player = game.add_player("Player 3") + labels = [player.label for player in game.players] + game.set_players(labels + ["Player 3"]) + new_player = game.players["Player 3"] assert len(game.players) == 3 assert len(new_player.strategies) == 1 assert next(iter(new_player.strategies)).label == "1" -def test_extensive_game_add_player(): +def test_extensive_game_set_players_add(): game = gbt.Game.new_tree() - game.add_player("Alice") + game.set_players(["Alice"]) pl1 = next(iter(game.players)) assert len(game.players) == 1 assert len(pl1.infosets) == 0 @@ -464,3 +465,49 @@ def test_set_strategies_empty_label_raises_and_leaves_game_unchanged(): with pytest.raises(ValueError): game.set_strategies(pl, labels + [""]) assert [s.label for s in pl.strategies] == labels + + +def test_set_players_empty_raises(): + game = gbt.Game.new_table([2, 2]) + with pytest.raises(gbt.UndefinedOperationError): + game.set_players([], drop=True) + + +def test_set_players_reorder_transposes_table(): + """Reordering the players permutes the axes of the payoff table.""" + game = gbt.Game.from_arrays([[1, 2, 3], [4, 5, 6]], [[7, 8, 9], [10, 11, 12]]) + a, b = (player.label for player in game.players) + game.set_players([b, a]) + assert [player.label for player in game.players] == [b, a] + assert game.to_arrays()[0].tolist() == [[7, 10], [8, 11], [9, 12]] + assert game.to_arrays()[1].tolist() == [[1, 4], [2, 5], [3, 6]] + + +def test_set_players_add_then_drop_round_trips(): + game = gbt.Game.from_arrays([[1, 2], [3, 4]], [[5, 6], [7, 8]]) + labels = [player.label for player in game.players] + game.set_players(labels + ["X"]) + assert all(outcome["X"] == 0 for outcome in game.outcomes) + game.set_players(labels, drop=True) + assert [player.label for player in game.players] == labels + assert game.to_arrays()[0].tolist() == [[1, 2], [3, 4]] + + +def test_set_players_drop_requires_deletable_player(): + game = gbt.Game.new_table([2, 2]) + a, _ = (player.label for player in game.players) + with pytest.raises(gbt.UndefinedOperationError): + game.set_players([a], drop=True) + tree = games.create_stripped_down_poker_efg() + with pytest.raises(gbt.UndefinedOperationError): + tree.set_players(["Bob"], drop=True) + + +def test_set_players_unconfirmed_drop_and_disabled_add_raise(): + game = gbt.Game.new_table([2, 2]) + labels = [player.label for player in game.players] + with pytest.raises(ValueError): + game.set_players(labels[:1]) + with pytest.raises(ValueError): + game.set_players(labels + ["X"], add=False) + assert [player.label for player in game.players] == labels