[Game][Arrows] Split Arrows into ArrowData and ArrowItem (#6918)

* [Game][Arrows] Split Arrows into ArrowData and ArrowItem

Took 13 minutes

Took 5 seconds

Took 1 minute

Took 26 seconds

* Address comments.

Took 17 minutes

Took 9 seconds


Took 1 minute

* Change check.

Took 3 minutes

* Pass by const reference.

Took 10 minutes

* Remove extra method

Took 2 minutes

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/CMakeLists.txt

@@ -59,6 +59,7 @@ set(cockatrice_SOURCES
     src/game/board/abstract_card_drag_item.cpp
     src/game/board/abstract_card_item.cpp
     src/game/board/abstract_counter.cpp
+    src/game/board/arrow_data.cpp
     src/game/board/arrow_item.cpp
     src/game/board/arrow_target.cpp
     src/game/board/card_drag_item.cpp

cockatrice/src/game/board/arrow_data.cpp

@@ -0,0 +1,19 @@
+#include "arrow_data.h"
+
+ArrowData ArrowData::fromProto(const ServerInfo_Arrow &arrow)
+{
+    ArrowData data;
+    data.id = arrow.id();
+    data.startPlayerId = arrow.start_player_id();
+    data.startZone = QString::fromStdString(arrow.start_zone());
+    data.startCardId = arrow.start_card_id();
+    data.targetPlayerId = arrow.target_player_id();
+    data.color = convertColorToQColor(arrow.arrow_color());
+
+    if (arrow.has_target_zone()) {
+        data.targetZone = QString::fromStdString(arrow.target_zone());
+        data.targetCardId = arrow.target_card_id();
+    }
+
+    return data;
+}

cockatrice/src/game/board/arrow_data.h

@@ -0,0 +1,28 @@
+#ifndef COCKATRICE_ARROW_DATA_H
+#define COCKATRICE_ARROW_DATA_H
+
+#include <QColor>
+#include <QString>
+#include <libcockatrice/protocol/pb/serverinfo_arrow.pb.h>
+#include <libcockatrice/utility/color.h>
+
+struct ArrowData
+{
+    int id;
+    int startPlayerId;
+    QString startZone;
+    int startCardId;
+    int targetPlayerId;
+    QString targetZone;    // empty = targeting a player
+    int targetCardId = -1; // -1 = targeting a player
+    QColor color;
+
+    static ArrowData fromProto(const ServerInfo_Arrow &arrow);
+
+    bool isPlayerTargeted() const
+    {
+        return targetZone.isEmpty();
+    }
+};
+
+#endif // COCKATRICE_ARROW_DATA_H

cockatrice/src/game/board/arrow_item.cpp

@@ -26,16 +26,23 @@ ArrowItem::ArrowItem(PlayerLogic *_player,
                      ArrowTarget *_startItem,
                      ArrowTarget *_targetItem,
                      const QColor &_color)
-    : QGraphicsItem(), player(_player), id(_id), startItem(_startItem), targetItem(_targetItem), targetLocked(false),
+    : player(_player), id(_id), startItem(_startItem), targetItem(_targetItem), color(_color)
-      color(_color), fullColor(true)
 {
     setZValue(ZValues::ARROWS);
 
+    auto doUpdate = [this]() {
+        if (startItem && targetItem) {
+            updatePath();
+        }
+    };
+
     if (startItem) {
-        startItem->addArrowFrom(this);
+        connect(startItem, &ArrowTarget::scenePositionChanged, this, doUpdate);
+        connect(startItem, &QObject::destroyed, this, &ArrowItem::delArrow);
     }
     if (targetItem) {
-        targetItem->addArrowTo(this);
+        connect(targetItem, &ArrowTarget::scenePositionChanged, this, doUpdate);
+        connect(targetItem, &QObject::destroyed, this, &ArrowItem::delArrow);
     }
 
     if (startItem && targetItem) {
@@ -43,24 +50,11 @@ ArrowItem::ArrowItem(PlayerLogic *_player,
     }
 }
 
-ArrowItem::~ArrowItem()
-{
-}
-
 void ArrowItem::delArrow()
 {
-    if (startItem) {
-        startItem->removeArrowFrom(this);
-        startItem = 0;
-    }
-
     if (targetItem) {
         targetItem->setBeingPointedAt(false);
-        targetItem->removeArrowTo(this);
-        targetItem = 0;
     }
-
-    player->removeArrow(this);
     deleteLater();
 }
 
@@ -148,8 +142,7 @@ void ArrowItem::mousePressEvent(QGraphicsSceneMouseEvent *event)
         return;
     }
 
-    QList<QGraphicsItem *> colliding = scene()->items(event->scenePos());
+    for (auto *item : scene()->items(event->scenePos())) {
-    for (QGraphicsItem *item : colliding) {
         if (qgraphicsitem_cast<CardItem *>(item)) {
             event->ignore();
             return;
@@ -164,60 +157,62 @@ void ArrowItem::mousePressEvent(QGraphicsSceneMouseEvent *event)
     }
 }
 
+// ArrowDragItem
+
 ArrowDragItem::ArrowDragItem(PlayerLogic *_owner, ArrowTarget *_startItem, const QColor &_color, int _deleteInPhase)
-    : ArrowItem(_owner, -1, _startItem, 0, _color), deleteInPhase(_deleteInPhase)
+    : ArrowItem(_owner, -1, _startItem, nullptr, _color), deleteInPhase(_deleteInPhase)
 {
 }
 
-void ArrowDragItem::addChildArrow(ArrowDragItem *childArrow)
+void ArrowDragItem::addChildArrow(ArrowDragItem *child)
 {
-    childArrows.append(childArrow);
+    childArrows.append(child);
 }
 
 void ArrowDragItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
 {
-    // This ensures that if a mouse move event happens after a call to delArrow(),
-    // the event will be discarded as it would create some stray pointers.
     if (targetLocked || !startItem) {
         return;
     }
 
-    QPointF endPos = event->scenePos();
+    const QPointF endPos = event->scenePos();
 
-    QList<QGraphicsItem *> colliding = scene()->items(endPos);
+    ArrowTarget *cursorItem = nullptr;
-    ArrowTarget *cursorItem = 0;
     qreal cursorItemZ = -1;
-    for (int i = colliding.size() - 1; i >= 0; i--) {
+    for (auto *item : scene()->items(endPos)) {
-        if (qgraphicsitem_cast<PlayerTarget *>(colliding.at(i)) || qgraphicsitem_cast<CardItem *>(colliding.at(i))) {
+        ArrowTarget *candidate = nullptr;
-            if (colliding.at(i)->zValue() > cursorItemZ) {
+        if (auto *card = qgraphicsitem_cast<CardItem *>(item)) {
-                cursorItem = static_cast<ArrowTarget *>(colliding.at(i));
+            candidate = card;
-                cursorItemZ = cursorItem->zValue();
+        } else if (auto *pt = qgraphicsitem_cast<PlayerTarget *>(item)) {
-            }
+            candidate = pt;
+        }
+
+        if (candidate && candidate->zValue() > cursorItemZ) {
+            cursorItem = candidate;
+            cursorItemZ = candidate->zValue();
         }
     }
 
-    if ((cursorItem != targetItem) && targetItem) {
+    if (cursorItem != targetItem) {
-        targetItem->setBeingPointedAt(false);
+        if (targetItem) {
-        targetItem->removeArrowTo(this);
+            disconnect(positionConnection);
-    }
+            targetItem->setBeingPointedAt(false);
-    if (!cursorItem) {
+        }
-        fullColor = false;
+
-        targetItem = 0;
+        targetItem = cursorItem;
-        updatePath(endPos);
+        fullColor = (cursorItem != nullptr);
-    } else {
+
-        if (cursorItem != targetItem) {
+        if (cursorItem && cursorItem != startItem) {
-            fullColor = true;
+            cursorItem->setBeingPointedAt(true);
-            if (cursorItem != startItem) {
+            positionConnection =
-                cursorItem->setBeingPointedAt(true);
+                connect(cursorItem, &ArrowTarget::scenePositionChanged, this, [this]() { updatePath(); });
-                cursorItem->addArrowTo(this);
-            }
-            targetItem = cursorItem;
         }
-        updatePath();
     }
+
+    targetItem ? updatePath() : updatePath(endPos);
     update();
 
-    for (ArrowDragItem *child : childArrows) {
+    for (auto *child : childArrows) {
         child->mouseMoveEvent(event);
     }
 }
@@ -228,12 +223,16 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
         return;
     }
 
-    if (targetItem && (targetItem != startItem)) {
+    if (targetItem && targetItem != startItem) {
-        CardZoneLogic *startZone = static_cast<CardItem *>(startItem)->getZone();
+        CardItem *startCard = qgraphicsitem_cast<CardItem *>(startItem);
         // For now, we can safely assume that the start item is always a card.
         // The target item can be a player as well.
-        CardItem *startCard = qgraphicsitem_cast<CardItem *>(startItem);
+        if (!startCard) {
-        CardItem *targetCard = qgraphicsitem_cast<CardItem *>(targetItem);
+            delArrow();
+            return;
+        }
+
+        CardZoneLogic *startZone = startCard->getZone();
 
         Command_CreateArrow cmd;
         cmd.mutable_arrow_color()->CopyFrom(convertQColorToColor(color));
@@ -241,14 +240,16 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
         cmd.set_start_zone(startZone->getName().toStdString());
         cmd.set_start_card_id(startCard->getId());
 
-        if (targetCard) {
+        if (auto *targetCard = qgraphicsitem_cast<CardItem *>(targetItem)) {
             CardZoneLogic *targetZone = targetCard->getZone();
             cmd.set_target_player_id(targetZone->getPlayer()->getPlayerInfo()->getId());
             cmd.set_target_zone(targetZone->getName().toStdString());
             cmd.set_target_card_id(targetCard->getId());
-        } else { // failed to cast target to card, this means it's a player
+        } else if (auto *targetPlayer = qgraphicsitem_cast<PlayerTarget *>(targetItem)) {
-            PlayerTarget *targetPlayer = qgraphicsitem_cast<PlayerTarget *>(targetItem);
             cmd.set_target_player_id(targetPlayer->getOwner()->getPlayerInfo()->getId());
+        } else {
+            delArrow();
+            return;
         }
 
         // if the card is in hand then we will move the card to stack or table as part of drawing the arrow
@@ -271,21 +272,22 @@ void ArrowDragItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
 
         player->getPlayerActions()->sendGameCommand(cmd);
     }
-    delArrow();
 
-    for (ArrowDragItem *child : childArrows) {
+    delArrow();
+    for (auto *child : childArrows) {
         child->mouseReleaseEvent(event);
     }
 }
 
+// ArrowAttachItem
 ArrowAttachItem::ArrowAttachItem(ArrowTarget *_startItem)
-    : ArrowItem(_startItem->getOwner(), -1, _startItem, 0, Qt::green)
+    : ArrowItem(_startItem->getOwner(), -1, _startItem, nullptr, Qt::green)
 {
 }
 
-void ArrowAttachItem::addChildArrow(ArrowAttachItem *childArrow)
+void ArrowAttachItem::addChildArrow(ArrowAttachItem *child)
 {
-    childArrows.append(childArrow);
+    childArrows.append(child);
 }
 
 void ArrowAttachItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
@@ -294,67 +296,43 @@ void ArrowAttachItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
         return;
     }
 
-    QPointF endPos = event->scenePos();
+    const QPointF endPos = event->scenePos();
 
-    QList<QGraphicsItem *> colliding = scene()->items(endPos);
+    ArrowTarget *cursorItem = nullptr;
-    ArrowTarget *cursorItem = 0;
     qreal cursorItemZ = -1;
-    for (int i = colliding.size() - 1; i >= 0; i--) {
+    for (auto *item : scene()->items(endPos)) {
-        if (qgraphicsitem_cast<CardItem *>(colliding.at(i))) {
+        if (auto *card = qgraphicsitem_cast<CardItem *>(item)) {
-            if (colliding.at(i)->zValue() > cursorItemZ) {
+            if (card->zValue() > cursorItemZ) {
-                cursorItem = static_cast<ArrowTarget *>(colliding.at(i));
+                cursorItem = card;
-                cursorItemZ = cursorItem->zValue();
+                cursorItemZ = card->zValue();
             }
         }
     }
 
-    if ((cursorItem != targetItem) && targetItem) {
+    if (cursorItem != targetItem) {
-        targetItem->setBeingPointedAt(false);
+        if (targetItem) {
-    }
+            disconnect(positionConnection);
-    if (!cursorItem) {
+            targetItem->setBeingPointedAt(false);
-        fullColor = false;
-        targetItem = 0;
-        updatePath(endPos);
-    } else {
-        fullColor = true;
-        if (cursorItem != startItem) {
-            cursorItem->setBeingPointedAt(true);
         }
+
         targetItem = cursorItem;
-        updatePath();
+        fullColor = (cursorItem != nullptr);
+
+        if (cursorItem && cursorItem != startItem) {
+            cursorItem->setBeingPointedAt(true);
+            positionConnection =
+                connect(cursorItem, &ArrowTarget::scenePositionChanged, this, [this]() { updatePath(); });
+        }
     }
+
+    targetItem ? updatePath() : updatePath(endPos);
     update();
 
-    for (ArrowAttachItem *child : childArrows) {
+    for (auto *child : childArrows) {
         child->mouseMoveEvent(event);
     }
 }
 
-void ArrowAttachItem::attachCards(CardItem *startCard, const CardItem *targetCard)
-{
-    // do nothing if target is already attached to another card or is not in play
-    if (targetCard->getAttachedTo() || targetCard->getZone()->getName() != ZoneNames::TABLE) {
-        return;
-    }
-
-    CardZoneLogic *startZone = startCard->getZone();
-    CardZoneLogic *targetZone = targetCard->getZone();
-
-    // move card onto table first if attaching from some other zone
-    if (startZone->getName() != ZoneNames::TABLE) {
-        player->getPlayerActions()->playCardToTable(startCard, false);
-    }
-
-    Command_AttachCard cmd;
-    cmd.set_start_zone(ZoneNames::TABLE);
-    cmd.set_card_id(startCard->getId());
-    cmd.set_target_player_id(targetZone->getPlayer()->getPlayerInfo()->getId());
-    cmd.set_target_zone(targetZone->getName().toStdString());
-    cmd.set_target_card_id(targetCard->getId());
-
-    player->getPlayerActions()->sendGameCommand(cmd);
-}
-
 void ArrowAttachItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
 {
     if (!startItem) {
@@ -363,21 +341,40 @@ void ArrowAttachItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
 
     // Attaching could move startItem under the current cursor position, causing all children to retarget to it right
     // before they are processed. Prevent that.
-    for (ArrowAttachItem *child : childArrows) {
+    for (auto *child : childArrows) {
         child->setTargetLocked(true);
     }
 
-    if (targetItem && (targetItem != startItem)) {
+    if (targetItem && targetItem != startItem) {
-        auto startCard = qgraphicsitem_cast<CardItem *>(startItem);
+        auto *startCard = qgraphicsitem_cast<CardItem *>(startItem);
-        auto targetCard = qgraphicsitem_cast<CardItem *>(targetItem);
+        auto *targetCard = qgraphicsitem_cast<CardItem *>(targetItem);
         if (startCard && targetCard) {
             attachCards(startCard, targetCard);
         }
     }
 
     delArrow();
-
+    for (auto *child : childArrows) {
-    for (ArrowAttachItem *child : childArrows) {
         child->mouseReleaseEvent(event);
     }
 }
+
+void ArrowAttachItem::attachCards(CardItem *startCard, const CardItem *targetCard)
+{
+    if (targetCard->getAttachedTo() || targetCard->getZone()->getName() != ZoneNames::TABLE) {
+        return;
+    }
+
+    // move card onto table first if attaching from some other zone
+    if (startCard->getZone()->getName() != ZoneNames::TABLE) {
+        player->getPlayerActions()->playCardToTable(startCard, false);
+    }
+
+    Command_AttachCard cmd;
+    cmd.set_start_zone(ZoneNames::TABLE);
+    cmd.set_card_id(startCard->getId());
+    cmd.set_target_player_id(targetCard->getZone()->getPlayer()->getPlayerInfo()->getId());
+    cmd.set_target_zone(targetCard->getZone()->getName().toStdString());
+    cmd.set_target_card_id(targetCard->getId());
+    player->getPlayerActions()->sendGameCommand(cmd);
+}

cockatrice/src/game/board/arrow_item.h

@@ -7,13 +7,15 @@
 #ifndef ARROWITEM_H
 #define ARROWITEM_H
 
+#include "arrow_target.h"
+
 #include <QGraphicsItem>
+#include <QPointer>
 
 class CardItem;
 class QGraphicsSceneMouseEvent;
 class QMenu;
 class PlayerLogic;
-class ArrowTarget;
 
 class ArrowItem : public QObject, public QGraphicsItem
 {
@@ -21,20 +23,21 @@ class ArrowItem : public QObject, public QGraphicsItem
     Q_INTERFACES(QGraphicsItem)
 private:
     QPainterPath path;
-    QMenu *menu;
 
 protected:
     PlayerLogic *player;
     int id;
-    ArrowTarget *startItem, *targetItem;
+    QPointer<ArrowTarget> startItem;
-    bool targetLocked;
+    QPointer<ArrowTarget> targetItem;
+    bool targetLocked = false;
     QColor color;
-    bool fullColor;
+    bool fullColor = true;
+
     void mousePressEvent(QGraphicsSceneMouseEvent *event) override;
 
 public:
-    ArrowItem(PlayerLogic *_player, int _id, ArrowTarget *_startItem, ArrowTarget *_targetItem, const QColor &color);
+    ArrowItem(PlayerLogic *_player, int _id, ArrowTarget *_startItem, ArrowTarget *_targetItem, const QColor &_color);
-    ~ArrowItem() override;
+
     void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget) override;
     [[nodiscard]] QRectF boundingRect() const override
     {
@@ -44,6 +47,7 @@ public:
     {
         return path;
     }
+
     void updatePath();
     void updatePath(const QPointF &endPoint);
 
@@ -55,14 +59,6 @@ public:
     {
         return player;
     }
-    void setStartItem(ArrowTarget *_item)
-    {
-        startItem = _item;
-    }
-    void setTargetItem(ArrowTarget *_item)
-    {
-        targetItem = _item;
-    }
     [[nodiscard]] ArrowTarget *getStartItem() const
     {
         return startItem;
@@ -75,6 +71,7 @@ public:
     {
         targetLocked = _targetLocked;
     }
+
     void delArrow();
 };
 
@@ -84,10 +81,11 @@ class ArrowDragItem : public ArrowItem
 private:
     int deleteInPhase;
     QList<ArrowDragItem *> childArrows;
+    QMetaObject::Connection positionConnection;
 
 public:
     ArrowDragItem(PlayerLogic *_owner, ArrowTarget *_startItem, const QColor &_color, int _deleteInPhase);
-    void addChildArrow(ArrowDragItem *childArrow);
+    void addChildArrow(ArrowDragItem *child);
 
 protected:
     void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override;
@@ -99,12 +97,12 @@ class ArrowAttachItem : public ArrowItem
     Q_OBJECT
 private:
     QList<ArrowAttachItem *> childArrows;
-
+    QMetaObject::Connection positionConnection;
     void attachCards(CardItem *startCard, const CardItem *targetCard);
 
 public:
     explicit ArrowAttachItem(ArrowTarget *_startItem);
-    void addChildArrow(ArrowAttachItem *childArrow);
+    void addChildArrow(ArrowAttachItem *child);
 
 protected:
     void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override;

cockatrice/src/game/board/arrow_target.cpp

@@ -3,41 +3,21 @@
 #include "../player/player_logic.h"
 #include "arrow_item.h"
 
-ArrowTarget::ArrowTarget(PlayerLogic *_owner, QGraphicsItem *parent)
+ArrowTarget::ArrowTarget(PlayerLogic *_owner, QGraphicsItem *parent) : AbstractGraphicsItem(parent), owner(_owner)
-    : AbstractGraphicsItem(parent), owner(_owner), beingPointedAt(false)
 {
     setFlag(ItemSendsScenePositionChanges);
 }
 
-ArrowTarget::~ArrowTarget()
-{
-    for (int i = 0; i < arrowsFrom.size(); ++i) {
-        arrowsFrom[i]->setStartItem(0);
-        arrowsFrom[i]->delArrow();
-    }
-    for (int i = 0; i < arrowsTo.size(); ++i) {
-        arrowsTo[i]->setTargetItem(0);
-        arrowsTo[i]->delArrow();
-    }
-}
-
 void ArrowTarget::setBeingPointedAt(bool _beingPointedAt)
 {
     beingPointedAt = _beingPointedAt;
     update();
 }
 
-QVariant ArrowTarget::itemChange(QGraphicsItem::GraphicsItemChange change, const QVariant &value)
+QVariant ArrowTarget::itemChange(GraphicsItemChange change, const QVariant &value)
 {
-    if (change == ItemScenePositionHasChanged && scene()) {
+    if (change == ItemScenePositionHasChanged) {
-        for (auto *arrow : arrowsFrom) {
+        emit scenePositionChanged();
-            arrow->updatePath();
-        }
-
-        for (auto *arrow : arrowsTo) {
-            arrow->updatePath();
-        }
     }
-
+    return AbstractGraphicsItem::itemChange(change, value);
-    return QGraphicsItem::itemChange(change, value);
+}
-}

cockatrice/src/game/board/arrow_target.h

@@ -21,12 +21,14 @@ protected:
     PlayerLogic *owner;
 
 private:
-    bool beingPointedAt;
+    bool beingPointedAt = false;
-    QList<ArrowItem *> arrowsFrom, arrowsTo;
+
+signals:
+    void scenePositionChanged();
 
 public:
     explicit ArrowTarget(PlayerLogic *_owner, QGraphicsItem *parent = nullptr);
-    ~ArrowTarget() override;
+    ~ArrowTarget() override = default;
 
     [[nodiscard]] PlayerLogic *getOwner() const
     {
@@ -39,32 +41,7 @@ public:
         return beingPointedAt;
     }
 
-    [[nodiscard]] const QList<ArrowItem *> &getArrowsFrom() const
-    {
-        return arrowsFrom;
-    }
-    void addArrowFrom(ArrowItem *arrow)
-    {
-        arrowsFrom.append(arrow);
-    }
-    void removeArrowFrom(ArrowItem *arrow)
-    {
-        arrowsFrom.removeOne(arrow);
-    }
-    [[nodiscard]] const QList<ArrowItem *> &getArrowsTo() const
-    {
-        return arrowsTo;
-    }
-    void addArrowTo(ArrowItem *arrow)
-    {
-        arrowsTo.append(arrow);
-    }
-    void removeArrowTo(ArrowItem *arrow)
-    {
-        arrowsTo.removeOne(arrow);
-    }
-
 protected:
-    QVariant itemChange(QGraphicsItem::GraphicsItemChange change, const QVariant &value) override;
+    QVariant itemChange(GraphicsItemChange change, const QVariant &value) override;
 };
 #endif

cockatrice/src/game/game_scene.cpp

@@ -82,9 +82,25 @@ void GameScene::addPlayer(PlayerLogic *player)
 {
     qCInfo(GameScenePlayerAdditionRemovalLog) << "GameScene::addPlayer name=" << player->getPlayerInfo()->getName();
 
-    players << player->getGraphicsItem();
+    playerViews.insert(player->getPlayerInfo()->getId(), player->getGraphicsItem());
     addItem(player->getGraphicsItem());
     connect(player->getGraphicsItem(), &PlayerGraphicsItem::sizeChanged, this, &GameScene::rearrange);
+
+    connect(player, &PlayerLogic::concededChanged, this, [this](int id, bool conceded) {
+        if (conceded) {
+            clearArrowsForPlayer(id);
+        }
+        rearrange();
+    });
+
+    connect(player, &PlayerLogic::arrowCreateRequested, this, &GameScene::onArrowCreateRequested);
+    connect(player, &PlayerLogic::arrowDeleteRequested, this, &GameScene::onArrowDeleteRequested);
+    connect(player, &PlayerLogic::arrowsCleared, this,
+            [this, id = player->getPlayerInfo()->getId()]() { clearArrowsForPlayer(id); });
+
+    connect(player->getPlayerEventHandler(), &PlayerEventHandler::cardZoneChanged, this, &GameScene::onCardZoneChanged);
+
+    rearrange();
 }
 
 /**
@@ -97,13 +113,15 @@ void GameScene::removePlayer(PlayerLogic *player)
 {
     qCInfo(GameScenePlayerAdditionRemovalLog) << "GameScene::removePlayer name=" << player->getPlayerInfo()->getName();
 
+    clearArrowsForPlayer(player->getPlayerInfo()->getId());
+
     for (ZoneViewWidget *zone : zoneViews) {
         if (zone->getPlayer() == player) {
             zone->close();
         }
     }
-    players.removeOne(player->getGraphicsItem());
+    auto *view = playerViews.take(player->getPlayerInfo()->getId());
-    removeItem(player->getGraphicsItem());
+    removeItem(view);
     rearrange();
 }
 
@@ -184,7 +202,7 @@ QList<PlayerLogic *> GameScene::collectActivePlayers(int &firstPlayerIndex) cons
     firstPlayerIndex = 0;
     bool firstPlayerFound = false;
 
-    for (auto *pgItem : players) {
+    for (auto *pgItem : playerViews.values()) {
         PlayerLogic *p = pgItem->getPlayer();
         if (p && !p->getConceded()) {
             activePlayers.append(p);
@@ -348,6 +366,77 @@ void GameScene::resizeColumnsAndPlayers(const QList<qreal> &minWidthByColumn, qr
     }
 }
 
+void GameScene::onArrowCreateRequested(const ArrowData &data)
+{
+    auto *startView = playerViews.value(data.startPlayerId);
+    auto *targetView = playerViews.value(data.targetPlayerId);
+    if (!startView || !targetView) {
+        return;
+    }
+
+    PlayerLogic *startLogic = startView->getPlayer();
+    auto *startZone = startLogic->getZones().value(data.startZone);
+    if (!startZone) {
+        return;
+    }
+
+    CardItem *startCard = startZone->getCard(data.startCardId);
+    if (!startCard) {
+        return;
+    }
+
+    ArrowTarget *targetItem = nullptr;
+    if (data.isPlayerTargeted()) {
+        targetItem = targetView->getPlayerTarget();
+    } else {
+        auto *zone = targetView->getPlayer()->getZones().value(data.targetZone);
+        if (zone) {
+            targetItem = zone->getCard(data.targetCardId);
+        }
+    }
+    if (!targetItem) {
+        return;
+    }
+
+    auto *arrow = new ArrowItem(startView->getPlayer(), data.id, startCard, targetItem, data.color);
+    addItem(arrow);
+    arrowRegistry.insert(data.id, arrow);
+    connect(arrow, &QObject::destroyed, this, [this, id = data.id]() { arrowRegistry.remove(id); });
+}
+
+void GameScene::onArrowDeleteRequested(int arrowId)
+{
+    if (arrowRegistry.contains(arrowId)) {
+        emit requestArrowDeletion(arrowId);
+    }
+}
+
+void GameScene::onCardZoneChanged(CardItem *card, bool sameZone)
+{
+    QList<ArrowItem *> toDelete;
+    for (auto *arrow : arrowRegistry.values()) {
+        if (arrow->getStartItem() == card || arrow->getTargetItem() == card) {
+            if (sameZone) {
+                arrow->updatePath();
+            } else {
+                toDelete.append(arrow);
+            }
+        }
+    }
+    for (auto *arrow : toDelete) {
+        emit requestArrowDeletion(arrow->getId());
+    }
+}
+
+void GameScene::clearArrowsForPlayer(int playerId)
+{
+    for (auto *arrow : arrowRegistry.values()) {
+        if (arrow->getPlayer()->getPlayerInfo()->getId() == playerId) {
+            emit requestArrowDeletion(arrow->getId());
+        }
+    }
+}
+
 // ---------- Hover Handling ----------
 
 void GameScene::updateHover(const QPointF &scenePos)

cockatrice/src/game/game_scene.h

@@ -1,6 +1,8 @@
 #ifndef GAMESCENE_H
 #define GAMESCENE_H
 
+#include "board/arrow_data.h"
+#include "board/arrow_item.h"
 #include "zones/card_zone_logic.h"
 
 #include <QGraphicsScene>
@@ -41,8 +43,9 @@ private:
     static const int playerAreaSpacing = 5; ///< Space between player areas
 
     PhasesToolbar *phasesToolbar;                       ///< Toolbar showing game phases
-    QList<PlayerGraphicsItem *> players;                ///< All player graphics items
+    QMap<int, PlayerGraphicsItem *> playerViews;        ///< ID lookup for player graphics items
     QList<QList<PlayerGraphicsItem *>> playersByColumn; ///< Players organized by column
+    QMap<int, ArrowItem *> arrowRegistry;               ///< ID registry for arrow graphics items
     QList<ZoneViewWidget *> zoneViews;                  ///< Active zone view widgets
     QSize viewSize;                                     ///< Current view size
     QPointer<CardItem> hoveredCard;                     ///< Currently hovered card
@@ -196,6 +199,11 @@ public slots:
     QTransform getViewTransform() const;
     QTransform getViewportTransform() const;
 
+    void onArrowCreateRequested(const ArrowData &data);
+    void onArrowDeleteRequested(int arrowId);
+    void onCardZoneChanged(CardItem *card, bool sameZone);
+    void clearArrowsForPlayer(int playerId);
+
 protected:
     /** Handles hover updates. */
     bool event(QEvent *event) override;
@@ -207,6 +215,7 @@ signals:
     void sigStartRubberBand(const QPointF &selectionOrigin);
     void sigResizeRubberBand(const QPointF &cursorPoint, int selectedCount);
     void sigStopRubberBand();
+    void requestArrowDeletion(int arrowId);
 };
 
 #endif

cockatrice/src/game/player/player_event_handler.cpp

@@ -2,9 +2,11 @@
 
 #include "../../game_graphics/zones/view_zone.h"
 #include "../../interface/widgets/tabs/tab_game.h"
+#include "../board/arrow_data.h"
 #include "../board/arrow_item.h"
 #include "../board/card_item.h"
 #include "../board/card_list.h"
+#include "libcockatrice/utility/color.h"
 #include "player_actions.h"
 #include "player_logic.h"
 
@@ -90,25 +92,43 @@ void PlayerEventHandler::eventRollDie(const Event_RollDie &event)
 
 void PlayerEventHandler::eventCreateArrow(const Event_CreateArrow &event)
 {
-    ArrowItem *arrow = player->addArrow(event.arrow_info());
+    const ArrowData data = ArrowData::fromProto(event.arrow_info());
-    if (!arrow) {
+
-        return;
+    // Resolve names for logging
+    const auto &playerList = player->getGame()->getPlayerManager()->getPlayers();
+    PlayerLogic *startPlayer = playerList.value(data.startPlayerId);
+    PlayerLogic *targetPlayer = playerList.value(data.targetPlayerId);
+
+    QString startCardName, targetCardName;
+    if (startPlayer) {
+        auto *zone = startPlayer->getZones().value(data.startZone);
+        if (zone) {
+            if (auto *card = zone->getCard(data.startCardId)) {
+                startCardName = card->getName();
+            }
+        }
+    }
+    if (!data.isPlayerTargeted() && targetPlayer) {
+        auto *zone = targetPlayer->getZones().value(data.targetZone);
+        if (zone) {
+            if (auto *card = zone->getCard(data.targetCardId)) {
+                targetCardName = card->getName();
+            }
+        }
     }
 
-    auto *startCard = static_cast<CardItem *>(arrow->getStartItem());
+    emit player->arrowCreateRequested(data);
-    auto *targetCard = qgraphicsitem_cast<CardItem *>(arrow->getTargetItem());
+
-    if (targetCard) {
+    const bool validForLogging = !startCardName.isEmpty() && (data.isPlayerTargeted() || !targetCardName.isEmpty());
-        emit logCreateArrow(player, startCard->getOwner(), startCard->getName(), targetCard->getOwner(),
+
-                            targetCard->getName(), false);
+    if (startPlayer && targetPlayer && validForLogging) {
-    } else {
+        emit logCreateArrow(player, startPlayer, startCardName, targetPlayer, targetCardName, data.isPlayerTargeted());
-        emit logCreateArrow(player, startCard->getOwner(), startCard->getName(), arrow->getTargetItem()->getOwner(),
-                            QString(), true);
     }
 }
 
 void PlayerEventHandler::eventDeleteArrow(const Event_DeleteArrow &event)
 {
-    player->delArrow(event.arrow_id());
+    emit player->arrowDeleteRequested(event.arrow_id());
 }
 
 void PlayerEventHandler::eventCreateToken(const Event_CreateToken &event)
@@ -352,28 +372,7 @@ void PlayerEventHandler::eventMoveCard(const Event_MoveCard &event, const GameEv
 
     targetZone->addCard(card, true, x, y);
 
-    // Look at all arrows from and to the card.
+    emit cardZoneChanged(card, startZone == targetZone);
-    // If the card was moved to another zone, delete the arrows, otherwise update them.
-    QMapIterator<int, PlayerLogic *> playerIterator(player->getGame()->getPlayerManager()->getPlayers());
-    while (playerIterator.hasNext()) {
-        PlayerLogic *p = playerIterator.next().value();
-
-        QList<ArrowItem *> arrowsToDelete;
-        QMapIterator<int, ArrowItem *> arrowIterator(p->getArrows());
-        while (arrowIterator.hasNext()) {
-            ArrowItem *arrow = arrowIterator.next().value();
-            if ((arrow->getStartItem() == card) || (arrow->getTargetItem() == card)) {
-                if (startZone == targetZone) {
-                    arrow->updatePath();
-                } else {
-                    arrowsToDelete.append(arrow);
-                }
-            }
-        }
-        for (auto &i : arrowsToDelete) {
-            i->delArrow();
-        }
-    }
     player->getPlayerMenu()->updateCardMenu(card);
 
     if (player->getPlayerActions()->isMovingCardsUntil() && startZoneString == ZoneNames::DECK &&

cockatrice/src/game/player/player_event_handler.h

@@ -82,6 +82,7 @@ signals:
                         bool isLentToAnotherPlayer = false);
     void logAlwaysRevealTopCard(PlayerLogic *player, CardZoneLogic *zone, bool reveal);
     void logAlwaysLookAtTopCard(PlayerLogic *player, CardZoneLogic *zone, bool reveal);
+    void cardZoneChanged(CardItem *card, bool sameZone);
 
 public:
     PlayerEventHandler(PlayerLogic *player);

cockatrice/src/game/player/player_logic.cpp

@@ -74,7 +74,7 @@ PlayerLogic::~PlayerLogic()
 
 void PlayerLogic::clear()
 {
-    clearArrows();
+    emit arrowsCleared();
 
     QMapIterator<QString, CardZoneLogic *> i(zones);
     while (i.hasNext()) {
@@ -115,7 +115,7 @@ void PlayerLogic::processPlayerInfo(const ServerInfo_Player &info)
                                       /* HandZone */
                                       ZoneNames::HAND};
     clearCounters();
-    clearArrows();
+    emit arrowsCleared();
 
     QMutableMapIterator<QString, CardZoneLogic *> zoneIt(zones);
     while (zoneIt.hasNext()) {
@@ -231,7 +231,7 @@ void PlayerLogic::processCardAttachment(const ServerInfo_Player &info)
 
     const int arrowListSize = info.arrow_list_size();
     for (int i = 0; i < arrowListSize; ++i) {
-        addArrow(info.arrow_list(i));
+        emit arrowCreateRequested(ArrowData::fromProto(info.arrow_list(i)));
     }
 }
 
@@ -340,75 +340,6 @@ void PlayerLogic::incrementAllCardCounters()
     }
 }
 
-ArrowItem *PlayerLogic::addArrow(const ServerInfo_Arrow &arrow)
-{
-    const QMap<int, PlayerLogic *> &playerList = game->getPlayerManager()->getPlayers();
-    PlayerLogic *startPlayer = playerList.value(arrow.start_player_id(), 0);
-    PlayerLogic *targetPlayer = playerList.value(arrow.target_player_id(), 0);
-    if (!startPlayer || !targetPlayer) {
-        return nullptr;
-    }
-
-    CardZoneLogic *startZone = startPlayer->getZones().value(QString::fromStdString(arrow.start_zone()), 0);
-    CardZoneLogic *targetZone = nullptr;
-    if (arrow.has_target_zone()) {
-        targetZone = targetPlayer->getZones().value(QString::fromStdString(arrow.target_zone()), 0);
-    }
-    if (!startZone || (!targetZone && arrow.has_target_zone())) {
-        return nullptr;
-    }
-
-    CardItem *startCard = startZone->getCard(arrow.start_card_id());
-    CardItem *targetCard = nullptr;
-    if (targetZone) {
-        targetCard = targetZone->getCard(arrow.target_card_id());
-    }
-    if (!startCard || (!targetCard && arrow.has_target_card_id())) {
-        return nullptr;
-    }
-
-    if (targetCard) {
-        return addArrow(arrow.id(), startCard, targetCard, convertColorToQColor(arrow.arrow_color()));
-    } else {
-        return addArrow(arrow.id(), startCard, targetPlayer->getGraphicsItem()->getPlayerTarget(),
-                        convertColorToQColor(arrow.arrow_color()));
-    }
-}
-
-ArrowItem *PlayerLogic::addArrow(int arrowId, CardItem *startCard, ArrowTarget *targetItem, const QColor &color)
-{
-    auto *arrow = new ArrowItem(this, arrowId, startCard, targetItem, color);
-    arrows.insert(arrowId, arrow);
-
-    getGameScene()->addItem(arrow);
-    return arrow;
-}
-
-void PlayerLogic::delArrow(int arrowId)
-{
-    ArrowItem *arr = arrows.value(arrowId, 0);
-    if (!arr) {
-        return;
-    }
-    arr->delArrow();
-}
-
-void PlayerLogic::removeArrow(ArrowItem *arrow)
-{
-    if (arrow->getId() != -1) {
-        arrows.remove(arrow->getId());
-    }
-}
-
-void PlayerLogic::clearArrows()
-{
-    QMapIterator<int, ArrowItem *> arrowIterator(arrows);
-    while (arrowIterator.hasNext()) {
-        arrowIterator.next().value()->delArrow();
-    }
-    arrows.clear();
-}
-
 bool PlayerLogic::clearCardsToDelete()
 {
     if (cardsToDelete.isEmpty()) {

cockatrice/src/game/player/player_logic.h

@@ -8,6 +8,7 @@
 #define PLAYER_H
 
 #include "../../interface/widgets/menus/tearoff_menu.h"
+#include "../board/arrow_data.h"
 #include "../interface/deck_loader/loaded_deck.h"
 #include "../zones/hand_zone_logic.h"
 #include "../zones/pile_zone_logic.h"
@@ -77,6 +78,9 @@ signals:
     void clearCustomZonesMenu();
     void addViewCustomZoneActionToCustomZoneMenu(QString zoneName);
     void resetTopCardMenuActions();
+    void arrowCreateRequested(ArrowData data);
+    void arrowDeleteRequested(int arrowId);
+    void arrowsCleared(); // fires on clear() and processPlayerInfo
 
 public slots:
     void setActive(bool _active);
@@ -205,17 +209,6 @@ public:
      */
     CounterState *getLifeCounter() const;
 
-    ArrowItem *addArrow(const ServerInfo_Arrow &arrow);
-    ArrowItem *addArrow(int arrowId, CardItem *startCard, ArrowTarget *targetItem, const QColor &color);
-    void delArrow(int arrowId);
-    void removeArrow(ArrowItem *arrow);
-    void clearArrows();
-
-    const QMap<int, ArrowItem *> &getArrows() const
-    {
-        return arrows;
-    }
-
     void setConceded(bool _conceded);
     bool getConceded() const
     {
@@ -252,12 +245,9 @@ private:
     int zoneId;
     QMap<QString, CardZoneLogic *> zones;
     QMap<int, CounterState *> counters;
-    QMap<int, ArrowItem *> arrows;
 
     bool dialogSemaphore;
     QList<CardItem *> cardsToDelete;
-
-    // void eventConnectionStateChanged(const Event_ConnectionStateChanged &event);
 };
 
 class AnnotationDialog : public QInputDialog

cockatrice/src/interface/widgets/tabs/tab_game.cpp

@@ -606,17 +606,9 @@ void TabGame::actNextPhaseAction()
 
 void TabGame::actRemoveLocalArrows()
 {
-    QMapIterator<int, PlayerLogic *> playerIterator(game->getPlayerManager()->getPlayers());
+    auto *local = game->getPlayerManager()->getActiveLocalPlayer(game->getGameState()->getActivePlayer());
-    while (playerIterator.hasNext()) {
+    if (local) {
-        PlayerLogic *player = playerIterator.next().value();
+        scene->clearArrowsForPlayer(local->getPlayerInfo()->getId());
-        if (!player->getPlayerInfo()->getLocal()) {
-            continue;
-        }
-        QMapIterator<int, ArrowItem *> arrowIterator(player->getArrows());
-        while (arrowIterator.hasNext()) {
-            ArrowItem *a = arrowIterator.next().value();
-            emit arrowDeletionRequested(a->getId());
-        }
     }
 }
 
@@ -970,8 +962,6 @@ void TabGame::createMenuItems()
     connect(aReverseTurn, &QAction::triggered, game->getGameEventHandler(), &GameEventHandler::handleReverseTurn);
     aRemoveLocalArrows = new QAction(this);
     connect(aRemoveLocalArrows, &QAction::triggered, this, &TabGame::actRemoveLocalArrows);
-    connect(this, &TabGame::arrowDeletionRequested, game->getGameEventHandler(),
-            &GameEventHandler::handleArrowDeletion);
     aRotateViewCW = new QAction(this);
     connect(aRotateViewCW, &QAction::triggered, this, &TabGame::actRotateViewCW);
     aRotateViewCCW = new QAction(this);
@@ -1155,6 +1145,8 @@ void TabGame::createPlayAreaWidget(bool bReplay)
     scene = new GameScene(phasesToolbar, this);
     connect(game->getPlayerManager(), &PlayerManager::playerConceded, scene, &GameScene::rearrange);
     connect(game->getPlayerManager(), &PlayerManager::playerCountChanged, scene, &GameScene::rearrange);
+    connect(scene, &GameScene::requestArrowDeletion, game->getGameEventHandler(),
+            &GameEventHandler::handleArrowDeletion);
     gameView = new GameView(scene);
 
     auto gamePlayAreaVBox = new QVBoxLayout;

cockatrice/src/interface/widgets/tabs/tab_game.h

@@ -137,7 +137,6 @@ signals:
     void gameLeft();
     void chatMessageSent(QString chatMessage);
     void turnAdvanced();
-    void arrowDeletionRequested(int arrowId);
 
 private slots:
     void adminLockChanged(bool lock);