2
\$\begingroup\$

I am trying to translate the book Game Coding Complete by Mike McShaffry 4E to modern C++17 standard and faced with the code of Mersenne Twister by Takuji Nishimura and Makoto Matsumoto. Is it right to convert random generator code from book like that:

from:

#include <ctime>
#define CMATH_N 624
#define CMATH_M 397
#define CMATH_MATRIX_A 0x9908b0df   /* constant vector a */
#define CMATH_UPPER_MASK 0x80000000 /* most significant w-r bits */
#define CMATH_LOWER_MASK 0x7fffffff /* least significant r bits */
#define CMATH_TEMPERING_MASK_B 0x9d2c5680
#define CMATH_TEMPERING_MASK_C 0xefc60000
#define CMATH_TEMPERING_SHIFT_U(y)  (y >> 11)
#define CMATH_TEMPERING_SHIFT_S(y)  (y << 7)
#define CMATH_TEMPERING_SHIFT_T(y)  (y << 15)
#define CMATH_TEMPERING_SHIFT_L(y)  (y >> 18)
typedef int INT;
typedef unsigned int UINT;
#define MAXUINT ((UINT)~((UINT)0))
#define MAXINT ((INT)(MAXUINT >> 1))
class GCCRandom {
private:
    unsigned int rseed;
    unsigned int rseed_sp;
    unsigned long mt[CMATH_N]; /* the array for the state vector  */
    int mti; /* mti==N+1 means mt[N] is not initialized */
public:
    GCCRandom() {
        rseed = 1;
        rseed_sp = 0;
        mti = CMATH_N + 1;
    }
    unsigned int Random(unsigned int n) {
        unsigned long y;
        static unsigned long mag01[2] = { 0x0, CMATH_MATRIX_A };
        if (n == 0)
            return(0);
        if (mti >= CMATH_N) { /* generate N words at one time */
            int kk;
            if (mti == CMATH_N + 1)   /* if sgenrand() has not been called, */
                SetRandomSeed(4357); /* a default initial seed is used   */
            for (kk = 0; kk < CMATH_N - CMATH_M; kk++) {
                y = (mt[kk] & CMATH_UPPER_MASK) | (mt[kk + 1] & CMATH_LOWER_MASK);
                mt[kk] = mt[kk + CMATH_M] ^ (y >> 1) ^ mag01[y & 0x1];
            }
            for (; kk < CMATH_N - 1; kk++) {
                y = (mt[kk] & CMATH_UPPER_MASK) | (mt[kk + 1] & CMATH_LOWER_MASK);
                mt[kk] = mt[kk + (CMATH_M - CMATH_N)] ^ (y >> 1) ^ mag01[y & 0x1];
            }
            y = (mt[CMATH_N - 1] & CMATH_UPPER_MASK) | (mt[0] & CMATH_LOWER_MASK);
            mt[CMATH_N - 1] = mt[CMATH_M - 1] ^ (y >> 1) ^ mag01[y & 0x1];
            mti = 0;
        }
        y = mt[mti++];
        y ^= CMATH_TEMPERING_SHIFT_U(y);
        y ^= CMATH_TEMPERING_SHIFT_S(y) & CMATH_TEMPERING_MASK_B;
        y ^= CMATH_TEMPERING_SHIFT_T(y) & CMATH_TEMPERING_MASK_C;
        y ^= CMATH_TEMPERING_SHIFT_L(y);
        return (y % n);
    }
    float Random() {
        float r = (float)Random(MAXINT);
        float divisor = (float)MAXINT;
        return (r / divisor);
    }
    void SetRandomSeed(unsigned int n) {
        /* setting initial seeds to mt[N] using         */
        /* the generator Line 25 of Table 1 in          */
        /* [KNUTH 1981, The Art of Computer Programming */
        /*    Vol. 2 (2nd Ed.), pp102]                  */
        mt[0] = n & 0xffffffff;
        for (mti = 1; mti < CMATH_N; mti++)
            mt[mti] = (69069 * mt[mti - 1]) & 0xffffffff;
        rseed = n;
    }
    unsigned int GetRandomSeed() {
        return rseed;
    }
    void Randomize() {
        SetRandomSeed((unsigned int)time(NULL));
    }
};

to code with std::mt19937 engine and std::uniform_int_distribution:

#include <random>
#include <limits>
#include <chrono>
class MTRandom {
private:
    unsigned int m_rseed = 1;
    unsigned int m_rseed_sp = 0;
    static const unsigned int MAXINT = std::numeric_limits<int>::max();
    std::mt19937 m_engine;
public:
    MTRandom() { m_engine.seed(m_rseed); }
    unsigned int Random(unsigned int n) {
        std::uniform_int_distribution<unsigned int> distr(0, n);
        return distr(m_engine);
    };
    float Random() {
        std::uniform_real_distribution<float> distr(0.0f, 1.0f);
        return distr(m_engine);        
    }
    void SetRandomSeed(unsigned int n) {
        m_engine.seed(n);
        m_rseed = n;
    }
    unsigned int GetRandomSeed() {
        return m_rseed;
    }
    void Randomize() {
        SetRandomSeed((unsigned int)std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()));
    }
};
\$\endgroup\$
1
\$\begingroup\$

The short answer is (mostly) yes. Your use of C++17 and std::mt19937 looks mostly correct.

However:

  • You have std::uniform_int_distribution<unsigned int> distr(0, n), and this generates random values from 0 to n inclusive. Your original code appears to generate values from 0 to n-1 inclusive. So you should use std::uniform_int_distribution<unsigned int> distr(0, n-1). In addition, before doing that, you would need to check whether n equals zero:

    unsigned int Random(unsigned int n) {
        if (n == 0)
            return 0;
        std::uniform_int_distribution<unsigned int> distr(0, n-1);
        return distr(m_engine);
    };
    
  • Your code declares m_rseed_sp and MAXINT, both of which are not used. It would be better to remove them. Then you can also remove #include <limits>.

  • It is probably slightly more efficient to create member variables for std::uniform_int_distribution and for std::uniform_real_distribution. For std::uniform_real_distribution this is trivial because you never change the parameters of the distribution. For std::uniform_int_distribution, it is slightly more complicated because now you have to create parameters and pass them to the distribution. Something like this (untested):

    private:
        using IntDistribution = std::uniform_int_distribution<unsigned int>;
        IntDistribution m_int_distribution;
    
    public:
        unsigned int Random(unsigned int n) {
            if (n == 0)
                return 0;
            IntDistribution::param_type parameters{0, n-1};
            return m_int_distribution(m_engine, parameters);
        };
    
  • For the Randomize() function, you might consider something like this instead:

    void Randomize() {
        std::random_device randomDevice;
        SetRandomSeed(randomDevice());
    }
    

    Ideally, this seems better than just using the current time. However, I've seen rumors that std::random_device is not implemented well on a few platforms, so maybe what you have is better? That's a judgement call.

  • Note: There is no guarantee that std::mt19937 will initialize its internal state identically to the way your old code did, in fact I think it is very unlikely. So you can't expect identical output even if you use the same seed.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.