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

Ocarina of Time: options and general cleanup #3767

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

Silvris
Copy link
Collaborator

@Silvris Silvris commented Aug 11, 2024

What is this fixing or adding?

  • Updates to use the modern options API (mostly, a cleaner fix is possible, but I leave that to Espeon should they return)
  • Added option groups based on upstream
  • Removed world: MultiWorld
  • Removed the elusive multiworld: World
  • Fixed relative/absolute imports. Much more work needs done for apworld support though.
  • Attempted to remove any uses of the global and multiworld randoms, replaced with the world random
  • Attempted to fix the item link generation bug (that was caused by the way options are handled)

How was this tested?

Unittests and generations of 10 OoTs with fully random settings. I know this is not comprehensive, but it was the best I could manage. Generation with 2 OoTs itemlinked together with link_replacement and replacement_item null. Did not playtest.

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 11, 2024
@Exempt-Medic Exempt-Medic added is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. is: refactor/cleanup Improvements to code/output readability or organizization. labels Aug 11, 2024
@@ -208,8 +208,8 @@ def patch_rom(world, rom):

# Fix Ice Cavern Alcove Camera
if not world.dungeon_mq['Ice Cavern']:
rom.write_byte(0x2BECA25,0x01);
Copy link
Member

Choose a reason for hiding this comment

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

HELP!?!?!!!!?!??!??!?!?!?! I NEED ESPEON TO EXPLAIN HIMSELF RIGHT NOW

Copy link
Member

Choose a reason for hiding this comment

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

About to label this release/blocker over this oh my lord

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is your yearly reminder that ; is perfectly legal in Python and is just ignored. Apparently it's meant for multiline statements but is fine with the other statement being null?

@Exempt-Medic
Copy link
Member

There are two spots in ItemPool.py that are getting start_inventory using the old method

Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

There are two missed old option methods for start_inventory in ItemPool.py (commented on earlier). Otherwise, these changes LGTM. I ran until I had 100 successful 5-player fully random generations, there were several that failed, though many of these also failed with the previous code. No other old option methods were hit. A particularly strange (potentially concerning) thing is that generating with the same seed before and after these changes would give identical rolled options for players 2-5 but player 1 would have several deviations every single time (only ten such generations were tested).

It may be a good idea to remove the per_slot_randoms from OoTAdjuster.py as well.

The changes seem fine, and seem to work, I'm just slightly concerned about the player 1 problem

@Silvris
Copy link
Collaborator Author

Silvris commented Aug 14, 2024

A particularly strange (potentially concerning) thing is that generating with the same seed before and after these changes would give identical rolled options for players 2-5 but player 1 would have several deviations every single time (only ten such generations were tested).

OoT was using the global random in many places, it wouldn't surprise me if that changed the outcome of those options. It is a little weird to only affect player 1 though.

@NewSoupVi
Copy link
Member

A particularly strange (potentially concerning) thing is that generating with the same seed before and after these changes would give identical rolled options for players 2-5 but player 1 would have several deviations every single time (only ten such generations were tested).

OoT was using the global random in many places, it wouldn't surprise me if that changed the outcome of those options. It is a little weird to only affect player 1 though.

I've been testing this, and I just figured it out after a collective 2-ish hours digging.
This is completely insane. I don't even know how to explain it.

@NewSoupVi
Copy link
Member

NewSoupVi commented Aug 15, 2024

From what I understand, this happens under the conditions that:

  1. You move a choice option from the very start to the very end or vice versa (In this case, DeathLink)
  2. There are a fuckton of different kinds of options of different kinds (because you have to "get lucky", basically), that are all set to random

So random.getrandbits() has this line

        while r >= n:
            r = getrandbits(k)

which basically makes it "reroll until the random is in the correct range" (Note: getrandbits can only output in [0, 2^n) ranges, so if you have 3 choices [0, 1, 2], it will roll with [0, 4), and reroll on 3)

This means that each random call takes a random amount of getrandbits calls

Now, at the start, due to you moving DeathLink, you "misalign" the options generation on main and on this PR.
This means they are now "out of sync"
However, this makes future options take different random amounts of calls
Just via sheer luck, the two can end up "resynced" again, and from then on, all options will be the same.

This explains why for a single player, after some point, all the options are the same. The desynced random calls happen to sync up again on the same option, and we're synced again until we come up to the moved-to-the-end DeathLink option.
In fact, that effect can probably be observed by just adding or removing an option at the start/end.

But in the case where we didn't add/remove an option from the start, but instead moved one from the start to the end or vice versa, consider what happens at the end of player 1 / start of player 2:

Death Link gets evaluated for player 1 on main Branch
End of Player 1 Options
Start of Player 2 Options
Death Link gets evaluated for player 2 on Silvris Branch

Which means that random advances by the same amount, and Player 2 now starts synced between the two branches. Which means that from here on out, everything will be synced.

@NewSoupVi
Copy link
Member

Here's the diff checker for two generations where I output every call to "random.getrandbits()" (which covers all random methods), and you can see that at some point, they miraculously "sync up", and after that point, are only different briefly between the end-of-player DeathLink call on branch 1, and the start-of-player DeathLink call on branch 2

https://www.diffchecker.com/mq9NQQ2C/

@NewSoupVi NewSoupVi merged commit 025c550 into ArchipelagoMW:main Sep 18, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. 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.

3 participants