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: Make Elevators Come To You an OptionSet #4000

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

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Sep 25, 2024

Let's the player choose each bridge/elevator individually for elevators come to you.

This suffers a bit from OptionSets not allowing any kind of "random", but that can come in the future hopefully.

Also yay 4000 :)

Compatible client: https://github.com/NewSoupVi/The-Witness-Randomizer-for-Archipelago/releases/tag/v7.0.0p12

Tested: Unit tests

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 25, 2024
@NewSoupVi NewSoupVi added the is: enhancement Issues requesting new features or pull requests implementing new features. label Sep 25, 2024
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

LGTM, simple logic creating new entrances.

Since it turned from a toggle to an option set, might be a good idea to handle someone having it set to True or False (unless I missed that)

Either by adjusting True to just include all 3 of them, or a warning or something.

@NewSoupVi
Copy link
Member Author

I'm still having it error, but the message should be clearer now

@NewSoupVi
Copy link
Member Author

Welp. Unfortunately, I already turned off my PC now, so I'll fix these unit tests tomorrow lol

# Used to be a toggle
@classmethod
def from_text(cls, text: str):
if text.lower() in {"off", "0", "false", "none", "null", "no"}:
Copy link
Collaborator

Choose a reason for hiding this comment

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

null did not work before

Suggested change
if text.lower() in {"off", "0", "false", "none", "null", "no"}:
if text.lower() in {"off", "0", "false", "none", "no"}:

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, I copied this from the Toggle code

@@ -279,12 +290,31 @@ class ChallengeLasers(Range):
default = 11


class ElevatorsComeToYou(Toggle):
class ElevatorsComeToYou(OptionSet):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be a good idea to add the list of valid keys to the option description, this makes it clear what the options are even in the template yaml.

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.

Two small comments, also the test could include the Swamp EP, but I think it's fine. Tested a few generations myself using plando, including said EP, and it was all accurate. Everything looks good

@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: author Issue/PR is waiting for feedback or changes from its author. 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.

3 participants