SC2: Fix unused_items refill to respect item dependencies.

[Ziktofel]

What is this fixing or adding?

See bug: Ziktofel#187
After main culling process done, some items might be brought back. However, the item dependencies were ignored. This fixes that by filtering unused_items by using dependency rules

How was this tested?

Tested with YAML that produced the issue, the problem is gone now

If this makes graphical changes, please attach screenshots.

No

[MatthewMarinets]

The change makes sense, though the style looks a bit error prone. Tidying this up would be nice, but would require a significant refactor in how we treat item parents / dependencies. This bugfix looks good for now.

To explain what's going on (for future reviewers and to test my knowledge):

• Currently, if there are too many items for the number of locations, items are culled
• Some items depend on other items to have any effect; these items are also culled along with their parent items
• The generator keeps track of some unused items that can be added back after further culling (line 395) -- when an upgrade item gets culled directly, it gets added to the unused items list
• This list of unused items wasn't properly culled if parent items were removed from the item pool

I see two ways that things could have been failing:

1. An upgrade item could be culled (say baneling splash area), which would add it to the unused items list. Then the parent item gets culled, which also culls all the child items from the item pool, but doesn't properly remove baneling splash from the unused items list. Baneling splash could then be added back to the item pool when items are pulled back from the unused list
2. We have some upgrades with multiple parents -- e.g. Zealot leg speed can affect Zealots, Centurions, and Sentinels. Because it's not a direct child, zealot leg speed is not culled when any of its parents are culled. This multi-parent culling happens below (around line 405 onwards) -- but by that point the unused items list was already populated, and it didn't get culled.

This change should fix both possible paths to a bug.

For a future refactor, it would be nice if we could package up the items lists with a structure of some sort and helper functions that could handle these complicated parent-child item dependencies.

[Ziktofel]

Yeah, the item dependencies could use some refactoring but that's out of scope of this PR

[Bicoloursnake]

Adding typing to variables is always appreciated. It makes sense for unused_items to have the same filters as inventory. That should fix the problems we're seeing. Disclaimer: did not test, but extra filters on unused_items look to correctly mirror filters on inventory.

[Magnemania]

There's probably more elegant ways to do this, but it works and is readable.

[Ziktofel]

Yeah, it's a bugfix, I've straightly fixed that so it can't break anything else

[Ziktofel]

Changed back due to @Salzkorn's tests

[Bicoloursnake]

Thanks Salz for catching that. I forgot that the parent name is an item name string, not an item itself. This looks good now.

[Ziktofel]

Yeah, I've added a type check to validate that

[MatthewMarinets]

Fix looks good at a glance. Not sure if the type check was checked with mypy / a static analyzer, but I'd recommend doing so if it hasn't been done already.

[Ziktofel]

In Intellij it highlights the code if it fails type check