From 9c60ae98d4e0571512b994639342b3952dff00f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= <44814898+BruebachL@users.noreply.github.com> Date: Sun, 8 Nov 2026 09:51:31 +0100 Subject: [PATCH] Extract the shared reveal helper and name the test's CardRef fields RickyRister asked for cmdRevealCards and cmdRevealRandomCards to share a helper instead of threading a randomCount parameter through the reveal virtual. Restore the three-argument cmdRevealCards signature, resolve the reveal preconditions and zone in resolveRevealZone, and let both commands build their card list and hand it to revealCards, which owns the private and public Event_RevealCards construction and the enqueue. The random command no longer fabricates a Command_RevealCards with the -2 sentinel to re-enter the legacy path; its behaviour is unchanged. Name the CardRef fields in random_reveal_test instead of relying on declaration order, and state in the user documentation that a rejected reveal is not reported in the client, matching what the client does. --- .../gameplay/revealing_cards.md | 4 +- .../game/server_abstract_participant.cpp | 3 +- .../remote/game/server_abstract_participant.h | 2 +- .../remote/game/server_abstract_player.cpp | 122 +++++++++++------- .../remote/game/server_abstract_player.h | 18 ++- tests/random_reveal_test.cpp | 3 +- 6 files changed, 98 insertions(+), 54 deletions(-) diff --git a/doc/doxygen/pages/user_documentation/gameplay/revealing_cards.md b/doc/doxygen/pages/user_documentation/gameplay/revealing_cards.md index 3e6252511..5d47bd3b4 100644 --- a/doc/doxygen/pages/user_documentation/gameplay/revealing_cards.md +++ b/doc/doxygen/pages/user_documentation/gameplay/revealing_cards.md @@ -6,6 +6,6 @@ The server chooses distinct cards without replacement. Two copies of the same ca When you choose a specific player, that player receives the selected card identities. Judges and spectators configured to see all hidden information retain their usual access. Ordinary participants other than the chosen player, including the hand's owner, receive a notice with the count and recipient, but do not learn which cards were chosen. Revealing to all players sends the selected identities to everyone, using the ordinary reveal window. -If the hand becomes too small or the recipient leaves before the server processes the request, the request fails without a partial reveal. Cancelling the count dialog sends nothing. +If the hand becomes too small or the recipient leaves before the server processes the request, the server rejects the request and no cards are revealed. Cancelling the count dialog sends nothing. -This operation requires a server that supports multiple random reveals. An older server rejects it without revealing any cards. The existing **Reveal random card to...** action and its shortcut remain available for single-card reveals. +This operation requires a server that supports multiple random reveals. An older server rejects it without revealing any cards. Neither rejection is reported in the client; the reveal simply does not happen. The existing **Reveal random card to...** action and its shortcut remain available for single-card reveals. diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.cpp index f23917e6f..f7739bc85 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.cpp @@ -400,8 +400,7 @@ Response::ResponseCode Server_AbstractParticipant::cmdDumpZone(const Command_Dum Response::ResponseCode Server_AbstractParticipant::cmdRevealCards(const Command_RevealCards & /*cmd*/, ResponseContainer & /*rc*/, - GameEventStorage & /*ges*/, - int /*randomCount*/) + GameEventStorage & /*ges*/) { return Response::RespFunctionNotAllowed; } diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.h index efba983b5..6b2a93e48 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_participant.h @@ -175,7 +175,7 @@ public: virtual Response::ResponseCode cmdDumpZone(const Command_DumpZone &cmd, ResponseContainer &rc, GameEventStorage &ges); virtual Response::ResponseCode - cmdRevealCards(const Command_RevealCards &cmd, ResponseContainer &rc, GameEventStorage &ges, int randomCount = 1); + cmdRevealCards(const Command_RevealCards &cmd, ResponseContainer &rc, GameEventStorage &ges); virtual Response::ResponseCode cmdRevealRandomCards(const Command_RevealRandomCards &cmd, ResponseContainer &rc, GameEventStorage &ges); virtual Response::ResponseCode diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp index f6288960f..b9617b547 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp @@ -1494,26 +1494,10 @@ Server_AbstractPlayer::cmdDumpZone(const Command_DumpZone &cmd, ResponseContaine return Response::RespOk; } -Response::ResponseCode Server_AbstractPlayer::cmdRevealRandomCards(const Command_RevealRandomCards &cmd, - ResponseContainer &rc, - GameEventStorage &ges) -{ - if (cmd.number_of_cards() <= 0) { - return Response::RespInvalidData; - } - Command_RevealCards reveal; - reveal.set_zone_name(cmd.zone_name()); - reveal.add_card_id(-2); - if (cmd.has_player_id()) { - reveal.set_player_id(cmd.player_id()); - } - return cmdRevealCards(reveal, rc, ges, cmd.number_of_cards()); -} - -Response::ResponseCode Server_AbstractPlayer::cmdRevealCards(const Command_RevealCards &cmd, - ResponseContainer & /*rc*/, - GameEventStorage &ges, - int randomCount) +Response::ResponseCode Server_AbstractPlayer::resolveRevealZone(const std::string &zoneName, + bool hasRecipient, + int recipientId, + Server_CardZone *&zone) { if (!game->getGameStarted()) { return Response::RespGameNotStarted; @@ -1522,17 +1506,61 @@ Response::ResponseCode Server_AbstractPlayer::cmdRevealCards(const Command_Revea return Response::RespContextError; } - if (cmd.has_player_id()) { - Server_AbstractPlayer *otherPlayer = game->getPlayer(cmd.player_id()); - if (!otherPlayer) { - return Response::RespNameNotFound; - } + if (hasRecipient && !game->getPlayer(recipientId)) { + return Response::RespNameNotFound; } - Server_CardZone *zone = zones.value(nameFromStdString(cmd.zone_name())); + + zone = zones.value(nameFromStdString(zoneName)); if (!zone) { return Response::RespNameNotFound; } + return Response::RespOk; +} + +Response::ResponseCode Server_AbstractPlayer::cmdRevealRandomCards(const Command_RevealRandomCards &cmd, + ResponseContainer & /*rc*/, + GameEventStorage &ges) +{ + if (cmd.number_of_cards() <= 0) { + return Response::RespInvalidData; + } + + Server_CardZone *zone = nullptr; + const Response::ResponseCode result = + resolveRevealZone(cmd.zone_name(), cmd.has_player_id(), cmd.player_id(), zone); + if (result != Response::RespOk) { + return result; + } + + auto candidates = zone->getCards(); + if (cmd.number_of_cards() > candidates.size()) { + return Response::RespContextError; + } + + // Partial Fisher-Yates on a copy: each physical card can appear only once, + // and the hand's order and contents stay unchanged. + QList cardsToReveal; + for (int i = 0; i < cmd.number_of_cards(); ++i) { + const int selected = rng->rand(i, candidates.size() - 1); + candidates.swapItemsAt(i, selected); + cardsToReveal.append(candidates.at(i)); + } + + // -2 in the public event tells other participants only how many cards were revealed. + return revealCards(zone, cardsToReveal, {-2}, cmd.has_player_id(), cmd.player_id(), false, ges); +} + +Response::ResponseCode +Server_AbstractPlayer::cmdRevealCards(const Command_RevealCards &cmd, ResponseContainer & /*rc*/, GameEventStorage &ges) +{ + Server_CardZone *zone = nullptr; + const Response::ResponseCode result = + resolveRevealZone(cmd.zone_name(), cmd.has_player_id(), cmd.player_id(), zone); + if (result != Response::RespOk) { + return result; + } + QList cardsToReveal; if (cmd.top_cards() != -1) { for (int i = 0; i < cmd.top_cards(); i++) { @@ -1550,18 +1578,11 @@ Response::ResponseCode Server_AbstractPlayer::cmdRevealCards(const Command_Revea // // This is to be compatible with clients supporting a single card_id // value, which send value -2 to request a random card. - if (randomCount <= 0 || randomCount > zone->getCards().size()) { + if (zone->getCards().isEmpty()) { return Response::RespContextError; } - // Partial Fisher-Yates on a copy: each physical card can appear only once, - // and the hand's order and contents stay unchanged. Legacy requests use one. - auto candidates = zone->getCards(); - for (int i = 0; i < randomCount; ++i) { - const int selected = rng->rand(i, candidates.size() - 1); - candidates.swapItemsAt(i, selected); - cardsToReveal.append(candidates.at(i)); - } + cardsToReveal.append(zone->getCards().at(rng->rand(0, zone->getCards().size() - 1))); } else { for (auto cardId : cmd.card_id()) { Server_Card *card = zone->getCard(cardId); @@ -1572,15 +1593,28 @@ Response::ResponseCode Server_AbstractPlayer::cmdRevealCards(const Command_Revea } } + const QList cardIdsForOthers(cmd.card_id().begin(), cmd.card_id().end()); + return revealCards(zone, cardsToReveal, cardIdsForOthers, cmd.has_player_id(), cmd.player_id(), + cmd.grant_write_access(), ges); +} + +Response::ResponseCode Server_AbstractPlayer::revealCards(Server_CardZone *zone, + const QList &cardsToReveal, + const QList &cardIdsForOthers, + bool hasRecipient, + int recipientId, + bool grantWriteAccess, + GameEventStorage &ges) +{ Event_RevealCards eventOthers; - eventOthers.set_grant_write_access(cmd.grant_write_access()); + eventOthers.set_grant_write_access(grantWriteAccess); eventOthers.set_zone_name(zone->getName().toStdString()); eventOthers.set_number_of_cards(cardsToReveal.size()); - for (auto cardId : cmd.card_id()) { + for (int cardId : cardIdsForOthers) { eventOthers.add_card_id(cardId); } - if (cmd.has_player_id()) { - eventOthers.set_other_player_id(cmd.player_id()); + if (hasRecipient) { + eventOthers.set_other_player_id(recipientId); } Event_RevealCards eventPrivate(eventOthers); @@ -1617,19 +1651,19 @@ Response::ResponseCode Server_AbstractPlayer::cmdRevealCards(const Command_Revea } } - if (cmd.has_player_id()) { - if (cmd.grant_write_access()) { - zone->addWritePermission(cmd.player_id()); + if (hasRecipient) { + if (grantWriteAccess) { + zone->addWritePermission(recipientId); } if (isJudge()) { ges.setOverwriteOwnership(true); } - ges.enqueueGameEvent(eventPrivate, playerId, GameEventStorageItem::SendToPrivate, cmd.player_id()); + ges.enqueueGameEvent(eventPrivate, playerId, GameEventStorageItem::SendToPrivate, recipientId); ges.enqueueGameEvent(eventOthers, playerId, GameEventStorageItem::SendToOthers); } else { - if (cmd.grant_write_access()) { + if (grantWriteAccess) { const QList &participantIds = game->getParticipants().keys(); for (int anyParticipantId : participantIds) { zone->addWritePermission(anyParticipantId); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h index 17b4016ad..c92c4c3dd 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h @@ -32,6 +32,18 @@ private: void sendCreateTokenEvents(Server_CardZone *zone, Server_Card *card, int xCoord, int yCoord, GameEventStorage &ges); void getPlayerProperties(ServerInfo_PlayerProperties &result) override; + /// Validates the preconditions shared by every reveal and resolves the source zone. + Response::ResponseCode + resolveRevealZone(const std::string &zoneName, bool hasRecipient, int recipientId, Server_CardZone *&zone); + /// Builds the private and public reveal events for an already chosen set of cards and enqueues them. + Response::ResponseCode revealCards(Server_CardZone *zone, + const QList &cardsToReveal, + const QList &cardIdsForOthers, + bool hasRecipient, + int recipientId, + bool grantWriteAccess, + GameEventStorage &ges); + protected: bool conceded; DeckList *deck; @@ -161,10 +173,8 @@ public: cmdIncCardCounter(const Command_IncCardCounter &cmd, ResponseContainer &rc, GameEventStorage &ges) override; virtual Response::ResponseCode cmdDumpZone(const Command_DumpZone &cmd, ResponseContainer &rc, GameEventStorage &ges) override; - virtual Response::ResponseCode cmdRevealCards(const Command_RevealCards &cmd, - ResponseContainer &rc, - GameEventStorage &ges, - int randomCount = 1) override; + virtual Response::ResponseCode + cmdRevealCards(const Command_RevealCards &cmd, ResponseContainer &rc, GameEventStorage &ges) override; Response::ResponseCode cmdRevealRandomCards(const Command_RevealRandomCards &cmd, ResponseContainer &rc, GameEventStorage &ges) override; virtual Response::ResponseCode cmdChangeZoneProperties(const Command_ChangeZoneProperties &cmd, diff --git a/tests/random_reveal_test.cpp b/tests/random_reveal_test.cpp index b42584361..62defe183 100644 --- a/tests/random_reveal_test.cpp +++ b/tests/random_reveal_test.cpp @@ -95,7 +95,8 @@ protected: player = game->getPlayer(0); hand = player->getZones().value(ZoneNames::HAND); for (int i = 0; i < 4; ++i) { - hand->insertCard(new Server_Card({"Twin", QString::number(i)}, player->newCardId(), i, 0), -1, 0); + hand->insertCard( + new Server_Card({.name = "Twin", .providerId = QString::number(i)}, player->newCardId(), i, 0), -1, 0); } }