SC2: Local Victory Items option

[Alchav]

Wrote this for my own purposes but thought I would offer it upstream.

What is this fixing or adding?

Adds a Local Victory Items option to SC2. This places local items into Victory locations, which prevents other players' !collects from marking missions as completed when they are not. Items are not locked, but an Item Rule is added to these locations to ensure that if Progression Balancing, etc, swap items out, they do so only with other local items.

How was this tested?

Generating a multiworld and ensuring all victory locations contained local items.

[Alchav]

@Ziktofel

[Ziktofel]

I want some playtesting there to ensure that nothing breaks

[Ziktofel]

Did you do a playtest?

[Alchav]

Did you do a playtest?

I have not yet playtested a game. I've generated multiworlds and observed the spoiler log for correct behavior. Item placement is the only thing this PR changes

[Ziktofel]

I want to be sure that it won't mess up beat events and things like that are also attached to victory locations
(a multiworld with 2 tiny grids using terribleterribledamage should suffice)

As we're heading to release I don't want to introduce any game-breaking bug in the release

[Alchav]

I want to be sure that it won't mess up beat events and things like that are also attached to victory locations (a multiworld with 2 tiny grids using terribleterribledamage should suffice)

As we're heading to release I don't want to introduce any game-breaking bug in the release

Played a two-player game, one with local victory items on, one without. The one with the option on had local items in all victory locations, and there were no issues completing.

[Bicoloursnake]

I'm more familiar with SC2 specific shenanigans than the general AP architecture, but this all looks correct to me. Thanks for the new cool option!

[MatthewMarinets]

Is it possible to set up a unit test for this? I'm working on refactoring this code on my branch, it would be nice to have a way to verify I don't break this functionality when I merge it in.

[MatthewMarinets]

Berserker just raised a good point in the discord -- do we want to solve this problem in this way? An alternative approach would be to use a datastore to save mission completion state separately from items. That way, mission unlocks would not be affected by !collect / !release. I agree that method would be cleaner.

[NewSoupVi]

What are you wanting to go with?

For the record, I agree with the datastorage approach over this

[Ziktofel]

!release should always lead to all missions done. It'd be weird if all missions are only white (cleared) or grey (inaccessible) and nothing is blue (able to play, some checks left) if there's at least one grey mission

BTW nothing's changed since my last code review approval.

[NewSoupVi]

Ok, I finally had a look at this.

This current implementation breaks plando.

If a user plandos one of the randomly selected items that get placed during the restrictive_fill with from_pool: true, the generation will crash because the item has been taken out of the pool before plando runs.

"Place random items from the local itempool onto local checks before regular fill" is a use case that will always run into this issue unless the relevant filling is done in fill_hook.
Question at this point: Was the item rule not enough on its own? Did that lead to fill errors?

Sorry for asking so many questions, but I think I would specifically like to know if you're okay with breaking plando that way. Idk how much your userbase uses plando and would complain about it not working correctly anymore

[Ziktofel]

There's already a function to get locations subject to plando

def get_plando_locations(world: World) -> List[str]:
    """

    :param multiworld:
    :param player:
    :return: A list of locations affected by a plando in a world
    """
    if world is None:
        return []
    plando_locations = []
    for plando_setting in world.multiworld.plando_items[world.player]:
        plando_locations += plando_setting.get("locations", [])
        plando_setting_location = plando_setting.get("location", None)
        if plando_setting_location is not None:
            plando_locations.append(plando_setting_location)

    return plando_locations

Discard any plando'd locations from victory_locations