From 9a7b15d19bb28a77c75c5aa4b68e5e9c5587439b Mon Sep 17 00:00:00 2001 From: Basile Clement Date: Tue, 7 Feb 2023 22:12:04 +0000 Subject: [PATCH] Allow revealing specific cards from hand and library (#4743) Currently Cockatrice allows revealing the whole hand, or one card at random from the hand. Sometimes, a player needs to reveal a specific card from their hand instead, which is not supported. To achieve a similar effect, players usually move the corresponding card (or cards) to a public zone, then back to their hand. While this works, it is unsatisfactory (compared to a regular reveal, you can't keep the "revealed" window around, for one) and somewhat unintuitive. This patch adds a "Reveal to..." menu to cards and card selections in the player's hand or in custom zones (this includes looking at the player's library). This menu allows revealing a card or set of cards to any given player, or to all players. To implement this functionality at the protocol level, the existing RevealCards command is extended to support revealing multiple specific cards. This is done by making `card_id` a non-packed repeated field in the `Command_RevealCards` and `Event_RevealCards` protobufs. Using a non-packed repeated fields allows maintaining backwards compatibility: an empty optional field is encoded the same way as an empty non-packed list, an optional field with a value is encoded the same way as a one-element non-packed list, and when decoding a multi-elements non-packed list as an optional, only the last item in the list is read. Since the RevealCards command already exists, and due to the compatible encodings, a new client connecting to an old server can reveal a single specific card from their hand. When trying to reveal multiple cards at once, the old server will only see the request for one of the cards to be revealed, and the player will have to reveal each card separately. On the other hand, `Event_RevealedCards` already has an explicit list of cards revealed by the server, and the `card_id` field is only used when exactly one card has been revealed: thus, old and new clients will behave identically when receiving a new `Event_RevealedCards`. In particular, if a player using a new client reveals multiple cards from their hand on a new server, another player using an old client will correctly see all the revealed cards. The approach used to build the "Reveal to..." menu is slightly different from the approach used to build other player selection menus. Because the "Reveal to..." menu is specific to each card, but must also be updated whenever a player is added to or removed from the game, I chose to re-create it on the fly whenever a card is clicked, as that seemed the safest way to avoid both memory leaks and inconsistent state given my understanding of the code. --- cockatrice/src/player.cpp | 59 +++++++++++++++++++++++++--- cockatrice/src/player.h | 3 ++ common/pb/command_reveal_cards.proto | 2 +- common/pb/event_reveal_cards.proto | 2 +- common/server_player.cpp | 28 ++++++++----- 5 files changed, 76 insertions(+), 18 deletions(-) diff --git a/cockatrice/src/player.cpp b/cockatrice/src/player.cpp index fc49b9d5..560523f3 100644 --- a/cockatrice/src/player.cpp +++ b/cockatrice/src/player.cpp @@ -573,6 +573,8 @@ void Player::addPlayer(Player *player) newAction->setData(player->getId()); connect(newAction, SIGNAL(triggered()), this, SLOT(playerListActionTriggered())); } + + playersInfo.append(qMakePair(player->getName(), player->getId())); } void Player::removePlayer(Player *player) @@ -589,6 +591,14 @@ void Player::removePlayer(Player *player) j->deleteLater(); } } + + for (auto it = playersInfo.begin(); it != playersInfo.end();) { + if (it->second == player->getId()) { + it = playersInfo.erase(it); + } else { + ++it; + } + } } void Player::playerListActionTriggered() @@ -614,14 +624,14 @@ void Player::playerListActionTriggered() cmd.set_zone_name("deck"); cmd.set_top_cards(number); // backward compatibility: servers before #1051 only permits to reveal the first card - cmd.set_card_id(0); + cmd.add_card_id(0); } } else if (menu == mRevealHand) { cmd.set_zone_name("hand"); } else if (menu == mRevealRandomHandCard) { cmd.set_zone_name("hand"); - cmd.set_card_id(RANDOM_CARD_FROM_ZONE); + cmd.add_card_id(RANDOM_CARD_FROM_ZONE); } else { return; } @@ -1000,6 +1010,16 @@ void Player::initSayMenu() } } +void Player::initContextualPlayersMenu(QMenu *menu) +{ + menu->addAction(tr("&All players"))->setData(-1); + menu->addSeparator(); + + for (const auto &playerInfo : playersInfo) { + menu->addAction(playerInfo.first)->setData(playerInfo.second); + } +} + void Player::setDeck(const DeckLoader &_deck) { deck = new DeckLoader(_deck); @@ -1086,7 +1106,7 @@ void Player::actRevealRandomGraveyardCard() cmd.set_player_id(otherPlayerId); } cmd.set_zone_name("grave"); - cmd.set_card_id(RANDOM_CARD_FROM_ZONE); + cmd.add_card_id(RANDOM_CARD_FROM_ZONE); sendGameCommand(cmd); } @@ -2257,9 +2277,10 @@ void Player::eventRevealCards(const Event_RevealCards &event) } else { bool showZoneView = true; QString cardName; + auto cardId = event.card_id_size() == 0 ? -1 : event.card_id(0); if (cardList.size() == 1) { cardName = QString::fromStdString(cardList.first()->name()); - if ((event.card_id() == 0) && dynamic_cast(zone)) { + if ((cardId == 0) && dynamic_cast(zone)) { zone->getCards().first()->setName(cardName); zone->update(); showZoneView = false; @@ -2269,7 +2290,7 @@ void Player::eventRevealCards(const Event_RevealCards &event) static_cast(scene())->addRevealedZoneView(this, zone, cardList, event.grant_write_access()); } - emit logRevealCards(this, zone, event.card_id(), cardName, otherPlayer, false, + emit logRevealCards(this, zone, cardId, cardName, otherPlayer, false, event.has_number_of_cards() ? event.number_of_cards() : cardList.size()); } } @@ -2826,7 +2847,7 @@ void Player::cardMenuAction() case cmPeek: { auto *cmd = new Command_RevealCards; cmd->set_zone_name(card->getZone()->getName().toStdString()); - cmd->set_card_id(card->getId()); + cmd->add_card_id(card->getId()); cmd->set_player_id(id); commandList.append(cmd); break; @@ -3304,6 +3325,27 @@ void Player::actPlayFacedown() playCard(game->getActiveCard(), true, false); } +void Player::actReveal(QAction *action) +{ + const int otherPlayerId = action->data().toInt(); + + Command_RevealCards cmd; + if (otherPlayerId != -1) { + cmd.set_player_id(otherPlayerId); + } + + QList sel = scene()->selectedItems(); + while (!sel.isEmpty()) { + const auto *card = qgraphicsitem_cast(sel.takeFirst()); + if (!cmd.has_zone_name()) { + cmd.set_zone_name(card->getZone()->getName().toStdString()); + } + cmd.add_card_id(card->getId()); + } + + sendGameCommand(cmd); +} + void Player::refreshShortcuts() { if (shortcutsActive) { @@ -3429,6 +3471,11 @@ void Player::updateCardMenu(const CardItem *card) // Card is in hand or a custom zone specified by server cardMenu->addAction(aPlay); cardMenu->addAction(aPlayFacedown); + + QMenu *revealMenu = cardMenu->addMenu(tr("Re&veal to...")); + initContextualPlayersMenu(revealMenu); + connect(revealMenu, &QMenu::triggered, this, &Player::actReveal); + cardMenu->addMenu(moveMenu); addRelatedCardView(card, cardMenu); } diff --git a/cockatrice/src/player.h b/cockatrice/src/player.h index d4e50401..44429567 100644 --- a/cockatrice/src/player.h +++ b/cockatrice/src/player.h @@ -217,6 +217,7 @@ private slots: void actPlay(); void actHide(); void actPlayFacedown(); + void actReveal(QAction *action); void refreshShortcuts(); private: @@ -227,6 +228,7 @@ private: *bottomLibraryMenu, *rfgMenu, *playerMenu; QList playerLists; QList allPlayersActions; + QList> playersInfo; QAction *aMoveHandToTopLibrary, *aMoveHandToBottomLibrary, *aMoveHandToGrave, *aMoveHandToRfg, *aMoveGraveToTopLibrary, *aMoveGraveToBottomLibrary, *aMoveGraveToHand, *aMoveGraveToRfg, *aMoveRfgToTopLibrary, *aMoveRfgToBottomLibrary, *aMoveRfgToHand, *aMoveRfgToGrave, *aViewHand, *aViewLibrary, *aViewTopCards, @@ -297,6 +299,7 @@ private: void rearrangeCounters(); void initSayMenu(); + void initContextualPlayersMenu(QMenu *menu); // void eventConnectionStateChanged(const Event_ConnectionStateChanged &event); void eventGameSay(const Event_GameSay &event); diff --git a/common/pb/command_reveal_cards.proto b/common/pb/command_reveal_cards.proto index 5c4024ba..82216911 100644 --- a/common/pb/command_reveal_cards.proto +++ b/common/pb/command_reveal_cards.proto @@ -5,7 +5,7 @@ message Command_RevealCards { optional Command_RevealCards ext = 1026; } optional string zone_name = 1; - optional sint32 card_id = 2 [default = -1]; + repeated sint32 card_id = 2 [packed = false]; optional sint32 player_id = 3 [default = -1]; optional bool grant_write_access = 4; optional sint32 top_cards = 5 [default = -1]; diff --git a/common/pb/event_reveal_cards.proto b/common/pb/event_reveal_cards.proto index b525093a..a64276c3 100644 --- a/common/pb/event_reveal_cards.proto +++ b/common/pb/event_reveal_cards.proto @@ -7,7 +7,7 @@ message Event_RevealCards { optional Event_RevealCards ext = 2006; } optional string zone_name = 1; - optional sint32 card_id = 2 [default = -1]; + repeated sint32 card_id = 2 [packed = false]; optional sint32 other_player_id = 3 [default = -1]; repeated ServerInfo_Card cards = 4; optional bool grant_write_access = 5; diff --git a/common/server_player.cpp b/common/server_player.cpp index ea8a6de7..5cd95ce9 100644 --- a/common/server_player.cpp +++ b/common/server_player.cpp @@ -354,7 +354,7 @@ void Server_Player::revealTopCardIfNeeded(Server_CardZone *zone, GameEventStorag if (zone->getAlwaysRevealTopCard()) { Event_RevealCards revealEvent; revealEvent.set_zone_name(zone->getName().toStdString()); - revealEvent.set_card_id(0); + revealEvent.add_card_id(0); zone->getCards().first()->getInfo(revealEvent.add_cards()); ges.enqueueGameEvent(revealEvent, playerId); @@ -370,7 +370,7 @@ void Server_Player::revealTopCardIfNeeded(Server_CardZone *zone, GameEventStorag Event_RevealCards revealEvent; revealEvent.set_zone_name(zone->getName().toStdString()); revealEvent.set_number_of_cards(1); - revealEvent.set_card_id(0); + revealEvent.add_card_id(0); zone->getCards().first()->getInfo(revealEvent.add_cards()); ges.enqueueGameEvent(revealEvent, playerId, GameEventStorageItem::SendToPrivate, playerId); } @@ -1843,27 +1843,35 @@ Server_Player::cmdRevealCards(const Command_RevealCards &cmd, ResponseContainer } cardsToReveal.append(card); } - } else if (!cmd.has_card_id()) { + } else if (cmd.card_id_size() == 0) { cardsToReveal = zone->getCards(); - } else if (cmd.card_id() == -2) { + } else if (cmd.card_id_size() == 1 && cmd.card_id(0) == -2) { + // If there is a single card_id with value -2 (ie + // Player::RANDOM_CARD_FROM_ZONE), pick a random card. + // + // This is to be compatible with clients supporting a single card_id + // value, which send value -2 to request a random card. if (zone->getCards().isEmpty()) { return Response::RespContextError; } + cardsToReveal.append(zone->getCards().at(rng->rand(0, zone->getCards().size() - 1))); } else { - Server_Card *card = zone->getCard(cmd.card_id()); - if (!card) { - return Response::RespNameNotFound; + for (auto cardId : cmd.card_id()) { + Server_Card *card = zone->getCard(cardId); + if (!card) { + return Response::RespNameNotFound; + } + cardsToReveal.append(card); } - cardsToReveal.append(card); } Event_RevealCards eventOthers; eventOthers.set_grant_write_access(cmd.grant_write_access()); eventOthers.set_zone_name(zone->getName().toStdString()); eventOthers.set_number_of_cards(cardsToReveal.size()); - if (cmd.has_card_id()) { - eventOthers.set_card_id(cmd.card_id()); + for (auto cardId : cmd.card_id()) { + eventOthers.add_card_id(cardId); } if (cmd.has_player_id()) { eventOthers.set_other_player_id(cmd.player_id());