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

Options: support "random" and variations for OptionSet with defined valid_keys #4418

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

Conversation

Silvris
Copy link
Collaborator

@Silvris Silvris commented Jan 1, 2025

What is this fixing or adding?

Adds support for random, random-low, random-high, random-mid, and random-range-x-y to OptionSet. The behavior is defined as follows.

  • random is a random number of random items
  • random-low, random-high, and random-mid is a random number (weighted towards the argument) of random items
  • random-range-x-y is a random number between x and y of random items. This also supports random-range-low and similar variants.

Only OptionSets that define valid keys (and thus, could appear on WebHost) can utilize this behavior. No WebHost behavior has been changed for now. Item/LocationSets are supported.

How was this tested?

Manually by providing a number of possible values to several OptionSet options in core. No automated testing has been added.

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jan 1, 2025
Options.py Outdated Show resolved Hide resolved
Comment on lines +954 to +956
raise Exception(
f"{random_range[0]}-{random_range[1]} is outside allowed range "
f"0-{len(choice_list)} for option {self.__name__} for player {player_name}")
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make more sense to just modify the chosen range instead of raising an exception because unlike normal random range the only way to find the "allowed range" is to hit this exception

Co-authored-by: qwint <[email protected]>

def __init__(self, value: typing.Iterable[str]):
def __init__(self, value: typing.Iterable[str], random_str: str = None):
Copy link
Member

Choose a reason for hiding this comment

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

None is not a string

@Exempt-Medic
Copy link
Member

Related / Things you may need to account for:
#4016
#4283

@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jan 1, 2025
@Silvris
Copy link
Collaborator Author

Silvris commented Jan 1, 2025

Related / Things you may need to account for: #4016 #4283

#4016 should be irrelevant since this code is actually just duplicated, so changes to Range do not affect us, and we can't have a negative number of entries in a set.

#4283 probably applies here though.

@NewSoupVi
Copy link
Member

Really makes me wish we found a way already to actually put location groups and item groups in valid_keys

Pretty cool tho

@NewSoupVi
Copy link
Member

NewSoupVi commented Jan 2, 2025

Oh, just realized something
You can consider this out of scope for this PR, but it would obviously be nice if the "mystery dice" button on WebHost could now be added for OptionSets as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. 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.

4 participants