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

Isolate shrine/quest pool RNG from global RNG state #6831

Merged
merged 4 commits into from
Feb 15, 2024

Conversation

StephenCWills
Copy link
Member

This PR begins the process of isolating seeded random number generators from the global RNG state. I expect that item generation and dungeon generation will each need a fairly sizeable set of changes to ensure RNG instances are being passed around appropriately to avoid compatibility issues so I plan to do the same for each of them in separate PRs. After those are out of the way, we can look into potentially scrubbing out the remaining calls to SetRndSeed() and changing up how various seeds get generated.

@julealgon
Copy link
Contributor

What's the overarching idea behind this? Do you want for each "area" of the game to have a different seed potentially?

@StephenCWills
Copy link
Member Author

StephenCWills commented Nov 24, 2023

Eventually, I'd like to fix #6261. But also, refer to diasurgical/devilution#64 (comment) for a description of how abuse of the SetRndSeed() function can cause problems for the global RNG.

EDIT: You may also want to refer to a pair of PRs I submitted in the past which provide a more concrete example of an issue caused by this lack of RNG isolation: #3296 and #3314

AJenbo
AJenbo previously approved these changes Nov 25, 2023
Source/engine/random.hpp Outdated Show resolved Hide resolved
@StephenCWills
Copy link
Member Author

Rebased, on a whim.

@AJenbo AJenbo enabled auto-merge (rebase) February 15, 2024 00:22
@AJenbo AJenbo merged commit 8603696 into diasurgical:master Feb 15, 2024
21 checks passed
@StephenCWills StephenCWills deleted the rng-instances branch February 15, 2024 00:22
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.

4 participants