Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMON: Change PRNG Function to Xorshift #3341

Merged
merged 9 commits into from Sep 9, 2021
Merged

Conversation

@TehGelly
Copy link
Contributor

@TehGelly TehGelly commented Sep 4, 2021

The old RNG method had non-standard periods, ranging from some seeds looping on themselves (seed = 1184201285) to some seeds having periods as low as 11 or 48, as listed in #3340. This is a problem even for games that run the RNG once a frame, as the possibilities for random events is greatly reduced should the initial seed be in one of these sets of small periods.

Xorshift is a standard, fast, non-cryptographic PRNG with academic backing that has period 2^32-1 (all seeds lead to another seed except 0, which is excluded from the initial seeds). Many different flavors are possible, as listed in the paper, but the choice implemented in this pull request uses only a single 32-bit integer as a state, like the old PRNG.

TehGelly added 3 commits Sep 4, 2021
The old RNG method had non-standard periods, ranging from some seeds looping on themselves (seed = 1184201285) to some seeds having periods as low as 11 or 48, as listed in scummvm#3340. This is a problem even for games that run the RNG once a frame, as the possibilities for random events is greatly reduced should the initial seed be in one of these sets of small periods.

Xorshift is a standard, fast, non-cryptographic PRNG with academic backing that has period 2^32-1 (all seeds lead to another seed except 0, which is excluded from the initial seeds).  Many different flavors are possible, as listed in the paper, but the choice implemented in this pull request uses only a single 32-bit integer as a state, like the old PRNG.
Prototypes the inlined scrambleSeed function, as well as a new description for what occurs in the RNG.
0 is a fixed point for xorshift, so avoiding its use is important. It was provisioned as much for RandomSource, but now will be used correctly for setSeed as well.
@Die4Ever
Copy link
Member

@Die4Ever Die4Ever commented Sep 4, 2021

looks good to me, I used my code to verify no short loops

test code

#include <iostream>
#include <thread>
#include <vector>
#include <mutex>

std::mutex mtx;
typedef unsigned int uint32;

/*uint32 rng(uint32 _randSeed) {
	_randSeed = 0xDEADBF03 * (_randSeed + 1);
	_randSeed = (_randSeed >> 13) | (_randSeed << 19);
	return _randSeed;
}*/

uint32 rng(uint32 _randSeed) {
	//marsaglia's paper says that any of 81 triplets are feasible
	//(11,21,13) was chosen, with (cba) and (>>,<<,>>)
	_randSeed ^= _randSeed >> 13;
	_randSeed ^= _randSeed << 21;
	_randSeed ^= _randSeed >> 11;
	return _randSeed;
}

int test_seed(uint32 orig_seed) {
	uint32 s = orig_seed;
	uint32 prev = s;
	for (int i = 0; i < 100; i++) {
		s = rng(s);
		if (s == orig_seed) return i+1;
		if (s == prev) return i + 1;
		prev = s;
	}
	return -1;
}

void test_seeds(uint32 first, uint32 last) {
	for (uint32 s = first; ; s++) {
		int loops = test_seed(s);
		if (loops != -1) {
			std::lock_guard<std::mutex> lck(mtx);
			std::cout << s << " failed with " << loops << " iterations\n";
		}
		if (s == last) break;
	}
}

void test_rng() {
	std::vector<std::thread> threads;
	uint32 start = 0;
	int num_threads = 12;
	uint32 max = UINT_MAX;
	//uint32 max = 10000000;
	uint32 each = max / num_threads;
	for (int i = 0; i < num_threads-1; i++) {
		threads.push_back(std::thread(test_seeds, start, start +each));
		start += each + 1;
	}
	threads.push_back(std::thread(test_seeds, start, max));

	for (uint32 i = 0; i < threads.size(); i++) {
		threads[i].join();
	}
}

int main()
{
	/*auto r = rand();
	for (int i = 0; i < 1000000; i++)
		r = std::max(r, rand());
	std::cout << "r == " << r << "\n";
	return 0;*/
	std::cout << "start!\n\n";
	test_rng();
}

@TehGelly
Copy link
Contributor Author

@TehGelly TehGelly commented Sep 4, 2021

It would be possible to upgrade this to Xorshift*, a much more secure PRNG, by changing the return line in getRandomNumber to return (_randSeed * 0xDEADBF03) % (max + 1);, and would also re-introduce the magic constant 0xDEADBF03 back into the PRNG - one of my favorite hallmarks of the old PRNG.

I leave it open, however, as it would introduce an additional multiplication, which seems to be important to this project - getRandomBit was intentionally shifted to save on computation.

Copy link
Member

@criezy criezy left a comment

The change looks good to me, although I suggested a couple of change.

Also the first two commits (627ee43 and 7a1414a) will need to be squashed as they are two parts of a single change (if we checkout 627ee43, it will not compile). You can either do that before we merge the pull request, or we can squash all three commits when we decide to merge it.

Using an Xorshift* generator seems fine to me. ScummVM can run on devices with low processing power, but I don't expect that an additional multiplication would be an issue even for those. Somebody with more experience than me with those devices might disagree though. And the change as it is in this pull request should be sufficient for what we need.

common/random.h Outdated Show resolved Hide resolved
common/random.h Outdated Show resolved Hide resolved
TehGelly added 2 commits Sep 7, 2021
Changed commenting as recommended, as well as making `scrambleSeed()` private
As well as updating the PRNG to be Xorshift*, this also moves `scrambleSeed` to the bottom and makes it private, as suggested.
@TehGelly
Copy link
Contributor Author

@TehGelly TehGelly commented Sep 7, 2021

I have made the changes as suggested. Squashing the commits on merge seems to be the easiest and cleanest way to handle that issue, as I think the two commits I have just made are similarly problematic.

I have also made the PRNG into Xorshift* with your approval 👍

common/random.h Outdated Show resolved Hide resolved
Co-authored-by: Thierry Crozat <criezy@scummvm.org>
common/random.cpp Outdated Show resolved Hide resolved
common/random.cpp Outdated Show resolved Hide resolved
common/random.cpp Show resolved Hide resolved
Co-authored-by: Filippos Karapetis <bluegr@gmail.com>
bluegr
bluegr approved these changes Sep 9, 2021
@bluegr
Copy link
Member

@bluegr bluegr commented Sep 9, 2021

Overall, very nice work! Thanks for your research and for your work on this

criezy
criezy approved these changes Sep 9, 2021
Copy link
Member

@criezy criezy left a comment

It looks good to me now.
We just have to remember it needs to be squashed when merging.

@bluegr
Copy link
Member

@bluegr bluegr commented Sep 9, 2021

Thanks again. Squashing the commits on this one

@bluegr bluegr merged commit 0992fa9 into scummvm:master Sep 9, 2021
8 checks passed
@TehGelly TehGelly deleted the patch-1 branch Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants