Skip to content

Core: Don't return placed items twice in multiworld.get_items() after fill - #6336

Open
Mysteryem wants to merge 1 commit into
ArchipelagoMW:mainfrom
Mysteryem:patch-21
Open

Core: Don't return placed items twice in multiworld.get_items() after fill#6336
Mysteryem wants to merge 1 commit into
ArchipelagoMW:mainfrom
Mysteryem:patch-21

Conversation

@Mysteryem

Copy link
Copy Markdown
Contributor

What is this fixing or adding?

Similar to using multiworld.get_all_state() after core AP's main fill is complete (see #6335), using multiworld.get_items() after core AP's main fill is complete is another common 'gotcha' that apworld developers fall for, not realising that it will end up returning a list containing most items twice.

This changes multiworld.get_items() to only return items from self.itempool that are not placed, therefore avoiding the issue of returning an item twice if it is both placed and in self.itempool, which often occurs after core AP's main fill is complete.

There are still potentially some issues from the fact that not necessarily all items from self.itempool will be placed somewhere, e.g. additional items could have been added into the multiworld through item plando. But, I don't know how get_items() is actually supposed to work due to it being undocumented. Additionally, changing get_items() to be aware of what it should return depending on which generation steps have been run would be a more involved change to parts of core AP.

How was this tested?

Only by running the unit tests.

Similar to using `multiworld.get_all_state()` after core AP's main fill is complete (see ArchipelagoMW#6335), using `multiworld.get_items()` after core AP's main fill is complete is another common 'gotcha' that apworld developers fall for, not realising that it will end up returning a list containing most items twice.

This changes `multiworld.get_items()` to only return items from `self.itempool` that are not placed, therefore avoiding the issue of returning an item twice if it is both placed and in `self.itempool`, which often occurs after core AP's main fill is complete.

There are still potentially some issues from the fact that not necessarily all items from `self.itempool` will be placed somewhere, e.g. additional items could have been added into the multiworld through item plando. But, I don't know how `get_items()` is actually supposed to work due to it being undocumented. Additionally, changing `get_items()` to be aware of what it should return depending on which generation steps have been run would be a more involved change to parts of core AP.
@github-actions github-actions Bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 18, 2026
@Mysteryem

Copy link
Copy Markdown
Contributor Author

An alternative to filtering the items from multiworld.itempool would be clearing multiworld.itempool once the item pool has been split into the separate progression, useful and filler pools in Fill.distribute_items_restrictive, but there are a few potential issues from doing this:

  1. multiworld.itempool is useful for developers to debug the item pool after fill is complete, e.g. an issue occurs after fill and a developer sets a breakpoint there, but wants to inspect the item pool from that breakpoint.
  2. If multiworld.itempool would be cleared after use, there would not currently be any way to inspect the multiworld to see what items could not be placed anywhere.
  3. Clearing multiworld.itempool too early could break apworlds currently using multiworld.get_all_state() late into generation. e.g. Pokemon RB in the fill_hook generation step (slightly invalid use) and TUNIC/Messenger in extend_hint_information (invalid use).
  4. Maybe there are worlds that have valid use for iterating multiworld.itempool after fill is complete?

@Jouramie Jouramie added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Jul 24, 2026
@Jouramie

Copy link
Copy Markdown
Collaborator

Did not test as the only two usages outside tests seem trivial. Code LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

affects: core Issues/PRs that touch core and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants