RNG: added additional checks (which forced an interface redesign), updated comments

common/rng_abstract.cpp

@@ -7,9 +7,9 @@ QVector<int> RNG_Abstract::makeNumbersVector(int n, int min, int max)
 	const int bins = max - min + 1;
 	QVector<int> result(bins);
 	for (int i = 0; i < n; ++i) {
-		int number = getNumber(min, max);
+		int number = rand(min, max);
 		if ((number < min) || (number > max))
-			qDebug() << "getNumber(" << min << "," << max << ") returned " << number;
+			qDebug() << "rand(" << min << "," << max << ") returned " << number;
 		else
 			result[number - min]++;
 	}

common/rng_abstract.h

@@ -8,7 +8,7 @@ class RNG_Abstract : public QObject {
 	Q_OBJECT
 public:
 	RNG_Abstract(QObject *parent = 0) : QObject(parent) { }
-	virtual unsigned int getNumber(unsigned int min, unsigned int max) = 0;
+	virtual unsigned int rand(int min, int max) = 0;
 	QVector<int> makeNumbersVector(int n, int min, int max);
 	double testRandom(const QVector<int> &numbers) const;
 };

common/rng_sfmt.cpp

@@ -17,6 +17,44 @@ RNG_SFMT::RNG_SFMT(QObject *parent)
 	sfmt_init_gen_rand(&sfmt, QDateTime::currentDateTime().toTime_t());
 }
 
+/**
+ * This method is the rand() equivalent which calls the cdf with proper bounds.
+ *
+ * It is possible to generate random numbers from [-min, +/-max] though the RNG uses
+ * unsigned numbers only, so this wrapper handles some special cases for min and max.
+ *
+ * It is only necessary that the upper bound is larger or equal to the lower bound - with the exception
+ * that someone wants something like rand() % -foo.
+ */
+unsigned int RNG_SFMT::rand(int min, int max) {
+  /* If min is negative, it would be possible to calculate
+   * cdf(0, max - min) + min
+   * There has been no use for negative random numbers with rand() though, so it's treated as error.
+   */
+  if(min < 0) {
+	throw std::invalid_argument(
+	  QString("Invalid bounds for RNG: Got min " +
+	  QString::number(min) + " < 0!\n").toStdString());
+	// at this point, the method exits. No return value is needed, because
+	// basically the exception itself is returned.
+  }
+
+  // For complete fairness and equal timing, this should be a roll, but let's skip it anyway
+  if(min == max)
+    return max;
+
+  // This is actually not used in Cockatrice:
+  // Someone wants rand() % -foo, so we compute -rand(0, +foo)
+  // This is the only time where min > max is (sort of) legal.
+  // Not handling this will cause the application to crash.
+  if(min == 0 && max < 0) {
+    return -cdf(0, -max);
+  }
+
+  // No special cases are left, except !(min > max) which is caught in the cdf itself.
+  return cdf(min, max);
+}
+
 /**
  * Much thought went into this, please read this comment before you modify the code.
  * Let SFMT() be an alias for sfmt_genrand_uint64() aka SFMT's rand() function.
@@ -33,7 +71,7 @@ RNG_SFMT::RNG_SFMT(QObject *parent)
  *
  * To get out the random variable, solve for X:
  * floor(X) = SFMT(X; min, max) * (max - min + 1) + min - 1
- * So this is, what getNumber(min, max) should look like.
+ * So this is, what rand(min, max) should look like.
  * Problem: SFMT(X; min, max) * (max - min + 1) could produce an integer overflow,
  * so it is not safe.
  *
@@ -56,19 +94,17 @@ RNG_SFMT::RNG_SFMT(QObject *parent)
  * then you _need_ to change the UINT64_MAX constant to the largest possible random 
  * number which can be created by the new rand() function. This value is often defined
  * in a RAND_MAX constant.
- * Otherwise you will probably skew the outcome of the getNumber() method or worsen the
+ * Otherwise you will probably skew the outcome of the rand() method or worsen the
  * performance of the application.
  */
-unsigned int RNG_SFMT::getNumber(unsigned int min, unsigned int max)
+unsigned int RNG_SFMT::cdf(unsigned int min, unsigned int max)
 {
 	// This all makes no sense if min > max, which should never happen.
 	if(min > max) {
 		throw std::invalid_argument(
 		  QString("Invalid bounds for RNG: min > max! Values were: min = " +
 		  QString::number(min) + ", max = " +
-		  QString::number(max) +
-		  ". This is either a bug or something even more serious happened."
-		  ).toStdString());
+		  QString::number(max)).toStdString());
 		// at this point, the method exits. No return value is needed, because
 		// basically the exception itself is returned.
 	}

common/rng_sfmt.h

@@ -1,27 +1,28 @@
 #ifndef RNG_SFMT_H
 #define RNG_SFMT_H
 
+#include <climits>
+#include <QMutex>
 #include "sfmt/SFMT.h"
 #include "rng_abstract.h"
-#include <QMutex>
 
 /**
- * This class represents the random number generator.
- * Usage instructions:
- * Create an instance of RNG_SFMT, then call getNumber(min, max).
- * You should never call this function with min > max, this throws a
- * std::invalid_argument exception. It is best practice to use getNumber() in a try block
- * and catch the exception if min, max are entered by the user or computed somehow.
+ * This class encapsulates a state of the art PRNG and can be used
+ * to return uniformly distributed integer random numbers from a range [min, max].
+ * Though technically possible, min must be >= 0 and max should always be > 0.
+ * If max < 0 and min == 0 it is assumed that rand() % -max is wanted and the result will
+ * be -rand(0, -max).
+ * This is the only exception to the rule that !(min > max) and is actually unused in
+ * Cockatrice.
  *
  * Technical details:
  * The RNG uses the SIMD-oriented Fast Mersenne Twister code v1.4.1 from
  * http://www.math.sci.hiroshima-u.ac.jp/~%20m-mat/MT/SFMT/index.html
- * To use this RNG, the class needs a sfmt_t structure for the RNG's internal state.
- * It has to be initialized by sfmt_init_gen_rand() which is done in the constructor.
- * The function sfmt_genrand_uint64() can then be used to create a 64 bit unsigned int 
- * pseudorandom number. This is done in getNumber().
- * For more information see the author's website and look at the documentation and 
- * examples that are part of the official downloads.
+ * The SFMT RNG creates unsigned int 64bit pseudo random numbers.
+ *
+ * These are mapped to values from the interval [min, max] without bias by using Knuth's
+ * "Algorithm S (Selection sampling technique)" from "The Art of Computer Programming 3rd
+ * Edition Volume 2 / Seminumerical Algorithms".
  */
 
 class RNG_SFMT : public RNG_Abstract {
@@ -29,9 +30,11 @@ class RNG_SFMT : public RNG_Abstract {
 private:
 	QMutex mutex;
 	sfmt_t sfmt;
+	// The discrete cumulative distribution function for the RNG
+	unsigned int cdf(unsigned int min, unsigned int max);
 public:
 	RNG_SFMT(QObject *parent = 0);
-	unsigned int getNumber(unsigned int min, unsigned int max);
+	unsigned int rand(int min, int max);
 };
 
 #endif

common/server_cardzone.cpp

@@ -45,7 +45,7 @@ void Server_CardZone::shuffle()
 {
 	QList<Server_Card *> temp;
 	for (int i = cards.size(); i; i--)
-		temp.append(cards.takeAt(rng->getNumber(0, i - 1)));
+		temp.append(cards.takeAt(rng->rand(0, i - 1)));
 	cards = temp;
 	
 	playersWithWritePermission.clear();

common/server_player.cpp

@@ -844,7 +844,7 @@ Response::ResponseCode Server_Player::cmdRollDie(const Command_RollDie &cmd, Res
 	
 	Event_RollDie event;
 	event.set_sides(cmd.sides());
-	event.set_value(rng->getNumber(1, cmd.sides()));
+	event.set_value(rng->rand(1, cmd.sides()));
 	ges.enqueueGameEvent(event, playerId);
 	
 	return Response::RespOk;
@@ -1524,7 +1524,7 @@ Response::ResponseCode Server_Player::cmdRevealCards(const Command_RevealCards &
 	else if (cmd.card_id() == -2) {
 		if (zone->getCards().isEmpty())
 			return Response::RespContextError;
-		cardsToReveal.append(zone->getCards().at(rng->getNumber(0, zone->getCards().size() - 1)));
+		cardsToReveal.append(zone->getCards().at(rng->rand(0, zone->getCards().size() - 1)));
 	} else {
 		Server_Card *card = zone->getCard(cmd.card_id());
 		if (!card)