From 689f65b38ab2e1c878c7acc774fd4c85da843afe Mon Sep 17 00:00:00 2001 From: Danny Piper Date: Tue, 14 Sep 2021 21:35:47 +0100 Subject: [PATCH] Fix for poor performance with large decks (#4347) * Fix for #4284 -> The menus have the update menu thing emitted when they get triggered. | -> works surprising well https://youtu.be/KOOmhxvHA2c is a demo on a 10000ish card deck * changed my comment to make sense * fix to the issues that @ebbit1q found what caued them idk * Revert "fix to the issues that @ebbit1q found" This reverts commit 20b1ad9f7a675fd3b0d1be7452f71160ce06de71. * actual fix for the issues @ebbit1q found * its dirty but works * fix cards in zoneviews not having a menu * deleted isempty check as it is updated after the check * key binds should work now -> menus updated on zone change/attach/retranslate UI if selected * clangify * remove updateCardMenu from carditem entirely updateCardMenu is done by the player and having it in carditem led to it often being run multiple times, I've opted to instead run it in the player and remove the signal entirely the new logic updates the cardMenu every time a card is set as the activeCard in the game tab additionally a cardmenu can change while selected if the selected card: moves zone, is flipped, or is attached (it receives the unattach action) this is done in the player instead now, checking if the card is the activeCard this however exposes a flaw in the selection management where if you unselect a card the activeCard is set to nullptr, this was an existing bug and causes the action on selected cards to suddenly disappear, even if there are other cards selected! * revert null test of aCardMenu Co-authored-by: ebbit1q --- cockatrice/src/abstractcarditem.cpp | 1 - cockatrice/src/abstractcarditem.h | 1 - cockatrice/src/carditem.cpp | 32 +++++++++++++---------------- cockatrice/src/carditem.h | 3 +-- cockatrice/src/player.cpp | 22 ++++++++++---------- cockatrice/src/tab_game.cpp | 7 ++++++- cockatrice/src/tab_game.h | 5 +---- cockatrice/src/zoneviewzone.cpp | 5 +---- 8 files changed, 34 insertions(+), 42 deletions(-) diff --git a/cockatrice/src/abstractcarditem.cpp b/cockatrice/src/abstractcarditem.cpp index 5325409e..88145bfb 100644 --- a/cockatrice/src/abstractcarditem.cpp +++ b/cockatrice/src/abstractcarditem.cpp @@ -272,7 +272,6 @@ void AbstractCardItem::setFaceDown(bool _facedown) { facedown = _facedown; update(); - emit updateCardMenu(this); } void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) diff --git a/cockatrice/src/abstractcarditem.h b/cockatrice/src/abstractcarditem.h index 5367ab5b..20215ae5 100644 --- a/cockatrice/src/abstractcarditem.h +++ b/cockatrice/src/abstractcarditem.h @@ -36,7 +36,6 @@ signals: void hovered(AbstractCardItem *card); void showCardInfoPopup(QPoint pos, QString cardName); void deleteCardInfoPopup(QString cardName); - void updateCardMenu(AbstractCardItem *card); void sigPixmapUpdated(); void cardShiftClicked(QString cardName); diff --git a/cockatrice/src/carditem.cpp b/cockatrice/src/carditem.cpp index 84d09df1..42b0766e 100644 --- a/cockatrice/src/carditem.cpp +++ b/cockatrice/src/carditem.cpp @@ -23,8 +23,7 @@ CardItem::CardItem(Player *_owner, int _cardid, bool _revealedCard, QGraphicsItem *parent, - CardZone *_zone, - bool updateMenu) + CardZone *_zone) : AbstractCardItem(_name, _owner, _cardid, parent), zone(_zone), revealedCard(_revealedCard), attacking(false), destroyOnZoneChange(false), doesntUntap(false), dragItem(nullptr), attachedTo(nullptr) { @@ -35,9 +34,6 @@ CardItem::CardItem(Player *_owner, moveMenu = new QMenu; retranslateUi(); - if (updateMenu) { // avoid updating card menu too often - emit updateCardMenu(this); - } } CardItem::~CardItem() @@ -84,7 +80,6 @@ void CardItem::deleteLater() void CardItem::setZone(CardZone *_zone) { zone = _zone; - emit updateCardMenu(this); } void CardItem::retranslateUi() @@ -203,23 +198,25 @@ void CardItem::setPT(const QString &_pt) void CardItem::setAttachedTo(CardItem *_attachedTo) { - if (attachedTo) + if (attachedTo != nullptr) { attachedTo->removeAttachedCard(this); + } gridPoint.setX(-1); attachedTo = _attachedTo; - if (attachedTo) { + if (attachedTo != nullptr) { setParentItem(attachedTo->getZone()); attachedTo->addAttachedCard(this); - if (zone != attachedTo->getZone()) + if (zone != attachedTo->getZone()) { attachedTo->getZone()->reorganizeCards(); - } else + } + } else { setParentItem(zone); + } - if (zone) + if (zone != nullptr) { zone->reorganizeCards(); - - emit updateCardMenu(this); + } } void CardItem::resetState() @@ -371,8 +368,7 @@ void CardItem::playCard(bool faceDown) void CardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) { if (event->button() == Qt::RightButton) { - if (cardMenu && !cardMenu->isEmpty() && owner != nullptr) { - owner->updateCardMenu(this); + if (cardMenu != nullptr && !cardMenu->isEmpty() && owner != nullptr) { cardMenu->exec(event->screenPos()); } } else if ((event->modifiers() != Qt::AltModifier) && (event->button() == Qt::LeftButton) && @@ -437,13 +433,13 @@ bool CardItem::animationEvent() QVariant CardItem::itemChange(GraphicsItemChange change, const QVariant &value) { - if ((change == ItemSelectedHasChanged) && owner) { + if ((change == ItemSelectedHasChanged) && owner != nullptr) { if (value == true) { owner->setCardMenu(cardMenu); owner->getGame()->setActiveCard(this); } else if (owner->getCardMenu() == cardMenu) { - owner->setCardMenu(0); - owner->getGame()->setActiveCard(0); + owner->setCardMenu(nullptr); + owner->getGame()->setActiveCard(nullptr); } } return QGraphicsItem::itemChange(change, value); diff --git a/cockatrice/src/carditem.h b/cockatrice/src/carditem.h index 5c04d97f..c480f6a0 100644 --- a/cockatrice/src/carditem.h +++ b/cockatrice/src/carditem.h @@ -53,8 +53,7 @@ public: int _cardid = -1, bool revealedCard = false, QGraphicsItem *parent = nullptr, - CardZone *_zone = nullptr, - bool updateMenu = false); + CardZone *_zone = nullptr); ~CardItem(); void retranslateUi(); CardZone *getZone() const diff --git a/cockatrice/src/player.cpp b/cockatrice/src/player.cpp index a853e0e3..e50a48e5 100644 --- a/cockatrice/src/player.cpp +++ b/cockatrice/src/player.cpp @@ -1945,7 +1945,6 @@ void Player::eventSetCardCounter(const Event_SetCardCounter &event) int oldValue = card->getCounters().value(event.counter_id(), 0); card->setCounter(event.counter_id(), event.counter_value()); - updateCardMenu(card); emit logSetCardCounter(this, card->getName(), event.counter_id(), event.counter_value(), oldValue); } @@ -2014,7 +2013,7 @@ void Player::eventMoveCard(const Event_MoveCard &event, const GameEventContext & x = 0; } CardItem *card = startZone->takeCard(position, event.card_id(), startZone != targetZone); - if (!card) { + if (card == nullptr) { return; } if (startZone != targetZone) { @@ -2080,6 +2079,7 @@ void Player::eventMoveCard(const Event_MoveCard &event, const GameEventContext & i->delArrow(); } } + updateCardMenu(card); } void Player::eventFlipCard(const Event_FlipCard &event) @@ -2094,6 +2094,7 @@ void Player::eventFlipCard(const Event_FlipCard &event) } emit logFlipCard(this, card->getName(), event.face_down()); card->setFaceDown(event.face_down()); + updateCardMenu(card); } void Player::eventDestroyCard(const Event_DestroyCard &event) @@ -2111,7 +2112,7 @@ void Player::eventDestroyCard(const Event_DestroyCard &event) QList attachedCards = card->getAttachedCards(); // This list is always empty except for buggy server implementations. for (auto &attachedCard : attachedCards) { - attachedCard->setAttachedTo(0); + attachedCard->setAttachedTo(nullptr); } emit logDestroyCard(this, card->getName()); @@ -2162,6 +2163,7 @@ void Player::eventAttachCard(const Event_AttachCard &event) } else { emit logUnattachCard(this, startCard->getName()); } + updateCardMenu(startCard); } void Player::eventDrawCards(const Event_DrawCards &event) @@ -3278,17 +3280,14 @@ void Player::refreshShortcuts() { if (shortcutsActive) { setShortcutsActive(); - - for (const CardItem *cardItem : table->getCards()) { - updateCardMenu(cardItem); - } } } void Player::updateCardMenu(const CardItem *card) { // If bad card OR is a spectator (as spectators don't need card menus), return - if (card == nullptr || (game->isSpectator() && !judge)) { + // only update the menu if the card is actually selected + if (card == nullptr || (game->isSpectator() && !judge) || game->getActiveCard() != card) { return; } @@ -3521,7 +3520,7 @@ void Player::addRelatedCardActions(const CardItem *card, QMenu *cardMenu) void Player::setCardMenu(QMenu *menu) { - if (aCardMenu) { + if (aCardMenu != nullptr) { aCardMenu->setEnabled(menu != nullptr); aCardMenu->setMenu(menu); } @@ -3529,10 +3528,11 @@ void Player::setCardMenu(QMenu *menu) QMenu *Player::getCardMenu() const { - if (aCardMenu) { + if (aCardMenu != nullptr) { return aCardMenu->menu(); + } else { + return nullptr; } - return nullptr; } QString Player::getName() const diff --git a/cockatrice/src/tab_game.cpp b/cockatrice/src/tab_game.cpp index 9cd7a928..f7232a7f 100644 --- a/cockatrice/src/tab_game.cpp +++ b/cockatrice/src/tab_game.cpp @@ -1349,7 +1349,6 @@ void TabGame::newCardAdded(AbstractCardItem *card) connect(card, SIGNAL(hovered(AbstractCardItem *)), cardInfo, SLOT(setCard(AbstractCardItem *))); connect(card, SIGNAL(showCardInfoPopup(QPoint, QString)), this, SLOT(showCardInfoPopup(QPoint, QString))); connect(card, SIGNAL(deleteCardInfoPopup(QString)), this, SLOT(deleteCardInfoPopup(QString))); - connect(card, SIGNAL(updateCardMenu(AbstractCardItem *)), this, SLOT(updateCardMenu(AbstractCardItem *))); connect(card, SIGNAL(cardShiftClicked(QString)), this, SLOT(linkCardToChat(QString))); } @@ -1413,6 +1412,12 @@ Player *TabGame::getActiveLocalPlayer() const return nullptr; } +void TabGame::setActiveCard(CardItem *card) +{ + activeCard = card; + updateCardMenu(card); +} + void TabGame::updateCardMenu(AbstractCardItem *card) { Player *player; diff --git a/cockatrice/src/tab_game.h b/cockatrice/src/tab_game.h index 97c36498..0c35262c 100644 --- a/cockatrice/src/tab_game.h +++ b/cockatrice/src/tab_game.h @@ -298,10 +298,7 @@ public: Player *getActiveLocalPlayer() const; AbstractClient *getClientForPlayer(int playerId) const; - void setActiveCard(CardItem *_card) - { - activeCard = _card; - } + void setActiveCard(CardItem *card); CardItem *getActiveCard() const { return activeCard; diff --git a/cockatrice/src/zoneviewzone.cpp b/cockatrice/src/zoneviewzone.cpp index 84045231..87d561d7 100644 --- a/cockatrice/src/zoneviewzone.cpp +++ b/cockatrice/src/zoneviewzone.cpp @@ -83,15 +83,12 @@ void ZoneViewZone::initializeCards(const QList &cardLis void ZoneViewZone::zoneDumpReceived(const Response &r) { - static constexpr int MAX_VIEW_MENU = 300; const Response_DumpZone &resp = r.GetExtension(Response_DumpZone::ext); const int respCardListSize = resp.zone_info().card_list_size(); for (int i = 0; i < respCardListSize; ++i) { const ServerInfo_Card &cardInfo = resp.zone_info().card_list(i); auto cardName = QString::fromStdString(cardInfo.name()); - // stop updating card menus after MAX_VIEW_MENU cards - // this means only the first cards in the menu have actions but others can still be dragged - auto *card = new CardItem(player, cardName, cardInfo.id(), revealZone, this, this, i < MAX_VIEW_MENU); + auto *card = new CardItem(player, cardName, cardInfo.id(), revealZone, this, this); cards.insert(i, card); } reorganizeCards();