Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Actually take item from pool when plandoing from_pool #2420

Merged
merged 11 commits into from
Nov 29, 2024

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Nov 3, 2023

When plandoing from_pool, currently it creates a new copy of the desired item using the worlds create_item.

This means that in later stages, any reference the world kept to the item is lost. The only way to consistently find your items is by using... well, find_items, which is slow.

Two specific use cases I can think of:

  • Hints. Witness has recently switched over to keeping references to its items instead of using find_item_locations for generating in-game hints. But, because of this, it lost the ability to hint plandoed items, even if plandoed from_pool.
  • Knowing that an item was plandoed or otherwise removed from the itempool. With this PR, there is now a 1:1 relationship between "has been taken out of the itempool" and item.location is not None

This PR makes it so the item is actually taken from the itempool and then that copy of it is used

Tested:
All four combinations of "from_pool: true/false" and "force: true/false" on a Witness world, also making sure that if the same item is plandoed multiple times, it doesn't try to plando the same copy twice. Did not test it on any other worlds. I'm not sure

Sort of meh thing: Each item now uses a "next" operation and then a "pop" operation on the multiworld.itempool, for the same item. I couldn't work out a way to only perform one O(n) operation the itempool. It doesn't seem like there is a version of "list.pop" with a condition - If someone knows something like that, I would instead pop the item from the list, and then put it back if placing it wasn't successful.

My first venture into Core. Let's see how horribly I did

@NewSoupVi NewSoupVi added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Nov 3, 2023
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
Fill.py Outdated Show resolved Hide resolved
@PoryGone PoryGone added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Mar 6, 2024
@NewSoupVi NewSoupVi added waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Mar 12, 2024
Fill.py Outdated Show resolved Hide resolved
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM. Tested various configurations of plando with multiple yamls from different and the same worlds with all six combinations of from_pool and force and everything worked perfectly. One small comment above and you could potentially change the sorting key since None and 0 can be in any order, but None can't interfere with the 0 pop so it's fine as-is anyway. Specifically tested a case where the ordering was None, 0, None and 0 was to be removed and it worked just the same.

Co-authored-by: Exempt-Medic <[email protected]>
@NewSoupVi NewSoupVi added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 28, 2024
@Exempt-Medic Exempt-Medic added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Sep 26, 2024
@NewSoupVi NewSoupVi merged commit 1371c63 into ArchipelagoMW:main Nov 29, 2024
17 checks passed
AustinSumigray pushed a commit to AustinSumigray/Archipelago that referenced this pull request Jan 4, 2025
…agoMW#2420)

* Actually take item from pool when plandoing from_pool

* Remove the awkward index thing

* oops left a comment in

* there wasn't a line break here before

* Only remove if actually found, check against player number

* oops

* Go back to index based system so we can just remove at the end

* Comment

* Fix error on None

* Update Fill.py

Co-authored-by: Exempt-Medic <[email protected]>

---------

Co-authored-by: Exempt-Medic <[email protected]>
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. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants