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

[remyjette]

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]

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

[LegendaryLinux]

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]

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.

[Exempt-Medic]

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]

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]

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"

[LegendaryLinux]

If the goal is to provide a different game experience altogether, why not have a different boolean option which if true would override the range option? Back in the day LttP used to have triforce options which would only apply if "Triforce Hunt" was chosen.

[Zunawe]

With no way to have the value of one option enable or disable another, it feels more elegant to have a special number that can live in the same option it's overriding.

Splitting HM compatibility into a "Modify HM Compatibility" and "HM Compatibility Chance" means having an option value that potentially has no effect. There are other options in Emerald that are in this redundancy situation, but I don't like those either. And I have seen them cause minor confusion.

[NewSoupVi]

To add my two cents to this:

These games are doing something that is supported by the NamedRange class, which is a class provided by core. For a world dev, using a core class in a way it was intended to be used should not come with unexpected surprises. Players are negatively affected by this right now in 5 immensely popular games that are merged into main.

We can design / redesign core classes with the GUI applications in mind, but most of the time, the GUI should properly support the current state of the program logic unless it's somehow unreasonable. I don't think this change/fix is unreasonable.

In my opinion, this situation is pretty dire and I think it absolutely needs to be addressed before the release of the next version of Archipelago, and I'm happy someone found a relatively low amount of code changes that seems to fix the issue on the GUI side. (In contrast, the core change would require redesigning a class & touching 5 worlds, which requires getting approval from all their maintainers, and potentially breaking a bunch of unsupported apworlds)

[NewSoupVi]

Essentially, I'm very surprised and taken aback that, even if you believe that the design of the NamedRange class is strange here, the reaction hasn't been more along the lines of "Well, that's annoying, I wish NamedRange didn't do that; But yeah the effect on players right now seems pretty freaking bad, so yeah let's implement a GUI side fix, it's not gonna be that difficult even if I think it's ugly"

I guess if nothing else, I really want to stress the importance of this issue to me, I easily consider this a blocker for 0.5.1 (i.e. I agree with the labeling), ~~and it should've been a blocker for 0.5.0~~ (Edit: Exempt-Medic has made me aware this bug wasn't discovered until after the release of 0.5.0, so that's fair, my bad)

[LegendaryLinux]

I'm very surprised and taken aback A little dramatic, but whatever.

There is nothing technically unsound about the fix, so I will accept it.

That said, I think the people writing the core options should consider how they will be presented in GUIs. Perhaps the proper solution for this is a new option type which is a Choice, but with a "custom" value which is represented by a range.

[NewSoupVi]

Yeah I'll admit, it was probably a little dramatic :D

If you wanna talk about how this feature could be reimplemented in a way that supports UI stuff better, we can probably do that as well. I'm unfortunately not super familiar with this class myself, so I might be a bit useless on that front