Use CXX11 pseudo-random engine and distributions for all RNG

[Muffindrake]

Summary

SUMMARY: Infrastructure "Use CXX11 pseudo-random engine for all RNG"

Purpose of change

Avoid bias introduced through int -> float -> int bit discard, and potentially terrible PRNG implementations. Have relatively more consistent runtime for RNG code, rather than completely unpredictable rand() runtime. Clean up code.

Describe the solution

• use single static mt19937 random engine with several static number distributions
• replace wild double casts with uniform int/real distributions
• touch on code that relies on rand()

[kevingranade]

Just FYI, I'm going to runoff mt19937 vs std::minstd_rand and some of the other options and keep the speed winner. MT is absolutely not necessary for the workload we're throwing at it, but we can go with that one if there's no perf regression from using it.

[jbytheway]

Thanks for working on this. Now I can cross it off my TODO list :).

[jbytheway]

Thanks for working on this. Now I can cross it off my TODO list :).

[jbytheway]

Why the switch to long? Just because rng takes long arguments? Use of long tends to lead to odd platform-specific issues so I'd prefer to move in the direction of less long, not more (but that's a personal opinion).

[Muffindrake]

Consistency's sake. What the function actually needed was unsigned arguments (and you probably want to be using uint_leastN_t types because that makes the most sense), because there's no reason for any of the parameters or return value to be negative, but hardly any part of the code base cares about writing correct code. Signed types are used willy-nilly like we're in Java land where everything is signed and wraps.

long is 32-bit on MSVC and 64-bit on Linux.

[kevingranade]

Consistency with what? The rest of the project uses int, unless you have a demonstrable problem that's caused by using int here, this is more likely to break things than to fix anything.

[Muffindrake]

Consistency with the rest of the random number functions (they use long).

Besides, this makes no difference on MSVC (sizeof (long) == sizeof (int)). The only time this is going to cause issues is when the code using the dice function was broken to begin with - a dice roll so big that it would have overflowed an int (which may still be a problem with the current code). As such there is no reason to deny changing it here.

None of the (mostly trivial) uses of this function do anything crazy with large values - changing the signature to longs will not cause any bugs in reasonable code.

[jbytheway]

Rather than rolling your own implementation here you could use std::uniform_int_distribution<uintmax_t>(0, std::numeric_limits<uintmax_t>::max()). It may be slower, but probably not significantly so.

[Muffindrake]

I noticed that uniform_real_distribution isn't inclusive-inclusive, unlike uniform_int_distribution. I fixed that now. Thanks for nothing, CXX standard committee.

Also, what the fuck does astyle want me to do with the indentation there? I already have little tolerance for automatic code formatting tools, but that looks like a bug to me nonetheless.

[Muffindrake]

It turned out to be just a missing parenthesis, which would have been caught by the compiler, but astyle assumes that the code is correct.

Please run astyle after the code has compiled, if not remove it altogether.

[jbytheway]

I noticed that uniform_real_distribution isn't inclusive-inclusive, unlike uniform_int_distribution. I fixed that now. Thanks for nothing, CXX standard committee.

This may seem inconsistent, but the committee has good reasons for this design, and we probably shouldn't deviate from it without good reason. It's what C++ devs are accustomed to. It's also how the old rng_float worked. In particular, the intention is that rng_float(0, 1) < p should be true with probability p, which is no longer the case after your change.

[jbytheway]

LGTM

[jbytheway]

LGTM

[Muffindrake]

What's the holdup on merging this?

[kevingranade]

You haven't supported your decision to switch to a merseinne twister prng, so I need to write a benchmark for it and verify that the slowdown doesnt impact the game, and that means it's on hold since this isn't a priority for me.
Also you've ignored jbtw and my feedback on the int/long thing.

[Muffindrake]

You haven't supported your decision to switch to a merseinne twister prng, so I need to write a benchmark for it and verify that the slowdown doesnt impact the game, and that means it's on hold since this isn't a priority for me.
Also you've ignored jbtw and my feedback on the int/long thing.

I've literally gone through the entire code base and looked at every use of the functions, none of which will break if the signature is changed to longs. The more base reason is that you can now use rng() with dice() without possible reduction in range because of different integer widths.

glibc's rand() implementation is so slow that there's no way MT is in any way slower or at least not significantly so - in my testing I haven't experienced any noticeable slowdown whatsoever. If you are worried that this might cause any nebulous performance issues anyway, merge this now and change the algorithm later. My money is on the fact that nobody will notice any slowdown besides crazy people running this on an i386.

jbtw literally wants this merged now - the blocker is you.

[Muffindrake]

So, I'm getting quite tired of this nonsense - let me break this down for you.

The current implementation:

• It's idiotstack copy-paste garbage without a second thought put into it
• Has exactly zero guarantees as to what numbers, whether numbers output even have to be different, or (which you require proof for, despite not bringing any proof yourself that the current algorithm is okay)
• Uses floating point (and floating point division at that) for no palpable reason
• Isn't platform-agnostic. We offered to make mapgen consistent between systems at some point, which would require splitting PRNG internal states and only using functions specific to their PRNG state, that is making mapgen and combat manipulate different states, for example. You struck that down, because you could and you provided no compelling reason as to why
• May be really fast if the rand implementation is an LCG or something - and in the case that double floating point is also fast on the given platform. You pointed out at one point that you test this game on a very resource-constrained platform. Since we're talking about frivolous garbage - some platforms don't deal with floating point/double precision floating point particularly well. Some even have to emulate them in software (see e.g. ARM soft float)

This PR (the precise algorithm of choice for the PRNG is irrelevant and could be trivially switched using compile time options, but I'll entertain MT for now):

• The algorithm choice, from a statistical perspective, is clearly and obviously not worse than 99.99% of all rand implementations out there. Many statistical tests are unable to distinguish this algorithm from an actual random device and none of the output bits are particularly biased to any degree. You wanted proof that an algorithm fulfilling these criteria (see my last attempt to replace the PRNG in this game) is not worse than many implementations of rand that are hot garbage in light of statistical tests. Are you okay, buddy?
• Uses far more internal state than, say, an LCG. This has performance impact on platforms with undersized caches
• No uncalled-for floating point in sight
• Has no tangible performance impact on a machine built in the last 10 years with how the game uses it in this PR. None. Zero. Go find other senseless arguments to stonewall people to death with

Crawl has issues with accepting new contributions (they're not welcome unless you're friends with the existing dev team, and frivolous "game philosophy" reasons will be provided to strike you down where you stand), Nethack has had a policy of "one dev may veto any change" (Nethack is dead), CDDA has a lead developer who requires volumes of pages of academically-verified proof to accept any singular technical change to the game, despite almost never bringing any proof saying that the current technical direction is in any way better than the proposed change. Or he will say that and then put the change on hold for 6 months, at which point the original contributor has lost interest and left the project. Unless of course that technical change is brought on by himself, in which case it's mostly just committed.

This project has a lot of contributors - one-time contributors. Ostensibly, this is one of the better games out there, so everyone must be dying to work and improve on it every waking minute of the day. They stopped contributing? Clearly RL must have thrown a wrench at them at some point - the development process, which ends at frustrating stone walls erected for the fun of it, not having anything to do with it.

Have you considered that you're the reason why this project has massive issues retaining its contributors, despite its premise (a sandbox survival turn-based game) being so great?

[kevingranade]

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/the-issue-of-kev-jong-un/18526/2

[ZhilkinSerg]

Marking as stalled.

[kevingranade]

Ok this just needs some light testing and it's ready to go (please squash it if you merge it).

[ZhilkinSerg]

Jenkins rebuild.

[jbytheway]

Looking mostly reasonable, but I have a few comments.

[jbytheway]

Looking mostly reasonable, but I have a few comments.

[kevingranade]

Still needs a rebase, I know some actual conflicts have crept in.

[jbytheway]

Looks like this might be a merge resolution mistake?

[kevingranade]

Yes it was, and I've cleaned it up.

[kevingranade]

Rebased to latest and squashed a bunch of noisy commits.

[jbytheway]

LGTM, but should probably be rebased again to get a full clean test run.

[jbytheway]

LGTM, but should probably be rebased again to get a full clean test run.