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

The Witness: Change all option name comparisons to strings instead of numeric values #2503

Merged
merged 21 commits into from
Feb 11, 2024

Conversation

NewSoupVi
Copy link
Member

Refactor all the options checking to do world.options.shuffle_EPs == "individual" instead of world.options.shuffle_EPs == 1

Also, do a refactor of the shuffle_postgame code, which is where most of the option checking happens. This part probably needs to be looked over by someone who understands the game. I'm gonna request a review from Exempt-Medic I think.

@NewSoupVi NewSoupVi marked this pull request as draft November 25, 2023 15:11
@ThePhar ThePhar added the is: refactor/cleanup Improvements to code/output readability or organizization. label Nov 25, 2023
@NewSoupVi
Copy link
Member Author

NewSoupVi commented Nov 25, 2023

This will likely conflict with #2499 so I'll wait for that before un-drafting this one, bc that one is more important

@Exempt-Medic
Copy link
Collaborator

I used a regex search and found some other comparisons that could be strings and some simplify-able comparisons:

@NewSoupVi
Copy link
Member Author

I'm happy with shuffle_doors >= 2

The line
+= shuffle_doors > 1
is necessary because the result needs to be exactly 1 or 0 numerically.

Some of the code is not mine and I'm worried that removing "> 0" could cause some problem with negative values and it's outside of the scope of this PR anyway

As for the two remaining things relating to options, I've changed those now :) Thanks

@alwaysintreble
Copy link
Collaborator

alwaysintreble commented Dec 1, 2023

Some of the code is not mine and I'm worried that removing "> 0" could cause some problem with negative values and it's outside of the scope of this PR anyway

just peeked at it and .value would be better/faster if it's a toggle. if it's a choice it's fine

@NewSoupVi NewSoupVi marked this pull request as ready for review December 7, 2023 07:51
Copy link
Collaborator

@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.

I read through the changes, primarily to the postgame section, to check that they lined up with my understanding of the game. I then ran 1500 fully-random, differently-seeded generations on main and option_names and compared the outputs. I also specifically checked all 96 reasonable combinations of postgame settings. The outputs were identical except when the one additional postgame consideration occurred (lines 324-235 of player_logic.py), which is the expected behavior.

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Jan 16, 2024

This PR is at the bottom of a pretty big dependency chain at this point, so it'd be epic if this could be prioritised over other Witness PRs 🙏 It shouldn't be super hard to review I think

@NewSoupVi NewSoupVi added the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Feb 10, 2024
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

LGTM

Having any kind of unit test that verifies the options put the corresponding items/things into the pool/member variables, and that slot data has the correct output of the correct input wouldn't hurt going forward though. (It would make sure it behaves the same before and after, and save you from typos in strings)

@black-sliver black-sliver merged commit a6deffb into ArchipelagoMW:main Feb 11, 2024
12 checks passed
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
… numeric values (ArchipelagoMW#2503)

* Refactor postgame code to be more readable

* Change all references to options to strings

* oops

* Fix some outdated code related to yaml-disabled EPs

* Small fixes to short/longbox stuff (thanks Medic)

* comment

* fix duplicate

* Removed triplicate lmfao

* Better comment

* added another 'unfun' postgame consideration

* comment

* more option strings

* oops

* Remove an unnecessary comparison

* another string missed

* Another was missed

* This would create a really bad merge error
TheLX5 pushed a commit to TheLX5/Archipelago that referenced this pull request Mar 2, 2024
… numeric values (ArchipelagoMW#2503)

* Refactor postgame code to be more readable

* Change all references to options to strings

* oops

* Fix some outdated code related to yaml-disabled EPs

* Small fixes to short/longbox stuff (thanks Medic)

* comment

* fix duplicate

* Removed triplicate lmfao

* Better comment

* added another 'unfun' postgame consideration

* comment

* more option strings

* oops

* Remove an unnecessary comparison

* another string missed

* Another was missed

* This would create a really bad merge error
TheLX5 pushed a commit to TheLX5/Archipelago that referenced this pull request Mar 2, 2024
… numeric values (ArchipelagoMW#2503)

* Refactor postgame code to be more readable

* Change all references to options to strings

* oops

* Fix some outdated code related to yaml-disabled EPs

* Small fixes to short/longbox stuff (thanks Medic)

* comment

* fix duplicate

* Removed triplicate lmfao

* Better comment

* added another 'unfun' postgame consideration

* comment

* more option strings

* oops

* Remove an unnecessary comparison

* another string missed

* Another was missed

* This would create a really bad merge error
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
… numeric values (ArchipelagoMW#2503)

* Refactor postgame code to be more readable

* Change all references to options to strings

* oops

* Fix some outdated code related to yaml-disabled EPs

* Small fixes to short/longbox stuff (thanks Medic)

* comment

* fix duplicate

* Removed triplicate lmfao

* Better comment

* added another 'unfun' postgame consideration

* comment

* more option strings

* oops

* Remove an unnecessary comparison

* another string missed

* Another was missed

* This would create a really bad merge error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants