Merge 80946b2fd4 into 80426d77bc

cockatrice/src/game/player/player_actions.cpp

@@ -1527,9 +1527,9 @@ void PlayerActions::offsetCardCounter(QList<CardItem *> selectedCards, int count
         int oldValue = card->getCounters().value(counterId, 0);
         int newValue = oldValue + offset;
 
-        // Early exit optimization: server enforces [0, MAX_COUNTERS_ON_CARD].
+        // Early exit optimization: server enforces [0, MAX_COUNTER_VALUE].
         // Compare clamped value to allow recovery from invalid states.
-        int clampedValue = qBound(0, newValue, MAX_COUNTERS_ON_CARD);
+        int clampedValue = qBound(0, newValue, MAX_COUNTER_VALUE);
         if (clampedValue != oldValue) {
             auto *cmd = new Command_SetCardCounter;
             cmd->set_zone(card->getZone()->getName().toStdString());
@@ -1563,7 +1563,7 @@ void PlayerActions::actSetCardCounter(QList<CardItem *> selectedCards, int count
         Expression exp(oldValue);
         double parsed = exp.parse(counterValue);
         // Clamp in double precision first to avoid UB, then cast
-        int number = static_cast<int>(qBound(0.0, parsed, static_cast<double>(MAX_COUNTERS_ON_CARD)));
+        int number = static_cast<int>(qBound(0.0, parsed, static_cast<double>(MAX_COUNTER_VALUE)));
 
         auto *cmd = new Command_SetCardCounter;
         cmd->set_zone(card->getZone()->getName().toStdString());
@@ -1593,7 +1593,7 @@ void PlayerActions::actIncrementAllCardCounters(QList<CardItem *> cardsToUpdate)
             counterIterator.next();
             int counterId = counterIterator.key();
             int currentValue = counterIterator.value();
-            if (currentValue >= MAX_COUNTERS_ON_CARD) {
+            if (currentValue >= MAX_COUNTER_VALUE) {
                 continue;
             }
 

libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp

@@ -114,8 +114,8 @@ QString Server_Card::setAttribute(CardAttribute attribute, const QString &avalue
 
 bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event)
 {
-    // Clamp to valid card counter range [0, MAX_COUNTERS_ON_CARD]
+    // Clamp to valid card counter range [0, MAX_COUNTER_VALUE]
-    value = qBound(0, value, MAX_COUNTERS_ON_CARD);
+    value = qBound(0, value, MAX_COUNTER_VALUE);
 
     const int oldValue = counters.value(_id, 0);
     if (value == oldValue) {
@@ -139,10 +139,8 @@ bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event)
 bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event)
 {
     const int oldValue = counters.value(counterId, 0);
-    const auto result = static_cast<int64_t>(oldValue) + static_cast<int64_t>(delta);
+    // Clamp to [0, MAX_COUNTER_VALUE] for card counters
-    // Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters
+    const int newValue = addClamped(oldValue, delta, 0, MAX_COUNTER_VALUE);
-    const int newValue =
-        static_cast<int>(qBound(static_cast<int64_t>(0), result, static_cast<int64_t>(MAX_COUNTERS_ON_CARD)));
 
     if (newValue == oldValue) {
         return false;

libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h

@@ -156,7 +156,7 @@ public:
     /**
      * @brief Sets a card counter to an exact value with clamping.
      * @param _id The counter ID.
-     * @param value The desired value (clamped to [0, MAX_COUNTERS_ON_CARD]; 0 removes the counter).
+     * @param value The desired value (clamped to [0, MAX_COUNTER_VALUE]; 0 removes the counter).
      * @param event Optional event to populate with counter state.
      * @return true if the value changed, false otherwise.
      */
@@ -168,7 +168,7 @@ public:
      * @param event Optional event to populate with counter state.
      * @return true if the value changed, false otherwise.
      * @note If counter does not exist, starts from 0. Counter is removed if result is 0.
-     * @note Clamps result to [0, MAX_COUNTERS_ON_CARD].
+     * @note Clamps result to [0, MAX_COUNTER_VALUE].
      */
     [[nodiscard]] bool incrementCounter(int counterId, int delta, Event_SetCardCounter *event = nullptr);
     void setTapped(bool _tapped)

libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.cpp

@@ -1,24 +1,19 @@
 #include "server_counter.h"
 
 #include <libcockatrice/protocol/pb/serverinfo_counter.pb.h>
-#include <limits>
 
-Server_Counter::Server_Counter(int _id, const QString &_name, const color &_counterColor, int _radius, int _count)
+Server_Counter::Server_Counter(int _id,
-    : id(_id), name(_name), counterColor(_counterColor), radius(_radius), count(_count)
+                               const QString &_name,
+                               const color &_counterColor,
+                               int _radius,
+                               int _count,
+                               int _minValue,
+                               int _maxValue)
+    : id(_id), name(_name), counterColor(_counterColor), radius(_radius), count(_count), minValue(_minValue),
+      maxValue(_maxValue)
 {
 }
 
-//! \todo Extract overflow-safe arithmetic into shared helper.
-//! Duplicated in Server_Card::incrementCounter() - keep in sync if modified.
-bool Server_Counter::incrementCount(int delta)
-{
-    const int oldCount = count;
-    const auto result = static_cast<int64_t>(count) + static_cast<int64_t>(delta);
-    count = static_cast<int>(qBound(static_cast<int64_t>(std::numeric_limits<int>::min()), result,
-                                    static_cast<int64_t>(std::numeric_limits<int>::max())));
-    return count != oldCount;
-}
-
 void Server_Counter::getInfo(ServerInfo_Counter *info)
 {
     info->set_id(id);

libcockatrice_network/libcockatrice/network/server/remote/game/server_counter.h

@@ -22,18 +22,19 @@
 
 #include <QString>
 #include <libcockatrice/protocol/pb/color.pb.h>
+#include <libcockatrice/utility/trice_limits.h>
+#include <limits>
 
 class ServerInfo_Counter;
 
 /**
  * @class Server_Counter
- * @brief Represents a player counter with overflow-safe increment arithmetic.
+ * @brief Represents a player counter with overflow-safe increment arithmetic and optional bounds.
  *
  * All value modifications return whether the value actually changed,
  * enabling callers to skip unnecessary network events.
  *
- * @note Direct assignment via setCount() does not clamp; only
+ * @note Values are clamped to [minValue, maxValue] on both setCount() and incrementCount().
- *       incrementCount() enforces int boundary saturation.
  * @note Unlike card counters, player counters are never auto-removed
  *       when they reach zero - they persist with value 0.
  */
@@ -45,9 +46,29 @@ protected:
     color counterColor;
     int radius;
     int count;
+    int minValue; ///< Minimum allowed value (default: INT_MIN, i.e. unbounded)
+    int maxValue; ///< Maximum allowed value (default: INT_MAX, i.e. unbounded)
+
+    static constexpr int DEFAULT_MAX_VALUE = std::numeric_limits<int>::max();
 
 public:
-    Server_Counter(int _id, const QString &_name, const color &_counterColor, int _radius, int _count = 0);
+    /**
+     * @brief Constructs a counter.
+     * @param _id Unique counter identifier
+     * @param _name Display name
+     * @param _counterColor Counter color
+     * @param _radius Display radius
+     * @param _count Initial value (default 0)
+     * @param _minValue Minimum allowed value (default INT_MIN)
+     * @param _maxValue Maximum allowed value (default INT_MAX)
+     */
+    Server_Counter(int _id,
+                   const QString &_name,
+                   const color &_counterColor,
+                   int _radius,
+                   int _count = 0,
+                   int _minValue = std::numeric_limits<int>::min(),
+                   int _maxValue = DEFAULT_MAX_VALUE);
     ~Server_Counter()
     {
     }
@@ -73,16 +94,15 @@ public:
     }
 
     /**
-     * @brief Sets the counter to an exact value.
+     * @brief Sets the counter value, clamping to [minValue, maxValue].
-     * @param _count The new value (assigned directly without clamping).
+     * @param _count The desired new value
-     * @return true if the value changed, false otherwise.
+     * @return true if the clamped value differs from the previous value
-     * @warning This performs raw assignment. For overflow-safe incrementing,
+     * @note For increment operations, prefer incrementCount() which handles overflow safely.
-     *          use incrementCount().
      */
     [[nodiscard]] bool setCount(int _count)
     {
-        const int oldCount = count;
+        int oldCount = count;
-        count = _count;
+        count = qBound(minValue, _count, maxValue);
         return count != oldCount;
     }
 
@@ -90,9 +110,14 @@ public:
      * @brief Increments the counter by delta with overflow-safe arithmetic.
      * @param delta The amount to add (may be negative for decrement).
      * @return true if the value changed, false otherwise.
-     * @note Clamps result to [INT_MIN, INT_MAX] to prevent overflow.
+     * @note Clamps result to [minValue, maxValue] to prevent overflow.
      */
-    [[nodiscard]] bool incrementCount(int delta);
+    [[nodiscard]] bool incrementCount(int delta)
+    {
+        const int oldCount = count;
+        count = addClamped(count, delta, minValue, maxValue);
+        return count != oldCount;
+    }
 
     /**
      * @brief Populates info with this counter's current state for network serialization.

libcockatrice_utility/libcockatrice/utility/trice_limits.h

@@ -2,6 +2,8 @@
 #define TRICE_LIMITS_H
 
 #include <QString>
+#include <QtGlobal>
+#include <cstdint>
 
 // max size for short strings, like names and things that are generally a single phrase
 constexpr int MAX_NAME_LENGTH = 0xff;
@@ -15,11 +17,23 @@ constexpr uint MAXIMUM_DIE_SIDES = 1000000;
 constexpr uint MINIMUM_DICE_TO_ROLL = 1;
 constexpr uint MAXIMUM_DICE_TO_ROLL = 100;
 
-// Card counter value bounds [0, MAX_COUNTERS_ON_CARD].
+// Counter value bounds [0, MAX_COUNTER_VALUE].
-// Counters on cards (e.g., +1/+1 counters, charge counters) are non-negative physical game objects.
+// Counters (on cards or players) are non-negative values.
 // The max of 999 is a display constraint (3-digit rendering) and reasonable gameplay limit.
 // Server enforces these bounds; client may also check for UX optimization.
-constexpr int MAX_COUNTERS_ON_CARD = 999;
+constexpr int MAX_COUNTER_VALUE = 999;
+
+/**
+ * @brief Overflow-safe clamped addition: returns value + delta bounded to [minValue, maxValue].
+ *
+ * Uses a 64-bit intermediate so the addition itself cannot overflow int. Shared by the
+ * counter arithmetic in Server_Card and Server_Counter so both stay in sync.
+ */
+inline int addClamped(int value, int delta, int minValue, int maxValue)
+{
+    const auto result = static_cast<int64_t>(value) + static_cast<int64_t>(delta);
+    return static_cast<int>(qBound(static_cast<int64_t>(minValue), result, static_cast<int64_t>(maxValue)));
+}
 
 // optimized functions to get qstrings that are at most that long
 static inline QString nameFromStdString(const std::string &_string)

tests/server_card_counter_test.cpp

@@ -28,9 +28,9 @@ TEST(ServerCardCounter, IncrementExistingCounter)
 TEST(ServerCardCounter, IncrementOverflowProtection)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE));
     EXPECT_FALSE(card.incrementCounter(1, 1));
-    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE);
 }
 
 TEST(ServerCardCounter, DecrementUnderflowProtection)
@@ -113,13 +113,13 @@ TEST(ServerCardCounter, IncrementCounterPopulatesEvent)
 TEST(ServerCardCounter, IncrementCounterEventReflectsClampedValue)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE - 5));
 
     Event_SetCardCounter event;
     EXPECT_TRUE(card.incrementCounter(1, 10, &event));
 
     EXPECT_EQ(event.counter_id(), 1);
-    EXPECT_EQ(event.counter_value(), MAX_COUNTERS_ON_CARD);
+    EXPECT_EQ(event.counter_value(), MAX_COUNTER_VALUE);
 }
 
 TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr)
@@ -133,7 +133,7 @@ TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr)
 TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE));
 
     Event_SetCardCounter event;
     event.set_counter_id(999);
@@ -156,7 +156,7 @@ TEST(ServerCardCounter, SetCounterClampsAboveMaxToMax)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
     EXPECT_TRUE(card.setCounter(1, 1500));
-    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE);
 }
 
 TEST(ServerCardCounter, IncrementDoesNotGoBelowZero)
@@ -171,9 +171,9 @@ TEST(ServerCardCounter, IncrementDoesNotGoBelowZero)
 TEST(ServerCardCounter, IncrementDoesNotExceedMax)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTER_VALUE - 5));
     EXPECT_TRUE(card.incrementCounter(1, 10));
-    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTER_VALUE);
 }
 
 int main(int argc, char **argv)

tests/server_counter_test.cpp

@@ -5,6 +5,7 @@
 
 #include <gtest/gtest.h>
 #include <libcockatrice/network/server/remote/game/server_counter.h>
+#include <libcockatrice/utility/trice_limits.h>
 #include <limits>
 
 TEST(ServerCounter, IncrementDoesNotOverflow)
@@ -79,6 +80,37 @@ TEST(ServerCounter, MixedExtremesDoNotClamp)
     EXPECT_EQ(c.getCount(), -1);
 }
 
+TEST(ServerCounter, SetCountClampsToCustomBounds)
+{
+    Server_Counter c(1, "test", color(), 10, 50, 0, 100);
+    EXPECT_TRUE(c.setCount(150));
+    EXPECT_EQ(c.getCount(), 100);
+    EXPECT_TRUE(c.setCount(-10));
+    EXPECT_EQ(c.getCount(), 0);
+}
+
+TEST(ServerCounter, IncrementClampsToCustomBounds)
+{
+    Server_Counter c(1, "test", color(), 10, 50, 0, 100);
+    EXPECT_TRUE(c.incrementCount(100));
+    EXPECT_EQ(c.getCount(), 100);
+    EXPECT_FALSE(c.incrementCount(1));
+    EXPECT_EQ(c.getCount(), 100);
+    EXPECT_TRUE(c.incrementCount(-200));
+    EXPECT_EQ(c.getCount(), 0);
+    EXPECT_FALSE(c.incrementCount(-1));
+    EXPECT_EQ(c.getCount(), 0);
+}
+
+TEST(ServerCounter, CustomBoundsClampToMaxCounterValue)
+{
+    Server_Counter c(1, "test", color(), 20, 0, 0, MAX_COUNTER_VALUE);
+    EXPECT_TRUE(c.setCount(1000));
+    EXPECT_EQ(c.getCount(), MAX_COUNTER_VALUE);
+    EXPECT_TRUE(c.setCount(-5));
+    EXPECT_EQ(c.getCount(), 0);
+}
+
 int main(int argc, char **argv)
 {
     ::testing::InitGoogleTest(&argc, argv);