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

Core: add visibility attribute to Option #3125

Merged
merged 5 commits into from
Apr 14, 2024
Merged

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

allows hiding of options in certain contexts. Multiple use cases have been discussed, this brings legacy alerts for LttP as one such use case with it.

How was this tested?

I've tested each flag barely, so this could use some more testing.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 9, 2024
worlds/alttp/Options.py Outdated Show resolved Hide resolved
Comment on lines +24 to +30
class Visibility(enum.IntFlag):
none = 0b0000
template = 0b0001
simple_ui = 0b0010 # show option in simple menus, such as player-options
complex_ui = 0b0100 # show option in complex menus, such as weighted-options
spoiler = 0b1000
all = 0b1111
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't be "not_simple_ui" a common value?

Copy link
Contributor

@KScl KScl Apr 9, 2024

Choose a reason for hiding this comment

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

(Visibility.all - Visibility.spoiler) may also be a common enough use case to be specified here, for options that only have visual effects on the game and have no effect on generation. (Flashing reduction, sprite choice, etc.)

It would certainly be nice to clean up the spoiler log a little bit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SC2 player colors are visual-only too

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say it's easy enough to get "all except X" with Visibility.all ^ Visibility.spoiler, to use spoiler for an example. I'd be fine to leave this as-is.

@Ziktofel
Copy link
Collaborator

Ziktofel commented Apr 9, 2024

Is this following test fragile?

=========================== short test summary info ============================
[Game] (game="Kirby's Dream Land 3") SUBFAIL worlds/kdl3/test/test_shuffles.py::TestAnimalShuffle::test_all_state_can_reach_everything - execnet.gateway_base.DumpError: can't serialize <class 'worlds.kdl3.Locations.KDL3Location'>
FAILED worlds/kdl3/test/test_shuffles.py::TestAnimalShuffle::test_all_state_can_reach_everything - AssertionError: <BaseClasses.MultiWorld object at 0x7f75f6191ad0> is not false : World Kirby's Dream Land 3 leaked MultiWorld object

@Silvris
Copy link
Collaborator

Silvris commented Apr 9, 2024

Is this following test fragile?

=========================== short test summary info ============================
[Game] (game="Kirby's Dream Land 3") SUBFAIL worlds/kdl3/test/test_shuffles.py::TestAnimalShuffle::test_all_state_can_reach_everything - execnet.gateway_base.DumpError: can't serialize <class 'worlds.kdl3.Locations.KDL3Location'>
FAILED worlds/kdl3/test/test_shuffles.py::TestAnimalShuffle::test_all_state_can_reach_everything - AssertionError: <BaseClasses.MultiWorld object at 0x7f75f6191ad0> is not false : World Kirby's Dream Land 3 leaked MultiWorld object

The failure is known (seems to be a shortcoming of fill_restrictive, hoping to have a PR for it soon). Hadn't seen the leaking before though, which is a little worrying. Wonder if that's a result of the weird failure state (since only Github hits the DumpError).

Options.py Outdated Show resolved Hide resolved
Co-authored-by: Doug Hoskisson <beauxq@users.noreply.github.com>
Options.py Show resolved Hide resolved
Options.py Outdated Show resolved Hide resolved
Berserker66 and others added 2 commits April 14, 2024 20:32
Co-authored-by: Doug Hoskisson <beauxq@users.noreply.github.com>
Co-authored-by: Doug Hoskisson <beauxq@users.noreply.github.com>
@Berserker66 Berserker66 merged commit 09abc5b into main Apr 14, 2024
23 of 24 checks passed
@Berserker66 Berserker66 deleted the option_visibility branch April 14, 2024 18:49
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
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. affects: webhost Issues/PRs that touch webhost and may need additional validation. 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.

None yet

7 participants