Replace rnd macros with STL alternatives

[voidanix]

Should address #191

Picked ranlux because it seems faster (and is less predictable) but any PRNG should do

EDIT: mt19937 is back again

[robalni]

I don't think we should include c++ standard library headers in common header files like tools.h because it makes the build time slow. I don't even think this is needed in this pull request because <random> is not used in tools.h.

[TheAssassin]

We could introduce a random.h, for instance, and include that wherever needed.

Re. ranlux48, I guess it makes sense not to hardcode an implementation (we never know where the game will be compiled) but rather use default_random_engine. OTOH, picking one will ensure results are reproducible. Regarding its performance, I don't think the overhead of an MT is really critical, but we're not using these for serious cryptographic work anyway, so I think that one's fine, too.

[TheAssassin]

Thanks a lot! Looks pretty good to me: you've made the right assumptions about the old code, and replaced them correctly with STL stuff.

I left some feedback below.

[TheAssassin]

We should provide a short comment why we store our seed statically. Of course it makes sense to do so (otherwise you could just use the random device directly, which is, in itself, a form of RNG). Future readers however might not be into PRNGs as much.

[TheAssassin]

I would appreciate a more descriptive method name, and perhaps a docstring.

[TheAssassin]

It's a good idea to replace these macros.

[TheAssassin]

I assume the following mapping now, given their previous use:

• rnd -> rnd
• rndscale -> rndfloat
• detrnd -> detrnd
• randomMT -> tmprnd

In order to allow people who are used to the old code understand the new format, I think it makes sense to add docstrings to the new functions, explaining what the new ones do.

[TheAssassin]

We should probably not do this. @robalni has had a very good point, which is STL headers likely slowing down compilation. I think a random.h is more appropriate for these new declarations.

[TheAssassin]

Regarding ranlux48 vs. MT19937, I've come to another conclusion than you, performance wise. I've put together some (very shady but good enough) "benchmark".

#include <random>

int main() {
    std::random_device rndseed;
    //std::ranlux48 rndalg(rndseed());
    std::mt19937 rndalg(rndseed());
    for (uint64_t i = 0; i < (1ULL << 24); ++i) {
        rndalg();
    } 
}

With 1 << 24 samples, ranlux48 takes around 8.5 seconds in average (~20 passes), whereas MT needs around 0.3 seconds. So, on my computer, the MT implementation outperforms ranlux easily. (Just for comparison, ranlux24 takes around 2.6 seconds.)
Hooking up an std::uniform_int_distribution<> dist(0, (1ULL << 15));, I get around 0.75 seconds for MT, 2.95 seconds for ranlux24, and 9.15 seconds for ranlux48. The slowdown seems somewhat constant, which means we can exclude it for our assessment.
These results seem to confirm some way better analyses on the Internet, which all confirm that performance wise, the MT is faster by some orders of magnitude.

We should discuss what we want. In games, I think "no predictability" (which is actually just a security property) isn't necessarily the goal. Performance, however, is. We should not use any of these PRNGs for serious work, as they're just not good at it (boost-random is significantly different, but also a lot better than what you get in the STL). We just need a reasonably well-distributed random number generation. MT19937 will provide that, but, looking at some Internet suggestions, it might even be overkill (many games just use xor shift algorithms for tasks like randomizing spawns or weapons). Given that we've been using MT so far, I think we should just continue to do so.

I am eager to see how that (shady) benchmark performs on @voidanix's computer. I can't reproduce that ranlux should run faster than MT.

[TheAssassin]

I understand detrnd is supposed to mean "deterministic random number", but I'm not sure what properties the implementation is supposed to provide. We might want to check its uses. The only reason I could come up with where you'd want to specify your own seed is to get the same results on more than one host. But that seems like a bad idea.

Regarding the reproducibility of the results of std::uniform_int_distribution, it is not stable over different implementations. While it will always return the same sequence of numbers in, e.g., libstdc++, libc++ differs.

Here's an example:

#include <iostream>
#include <random>

int main() {
    // https://xkcd.com/221/
    std::mt19937 rnd(4);
    std::uniform_int_distribution<> dist(0, (1ULL << 16));
    std::cout << dist(rnd) << std::endl;
}

With GCC 7.5.0 with libstdc++, it prints 63376. With Clang 9 and libc++, it gives you 11863. So, the deterministic property of this little function is only provided if you can guarantee the same STL implementation is used. Hence, depending on the purpose of this function, the entire concept should be reviewed in the context of Blue Nebula.

Might be worth creating a new issue.

[voidanix]

afaik detrnd provides more "consistent" and thus "closer" results than rnd, which for example results useful for picking a certain range of colors for exploding mines (which tend to turn red when using rnd) or the flashes on the player which holds the rocket (it's way too fast with rnd), so it's not about getting the same results.
std::uniform_int_distribution is a non-issue as the results don't need to be reproducible between different STL implementations

[TheAssassin]

afaik detrnd provides more "consistent" and thus "closer" results than rnd

Well, it has to do something with predictability, obviously, as the seed is defined by the using code. The intention must be to get reproducibe results, at least on the same machine.

If you feed detrnd with the same seed (which is, e.g., calculated from position data, in some function), then you'll get the same result, at least on that machine. I don't understand why that's needed, though. There must be reason for it.

Can you point me to where these colors are calculated, please?

[voidanix]

Player pulsing (also affects burn and shock):

base/src/game/game.cpp

Line 842 in 0da5037

int n = detrnd(seed, PULSECOLOURS), n2 = detrnd(seed + 1, PULSECOLOURS), q = clamp(i, 0, int(PULSE_LAST));

Mines and nades:

base/src/engine/lightning.h

Line 41 in 0da5037

int jitteroffset = detrnd(int(d.x+d.y+d.z), MAXLIGHTNINGSTEPS);

[voidanix]

We could introduce a random.h, for instance, and include that wherever needed.

Re. ranlux48, I guess it makes sense not to hardcode an implementation (we never know where the game will be compiled) but rather use default_random_engine. OTOH, picking one will ensure results are reproducible. Regarding its performance, I don't think the overhead of an MT is really critical, but we're not using these for serious cryptographic work anyway, so I think that one's fine, too.

The problem with default_random_engine is that it's implementation defined, so it could as well go and pick the slowest of the bunch.

As for a random.h, I guess it's fine just to define <random> in tools.cpp? It shaves around 6 seconds on my machine and it's used only there

[voidanix]

Regarding ranlux48 vs. MT19937, I've come to another conclusion than you, performance wise. I've put together some (very shady but good enough) "benchmark".

#include <random>

int main() {
    std::random_device rndseed;
    //std::ranlux48 rndalg(rndseed());
    std::mt19937 rndalg(rndseed());
    for (uint64_t i = 0; i < (1ULL << 24); ++i) {
        rndalg();
    } 
}

With 1 << 24 samples, ranlux48 takes around 8.5 seconds in average (~20 passes), whereas MT needs around 0.3 seconds. So, on my computer, the MT implementation outperforms ranlux easily. (Just for comparison, ranlux24 takes around 2.6 seconds.)
Hooking up an std::uniform_int_distribution<> dist(0, (1ULL << 15));, I get around 0.75 seconds for MT, 2.95 seconds for ranlux24, and 9.15 seconds for ranlux48. The slowdown seems somewhat constant, which means we can exclude it for our assessment.
These results seem to confirm some way better analyses on the Internet, which all confirm that performance wise, the MT is faster by some orders of magnitude.

We should discuss what we want. In games, I think "no predictability" (which is actually just a security property) isn't necessarily the goal. Performance, however, is. We should not use any of these PRNGs for serious work, as they're just not good at it (boost-random is significantly different, but also a lot better than what you get in the STL). We just need a reasonably well-distributed random number generation. MT19937 will provide that, but, looking at some Internet suggestions, it might even be overkill (many games just use xor shift algorithms for tasks like randomizing spawns or weapons). Given that we've been using MT so far, I think we should just continue to do so.

I am eager to see how that (shady) benchmark performs on @voidanix's computer. I can't reproduce that ranlux should run faster than MT.

# mt19937
./a.out  0.25s user 0.00s system 99% cpu 0.253 total

# ranlux48
./a.out  7.49s user 0.37s system 97% cpu 8.075 total

Here I'm guessing that my way of testing this was stupid: I moved std::random_device inside rnd and tested both mt19937 and ranlux48; ranlux48 gave me ~60fps more but that could be because it didn't call /dev/urandom as frequently.

So yes, I will switch back to std::mt19937

[TheAssassin]

As for a random.h, I guess it's fine just to define in tools.cpp? It shaves around 6 seconds on my machine and it's used only there.

Given your current implementation, yeah, that'll do. It's anyway best practice to include headers only where needed, in the smallest possible unit.