Core: Replace item link locations with modified collect/remove

[Mysteryem]

What is this fixing or adding?

This patch removes the locations from item links, fixing a couple of issues with item links:

1. By removing the locations, there are no longer any concerns about unreachable item link locations failing location accessibility requirements.
2. Because the linked items are no longer placed at locations and are collected with the group unlocker items simultaneously, additional spheres are no longer required to pick up the linked items.

However, there are some downsides:

1. In a spoiler, it is no longer immediately clear which players have an item locked behind a group unlocker item.
   • This has been resolved by changing the conversion to a string representation for group items/locations to also include the names of the players that belong to the group.
2. In a spoiler playthrough, it is no longer clear which of the linked items unlocked by collecting a group unlocker item are required to beat the game. Previously, each linked item would be placed and locked at a unique location, so only each individual linked item that was required to beat the game would be present in the spoiler playthrough. With this patch, if a group unlocker item unlocks linked items for 10 players, but only 1 of those linked items is needed to beat the game, there is no way to tell that this is the case because only the group unlocker item will show in the playthrough.
3. Because the linked items are no longer placed at locations, they will not be found by worlds that use multiworld.get_spheres() or similar.

How was this tested?

I generated seeds with Bumper Stickers with Everything item linked, as well as A Hat in Time with linked Time Pieces, Hookshot Badge and Scooter Badge.

If this makes graphical changes, please attach screenshots.

In a spoiler, a location/item belonging to a group will now also write the names of the players within that group:

Treasure Bumper 28 (Player5): 100 Pons (Player3)
Treasure Bumper 29 (Player5): Treasure Bumper (link_test (Player8, Player5, Player6, Player7))
Treasure Bumper 30 (Player5): Yarn (Player2)

[Mysteryem]

I could not find a good place to modify the collect/remove methods. I don't think monkey patching is great, but I don't think there are any good options.

MultiWorld.add_group() currently contains no behaviour that is clearly specific for supporting item links. From what I've seen discussed, the less that core has to specifically support item links behaviour the better.

Even if the collect/remove methods for handling item links were set in MultiWorld.add_group(), the item links have not been set up at that point, meaning that the collect/remove methods would have to get the linked Items to collect/remove from somewhere, likely the group's World or the Group (TypedDict) instance itself, and MultiWorld.link_items() would have to set the linked Items onto whichever type is chosen.

• Storing the linked Items on the Group isn't ideal because the Group already contains a reference to the World and this would result in the World's collect/remove now having reference to the Group. At which point, it feels like they should just be stored on the World instead, however...
• Storing the linked Items on the World isn't ideal because to get any type checking for where the linked Items are stored would requiring modifying AutoWorld.World to include a typed attribute for item links. As has already been mentioned, it is preferable to avoid modifying core specifically to support item links.
• Possibly the linked Items could be stored on the MultiWorld itself since MultiWorld already contains some the item_links attribute that is specific to supporting item links, though this would require an extra step of storing all the linked Items in a dictionary keyed by the group ID because there is only one MultiWorld shared by all the Worlds/Groups.

[Mysteryem]

Initially, I modified the name set in MultiWorld.player_name in MultiWorld.add_group(), when creating a Group for item links, to include all the players in the group. This seemed to work, but I suspect was not safe to do.

[Mysteryem]

group_players is a set, so has no particular ordering. It might be beneficial if the players were sorted into a specific order. Sorting group_players within each get_name_string_for_object call does not seem like it would be a good idea, but maybe a "sorted_players" key could be added to Group that will contain the players after sorting.

[Mysteryem]

As an extension to this PR, it would be possible to start with an empty List[List[Item]] and only append to it from within the loop below. Doing so would eliminate the creation of many empty List[Item] in the case of item linking non-advancement items.

[NewSoupVi]

I had a quick glance, this seems like the best solution so far. The only thing I'm concerned about is whether this still plays nicely with accessibility. Thinking about it, it should, but I'm not 100% sure immediately.

[Mysteryem]

This PR breaks HK's GrubHuntGoal: all option because it uses multiworld.get_items() to get all the Grub items in the multiworld. With this PR, it would need to use something similar to how MultiWorld.find_item_locations(resolve_group_locations=True) works with locations, such that each player's Grub count would include the counts of Grubs belonging to each group that each player is in. Though the option in HK seems to me to not be working as intended and/or not understanding how item links work to begin with:

On main:
With 4 yamls with RandomizeGrubs: true and Grubs linked with replacement_item: null, it's setting each player's world's .grub_count to 184 (despite only 46 existing for each player), as well as setting the group world's .grub_count to 46. It is then setting each player's completion_condition to require that the state has both 46 Grubs belonging to the player and 46 Grubs belonging to the group world, but including the group in the completion condition is pointless.

Before this PR, if HK had wanted to count the number of Grubs belonging to a player in the multiworld, then all it would have needed to do was count the number of Grub items belonging to each player, there was no need to look at groups at all. I'll submit a PR to fix this in HK.

Archipelago/worlds/hk/__init__.py

Line 491 in 33daebe

grubs = [item for item in multiworld.get_items() if item.name == "Grub"]

---

From looking at the HK code, I think there is a point to be made that there does need to be some way for worlds to access the linked items or tell the difference between an unplaced item and an item linked item.

Currently, the HK code assumes that each of the Grub items are classified as progression, which is not a problem, and if it needed to check how many were classified as progression, it could do so. But with this PR, the only items it would be able to access and check are the group items which do not give any information about the individual items that have been linked. An alternative is to keep track of the items as they are created, but it would then need to be able to tell the difference between an item that was not placed in the multiworld and an item that was item linked, which is not currently possible with this PR.

[NewSoupVi]

For the record: This PR also breaks Witness Audio Log Hints.
It won't crash, but in an everything-linked world, you'll just not get any hints.

This is because Witness keeps references to its item instances for the sake of performance (find_item() is very slow), and considers item.location is None as "unplaced", and "unplaced" as "unhintable".

Witness would greatly appreciate a way to tell easily whether an item was linked without having to search the multiworld.

[qwint]

RE Grubhunt
link replacement grubs currently do not create items for the sub-worlds, just the itemlink world/group
this is why that code looks ugly because I have two uses for count,

1. logically require every grub (if the user has chosen 'All', if they haven't i just use their number and don't care about links etc)
2. know how many grubs will be sent to the client to save in slot data

since the current code is explicitly working around an implementation detail of how item links used to work it will absolutely need to be rewritten, but my actual usecase is pretty simple under the assumption that replacement items will also be collected to the world(s) that they will be granted to by the server (because then its just how many grubs do i have in all-state for both)

[Mysteryem]

RE Grubhunt link replacement grubs currently do not create items for the sub-worlds, just the itemlink world/group this is why that code looks ugly because I have two uses for count,

I think the link_replacement: true case not creating items for the worlds in the group is bug in item links, and not something that HK should be trying to work around. Either all the linked items belonging to each group member should exist in the multiworld, or none of the linked items belonging to each group member should exist in the multiworld. I would say that multiworld.get_items() and the other methods for finding items in the multiworld are therefore already broken in main when link_replacement: true is used.

Due to how linked items are collected based on a count of group items collected, meaning the group items can be collected in any order and the linked items will be collected in a consistent order, adding extra group locations and linked items to the multiworld for link_replacement: true would normally have no effect on logic, so not adding them results in better performance. But I think that consistency of behaviour should be valued more than performance here.

---

The HK code does not look correct even when there are no linked replacement grubs:

With 4 yamls with RandomizeGrubs: true and Grubs linked with replacement_item: null, it's setting each player's world's .grub_count to 184 (despite only 46 existing for each player)

[qwint]

The HK code does not look correct even when there are no linked replacement grubs:

With 4 yamls with RandomizeGrubs: true and Grubs linked with replacement_item: null, it's setting each player's world's .grub_count to 184 (despite only 46 existing for each player)

looks like you found a bug that got introduced in berserker and vi's attempts to optimize grubhunt #4094

I think the link_replacement: true case not creating items for the worlds in the group is bug in item links, and not something that HK should be trying to work around. Either all the linked items belonging to each group member should exist in the multiworld, or none of the linked items belonging to each group member should exist in the multiworld. I would say that multiworld.get_items() and the other methods for finding items in the multiworld are therefore already broken in main when link_replacement: true is used.

Am I supposed to do something with this? If you fix item links (both link replacement items not existing for subworlds and locations being unreachable) in a way that I can just check all state for logically required grubs and total grubs to be sent client, then that code can be simplified and I'd be happy with that.

[Mysteryem]

Am I supposed to do something with this? If you fix item links (both link replacement items not existing for subworlds and locations being unreachable) in a way that I can just check all state for logically required grubs and total grubs to be sent client, then that code can be simplified and I'd be happy with that.

Nothing for you to do. I just think that any full fix/rewrite for item links should either have all individually receivable items present in the multiworld, or the multiworld should hide away linked items as if they no longer exist and force worlds to treat group items like another item that belongs to their world.

Ideally, to get the number of receivable grubs for each player you would end up being able to do:
If all individually receivable items are present in the multiworld:

            grub_count_per_player = Counter(item.player for item in multiworld.get_items()
                                            if item.name == "Grub" and item.player in all_grub_players)

or if linked items are hidden away and not visible to the multiworld:

            grub_count_per_player = Counter(item.player for item in multiworld.get_items()
                                            if item.name == "Grub" and item.player in all_grub_players_and_group_ids)
            for group_id in all_grub_groups:
                group_count = grub_count_per_player[group_id]
                for player in multiworld.groups[group_id]["players"]:
                    grub_count_per_player[player] += group_count

[NewSoupVi]

I'm removing release-blocker as #4096 is merged, putting a bandaid on the issue for now.

I'm adding meta: help wanted because I want this PR to be considered "important" + I think there are a lot of design problems to work out with games that do things like in-game hints so I would like some of those games (calling myself out here) to see what happens with this.