Skip to content

replace MT RNG with PCG#4972

Closed
serprex wants to merge 1 commit intoHarbourMasters:developfrom
serprex:remove-boost-random
Closed

replace MT RNG with PCG#4972
serprex wants to merge 1 commit intoHarbourMasters:developfrom
serprex:remove-boost-random

Conversation

@serprex
Copy link
Contributor

@serprex serprex commented Jan 30, 2025 •
edited by github-actions bot
Loading

Mersenne Twister isn't a particularly good RNG

For info about PCG see https://www.pcg-random.org

I've adapted the RNG I used in openEtG: https://github.com/serprex/openEtG/blob/master/src/rs/src/rng.rs

Which is based on rand_pcg of widely used rand crate: https://rust-random.github.io/rand/src/rand_pcg/pcg64.rs.html#140

This also reduces usage of boost, which could eventually be removed

Build Artifacts

@serprex serprex force-pushed the remove-boost-random branch from 36155ee to bc7d95c Compare January 30, 2025 00:46
serprex
serprex commented Jan 30, 2025
View reviewed changes
soh/soh/Enhancements/randomizer/3drando/random.cpp
uint64_t seed = static_cast<uint64_t>(std::random_device{}());
#else
uint32_t seed = static_cast<uint32_t>(std::hash<std::string>{}(std::to_string(rand())));
uint64_t seed = static_cast<uint64_t>(std::hash<std::string>{}(std::to_string(rand())));
Copy link
Contributor Author

@serprex serprex Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint64_t seed = static_cast<uint64_t>(std::hash<std::string>{}(std::to_string(rand())));
uint64_t seed = static_cast<uint64_t>(rand());

This string hash dance is ultimately pointless. Some checking of RAND_MAX could find it better to do (uint64_t)rand() | (uint64_t)rand() << 32

I left as is since this PR is focused on removing boost/random & replacing MT, but if nobody objects it'd be nice to change this too

Copy link
Contributor

@briaguya0 briaguya0 Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know this PR is closed but i do want to note something:

that line is behind a switch/wii u ifdef

as for how it got there, we originally had

Shipwright/soh/soh/Enhancements/randomizer/3drando/random.cpp

Lines 15 to 23 in 7fb8902

uint32_t Random(int min, int max) {
if (!init) {
//No seed given, get a random number from device to seed
const auto seed = static_cast<uint32_t>(std::random_device{}());
Random_Init(seed);
}
std::uniform_int_distribution<uint32_t> distribution(min, max-1);
return distribution(generator);
}

which was added in #416

then it was updated to

Shipwright/soh/soh/Enhancements/randomizer/3drando/random.cpp

Lines 18 to 30 in 7df9641

uint32_t Random(int min, int max) {
if (!init) {
//No seed given, get a random number from device to seed
#if !defined(__SWITCH__) && !defined(__WIIU__)
const auto seed = static_cast<uint32_t>(std::random_device{}());
#else
uint32_t seed = static_cast<uint32_t>(std::hash<std::string>{}(std::to_string(rand())));
#endif
Random_Init(seed);
}
boost::random::uniform_int_distribution<uint32_t> distribution(min, max-1);
return distribution(generator);
}

in #1608

not sure why the "dance" was used as a fix for wii u but that's the origin story at least

@serprex serprex force-pushed the remove-boost-random branch 2 times, most recently from 6fca562 to 67e6bbd Compare January 30, 2025 01:03
@serprex serprex force-pushed the remove-boost-random branch from 67e6bbd to 12e7ea2 Compare January 30, 2025 01:16
@serprex serprex mentioned this pull request Jan 30, 2025
Merged
@serprex serprex closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@briaguya0 briaguya0 briaguya0 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@serprex @briaguya0