Mm/item option refactor

[MatthewMarinets]

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

• Remove Kerrigan presence option "Not present and no passives"
  • Should just exclude "Evolution Pit" item group to get rid of Kerrigan passives
• Psi Disrupter and HME are now auto-excluded if there are no TvZ build missions
• flag_start_inventory() (former assign_starter_items()) now checks start no-logic locations based on a dummy CollectionState rather than checking equality with the default logic function
• Nova gets a weapon in start inventory if she appears in the first mission, even if it's not Sudden Strike
• Sudden Strike can no longer pick a start unit that has its support item excluded or plando'd
• Fixed a broken anti-air logic requirement on Cutthroat
• Logic requirement changed on Evil Awoken to no longer require Stalker upgrades. Those upgrades don't exist in vanilla, meaning a vanilla comp wouldn't be within logic (and the discord agreed)
• Added many item groups to allow users to fine-tune their item pools into what was preset by options, and many new combinations
  • Replaced separate bw_items, nco_items, and ext_items options with a unified vanilla_items_only option, which limits the pool to only those items that exist in the 3 main vanilla campaigns (WoL, HotS, and LotV)
  • Other compositions, such as NCO units+upgrades only, or NCO+WoL, or NCO units with all upgrades, etc, can be created with the new item groups
• Added an un-exclude option, to allow fine-tuning exclusions without locking items
  • Excluding group + unit is done with two entries in excluded_items, group and unit
  • Excluding group - unit is done with group excluded and unit unexcluded
  • Whitelisting behaviour is done by excluding a large group, and unexcluding items from that group. Example:

excluded_items:
  Terran Items: 0
unexcluded_items:
  NCO +Items: 0,
  NCO -Items: 1,

TODO

• Remove Kerrigan Presence option "Not present and no passives"
• Remove ItemOrigin or use it
• Remove bw_items, nco_items, ext_items; replace with item groups and a single vanilla_only switch
• Add an "un-exclude" option that allows for a whitelisting effect / adding items back to a pool, without necessarily locking them
• Apparently older Python doesn't have StrEnum, will have to either update all usages of ItemType and make it a regular enum, or make it a plain string.

[Ziktofel]

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

[Ziktofel]

Or you can use plain enum like in SC2Mission:

class SC2Mission(Enum):

[MatthewMarinets]

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]

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]

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]

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

Looks like the dynamic item filtering needs still adjusting

[Ziktofel]

This is mainly there for possible starter Flashpoint, not early End Game. End Game can be excluded and the player still might start with Flashpoint

[MatthewMarinets]

Flashpoint is classed as a hard mission, I don't see it as very likely that it would be a starter mission unless the player has very deliberately made a yaml to make that likely. If they did that, I would expect them to also handle setting the logic level and start inventory such that they could beat the seed.

Do you have specific settings that would cause a generation failure without this adjustment? If so, I could make a unit test to guarantee we solve the problem. Otherwise, I'm inclined to just remove this adjustment.

[MatthewMarinets]

Made a usecase test for NCO only with no-builds excluded, and the test passed fine with this code snippet removed. I'm removing it fully.

[Ziktofel]

Flashpoint is one of possible starters:

    if len(mission_pools[MissionPools.STARTER]) + len(mission_pools[MissionPools.EASY]) < 2:
        # Flashpoint needs just a few items at start but competent comp at the end
        move_mission(SC2Mission.FLASHPOINT, MissionPools.HARD, MissionPools.EASY)

The order is vanilla shuffled and the run is NCO only. (the NCO lane doesn't have starter a mission but starts with an easy one)

[Ziktofel]

Won't this following code remove the required items and fail the generation:

        # LotV
        # Shared unit upgrades between several units
        if not {ItemNames.STALKER, ItemNames.INSTIGATOR, ItemNames.SLAYER} & logical_inventory_set:
            inventory = [item for item in inventory if not item.name.endswith("(Stalker/Instigator/Slayer)")]
            unused_items = [item_name for item_name in unused_items if not item_name.endswith("(Stalker/Instigator/Slayer)")]

[MatthewMarinets]

You're right. The Evil Awoken logic is a real pain for generation; I'm asking in the discord if we should rework it somehow. If we keep it, I think the solution is to add the Locked flag in __init__.py when adding the AllowedOrphan flags. The alternatives are to loosen the logic or to also refactor PoolFilter.py, which would make this already-large PR even larger.

Will add a unit test for this as well.

[Ziktofel]

I think the solution is to:
While parsing excluded items in ValidInventory.generate_reduced_inventory(), first process excluded flags:
shuffle items with excluded flag ans validate against logic. If logic fails, the item gets locked instead and a warning is emitted
Then regular size-based filtering would happen
You also need to change that method in order to allowOrphan work correctly (and the rule dropping Stalker upgrades should take it into account)

Using this approach would also keep the logic able to handle standard tactics on future Mastery locations without making the game too rough

This rule is written in a way so the upgrades won't get orphaned thus the item filtering won't remove them. The proper handling of allowOrphan flag should solve that problem instead.

[MatthewMarinets]

I see where you're coming from implementation-wise, but I'm saying it's a bad requirement. If a player selects an option saying "only include vanilla items", and they got non-vanilla items, that's a bug, no two ways around it. When faced with a situation where it must be possible to generate this mission with vanilla items only, and where this mission requires non-vanilla items, one requirement must go. This is why I took this to the discord, and the users agreed that the logic requirement should be removed.

I don't want to refactor all of PoolFilter.py as well, as this PR is already colossal, and I think there are more easy improvements to be made by touching up mission selection / mission order generation / adding mission groups. If I was to refactor it, I would pass in the complete item list with flags, and use those throughout the file rather than the current set-inclusion based filtering. Work for a future PR, but I'd like to limit the scope at least somewhat.

[Ziktofel]

Clean it up (it gets unused) and mark those items as useful (they won't be logic items anymore).

[Ziktofel]

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

[MatthewMarinets]

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]

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"

[Ziktofel]

This change would require some kind of save/loading support due to skill issues at lower end. This mission is really painful.

[MatthewMarinets]

Removing the logic rule entirely, following the poll I put in the discord (removing logic beat out all other options with only one dissenting vote).

I think this is not an issue. We expect players to have already beaten the mission in vanilla, which is a case where they do not have these upgrades. We're also expecting them to either be playing at a difficulty level lower than they beat it in vanilla, or be doing a repeat playthrough and know what they're getting into. If they're really getting stuck, they can turn the difficulty down further. Finally, this logic rule only applies to the victory location, not to anything earlier. The escape sequence between temple investigated and victory barely requires stalkers to begin with, and is best done by just running zeratul and blinking (I know I consistently lose my stalkers within the first half, if not the first quarter of the sequence).

[Ziktofel]

How about NCO enabled? NCO isn't one of the 3 main campaigns, thus NCO items won't be present?
Surely, this option doesn't limit item availability cross campaign (like Vikings are still available in NCO)

[MatthewMarinets]

As I say in the comment, if players want more than the vanilla grouping, they should use item exclusions / item groups.

Exclude all terran units, unexclude NCO units. I'm not sure how we want to approach upgrades, WoL upgrades has a group, and could add NCO upgrades.

[MatthewMarinets]

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

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]

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)

[Ziktofel]

Just some formatting/cleanups

[Ziktofel]

Carrier is alone, Tempest, Mothership and Scout are grouped together in the command card

[Ziktofel]

(A Purifier variant of Carrier is planned for the future)

[MatthewMarinets]

I'm just going to separate them out on their own lines as the relationship is non-obvious to me. Whoever adds the new carrier variant item can reformat it as they wish.

[Ziktofel]

The other units there are put in lines as they're built in their buildings. Therefore it makes sense to follow that pattern for consistency

[MatthewMarinets]

I do not see a logic connection between scout, tempest, and mothership, that excludes carriers, and don't see the need to copy the command-card layout exactly seeing as it's subject to change. Personally, the grouping I would consider intuitive is (carrier, tempest) as capital ships and scout and mothership separated / as miscellaneous.

Remember this currently isn't user-facing, and thus the styling isn't immediately important, but in future we need to provide some way for yaml writers to look these groups up. That will involve either pointing them straight at this file, in which case the grouping doesn't really make much sense to me; or it will involve some automated script to collect the groups into a list we can post somewhere, in which case order matters but indentation doesn't. In the latter case, the order I'd expect for these "ungrouped" starships is scout -> carrier -> tempest -> mothership or scout -> tempest -> carrier -> mothership, as that's roughly the "other" category organized by size. I could maybe see it worthwhile to put arbiter and oracle before this segment.

Also, are we going to have the mothership on the stargate indefinitely, or are there plans to put it on the nexus like in melee?

[MatthewMarinets]

Stared at it very hard. If tempest goes first in that line then I guess the exported version will still put carrier and tempest next to each other, which is good enough I guess. Updated.

[MatthewMarinets]

Noting that this also resolves most of #160 .