-
Notifications
You must be signed in to change notification settings - Fork 723
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
WebHost, Core: Developer-defined game option presets. #2143
Conversation
const preset = localStorage.getItem(`${gameName}-preset`); | ||
switch (preset) { | ||
case "__default": | ||
settings["description"] = `Generated by https://archipelago.gg with the default preset.`; |
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.
This case does exactly the same thing as the default case, it's therefore not necessary
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.
Negative, because it would output Generated by https://archipelago.gg with the __default preset.
because it uses the value (not the display name).
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.
And I don't want that :P
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.
In that case, if __default and __custom include the leading underscores, would other presets also include it? Should it maybe get trimmed away here?
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.
Other preset names shouldn't have the leading underscores.
Perhaps we can have "All Random" as one of the pre-fab presets for all games. That way, the dropdown isn't simply a reset button for games that don't have developer-provided presets. |
Since "All Random" is a terrible idea for my world, I would actually prefer not having anything that could encourage people to do that. |
I want to maintain player name not being allowed as "Player." It forces people to enter a name, and prevents name conflicts when people try to generate a game if multiple people forgot to put in a name. |
Taking a look at the changes, it appears that "Player" is no longer the default value in the |
Eliminating the default value and requiring a value during validation is acceptable. |
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.
We should hide the presets <select />
if only the default value exists. Other than that, this looks good and I'll approve.
I don't want to hide the presets because it allows an easy way to reset all settings BACK to default values. Hiding the box removes that functionality. |
# Conflicts: # WebHostLib/options.py # WebHostLib/static/assets/player-options.js # WebHostLib/static/styles/player-options.css
I don't like the use of meta here as meta options already have actual defined functionality |
if option_value == "random": | ||
player_options["presetOptions"][preset_name][option_name] = option_value |
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.
If this codepath can only be reached if option_value
is "random", just explicitly assign "random" here
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.
option_value is "random". Why hardcode two strings, when one does the same thing?
There's a bunch of option types (such as |
OptionList, OptionSet, OptionDict, and FreeText are not supported. As listed in the docs. |
I suppose I could explicitly check for those in the test to ensure they're not used. |
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.
Looks good to me. I sign off on the HTML / JS / CSS. Nice work, Phar.
test/webhost/test_option_presets.py
Outdated
) | ||
else: | ||
self.assertTrue( | ||
option.name_lookup[option.value] == option_value, |
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.
This could check that option.value
is in option.name_lookup
first (or just use get
) instead of failing with KeyError
with self.subTest(game=game_name, preset=preset_name): | ||
for option_name, option_value in preset.items(): | ||
try: | ||
option = world_type.options_dataclass.type_hints[option_name].from_any(option_value) |
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.
Should we call verify
on the option here?
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.
verify
needs additional setup that I didn't think was necessary for this test.
I have a bunch of options that don't work properly with this, but they are all custom classes and are caught by the test, so I guess it's fine. |
ArchipelagoMW#1820 added the game name to template export, but when ArchipelagoMW#2143 began overriding the description in Javascript to name the preset it removed the game name. Also while I was here I figured we might as well not hardcode https://archipelago.gg, since it can be useful to know if a YAML was generated by some other Web Host (for example http://archipelago.gg:24242 if coming from the beta site)
What is this fixing or adding?
Adds a new attribute on
WebWorld
calledoptions_presets
which is aDict[preset_name: str, Dict[option_name: str, custom_value: any]]
that world developers can use to define special option presets for games displaying on the WebHost. The default values for anyOption
-derived option is also automatically added to a "Defaults" preset for easy resetting of option presets. (no more manual clearing local storage!)This also adds a new drop down for presets that will automatically set the settings accordingly for the player on the website. See below gif for an example.
This PR also makes a couple tweaks to Player Name field to remove default value and display the placeholder value instead, allowing players to use "Player" as a name if they so desire. Also tweaks the
description
property in the outputted YAML to include preset name, if one was used.For an example, check out the presets I created for
rogue_legacy
in thePresets.py
file. You will also note that any options that are not manually listed in the preset dictionary will use the default value on theOption
-derived option instead, so you don't have to manually add every individual option if they're staying as a default.Things that still need to be done:
How was this tested?
Ran WebHost and test.
If this makes graphical changes, please attach screenshots.
Example of a game with presets defined:
For games without custom presets defined: