Clean up game scene code.

Took 18 minutes

cockatrice/src/game/game_scene.cpp

@@ -15,6 +15,7 @@
 #include <QGraphicsView>
 #include <QSet>
 #include <QtMath>
+#include <numeric>
 
 GameScene::GameScene(PhasesToolbar *_phasesToolbar, QObject *parent)
     : QGraphicsScene(parent), phasesToolbar(_phasesToolbar), viewSize(QSize()), playerRotation(0)
@@ -32,8 +33,6 @@ GameScene::~GameScene()
     delete animationTimer;
 
     // DO NOT call clearViews() here
-    // clearViews calls close() on the zoneViews, which sends signals; sending signals in destructors leads to segfaults
-    // deleteLater() deletes the zoneView without allowing it to send signals
     for (const auto &zoneView : zoneViews) {
         zoneView->deleteLater();
     }
@@ -41,13 +40,14 @@ GameScene::~GameScene()
 
 void GameScene::retranslateUi()
 {
-    for (int i = 0; i < zoneViews.size(); ++i)
+    for (ZoneViewWidget *view : zoneViews)
-        zoneViews[i]->retranslateUi();
+        view->retranslateUi();
 }
 
 void GameScene::addPlayer(Player *player)
 {
     qCInfo(GameScenePlayerAdditionRemovalLog) << "GameScene::addPlayer name=" << player->getPlayerInfo()->getName();
+
     players << player->getGraphicsItem();
     addItem(player->getGraphicsItem());
     connect(player->getGraphicsItem(), &PlayerGraphicsItem::sizeChanged, this, &GameScene::rearrange);
@@ -55,7 +55,9 @@ void GameScene::addPlayer(Player *player)
 
 void GameScene::removePlayer(Player *player)
 {
-    qCInfo(GameScenePlayerAdditionRemovalLog) << "GameScene::removePlayer name=" << player->getPlayerInfo()->getName();
+    qCInfo(GameScenePlayerAdditionRemovalLog)
+        << "GameScene::removePlayer name=" << player->getPlayerInfo()->getName();
+
     for (ZoneViewWidget *zone : zoneViews) {
         if (zone->getPlayer() == player) {
             zone->close();
@@ -74,104 +76,50 @@ void GameScene::adjustPlayerRotation(int rotationAdjustment)
 
 void GameScene::rearrange()
 {
-    playersByColumn.clear();
-
-    // Create the list of players playing, noting the first player's index.
-    QList<Player *> playersPlaying;
     int firstPlayerIndex = 0;
-    bool firstPlayerFound = false;
+    auto playersPlaying = collectActivePlayers(firstPlayerIndex);
-    QListIterator<PlayerGraphicsItem *> playersIter(players);
+    playersPlaying = rotatePlayers(playersPlaying, firstPlayerIndex);
-    while (playersIter.hasNext()) {
-        Player *p = playersIter.next()->getPlayer();
-        if (p && !p->getConceded()) {
-            playersPlaying.append(p);
-            if (!firstPlayerFound && p->getPlayerInfo()->getLocal()) {
-                firstPlayerIndex = playersPlaying.size() - 1;
-                firstPlayerFound = true;
-            }
-        }
-    }
 
-    // Rotate the players playing list so that first player is first, then
+    int columns = determineColumnCount(playersPlaying.size());
-    // adjust by the additional rotation setting.
+    QSizeF sceneSize = computeSceneSizeAndPlayerLayout(playersPlaying, columns);
-    if (!playersPlaying.isEmpty()) {
-        int totalRotation = firstPlayerIndex + playerRotation;
-        while (totalRotation < 0)
-            totalRotation += playersPlaying.size();
-        for (int i = 0; i < totalRotation; ++i) {
-            playersPlaying.append(playersPlaying.takeFirst());
-        }
-    }
 
-    const int playersCount = playersPlaying.size();
+    phasesToolbar->setHeight(sceneSize.height());
-    const int columns = playersCount < SettingsCache::instance().getMinPlayersForMultiColumnLayout() ? 1 : 2;
+    setSceneRect(0, 0, sceneSize.width(), sceneSize.height());
-    const int rows = qCeil((qreal)playersCount / columns);
-    qreal sceneHeight = 0, sceneWidth = -playerAreaSpacing;
-    QList<int> columnWidth;
 
-    QListIterator<Player *> playersPlayingIter(playersPlaying);
-    for (int col = 0; col < columns; ++col) {
-        playersByColumn.append(QList<PlayerGraphicsItem *>());
-        columnWidth.append(0);
-        qreal thisColumnHeight = -playerAreaSpacing;
-        const int rowsInColumn = rows - (playersCount % columns) * col; // only correct for max. 2 cols
-        for (int j = 0; j < rowsInColumn; ++j) {
-            Player *player = playersPlayingIter.next();
-            if (col == 0)
-                playersByColumn[col].prepend(player->getGraphicsItem());
-            else
-                playersByColumn[col].append(player->getGraphicsItem());
-            thisColumnHeight += player->getGraphicsItem()->boundingRect().height() + playerAreaSpacing;
-            if (player->getGraphicsItem()->boundingRect().width() > columnWidth[col])
-                columnWidth[col] = player->getGraphicsItem()->boundingRect().width();
-        }
-        if (thisColumnHeight > sceneHeight)
-            sceneHeight = thisColumnHeight;
-        sceneWidth += columnWidth[col] + playerAreaSpacing;
-    }
-
-    phasesToolbar->setHeight(sceneHeight);
-    qreal phasesWidth = phasesToolbar->getWidth();
-    sceneWidth += phasesWidth;
-
-    qreal x = phasesWidth;
-    for (int col = 0; col < columns; ++col) {
-        qreal y = 0;
-        for (int row = 0; row < playersByColumn[col].size(); ++row) {
-            PlayerGraphicsItem *player = playersByColumn[col][row];
-            player->setPos(x, y);
-            player->setMirrored(row != rows - 1);
-            y += player->boundingRect().height() + playerAreaSpacing;
-        }
-        x += columnWidth[col] + playerAreaSpacing;
-    }
-
-    setSceneRect(sceneRect().x(), sceneRect().y(), sceneWidth, sceneHeight);
     processViewSizeChange(viewSize);
 }
 
+// ---------- Zone Views ----------
+
 void GameScene::toggleZoneView(Player *player, const QString &zoneName, int numberCards, bool isReversed)
 {
     for (auto &view : zoneViews) {
         ZoneViewZone *temp = view->getZone();
-        if (temp->getLogic()->getName() == zoneName && temp->getLogic()->getPlayer() == player &&
+        if (temp->getLogic()->getName() == zoneName &&
+            temp->getLogic()->getPlayer() == player &&
             qobject_cast<ZoneViewZoneLogic *>(temp->getLogic())->getNumberCards() == numberCards) {
             view->close();
         }
     }
 
     ZoneViewWidget *item =
-        new ZoneViewWidget(player, player->getZones().value(zoneName), numberCards, false, false, {}, isReversed);
+        new ZoneViewWidget(
+        player,
+        player->getZones().value(zoneName),
+        numberCards,
+        false,
+        false, {}, isReversed);
+
     zoneViews.append(item);
     connect(item, &ZoneViewWidget::closePressed, this, &GameScene::removeZoneView);
     addItem(item);
-    if (zoneName == "grave") {
+
+    if (zoneName == "grave")
         item->setPos(360, 100);
-    } else if (zoneName == "rfg") {
+    else if (zoneName == "rfg")
         item->setPos(380, 120);
-    } else {
+    else
         item->setPos(340, 80);
-    }
 }
 
 void GameScene::addRevealedZoneView(Player *player,
@@ -204,6 +152,8 @@ void GameScene::closeMostRecentZoneView()
         zoneViews.last()->close();
 }
 
+// ---------- View Transforms ----------
+
 QTransform GameScene::getViewTransform() const
 {
     return views().at(0)->transform();
@@ -214,83 +164,35 @@ QTransform GameScene::getViewportTransform() const
     return views().at(0)->viewportTransform();
 }
 
+// ---------- View Size ----------
+
 void GameScene::processViewSizeChange(const QSize &newSize)
 {
     viewSize = newSize;
 
-    qreal newRatio = ((qreal)newSize.width()) / newSize.height();
+    QList<qreal> minWidthByColumn = calculateMinWidthByColumn();
-    qreal minWidth = 0;
+    qreal minWidth = std::accumulate(minWidthByColumn.begin(),
-    QList<qreal> minWidthByColumn;
+                                     minWidthByColumn.end(), phasesToolbar->getWidth());
-    for (int col = 0; col < playersByColumn.size(); ++col) {
-        minWidthByColumn.append(0);
-        for (int row = 0; row < playersByColumn[col].size(); ++row) {
-            qreal w = playersByColumn[col][row]->getMinimumWidth();
-            if (w > minWidthByColumn[col])
-                minWidthByColumn[col] = w;
-        }
-        minWidth += minWidthByColumn[col];
-    }
-    minWidth += phasesToolbar->getWidth();
 
-    qreal minRatio = minWidth / sceneRect().height();
+    qreal newWidth = calculateNewSceneWidth(newSize, minWidth);
-    qreal newWidth;
-    if (minRatio > newRatio) {
-        // Aspect ratio is dominated by table width.
-        newWidth = minWidth;
-    } else {
-        // Aspect ratio is dominated by window dimensions.
-        newWidth = newRatio * sceneRect().height();
-    }
     setSceneRect(0, 0, newWidth, sceneRect().height());
 
-    qreal extraWidthPerColumn = (newWidth - minWidth) / playersByColumn.size();
+    resizeColumnsAndPlayers(minWidthByColumn, newWidth);
-    qreal newx = phasesToolbar->getWidth();
-    for (int col = 0; col < playersByColumn.size(); ++col) {
-        for (int row = 0; row < playersByColumn[col].size(); ++row) {
-            playersByColumn[col][row]->processSceneSizeChange(minWidthByColumn[col] + extraWidthPerColumn);
-            playersByColumn[col][row]->setPos(newx, playersByColumn[col][row]->y());
-        }
-        newx += minWidthByColumn[col] + extraWidthPerColumn;
-    }
 }
 
+// ---------- Hover Handling ----------
+
 void GameScene::updateHover(const QPointF &scenePos)
 {
-    QList<QGraphicsItem *> itemList =
+    auto itemList = items(scenePos, Qt::IntersectsItemBoundingRect, Qt::DescendingOrder, getViewTransform());
-        items(scenePos, Qt::IntersectsItemBoundingRect, Qt::DescendingOrder, getViewTransform());
 
-    // Search for the topmost zone and ignore all cards not belonging to that zone.
+    CardZone *zone = findTopmostZone(itemList);
-    CardZone *zone = 0;
+    CardItem *topCard = zone ? findTopmostCardInZone(itemList, zone) : nullptr;
-    for (int i = 0; i < itemList.size(); ++i)
+    updateHoveredCard(topCard);
-        if ((zone = qgraphicsitem_cast<CardZone *>(itemList[i])))
-            break;
-
-    CardItem *maxZCard = 0;
-    if (zone) {
-        qreal maxZ = -1;
-        for (int i = 0; i < itemList.size(); ++i) {
-            CardItem *card = qgraphicsitem_cast<CardItem *>(itemList[i]);
-            if (!card)
-                continue;
-            if (card->getAttachedTo()) {
-                if (card->getAttachedTo()->getZone() != zone->getLogic())
-                    continue;
-            } else if (card->getZone() != zone->getLogic())
-                continue;
-
-            if (card->getRealZValue() > maxZ) {
-                maxZ = card->getRealZValue();
-                maxZCard = card;
-            }
-        }
-    }
-    if (hoveredCard && (maxZCard != hoveredCard))
-        hoveredCard->setHovered(false);
-    if (maxZCard && (maxZCard != hoveredCard))
-        maxZCard->setHovered(true);
-    hoveredCard = maxZCard;
 }
 
+// ---------- Event Handling ----------
+
 bool GameScene::event(QEvent *event)
 {
     if (event->type() == QEvent::GraphicsSceneMouseMove)
@@ -325,6 +227,8 @@ void GameScene::unregisterAnimationItem(AbstractCardItem *card)
         animationTimer->stop();
 }
 
+// ---------- Rubber Band ----------
+
 void GameScene::startRubberBand(const QPointF &selectionOrigin)
 {
     emit sigStartRubberBand(selectionOrigin);
@@ -339,3 +243,175 @@ void GameScene::stopRubberBand()
 {
     emit sigStopRubberBand();
 }
+
+// ==================================================================
+//                      Private Helpers
+// ==================================================================
+
+QList<Player *> GameScene::collectActivePlayers(int &firstPlayerIndex) const
+{
+    QList<Player *> activePlayers;
+    firstPlayerIndex = 0;
+    bool firstPlayerFound = false;
+
+    for (auto *pgItem : players) {
+        Player *p = pgItem->getPlayer();
+        if (p && !p->getConceded()) {
+            activePlayers.append(p);
+            if (!firstPlayerFound && p->getPlayerInfo()->getLocal()) {
+                firstPlayerIndex = activePlayers.size() - 1;
+                firstPlayerFound = true;
+            }
+        }
+    }
+    return activePlayers;
+}
+
+QList<Player *> GameScene::rotatePlayers(const QList<Player *> &players, int firstPlayerIndex) const
+{
+    QList<Player *> rotated = players;
+    if (!rotated.isEmpty()) {
+        int totalRotation = firstPlayerIndex + playerRotation;
+        while (totalRotation < 0)
+            totalRotation += rotated.size();
+        for (int i = 0; i < totalRotation; ++i)
+            rotated.append(rotated.takeFirst());
+    }
+    return rotated;
+}
+
+int GameScene::determineColumnCount(int playerCount) const
+{
+    return playerCount < SettingsCache::instance().getMinPlayersForMultiColumnLayout() ? 1 : 2;
+}
+
+QSizeF GameScene::computeSceneSizeAndPlayerLayout(const QList<Player *> &playersPlaying, int columns)
+{
+    playersByColumn.clear();
+
+    int rows = qCeil((qreal)playersPlaying.size() / columns);
+    qreal sceneHeight = 0, sceneWidth = -playerAreaSpacing;
+    QList<int> columnWidth;
+
+    QListIterator<Player *> playersIter(playersPlaying);
+    for (int col = 0; col < columns; ++col) {
+        playersByColumn.append(QList<PlayerGraphicsItem *>());
+        columnWidth.append(0);
+        qreal thisColumnHeight = -playerAreaSpacing;
+        int rowsInColumn = rows - (playersPlaying.size() % columns) * col; // for 2 cols
+
+        for (int j = 0; j < rowsInColumn; ++j) {
+            Player *player = playersIter.next();
+            if (col == 0)
+                playersByColumn[col].prepend(player->getGraphicsItem());
+            else
+                playersByColumn[col].append(player->getGraphicsItem());
+
+            auto *pgItem = player->getGraphicsItem();
+            thisColumnHeight += pgItem->boundingRect().height() + playerAreaSpacing;
+            columnWidth[col] = std::max(columnWidth[col], (int)pgItem->boundingRect().width());
+        }
+
+        sceneHeight = std::max(sceneHeight, thisColumnHeight);
+        sceneWidth += columnWidth[col] + playerAreaSpacing;
+    }
+
+    qreal phasesWidth = phasesToolbar->getWidth();
+    sceneWidth += phasesWidth;
+
+    // Position players
+    qreal x = phasesWidth;
+    for (int col = 0; col < columns; ++col) {
+        qreal y = 0;
+        for (int row = 0; row < playersByColumn[col].size(); ++row) {
+            PlayerGraphicsItem *player = playersByColumn[col][row];
+            player->setPos(x, y);
+            player->setMirrored(row != rows - 1);
+            y += player->boundingRect().height() + playerAreaSpacing;
+        }
+        x += columnWidth[col] + playerAreaSpacing;
+    }
+
+    return QSizeF(sceneWidth, sceneHeight);
+}
+
+QList<qreal> GameScene::calculateMinWidthByColumn() const
+{
+    QList<qreal> minWidthByColumn;
+    for (const auto &col : playersByColumn) {
+        qreal maxWidth = 0;
+        for (PlayerGraphicsItem *player : col)
+            maxWidth = std::max(maxWidth, player->getMinimumWidth());
+        minWidthByColumn.append(maxWidth);
+    }
+    return minWidthByColumn;
+}
+
+qreal GameScene::calculateNewSceneWidth(const QSize &newSize, qreal minWidth) const
+{
+    qreal newRatio = (qreal)newSize.width() / newSize.height();
+    qreal minRatio = minWidth / sceneRect().height();
+
+    if (minRatio > newRatio) {
+        return minWidth; // dominated by table width
+    } else {
+        return newRatio * sceneRect().height(); // dominated by window dimensions
+    }
+}
+
+void GameScene::resizeColumnsAndPlayers(const QList<qreal> &minWidthByColumn, qreal newWidth)
+{
+    qreal minWidth = std::accumulate(minWidthByColumn.begin(), minWidthByColumn.end(), phasesToolbar->getWidth());
+
+    qreal extraWidthPerColumn = (newWidth - minWidth) / playersByColumn.size();
+    qreal newx = phasesToolbar->getWidth();
+
+    for (int col = 0; col < playersByColumn.size(); ++col) {
+        for (PlayerGraphicsItem *player : playersByColumn[col]) {
+            player->processSceneSizeChange(minWidthByColumn[col] + extraWidthPerColumn);
+            player->setPos(newx, player->y());
+        }
+        newx += minWidthByColumn[col] + extraWidthPerColumn;
+    }
+}
+
+CardZone *GameScene::findTopmostZone(const QList<QGraphicsItem *> &items) const
+{
+    for (QGraphicsItem *item : items)
+        if (auto *zone = qgraphicsitem_cast<CardZone *>(item))
+            return zone;
+    return nullptr;
+}
+
+CardItem *GameScene::findTopmostCardInZone(const QList<QGraphicsItem *> &items, CardZone *zone) const
+{
+    CardItem *maxZCard = nullptr;
+    qreal maxZ = -1;
+
+    for (QGraphicsItem *item : items) {
+        CardItem *card = qgraphicsitem_cast<CardItem *>(item);
+        if (!card)
+            continue;
+
+        if (card->getAttachedTo()) {
+            if (card->getAttachedTo()->getZone() != zone->getLogic())
+                continue;
+        } else if (card->getZone() != zone->getLogic())
+            continue;
+
+        if (card->getRealZValue() > maxZ) {
+            maxZ = card->getRealZValue();
+            maxZCard = card;
+        }
+    }
+    return maxZCard;
+}
+
+void GameScene::updateHoveredCard(CardItem *newCard)
+{
+    if (hoveredCard && (newCard != hoveredCard))
+        hoveredCard->setHovered(false);
+    if (newCard && (newCard != hoveredCard))
+        newCard->setHovered(true);
+    hoveredCard = newCard;
+}

cockatrice/src/game/game_scene.h

@@ -50,6 +50,16 @@ public:
     void startRubberBand(const QPointF &selectionOrigin);
     void resizeRubberBand(const QPointF &cursorPoint);
     void stopRubberBand();
+    QList<Player *> collectActivePlayers(int &firstPlayerIndex) const;
+    QList<Player *> rotatePlayers(const QList<Player *> &players, int firstPlayerIndex) const;
+    int determineColumnCount(int playerCount) const;
+    QSizeF computeSceneSizeAndPlayerLayout(const QList<Player *> &playersPlaying, int columns);
+    QList<qreal> calculateMinWidthByColumn() const;
+    qreal calculateNewSceneWidth(const QSize &newSize, qreal minWidth) const;
+    void resizeColumnsAndPlayers(const QList<qreal> &minWidthByColumn, qreal newWidth);
+    CardZone *findTopmostZone(const QList<QGraphicsItem *> &items) const;
+    CardItem *findTopmostCardInZone(const QList<QGraphicsItem *> &items, CardZone *zone) const;
+    void updateHoveredCard(CardItem *newCard);
 
     void registerAnimationItem(AbstractCardItem *item);
     void unregisterAnimationItem(AbstractCardItem *card);