Ziktofel / Archipelago

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

Mm/item option refactor #192

Closed MatthewMarinets closed 4 months ago

MatthewMarinets commented 5 months ago

What is this fixing or adding?

Updating locked_items and excluded_items to take an amount parameter. Reworked most of the item filtering logic in __init__.py, as I had to change that code anyways to work with the new data structure.

Most code changes roughly follows a chain-of-responsibility model, so different effects can easily be added / removed / shifted in order by just adding to or removing from the chain. Things are essentially just kept in a list, with each item getting its own flags marking locks / start inventory / exclusion / plando. Plando flags are underused because I don't know how plando works.

Excluding / locking / unexcluding an amount of 0 just sets the amount to the maximum quantity. For filler items (quantity 0), the amount is unchanged. For a group, the specified amount is distributed to all elements in the group, and 0 is resolved per-item (so Terran Items: 0 will have 1 marine, 2 Progressive Orbital commands, and 3 Progressive Regenerative Biosteels).

This targets #185

How was this tested?

Added a bunch of unit tests, and ran a generation and played a mission through with some start inventory items. On Harvest of Screams, zerg units, morphs, and unit upgrades seemed to work fine.

Other changes

TODO

Ziktofel commented 5 months ago

What about using this: https://pypi.org/project/StrEnum/

Ziktofel commented 5 months ago

Or you can use plain enum like in SC2Mission:

class SC2Mission(Enum):
MatthewMarinets commented 5 months ago

I'm going to go the basic Enum route; using StrEnum was just a temporary measure while I tried to get something that works, as it's backwards compatible with the existing string == checks we do everywhere. It shouldn't be too difficult to track down, and mypy should catch anything I miss.

MatthewMarinets commented 5 months ago

This is still barely tested, but I need to sleep. Will do some local generation tomorrow, add some unit test, and maybe add the unexclude option if people have had time to weigh in.

MatthewMarinets commented 5 months ago

There's definitely more that I can play around with, but I think this is a good place to stop for a PR. The review will likely already take a while with how big this change is :/

There's enough tests that I'm fairly confident things are stable and the new options do what they say they do. There's also now a framework we can use to make new tests as issues get reported.

This PR removes all dependencies in ItemData.origin, instead looking at item groups added in ItemGroups.py. ItemData.origin can probably be removed now, idk if that should be in this PR. There's also further cleanup that can be done -- many item groups defined in Items.py can be moved to ItemGroups.py and given a user-facing name (and many are probably unused). I've also tried to avoid having item generation look at enabled campaigns as much as possible, instead preferring to look at the mission table. This means that excluding all the missions in a campaign should behave the same as disabling the campaign, and also promotes more fine-tuned item exclusion around matchups / no-builds / mission flags.

Ziktofel commented 5 months ago

I think the item filtering needs touching in order to get everything working

MatthewMarinets commented 5 months ago

What happens if you have vanilla only items, standard tactics and Evil Awoken?

Looks like the dynamic item filtering needs still adjusting

Good catch, that breaks generation. I'm inclined to say that the logic should change with vanilla items only, rather than the item filtering. In my mind, vanilla items only means vanilla items only. Will ask in the discord before proceeding, though.

Ziktofel commented 5 months ago

What happens if you have vanilla only items, standard tactics and Evil Awoken? Looks like the dynamic item filtering needs still adjusting

Good catch, that breaks generation. I'm inclined to say that the logic should change with vanilla items only, rather than the item filtering. In my mind, vanilla items only means vanilla items only. Will ask in the discord before proceeding, though.

The thing is this exact case is very rough for less skilled players, therefore this logic rule exists. I think the case with warning should do the case (if your exclusions break logic, the offending item gets back into pool). That would also increase generation stability (also for async that like to put settings to random).

Surely, logic rules isn't the place that will do "if X is excluded, do something else"

MatthewMarinets commented 5 months ago

What happens if you have vanilla only items, standard tactics and Evil Awoken? Looks like the dynamic item filtering needs still adjusting

Good catch, that breaks generation. I'm inclined to say that the logic should change with vanilla items only, rather than the item filtering. In my mind, vanilla items only means vanilla items only. Will ask in the discord before proceeding, though.

The thing is this exact case is very rough for less skilled players, therefore this logic rule exists. I think the case with warning should do the case (if your exclusions break logic, the offending item gets back into pool). That would also increase generation stability (also for async that like to put settings to random).

Surely, logic rules isn't the place that will do "if X is excluded, do something else"

The people have spoken: https://discord.com/channels/731205301247803413/980554570075873300/1235470259620675645

image

I will remove the Stalker upgrade dependency entirely. I'm in agreement with the general sentiment that there should be a vanilla composition that is in-logic.

I also notice this logic requirement is only on the victory check, and stalkers are not at all necessary for the fainl escape sequence between Temple Investigated and Victory.

Ziktofel commented 5 months ago

BTW the ppl who voted aren't the target audience for who this one rule exists (it exists mainly for casual/normal players playing on standard tactics)

So either bring the rule back or do a cleanup -> remove the Stalker upgrade function and mark those items as useful (they aren't used in any other rule and this rule is just for Evil Awoken)

MatthewMarinets commented 4 months ago

Noting that this also resolves most of #160 .