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

Replace OOB guards with safe RNG calls when initialising quests #7013

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

ephphatha
Copy link
Contributor

@ephphatha ephphatha commented Mar 9, 2024

In order to hit the case that the QuestGroup2 and QuestGroup3 checks were guarding against a player would need to edit their save or modify the game to allow storing a raw seed value instead of the output of AdvanceRndSeed(). Since this can't happen in normal gameplay I don't think it's necessary to keep compatibility here. The other mitigating factor is quest availability is saved as part of the game state so there's no issues loading these saves on vanilla diablo or in devX.

It's possible that someone playing on an NT-based windows could set their system time to the 1977 date and start a new game at a time that lands on the seed 988045466 for dlvl 15. To ensure we leave all quests in this group active (as vanilla does) I've checked for that seed explicitly.

Massive props to staphen for doing the legwork to point out what quests impact level generation (and the overall discussion in discord), that gave me the idea to see if these checks actually impact normal gameplay.

test/quests_test.cpp Outdated Show resolved Hide resolved
AJenbo
AJenbo previously approved these changes Mar 9, 2024
@AJenbo
Copy link
Member

AJenbo commented Mar 9, 2024

Any tests you would like me to try and run on a PowerPC Mac?

@ephphatha
Copy link
Contributor Author

ephphatha commented Mar 10, 2024

Any tests you would like me to try and run on a PowerPC Mac?

Could you please try creating a save game with the system date set to 2039 or later? If it has the same behaviour as windows I expect the level table to look like the following no matter what date/time you start a new game at:

 0:   22695476 -- Town seed might end up rerolled
 1: 2110654659
 2: 1444857250 (with Poisoned Water Supply [Q_PWATER])
 3: 1368016523
 4: 1526527176 (with Ogden's Sign [Q_LTBANNER])
 5: 1086087831 (with Valor [Q_BLOOD])
 6:  220228418 (with Chamber of Bone [Q_SCHAMB])
 7: 1885843287 (with Halls of the Blind [Q_BLIND])
 8:  663105788
 9: 1179446315
10:  805306086 (with Anvil of Fury [Q_ANVIL])
11: 1921374621
12: 1674189952
13: 1031346559 (with Warlord of Blood [Q_ANVIL])
14:  360511818
15: 1960323153
16: 1971124540

@AJenbo
Copy link
Member

AJenbo commented Mar 10, 2024

Several crashes and freezes later:
diablo spawn 0.zip
(I don't even know if this is the same save format as the PC version... but it should be, just and odd file name)
The save was done using the 1.08 spawn version.

@ephphatha
Copy link
Contributor Author

Doesn't look like it accepted the system time change:

DungeonSeeds[]:
  | [1] | 905263464 | unsigned int
  | [2] | 2043843209 | unsigned int
  | [3] | 1852314718 | unsigned int
  | [4] | 416078711 | unsigned int
...
Starting seed: 1710044994 (2024-03-10 04:29:54 AM can be reached by setting the windows clock)
 0:  270961835
 1:  905263464
 2: 2043843209
 3: 1852314718 (with The Curse of King Leoric [Q_SKELKING])
 4:  416078711 (with Ogden's Sign [Q_LTBANNER])
...

Source/quests.cpp Outdated Show resolved Hide resolved
Reworded and added comments explaining what would've happened and calling out the case that is still technically possible.
Copy link
Member

@AJenbo AJenbo left a comment

Choose a reason for hiding this comment

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

Nice investigation and comments :)

@AJenbo AJenbo merged commit 6b2a0c4 into diasurgical:master Mar 15, 2024
22 checks passed
@ephphatha ephphatha deleted the quest/rng branch March 15, 2024 23:00
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.

2 participants