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

Add manual seed input for rando generation #2057

Merged
merged 13 commits into from
Jan 25, 2023

Conversation

Archez
Copy link
Contributor

@Archez Archez commented Nov 30, 2022

This adds an input field and some buttons for manually specifying a seed value when generating a rando. This means for the same seed value and settings combo, you should get the same spoiler log file.

I added the seed value and the SoH build version to the spoiler log, prefixed with _ so they appear at the top.

image

Probably out of scope, but I think this top section is getting a bit cluttered. May be worth tidying some things later. (I'm still trying to get familiar with ImGui).

{ // spoiler log
    "_seed": "123456",
    "_version": "FLYNN BRAVO (5.0.1)",
    ...
}

I also included a mechanism to generate seeds via the console. gen_rando will generate a seed based on the current settings set. You can provided a seed value via gen_rando 12345. I also utilized the multiple seed generation logic provided by 3ds with this command via gen_rando 500 1 to, for example generate 500 seeds back-to-back. This should make break-point debugging seed generation easy when looking for a specific spoiler output.

Update Jan 17

After some digging I came to the conclusion that the original seed generation was not reproducible across all machines. Although the underlying Mersenne Twister produces the same values for all platforms, the uniform distribution funcs and hash funcs are platform dependent.

I've adjusted the seed rando generation to be reproducible across all OS, 32bit and 64bit machines. This required a few concessions. Boost headers was added to the build target so we can leverage Boosts versions of uniform_int_distribution and uniform_real_distribution which are platform independent. Boost also provides hash functions that are identical for all platforms, but is unfortunately not identical between 32bit and 64bit machines. To address this, I made extensions to boost to provide minimal hash functions for string and integral values that are limited to 32bit hash values via boost::hash_32, which use uint32_t as the operating size instead of size_t.

To include the Boost headers, I opted to perform a download and extraction during CMake for Boost. The CMake logic first checks to see if it can find an installed copy for Boost, and if not, proceed to download it as a dep. The reasoning for this was to ensure the same version of Boost was used across all machines (linux via apt-get only had 1.71.0 as the latest version, while windows would get 1.81.0 via vcpkg, and macports had 1.81.0). The other reason was that wiiu and switch builds where failing to detect the Boost headers when installing via apt-get.

Build Artifacts

soh/soh/Enhancements/randomizer/randomizer.h Outdated Show resolved Hide resolved
soh/soh/Enhancements/randomizer/randomizer.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/randomizer/randomizer.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

i'm not sure if we want to have the boost headers checked in or if we want to do this some other way, my gut feeling is we should pull them from somewhere else instead of having them on the repo

@Archez
Copy link
Contributor Author

Archez commented Jan 19, 2023

i'm not sure if we want to have the boost headers checked in or if we want to do this some other way, my gut feeling is we should pull them from somewhere else instead of having them on the repo

I can't tell if your comment is directed towards me, or just letting others know what our initial choice was.
For clarification: I currently have this pulling in boost headers via CMake FetchContent. The 5 header files shown being checked into source here are custom extensions I had to make to support getting 32bit hashes on 64bit machines. Boost uses size_t as the hash value, so I needed to make overrides that explicitly use unit32_t instead. I just placed them in the same namespace/include directories for consistency.

If I had the actual boost headers checked in, it'd be ~190 files (for just the minimal headers needed).

@briaguya-ai
Copy link
Contributor

Boost uses size_t as the hash value, so I needed to make overrides that explicitly use unit32_t instead. I just placed them in the same namespace/include directories for consistency.

i'm a little worried that we might run into conflicts when pulling boost via FetchContent when we have these manually checked in

i'm not sure what a better solution would look like yet though, i'll have to think about/ask around about it

@Archez
Copy link
Contributor Author

Archez commented Jan 19, 2023

i'm a little worried that we might run into conflicts when pulling boost via FetchContent when we have these manually checked in

I should clarify, the extensions I made all have _32 appended to the end of the file names, and to the end of the method names, so they aren't actually "overrides" (poor word choice), just additions to the existing namespace. So changes in boost shouldn't make a big deal, and I have the boost version hardcoded in FetchContent so conflicts shouldn't happen unless we choose to use a new version.

i'm not sure what a better solution would look like yet though, i'll have to think about/ask around about it

Overall I feel that this should eventually be replaced with a custom PRNG solution, maybe provided by LUS.

@briaguya-ai
Copy link
Contributor

Overall I feel that this should eventually be replaced with a custom PRNG solution, maybe provided by LUS.

kenix is down to have LUS provide PRNG. how much effort would it be to find some MIT/other permissively licensed PRNG code and PR it to LUS, then use that instead of boost here?

I'm not sure about LUS providing a hash function, i'm also not sure if there's a way around needing to use the hash function

if moving away from boost will require a large amount of effort, i'll continue to look at this PR from a "what's the best way to include/link the minimum amount of boost to get the job done" perspective. if moving away from boost isn't that much effort, then we should probably just move ahead with the PRNG in LUS strat before trying to get this merged

set(Boost_NO_BOOST_CMAKE false)
set(BOOST_INCLUDEDIR ${FETCHCONTENT_BASE_DIR}/boost-src) # Location where FetchContent stores the source
message("Searching for Boost installation")
find_package(Boost)
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at the docs here https://cmake.org/cmake/help/latest/module/FindBoost.html#examples it seems we can specify the components we're looking for from boost

find_package(Boost 1.66.0 COMPONENTS date_time filesystem system ...)

i also don't see where/if this is linking boost, it seems that's done via ADDITIONAL_LIBRARY_DEPENDENCIES, should we do that for the MT?

@dcvz you might have a better sense of what the answers to these questions should be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding the COMPONENTS part is regarding libraries that need to be linked. In this case I do not need any Boost libraries, only the headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, so everything from boost that's being pulled in is a pure header lib? that makes life easy!

@briaguya-ai briaguya-ai merged commit 589e259 into HarbourMasters:develop Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants