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

Super Metroid: Replace random module with world random in variaRandomizer #4429

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Mysteryem
Copy link
Contributor

What is this fixing or adding?

Replaces direct use of the random module with passing around the world's .random attribute instead.

Fixes different generation on webhost compared to local generation, assuming no yaml options are using random options because all random option resolution is currently nondeterministic on webhost.

The new arguments for passing self.random through to the various variaRandomizer classes and functions have been added to the end of the arguments for each relevant function, except utils.randGaussBounds where the random argument is more like a self argument in a method, so has been added as the first argument instead.

The multiworld seed is now passed through to variaRandomizer instead of it generating its own seed.

How was this tested?

I started by removing all the random module imports and ran many generations with 5 almost fully random yamls, making adjustments to pass through the world's .random whenever generation crashed because it was trying to access the random module still. This helped me see the function calls through the code to determine what classes and functions I would have to pass the world's .random through.

Once generation was succeeding and I could not find any more direct use of the random module, I ran a generation of the template yaml on archipelago.gg (specifically the template yaml has no options set to random so should generate deterministically) and generated with the same seed locally and determined them to now produce the same results.

Additionally, I ran these changes along with the WIP test in #4410, and the failing test for Super Metroid would now pass.

I have not done any tests that go as far as connecting the client to a multiworld and patching the ROM, so I recommend more testing should be done by people more familiar with Super Metroid AP.

…izer

Fixes different generation on webhost compared to local generation,
assuming no yaml options are using random options because all random
option resolution is currently nondeterministic on webhost.

The new arguments for passing self.random through to the various
variaRandomizer classes and functions have been added to the end of the
arguments for each relevant function, except `utils.randGaussBounds`
where the `random` argument is more like a `self` argument in a method.

The multiworld seed is now passed through to variaRandomizer instead of
it generating its own seed.
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jan 3, 2025
@PoryGone PoryGone added the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Jan 3, 2025
@PoryGone
Copy link
Collaborator

PoryGone commented Jan 3, 2025

@lordlou

@Exempt-Medic Exempt-Medic added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: refactor/cleanup Improvements to code/output readability or organizization. labels Jan 5, 2025
@Exempt-Medic
Copy link
Member

This has conflicts btw

@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Jan 13, 2025
…ndom

# Conflicts:
#	worlds/sm/variaRandomizer/randomizer.py
@Exempt-Medic Exempt-Medic removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Jan 14, 2025
Copy link
Contributor

@lordlou lordlou left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@Exempt-Medic Exempt-Medic removed the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Jan 17, 2025
@@ -124,7 +124,7 @@ def generate_early(self):
Logic.factory('vanilla')

dummy_rom_file = Utils.user_path(SMSettings.RomFile.copy_to) # actual rom set in generate_output
self.variaRando = VariaRandomizer(self.options, dummy_rom_file, self.player)
self.variaRando = VariaRandomizer(self.options, dummy_rom_file, self.player, self.multiworld.seed, self.random)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think passing self.multiworld.seed might not be what we want.
This is an issue I've run into in other games.

If there are 2 Super Metroid players in the same multiworld, with similar enough settings, they might find that they both have certain things randomized to the same result.
For example: 2 Super Metroid players both turn on area rando, and they both end up with the exact same area connections - which is not what they expect.

I haven't tested whether this is the case (if not with area rando, then it might be with something else).

It might be better to pass self.multiworld.seed + self.player, or maybe just something from self.random.randrange

Copy link
Contributor Author

@Mysteryem Mysteryem Feb 2, 2025

Choose a reason for hiding this comment

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

The only use of the seed that I could find is in APPostPatchRom to call romPatcher.writeSeed(seed). The values written are used for the seed display (picks four enemy names to display at the top of the screen) after the save file selection screen.

Set into patcherSettings["seed"]

romPatcher is created and customPostPatchApply(romPatcher) is called, calling SMWorld.APPostPatchRom(romPatcher)

if args.rom is not None:
# patch local rom
# romFileName = args.rom
# shutil.copyfile(romFileName, outputFilename)
romPatcher = RomPatcher(settings=patcherSettings, magic=args.raceMagic, player=self.player)
else:
romPatcher = RomPatcher(settings=patcherSettings, magic=args.raceMagic)
if customPrePatchApply != None:
customPrePatchApply(romPatcher)
romPatcher.patchRom()
if customPostPatchApply != None:
customPostPatchApply(romPatcher)

romPatcher.writeSeed() is called with the seed as its argument

if not romPatcher.settings["isPlando"]:
romPatcher.writeSeed(romPatcher.settings["seed"]) # lol if race mode

Writes seed values starting at 0xdfff00

def writeSeed(self, seed):
random.seed(seed)
seedInfo = random.randint(0, 0xFFFF)
seedInfo2 = random.randint(0, 0xFFFF)
self.romFile.writeWord(seedInfo, snes_to_pc(0xdfff00))
self.romFile.writeWord(seedInfo2)

https://github.com/theonlydude/RandomMetroidSolver/blob/master/patches/common/src/seed_display.asm uses that same 0xdfff00 address.

@lordlou would know better if the seed controls anything else, and whether it should be the same for multiple SM players in the same multiworld (so use the multiworld's seed) or if it should be different, but still deterministic for each individual SM player in the same multiworld (so use each world's .random to create a seed). The PR is currently using the multiworld's seed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants