-
Notifications
You must be signed in to change notification settings - Fork 792
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
Super Mario 64: Option groups #4161
Conversation
worlds/sm64ex/Options.py
Outdated
@@ -127,6 +127,30 @@ class MoveRandomizerActions(OptionSet): | |||
valid_keys = [action for action in action_item_table if action != 'Double Jump'] | |||
default = valid_keys | |||
|
|||
sm64_options_groups = [ | |||
OptionGroup("Sanity Options", [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this logic options or something like that? "Sanity" doesn't really describe the contents super well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Item Options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not that, since the last group is always "Item & Location Options"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Changed to "Logic Options".
worlds/sm64ex/Options.py
Outdated
StrictCannonRequirements, | ||
StrictMoveRequirements, | ||
]), | ||
OptionGroup("Goal Options", [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only actual goal option is CompletionType
(and maybe StarsToFinish
), the rest arguably are not.
I'd say you could call this Star Options
/ Star Costs
instead, and remove CompletionType
from that category
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Star Options
group and separated from Goal Option(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code works, however I have a few comments about layout and organization.
worlds/sm64ex/Options.py
Outdated
EnableMoveRandomizer, | ||
MoveRandomizerActions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two options cause the options page to render awkwardly. Since they are related and would take up enough room, they could probably go into their own category.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Also moved StrictMoveRequirements
into this new grouping.
worlds/sm64ex/__init__.py
Outdated
@@ -3,7 +3,7 @@ | |||
import json | |||
from .Items import item_table, action_item_table, cannon_item_table, SM64Item | |||
from .Locations import location_table, SM64Location | |||
from .Options import SM64Options | |||
from .Options import SM64Options, sm64_options_groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from .Options import SM64Options, sm64_options_groups | |
from .Options import sm64_options_groups, SM64Options |
nitpick: Import order should match the rest of the file where it is variables followed by classes.
worlds/sm64ex/Options.py
Outdated
OptionGroup("Goal Options", [ | ||
CompletionType, | ||
]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OptionGroup("Goal Options", [ | |
CompletionType, | |
]), |
Having Completion Type be this low and alone is a funky decision to make. I would probably recommend leaving it out of the option groups and letting it go to the "Game Options" group since it is more fundamental to the randomizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything LGTM
I would actually like reapproval from @N00byKing here since significant changes were made in response to nicholassaylor's review after N00byKing's approval. |
* sm64ex: add option groups * sm64ex: rename sanity options group to item options * sm64ex: rename sanity options group to logic options * sm64ex: seperate star costs from goal options and add entrance rando to logic options * sm64ex: seperate ability options from logic options group
What is this fixing or adding?
Adds option groups to to options page for SM64.
Initially modelled after the option groups for SMW.
How was this tested?
Unit tests, and generating a YAML.
If this makes graphical changes, please attach screenshots.