search

ArchipelagoMW / Archipelago

Archipelago Multi-Game Randomizer and Server
https://archipelago.gg
Other
412 stars 557 forks source link

WebHost: Fix NamedRange values clamping to the range in player-options #3613

Open remyjette opened 5 days ago

remyjette commented 5 days ago

What is this fixing or adding?

If a NamedRange has a special_range_names entry outside the range_start and range_end, the HTML5 range input will clamp the submitted value to the closest value in the range.

These means that, for example, Pokemon RB's "HM Compatibility" option's "Vanilla (-1)" option would instead get posted as "0" rather than "-1".

This change updates NamedRange to behave like TextChoice, where both the select dropdown and the additional input element for custom values get posted with the form.

This uses a different suffix of -range rather than -custom that TextChoice uses. The reason for not just reusing -custom is we need some way to decide whether to use the custom value or the select value, and that method needs to work without JavaScript. For TextChoice this is easy, if the custom field is empty use the select element. For NamedRange this is more difficult as the browser will never submit an empty string for a range input. My choice was to only use the value from the range if the select box is set to "custom". Since this only happens with JS as "custom' is hidden, I made the range hidden under no-JS, thus limiting the options under no-JS to those provided by the select dropdown. If it's preferred, I could make the select box hidden instead. Let me know. But I didn't want to remove the custom entry field for TextChoice, which is why I went with a different suffix.

This PR also makes the js-required class set display: none with !important as otherwise the class wouldn't work on any rule that had display: flex with more specificity than a single class.

Fixes https://discord.com/channels/731205301247803413/1257712995593617449

How was this tested?

With both JS enabled and disabled, I created Pokemon RB YAMLs with the HM/TM compatibility options set to Vanilla (-1), None (0), Full (100), and for JS enabled only various custom options with the sliders.

NewSoupVi commented 5 days ago

@ThePhar I think you should have a look at this too, as you're working on another big WebHost rework.

LegendaryLinux commented 3 days ago

The original reason supporting values outside the normal range was implemented was, to my knowledge, to allow users to exclude certain options from being chosen if "random" is selected. Rather than add complexity to the WebHost to support this system, the proper solution is to change the system to more gracefully exclude specified options from "random." There was discussion about this in #ap-core-dev a while back.

remyjette commented 3 days ago

The games in core that create a NamedRange with options outside the range (and thus are affected by this bug) are:

In all cases, the reason for an option outside the range is to provide an experience that is completely different from what an option in the range would provide. Thus it seems less like the goal is 'allow users to exclude certain options from being chosen if "random" is selected' and more like "provide an option that provides a completely different experience than one you would select from the slider". Missing by a pixel and getting 51 instead of 50 for most of the above options would basically be the same rando. -2 vs -1 vs 0 in RandomCharmCosts are all completely different and much better selected by the dropdown, and it kind of makes sense that the slider applies to the range only.

Moreover, this is a bug actively impacting users right now. We've seen multiple reports of users trying to select -2 for RandomCharmCosts or -1 for TMHMCompatibility and getting a completely different rando than what they expected. In PokemonRB it's particularly bad because 0 means only one Pokemon in the entire game can learn each HM; that's a significantly different and more challenging experience than what they thought they signed up for with their YAML. I'm perfectly okay if my PR is later replaced with a "better" fix, but I wrote this to try and help users who are running into this now.

Exempt-Medic commented 1 day ago

The games in core that create a NamedRange with options outside the range (and thus are affected by this bug) are:

  • Hollow Knight RandomCharmCosts, to provide "vanilla" and "shuffled" options
  • Lufia 2 ShopInterval to provide "disabled" a.k.a. vanilla
  • Pokemon Emerald HmCompatibility and TmTutorCompatibility to provide "vanilla"
  • Pokemon Red and Blue TMHMCompatibility and SecondaryTypeChance to provide "vanilla"
  • Stardew Valley StartingMoney to provide "unlimited"

In all cases, the reason for an option outside the range is to provide an experience that is completely different from what an option in the range would provide. Thus it seems less like the goal is 'allow users to exclude certain options from being chosen if "random" is selected' and more like "provide an option that provides a completely different experience than one you would select from the slider". Missing by a pixel and getting 51 instead of 50 for most of the above options would basically be the same rando. -2 vs -1 vs 0 in RandomCharmCosts are all completely different and much better selected by the dropdown, and it kind of makes sense that the slider applies to the range only.

Moreover, this is a bug actively impacting users right now. We've seen multiple reports of users trying to select -2 for RandomCharmCosts or -1 for TMHMCompatibility and getting a completely different rando than what they expected. In PokemonRB it's particularly bad because 0 means only one Pokemon in the entire game can learn each HM; that's a significantly different and more challenging experience than what they thought they signed up for with their YAML. I'm perfectly okay if my PR is later replaced with a "better" fix, but I wrote this to try and help users who are running into this now.

Well, then your conclusion doesn't match the ones I found when asking world devs why they used this. The point is so that random doesn't pick the special values

remyjette commented 1 day ago

More than just "random doesn't pick it", my point was I think it also makes sense for the slider to not pick it either.

agilbert1412 commented 1 day ago

If I had been asked, my response would have been closer to remyjete's. Now that you mention it, I guess random is also a concern, but that's the kind of thing that I only would have thought of later, once someone brings it up.

My intent is definitely "Here is a range of normal values, and here is one or several special values that can be used instead of the range, affecting the same game mechanic but in a completely different way"