Core: Add "CounterOption", use it for generic "StartInventory" and Witness "TrapWeights" #3756

NewSoupVi · 2024-08-09T19:41:57Z

The purpose of this is best explained here:
#3754

Basically, CounterOption is an option whose value is a collections.Counter.

This means that (unlike an OptionDict), the values are always ints.
This means they can be rendered on the Website using the same code as ItemDicts, as long as they have valid_keys defined.

In fact, the ItemDict class has been changed to be a subclass of the new CounterOption.
For backwards compatibility (ensuring that in checks resolve the same way as before), ItemDicts make sure to cull 0s first before making the Counter.

This does make a change to Starting Inventory behavior: starting_inventory = {"Sword": 0} now behaves as if you had written starting_inventory = {}, instead of crashing. Imo this is an improvement, but your mileage may vary.

Tested:
Webhost Player Options: "Export Options" button, "Generate Single-Player Game" button
Generate: With yamls with varying keys missing or present

Not tested:
Advanced Options Page

Thing I'm unsure about:
Whether the Utils.py change is safe

NewSoupVi · 2024-08-09T19:52:51Z

Well, what did I do then

Exempt-Medic · 2024-08-09T21:48:38Z

Of note, this makes Subnautica fail differently if all values are set to 0. Previously it failed trying to generate the options, but now it would fail in Subnautica's own generate_early

NewSoupVi · 2024-08-09T22:06:51Z

Of note, this makes Subnautica fail differently if all values are set to 0. Previously it failed trying to generate the options, but now it would fail in Subnautica's own generate_early

The same thing happens on main when you put filler_items_distribution: {} in your yaml, actually. Which is a perfectly valid input, so I might claim this is just "exposing a different bug" at worst

In fact, the same should be true for all cases - "All 0s" will yield the same behavior as {} used to, if I did everything right.

NewSoupVi · 2024-08-09T22:52:34Z

Ok, Exempt-Medic and I went over a lot of the cases of worlds using start_inventory and ItemDict. It seems none of them are doing any of the operations that would make this change to start_inventory dangerous.

The big problem would be this:

try:
    self.options.start_inventory["Non existent item"]
catch KeyError:
    foo()

as this would no longer work - Counter objects never KeyError.

I think every other difference between a dict and a Counter can never be an issue due to how I've implemented CounterOption & ItemDict.

The only supported worlds using ItemDict right now are Subnautica and KH2. For KH2, the new implementation of ItemDict works fine. For Subnautica, there is currently a bug with filler_items_distribution = {}, that can now be "reached" by doing filler_items_distribution = {"Valid Key": 0} as well, which was not a valid input before, but is an "alias" for {} with my PR, so this is expected and acceptable to me.
Edit: There's also Lufia2, which seems to be fine with this change.

Happy to be proven wrong, in which case I can either amend the implementation of CounterOption, or drop the idea of changing StartInventory/ItemDict to a CounterOption if necessary.

alwaysintreble · 2024-08-10T11:51:13Z

you're only doing the culling and negative erroring for ItemDict which is a subclass of CounterOption, so TrapWeights can have negative values which sounds unintended from your description.

NewSoupVi · 2024-08-10T11:51:53Z

you're only doing the culling and negative erroring for ItemDict which is a subclass of CounterOption, so TrapWeights can have negative values which sounds unintended from your description.

TrapWeights still has a Schema. On main, it is an OptionDict with a Schema, not an ItemDict. I had a PR open changing it to an ItemDict, but I closed that in favor of this.

Update: TrapWeights no longer has a schema, as I added min and max arguments.

Options.py

alwaysintreble · 2024-08-16T01:49:08Z

Options.py

+
+
+class CounterOption(OptionDict):
+    min: typing.Optional[int] = None


Debatable but I think a default min of 1 probably has the most universal use cases

I'm not sure about that? I'm aware of three options that use this or would want to use this, and two of them want min = 0.

alwaysintreble · 2024-08-16T01:53:48Z

Options.py

    def __init__(self, value: typing.Dict[str, int]):
-        if any(item_count < 1 for item_count in value.values()):
-            raise Exception("Cannot have non-positive item counts.")
+        # Backwards compatibility: Cull 0s to make "in" checks behave the same as when this wasn't a CounterOption


Since this is just for back compat and verify is called after the option itself is initialized, min should probably be 1 and just leave the culling.

Separate conversation ig but it seems weird to leave in the culling at all since it's not like the actual option was getting those keys with 0 Val already

Ohh, hm, I definitely intended that for start_inventory, {"Sword": 0} is a valid input now.

Options.py

Exempt-Medic · 2024-12-06T21:40:15Z

test_option_presets probably needs to updated from ItemDict to CounterOption. Also "options api.md" should probably include information on the new option type

Exempt-Medic · 2024-12-06T21:49:19Z

WebHostLib/templates/playerOptions/playerOptions.html

@@ -93,8 +93,8 @@ <h1>Player Options</h1>
                                        {% elif issubclass(option, Options.FreeText) %}
                                            {{ inputs.FreeText(option_name, option) }}

-                                        {% elif issubclass(option, Options.ItemDict) and option.verify_item_name %}
-                                            {{ inputs.ItemDict(option_name, option) }}
+                                        {% elif issubclass(option, Options.OptionCounter) and option.valid_keys %}


This change from verify_item_name to valid_keys makes StartInventory no longer appear anywhere.

Gonna have to investigate that, because I thought verify_item_names makes it build a valid_keys from the metaclass
Been a while since I tested this now tho

Well, it did not work the way I thought it did. StartInventory should be back & code is now more flexible but also significantly uglier :/

…ly work

alwaysintreble · 2024-12-09T03:30:30Z

Code seems fine (I didn't test it :)) but the docs definitely should be updated as part of this PR.

NewSoupVi · 2024-12-09T08:56:15Z

True, I'll mark this as waiting on author for docs changes

NewSoupVi added 3 commits August 9, 2024 21:29

CounterOption

af277a3

bring back the negative exception for ItemDict

b67fe48

Backwards compatibility

271142c

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 Aug 9, 2024

NewSoupVi added the is: enhancement Issues requesting new features or pull requests implementing new features. label Aug 9, 2024

ruff on witness

28e99b7

NewSoupVi added 4 commits August 9, 2024 21:59

fix in calls

2ad0f62

move the contains

c9f1723

comment

51e5226

comment

34c9931

NewSoupVi added 5 commits August 10, 2024 14:49

Add option min and max values for CounterOption

45474ad

Use min 0 for TrapWeights

6a4b274

This is safe now

f4b18cd

ruff

ba5ffe9

This fits on one line again now

a3228ee

alwaysintreble reviewed Aug 16, 2024

View reviewed changes

NewSoupVi added 4 commits August 16, 2024 04:10

OptionCounter

5c2e8bc

Update Options.py

67448de

Couple more typing things

54ed306

Merge branch 'main' into intvalueoption

1f8a474

Exempt-Medic reviewed Dec 6, 2024

View reviewed changes

Options.py Outdated Show resolved Hide resolved

Exempt-Medic reviewed Dec 6, 2024

View reviewed changes

NewSoupVi added 3 commits December 7, 2024 08:02

Update Options.py

8e2bc08

Merge branch 'main' into intvalueoption

adbf883

Make StartInventory work again, also make LocationCounter theoretical…

7f7def4

…ly work

NewSoupVi added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add "CounterOption", use it for generic "StartInventory" and Witness "TrapWeights" #3756

Core: Add "CounterOption", use it for generic "StartInventory" and Witness "TrapWeights" #3756

NewSoupVi commented Aug 9, 2024 •

edited

Loading

NewSoupVi commented Aug 9, 2024

Exempt-Medic commented Aug 9, 2024 •

edited

Loading

NewSoupVi commented Aug 9, 2024 •

edited

Loading

NewSoupVi commented Aug 9, 2024 •

edited

Loading

alwaysintreble commented Aug 10, 2024

NewSoupVi commented Aug 10, 2024 •

edited

Loading

alwaysintreble Aug 16, 2024

NewSoupVi Aug 16, 2024

alwaysintreble Aug 16, 2024

NewSoupVi Aug 16, 2024

Exempt-Medic commented Dec 6, 2024 •

edited

Loading

Exempt-Medic Dec 6, 2024 •

edited

Loading

NewSoupVi Dec 7, 2024 •

edited

Loading

NewSoupVi Dec 7, 2024

alwaysintreble commented Dec 9, 2024

NewSoupVi commented Dec 9, 2024



		class CounterOption(OptionDict):
		min: typing.Optional[int] = None

Core: Add "CounterOption", use it for generic "StartInventory" and Witness "TrapWeights" #3756

Are you sure you want to change the base?

Core: Add "CounterOption", use it for generic "StartInventory" and Witness "TrapWeights" #3756

Conversation

NewSoupVi commented Aug 9, 2024 • edited Loading

NewSoupVi commented Aug 9, 2024

Exempt-Medic commented Aug 9, 2024 • edited Loading

NewSoupVi commented Aug 9, 2024 • edited Loading

NewSoupVi commented Aug 9, 2024 • edited Loading

alwaysintreble commented Aug 10, 2024

NewSoupVi commented Aug 10, 2024 • edited Loading

alwaysintreble Aug 16, 2024

Choose a reason for hiding this comment

NewSoupVi Aug 16, 2024

Choose a reason for hiding this comment

alwaysintreble Aug 16, 2024

Choose a reason for hiding this comment

NewSoupVi Aug 16, 2024

Choose a reason for hiding this comment

Exempt-Medic commented Dec 6, 2024 • edited Loading

Exempt-Medic Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

NewSoupVi Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

NewSoupVi Dec 7, 2024

Choose a reason for hiding this comment

alwaysintreble commented Dec 9, 2024

NewSoupVi commented Dec 9, 2024

NewSoupVi commented Aug 9, 2024 •

edited

Loading

Exempt-Medic commented Aug 9, 2024 •

edited

Loading

NewSoupVi commented Aug 9, 2024 •

edited

Loading

NewSoupVi commented Aug 9, 2024 •

edited

Loading

NewSoupVi commented Aug 10, 2024 •

edited

Loading

Exempt-Medic commented Dec 6, 2024 •

edited

Loading

Exempt-Medic Dec 6, 2024 •

edited

Loading

NewSoupVi Dec 7, 2024 •

edited

Loading