replace boost hashing with FNV-1a, & MT RNG with PCG #4973
Conversation
serprex
force-pushed
the
remove-boost-hash
branch
from
9960667 to
731aa9f
Compare
|
We should share a build of this with people across our 3 supported platforms and make sure they all generate the same hash. |
| double RandomDouble() { | ||
| boost::random::uniform_real_distribution<double> distribution(0.0, 1.0); | ||
| return distribution(generator); | ||
| return ldexp(next32(), -32); |
There was a problem hiding this comment.
this is what's suggested at bottom of https://www.pcg-random.org/using-pcg-c-basic.html
another option would be
(float)(next32() & 0xFFFFFF) * (1.0 / 0x1000000)which has better properties than dividing by uint32 max & won't return 1.0
meanwhile you can go full bit mode & generate the float with minimal floating point math, https://blog.bithole.dev/blogposts/random-float
float int_to_float(uint32_t random) {
union { uint32_t u32; float f; } u = { .u32 = random >> 9 | 0x3f800000 };
return u.f - 1.0;
}
serprex
changed the title
|
I'll start the testing. I generated this seed on Linux. Seed was I can test on Windows later, need someone to test the Mac Builid and make sure all three generate the same seed. @garrettjoecox would you available to try and gen this seed later on Mac? I didn't change any settings from a fresh download on Linux except for the volume, but you can drag the ship json if you want to be safe. |
|
tested on windows 
|
|
Promising! |
|
From my mac M1 |
|
Archez beat me, but same |
|
#4972 details why PCG is preferable to MT & gives some lineage to the code |
leggettc18
left a comment
leggettc18
left a comment
There was a problem hiding this comment.
Loaded all 3 spoilers into meld and it told me the only differences between all three files were the line endings. So I think this is good to go!
|
Before I can consider this mergeable, it think we need to discuss the hashing change. As it stands the PR description is not just dropping boost as a dependency, but it's also dropping 32bit support as well. I know we've already broken off WiiU to a standalone fork, but this change would mean seeds in theory cannot be shared with WiiU users. So the question is, should we still maintain 32bit hashing as a restriction or not? |
|
That's putting aside the fact that sharing seeds is nearly impossible as we have things setup currently anyway. They don't have drag and drop, and we don't have a file select option for loading spoilers. But if we could get something for that done, this would still be an important consideration. |
|
Or, rather, if whoever ends up updating/maintaining the console ports can get that done, tbf. |
|
Personally I don't think we should worry about how this affects unofficial ports. Sharing seeds with console isn't really a thing right now anyway, and when it is the preferred method would be spoiler parsing, in which case the hashing algorithm used no longer matters, since it's getting item locations from the log instead of generating them. Really the only reason we actually need to match across our three major platforms is because we haven't coded a way to share seed data in a non-human-readable way for races. In my opinion, avoiding debates like this is part of the reason we decided to stop doing official console ports. |
|
To give status from conversation on Discord: PR on hold since @Archez offered trying this on Wii U to potentially render compatibility conversation moot. I'm guessing it'll just work. But not a priority |
serprex
force-pushed
the
remove-boost-hash
branch
2 times, most recently
from
3451f11 to
539c138
Compare
|
Due to dependencies for apclientpp this is blocking progress on archipelago, would love to see this get in (aside from not going to miss the 3GB+ dependency lol) |
|
@aMannus it might be better if you split apclientpp as a separate PR, your changes broke build on linux/mac, & overwhelms this PR's original changes, which were a net drop in line count |
|
Wait what the hell happened there!? I didn't merge anything into this branch on purpose. How do I even have merge perms on your repo? I'm very sorry about that. If you want to revert the commit or force push to the previous state please do so. |
serprex
force-pushed
the
remove-boost-hash
branch
from
fd88f51 to
a5dc408
Compare
|
|
Welp today I learned. Will put the baguette in the oven now :) Thanks for fixing it serprex, and thanks for the work on this in general! |
replace boost hashing with FNV-1a removes boost as a dependency
serprex
force-pushed
the
remove-boost-hash
branch
from
a5dc408 to
150098f
Compare
replace boost hashing with FNV-1a removes boost as a dependency
7 participants
removes boost as a dependency
Build Artifacts