diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp index cc5220da5..96a8b61d6 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.cpp @@ -308,7 +308,28 @@ void Server_Game::createGameStateChangedEvent(Event_GameStateChanged *event, event->set_game_started(false); } - for (Server_AbstractParticipant *participant : participants.values()) { + // Emit the player list in seat order so every client (including old ones + // that ignore Event_PlayerOrderChanged) sees the current seating. Player + // ids are stable; only this ordering changes on shuffle/reorder. + QList ordered; + ordered.reserve(participants.size()); + for (int id : seatOrder) { + auto *participant = participants.value(id, nullptr); + if (participant && !participant->isSpectator() && !ordered.contains(participant)) { + ordered.append(participant); + } + } + for (auto *participant : participants.values()) { + if (!participant->isSpectator() && !ordered.contains(participant)) { + ordered.append(participant); + } + } + for (auto *participant : participants.values()) { + if (participant->isSpectator()) { + ordered.append(participant); + } + } + for (Server_AbstractParticipant *participant : ordered) { participant->getInfo(event->add_player_list(), recipient, omniscient, withUserInfo); } @@ -448,14 +469,24 @@ void Server_Game::shufflePlayerSeats() { QMutexLocker locker(&gameMutex); - // Split participants into players and spectators. Spectators keep their seats. + // Collect players in current seat order (fall back to id order for seats + // predating seatOrder). Spectators keep their seats. QList players; + for (int id : seatOrder) { + auto *participant = participants.value(id, nullptr); + if (participant && !participant->isSpectator()) { + players.append(participant); + } + } + for (auto *participant : participants.values()) { + if (!participant->isSpectator() && !players.contains(participant)) { + players.append(participant); + } + } QList spectators; for (auto *participant : participants.values()) { if (participant->isSpectator()) { spectators.append(participant); - } else { - players.append(participant); } } @@ -473,14 +504,24 @@ void Server_Game::reorderPlayerSeats(const QList &orderedNames) { QMutexLocker locker(&gameMutex); - // Split participants into players and spectators. Spectators keep their seats. + // Collect players in current seat order (fall back to id order for seats + // predating seatOrder). Spectators keep their seats. QList players; + for (int id : seatOrder) { + auto *participant = participants.value(id, nullptr); + if (participant && !participant->isSpectator()) { + players.append(participant); + } + } + for (auto *participant : participants.values()) { + if (!participant->isSpectator() && !players.contains(participant)) { + players.append(participant); + } + } QList spectators; for (auto *participant : participants.values()) { if (participant->isSpectator()) { spectators.append(participant); - } else { - players.append(participant); } } @@ -510,36 +551,17 @@ void Server_Game::reseatPlayers(const QList &order { QMutexLocker locker(&gameMutex); - // Reassign fresh, unique player ids following the given order. - QMap newParticipants; - int newHostId = -1; + // Keep player ids stable and only reorder the seating/turn order. Changing + // ids would break old clients (stale localPlayerId from Event_GameJoined + // duplicates their own seat as an opponent), invalidate the home-server + // per-user game/seat mapping used for ISL command routing, and orphan the + // host id. All of those maps stay valid because ids never change here. + // The current host therefore keeps its id and needs no re-key tracking. + seatOrder.clear(); for (auto *participant : orderedParticipants) { - const int newPlayerId = nextPlayerId++; - const int oldPlayerId = participant->getPlayerId(); - participant->setPlayerId(newPlayerId); - newParticipants.insert(newPlayerId, participant); - - // Keep the user interface's per-user game/seat mapping in sync so that - // incoming game commands are routed to the participant under its new id. - if (participant->getUserInterface()) { - participant->getUserInterface()->playerAddedToGame(gameId, room->getId(), newPlayerId); + if (!participant->isSpectator()) { + seatOrder.append(participant->getPlayerId()); } - - if (!participant->isSpectator() && (participant->getUserInfo()->user_level() & ServerInfo_User::IsRegistered)) { - const QString playerName = QString::fromStdString(participant->getUserInfo()->name()); - room->getServer()->removePersistentPlayer(playerName, room->getId(), gameId, oldPlayerId); - room->getServer()->addPersistentPlayer(playerName, room->getId(), gameId, newPlayerId); - } - - if (participant->getUserInfo()->name() == creatorInfo->name()) { - newHostId = newPlayerId; - } - } - participants = newParticipants; - - if (newHostId != -1) { - hostId = newHostId; - sendGameEventContainer(prepareGameEvent(Event_GameHostChanged(), hostId)); } // Broadcast the new seating order so that every client can display it. @@ -683,6 +705,7 @@ void Server_Game::addPlayer(Server_AbstractUserInterface *userInterface, allSpectatorsEver.insert(playerName); } else { allPlayersEver.insert(playerName); + seatOrder.append(newParticipant->getPlayerId()); // if the original creator of the game joins, give them host status back //! \todo Transferring host to spectators has side effects. @@ -712,6 +735,7 @@ void Server_Game::removeParticipant(Server_AbstractParticipant *participant, Eve room->getServer()->removePersistentPlayer(QString::fromStdString(participant->getUserInfo()->name()), room->getId(), gameId, participant->getPlayerId()); participants.remove(participant->getPlayerId()); + seatOrder.removeAll(participant->getPlayerId()); bool spectator = participant->isSpectator(); GameEventStorage ges; @@ -731,10 +755,20 @@ void Server_Game::removeParticipant(Server_AbstractParticipant *participant, Eve participant->prepareDestroy(); if (playerHost) { + // Pass host to the next player in seat order so the game always keeps + // a host while players remain. int newHostId = -1; - for (auto *otherPlayer : getPlayers().values()) { - newHostId = otherPlayer->getPlayerId(); - break; + for (int id : seatOrder) { + if (participants.contains(id)) { + newHostId = id; + break; + } + } + if (newHostId == -1) { + for (auto *otherPlayer : getPlayers().values()) { + newHostId = otherPlayer->getPlayerId(); + break; + } } if (newHostId != -1) { hostId = newHostId; @@ -891,7 +925,20 @@ void Server_Game::nextTurn() } auto players = getPlayers(); - const QList keys = players.keys(); + // Advance in seat order, not id order, so shuffles/reorders change the + // turn sequence while ids stay stable. + QList keys; + keys.reserve(players.size()); + for (int id : seatOrder) { + if (players.contains(id)) { + keys.append(id); + } + } + for (int id : players.keys()) { + if (!keys.contains(id)) { + keys.append(id); + } + } int listPos = -1; if (activePlayer != -1) { listPos = keys.indexOf(activePlayer); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.h index edb910682..769180a6d 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_game.h @@ -74,6 +74,11 @@ private: int hostId; ServerInfo_User *creatorInfo; QMap participants; + // Seating/turn order as an ordered list of player ids. Player ids themselves + // are stable for the lifetime of the participant; shuffling or reordering + // only permutes this list so old clients, ISL routes and persistent + // references (all keyed by player id) keep working. + QList seatOrder; QSet allPlayersEver, allSpectatorsEver; bool gameStarted; bool gameClosed; @@ -151,6 +156,10 @@ public: { return participants; } + QList getSeatOrder() const + { + return seatOrder; + } int getGameId() const { return gameId; diff --git a/tests/movecard_tests/player_order_test.cpp b/tests/movecard_tests/player_order_test.cpp index 92ee380c3..6e398af24 100644 --- a/tests/movecard_tests/player_order_test.cpp +++ b/tests/movecard_tests/player_order_test.cpp @@ -1,4 +1,5 @@ #include "game/game_config.h" +#include "game/role_info.h" #include "game/server_abstract_participant.h" #include "game/server_game.h" #include "server_abstractuserinterface.h" @@ -70,19 +71,18 @@ namespace QStringList participantNamesInOrder(const Server_Game &game) { QStringList names; - for (const auto *participant : game.getParticipants()) { - names.append(QString::fromStdString(participant->getUserInfo()->name())); + for (int id : game.getSeatOrder()) { + const auto *participant = game.getParticipants().value(id, nullptr); + if (participant) { + names.append(QString::fromStdString(participant->getUserInfo()->name())); + } } return names; } QList participantIdsInOrder(const Server_Game &game) { - QList ids; - for (int id : game.getParticipants().keys()) { - ids.append(id); - } - return ids; + return game.getSeatOrder(); } } // namespace @@ -129,9 +129,9 @@ TEST_F(PlayerOrderTest, JoinOrderIsSeatOrder) Server_Game game(config, &room); ResponseContainer rc(0); - game.addPlayer(&uiA, rc, false, false); - game.addPlayer(&uiB, rc, false, false); - game.addPlayer(&uiC, rc, false, false); + game.addPlayer(&uiA, rc, RoleInfo{}, false); + game.addPlayer(&uiB, rc, RoleInfo{}, false); + game.addPlayer(&uiC, rc, RoleInfo{}, false); EXPECT_EQ(participantNamesInOrder(game), QStringList({"A", "B", "C"})); EXPECT_EQ(participantIdsInOrder(game), QList({0, 1, 2})); @@ -164,27 +164,25 @@ TEST_F(PlayerOrderTest, ReorderPlayerSeats) Server_Game game(config, &room); ResponseContainer rc(0); - game.addPlayer(&uiA, rc, false, false); - game.addPlayer(&uiB, rc, false, false); - game.addPlayer(&uiC, rc, false, false); + game.addPlayer(&uiA, rc, RoleInfo{}, false); + game.addPlayer(&uiB, rc, RoleInfo{}, false); + game.addPlayer(&uiC, rc, RoleInfo{}, false); game.reorderPlayerSeats({"C", "A", "B"}); - const QList ids = participantIdsInOrder(game); + // player ids are stable; only the seat order changes EXPECT_EQ(participantNamesInOrder(game), QStringList({"C", "A", "B"})); - EXPECT_EQ(ids.size(), 3); - EXPECT_GE(ids.at(0), 3); // re-seated players get fresh ids - EXPECT_LT(ids.at(0), ids.at(1)); - EXPECT_LT(ids.at(1), ids.at(2)); + EXPECT_EQ(participantIdsInOrder(game), QList({2, 0, 1})); + EXPECT_EQ(game.getParticipants().size(), 3); - // the host seat follows the creator - EXPECT_EQ(game.getHostId(), ids.at(1)); + // the host keeps its seat: creator A is still in the game, so host stays 0 + EXPECT_EQ(game.getHostId(), 0); - // every user interface's game list is updated to the new player id + // no re-keying means every mapping keyed by player id stays valid const int gameId = game.getGameId(); - EXPECT_EQ(uiA.getGames().value(gameId).second, ids.at(1)); - EXPECT_EQ(uiB.getGames().value(gameId).second, ids.at(2)); - EXPECT_EQ(uiC.getGames().value(gameId).second, ids.at(0)); + EXPECT_EQ(uiA.getGames().value(gameId).second, 0); + EXPECT_EQ(uiB.getGames().value(gameId).second, 1); + EXPECT_EQ(uiC.getGames().value(gameId).second, 2); } TEST_F(PlayerOrderTest, ReorderPlayerSeatsKeepsPlayerCount) @@ -212,16 +210,17 @@ TEST_F(PlayerOrderTest, ReorderPlayerSeatsKeepsPlayerCount) Server_Game game(config, &room); ResponseContainer rc(0); - game.addPlayer(&uiA, rc, false, false); - game.addPlayer(&uiB, rc, false, false); - game.addPlayer(&uiC, rc, false, false); + game.addPlayer(&uiA, rc, RoleInfo{}, false); + game.addPlayer(&uiB, rc, RoleInfo{}, false); + game.addPlayer(&uiC, rc, RoleInfo{}, false); // listing only a subset appends the remaining players in their current order game.reorderPlayerSeats({"C"}); EXPECT_EQ(participantNamesInOrder(game), QStringList({"C", "A", "B"})); + EXPECT_EQ(participantIdsInOrder(game), QList({2, 0, 1})); EXPECT_EQ(game.getPlayerCount(), 3); - EXPECT_EQ(game.getHostId(), participantIdsInOrder(game).at(1)); + EXPECT_EQ(game.getHostId(), 0); } TEST_F(PlayerOrderTest, ShufflePlayerSeatsIsAPermutation) @@ -249,9 +248,9 @@ TEST_F(PlayerOrderTest, ShufflePlayerSeatsIsAPermutation) Server_Game game(config, &room); ResponseContainer rc(0); - game.addPlayer(&uiA, rc, false, false); - game.addPlayer(&uiB, rc, false, false); - game.addPlayer(&uiC, rc, false, false); + game.addPlayer(&uiA, rc, RoleInfo{}, false); + game.addPlayer(&uiB, rc, RoleInfo{}, false); + game.addPlayer(&uiC, rc, RoleInfo{}, false); // fisher-yates on [A, B, C]: rand(0, 2) -> 1 (take B), rand(0, 1) -> 0 (take A), rand(0, 0) -> 0 (take C) auto *scriptedRng = new ScriptedRng; @@ -261,9 +260,9 @@ TEST_F(PlayerOrderTest, ShufflePlayerSeatsIsAPermutation) game.shufflePlayerSeats(); EXPECT_EQ(participantNamesInOrder(game), QStringList({"B", "A", "C"})); - const QList ids = participantIdsInOrder(game); - EXPECT_EQ(ids.size(), 3); - QSet idSet(ids.cbegin(), ids.cend()); + EXPECT_EQ(participantIdsInOrder(game), QList({1, 0, 2})); + QSet idSet(participantIdsInOrder(game).cbegin(), participantIdsInOrder(game).cend()); EXPECT_EQ(idSet.size(), 3); // ids are unique - EXPECT_EQ(game.getHostId(), ids.at(1)); + // ids are stable, so the host (creator A, id 0) is unchanged + EXPECT_EQ(game.getHostId(), 0); }