1

Version 3 Moved generator into a class. Is my new random number generation technique correct this time?

template<typename T = int>
    class MyRandomGenerator
    {
    public:
        MyRandomGenerator()
        {
            std::random_device rd;
            gen = std::mt19937{rd()};
        }

        T getNumber(const T& From, const T& To)
        {
            std::uniform_int_distribution<T> dist(From, To);
            return dist(gen);
        }
    private:
        std::mt19937 gen;
    };

And here my new position calculators:

auto calculate_x_position = [&]() ->sf::Uint32 {return RandGen.getNumber(0, W - 1); };
auto calculate_y_position = [&]() ->sf::Uint32 {return RandGen.getNumber(0, H - 1); };

Same pattern/problem as before: enter image description here

Version 2 Creating ~3 maps per second of 10.000 stars per map. The outcome is still the same. Moreover, it gets even more clear that there is a pivot to the top of the maps. Example: enter image description here

Original Question: i went into trying to draw a simple star map. For this i first calculate the x and y position for the stars. X and Y are within the range of width and height of the window.

This is my random number generating function:

    template<typename T>
    T getRandomNumberBetween(const T& Min, const T& Max)
    {
        static std::random_device rd;
        static std::uniform_int_distribution<T> dist{Min, Max};
        return dist(rd);
    }

And i use it like this:

auto calculate_x_position = std::bind(inc::getRandomNumberBetween<sf::Uint32>, 0, Width-1);
    auto calculate_y_position = std::bind(inc::getRandomNumberBetween<sf::Uint32>, 0, Height-1);
x = calculate_x_position(); //...

But as i draw my map over and over again it seems to me that there is a pivot to where most stars tend to be. E.g. the majority of stars are in the upper half of my window.

Example:

enter image description here

Am i using it wrong or having a wrong expectation here?... Because here it says:

This distribution produces random integers in a range [a,b] where each possible value has an equal likelihood of being produced.

Kind Regards

Juarrow
  • 2,232
  • 5
  • 42
  • 61
  • 6
    you are always constructing a new distribution object, random device object and generator object upon each call… you are seeding it with the random engine's return value, which may be very badly distributed. – The Paramagnetic Croissant Mar 21 '15 at 14:00
  • 1
    @TheParamagneticCroissant Well, distribution is stateless, so that's not a problem. The problem is constructing a new random engine on each call. – Igor Tandetnik Mar 21 '15 at 14:02
  • 1
    @The Paramagnetic Croissant but since the entropy of my random_device is greather 0 shouldnt the randomness of the seed cope with this? – Juarrow Mar 21 '15 at 14:03
  • @IgorTandetnik I think generating a new instance of all three is a bad idea altogether. Added each class to my comment. – The Paramagnetic Croissant Mar 21 '15 at 14:04
  • 1
    @The Paramagnetic Croissant,IgorTandetnik: did modify all 3 to be static. Did not change the quality of the output. – Juarrow Mar 21 '15 at 14:06
  • 1
    Random number generators are designed and tested to ensure that a sequence produced by a given generator has certain "good" statistical properties. No one designs and tests generators to ensure that repeatedly creating a new generator and taking exactly one number from it would produce a sequence with good statistical properties. If you feel your seed is already sufficiently random, why bother with the generator at all? Just pass the seed directly to the distribution. – Igor Tandetnik Mar 21 '15 at 14:06
  • Maybe the implementation of `random_device` you're using has issues? Have you tried using a `mt19937` instead? – dyp Mar 21 '15 at 16:14
  • Your new code doesn't use a random generator at all. Which demonstrates that values coming out of `random_device` are not in fact sufficiently random on their own (which is not surprising). – Igor Tandetnik Mar 21 '15 at 17:15
  • The distribution shouldn't be `static` the way you are doing it, since otherwise it's initialized only on the first call, so even if you pass a different range later it would be ignored. – T.C. Mar 22 '15 at 06:17
  • Your `static std::uniform_int_distribution dist{Min, Max};` created only once for first call, so it will generate numbers only in `[0, width-1]` range, not `[0, height-1]`. – vladon May 07 '15 at 10:39

2 Answers2

4

Uniform distribution does not specifically mean that you will get an equal amount in each quadrant of the screen; It means that there is an equal chance of each point appearing. So if you do a simple 50:50 trial, it does not mean you will get 50:50 results.

However if you did a test with say 1,000,000 stars it is very likely that they will be nearly uniformly distributed. To me this seems like an error in sample size

Seraphim
  • 181
  • 8
  • 3
    In addition, the uniform distribution only provides that any given *value* will have an equal chance of appearing. As a result, to ensure that *points* achieve a uniform distribution would require significantly more samples than just along a single dimension. – Rollen Mar 21 '15 at 14:14
0

There are a couple of ways you could improve your code. An engine is only meant to be created once, not everytime you want a random number. And you usually don't want to use std::default_random_engine because it may default to std::rand. A better default is std::mt19937. My example is pulled from The bell has tolled for rand():

#include <random>

std::mt19937& prng_engine()
{
  thread_local static std::random_device rd{};
  thread_local static std::mt19937 engine{rd()};

  // Or you can replace the two previous lines with:
  //thread_local static std::mt19937
  //  prng{std::random_device{}()};

  return engine;
}

template<typename T>
T getRandomNumberBetween(T Min, T Max)
{
    thread_local static std::uniform_int_distribution<T> dist{Min, Max};
    return dist(prng_engine());
}
  • 2
    `static local dist` will be created once for very first set of `{Min, Max}`, and will not be recreated for another set, so it's wrong – vladon May 07 '15 at 09:20