Partial rng replace

[qndel]

Replaces crappy vanilla rng with a decent one for everything that doesn't break save compatibility (At least I hope so :D)

[ephphatha]

Using the default constructor here has the same effect, creating a distribution with a lower bound of 0 and upper bound of std::numeric_limits<int32_t>::max().

[qndel]

dunno, I tested and the results were off by 1

[ephphatha]

Given you're using std::uniform_int_distribution why not use distribution params with v-1 as the max? This would also let you avoid modulo bias, fitting into the stated goal of improving randomness. v-1 would be necessary as std::uniform_int_distribution uses a closed interval but the existing GenerateRnd function uses a half-open interval.

You could also simplify call sites that use a lower bound (weapon damage rolls for example) by providing an overload with a minimum bound parameter. If dist was created and stored alongside rng with the default distribution then something like the following would be suitable for bounded random number generation:

int32_t GenerateRndV2(int32_t max, int32_t min)
{
    if (max <= min)
        return min;

    return dist(rng, std::uniform_int_distribution<int32_t>::param_type{ min, max });
}

These callers currently add 1 to the argument to account for GenerateRnd's half-open interval, an approach like this would make that no longer necessary.

[qndel]

it's not the goal of this PR

[ephphatha]

Highest value this can generate is player._pIFMaxDam - 1, is this a pre-existing bug?

[ephphatha]

Same potential (existing) bug here, damage range is [min, max)

[AJenbo]

Is this algo guaranteed to give the same sequence when setting the same seed?

[ephphatha]

std::mt19937 is a default set of arguments for the mersenne twister engine, rng is guaranteed to give the same sequence when (re-)initialised with the same seed. I believe that distributions are guaranteed to map the same generated number to the same output within their interval but I can't find documentation covering this specifically.

Running a quick test shows that calling the distribution with different params doesn't affect the generator, and a uniform_int_distribution appears to be consistent for the same generator values.

In the following output all three lines were generated with the default mt19937 seed and a distribution having a default interval of [0, 100].
The top line used the interval [333, 999] for positions 0, 3, 6, 9, 12, 15, 18
The middle line used the same interval as the top line for positions 0, 6, 12, 18 and the same interval as the bottom line for positions 3, 9, 15
The bottom line used the interval [1, 4] for positions 0, 3, 6, 9, 12, 15, 18

547, 20, 50,647, 63, 43,616, 39, 83,950, 86, 22,635, 91, 15,589, 89, 56,650, 32
  1, 20, 50,647, 63, 43,  2, 39, 83,950, 86, 22,  1, 91, 15,589, 89, 56,  2, 32
  1, 20, 50,  2, 63, 43,  2, 39, 83,  4, 86, 22,  1, 91, 15,  1, 89, 56,  2, 32

The same numbers were generated in the same order, and mapped to the same output for a given interval even when the distribution params varied between runs (cols 3-6 for example).

[AJenbo]

My biggest worry is that there could be a difference between two std implementations, but sounds like it's part of the spec to be stable.

[qndel]

Is this algo guaranteed to give the same sequence when setting the same seed?

obviously

[qndel]

vanilla rng

monobit_test                             0.1016892493688818 PASS
frequency_within_block_test              0.5427869036748679 PASS
runs_test                                0.20869832039571723 PASS
longest_run_ones_in_a_block_test         0.10379179874654554 PASS
binary_matrix_rank_test                  0.10733785915603897 PASS
dft_test                                 0.16373315942288413 PASS
non_overlapping_template_matching_test   0.9999981915288942 PASS
overlapping_template_matching_test       0.2136566153526672 PASS
maurers_universal_test                   0.004284665889349451 FAIL
linear_complexity_test                   0.7170447464996982 PASS
serial_test                              0.28304219315039814 PASS
approximate_entropy_test                 0.3002676702843548 PASS
cumulative_sums_test                     0.08849918274824375 PASS
random_excursion_test                    0.029870664339639433 PASS
random_excursion_variant_test            0.08634782098366245 PASS

[qndel]

my rng

monobit_test                             0.2581142844998201 PASS
frequency_within_block_test              0.6608354754963154 PASS
runs_test                                0.8426423556097362 PASS
longest_run_ones_in_a_block_test         0.4422302667466412 PASS
binary_matrix_rank_test                  0.5712732704344778 PASS
dft_test                                 0.5626456741682362 PASS
non_overlapping_template_matching_test   1.0000000299879452 PASS
overlapping_template_matching_test       0.26038389890445107 PASS
maurers_universal_test                   0.13866138941713624 PASS
linear_complexity_test                   0.2408765957644154 PASS
serial_test                              0.1545838268001897 PASS
approximate_entropy_test                 0.38857399727239755 PASS
cumulative_sums_test                     0.17274721942550308 PASS
random_excursion_test                    0.291628919843869  PASS
random_excursion_variant_test            0.13470504984007847 PASS

[qndel]

tested with https://github.com/dj-on-github/sp800_22_tests

These numbers don't mean much but there were many rng flaws that could be observed during actual gameplay - stat shrine boosted dex 90% of the time etc. that's why one of the shrines even has "favors shields" in description. That's just poo rng.
None of these problems exist when using the new one.