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: CollectionState - Never pick up advancements #3786

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Aug 13, 2024

We can kinda "just do this" now, if we accept that we have to change some worlds

  • Makes both CollectionState.collect and CollectionState.remove exit immediately if the item isn't an advancement
  • Make World.collect and World.remove assert that you're passing an advancement
  • Remove as many core instances of bools called "changed" as possible

@NewSoupVi NewSoupVi requested a review from Berserker66 as a code owner August 13, 2024 21:10
@NewSoupVi NewSoupVi marked this pull request as draft August 13, 2024 21:10
@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Aug 13, 2024
@NewSoupVi
Copy link
Member Author

Sorry for the auto-request 🤷

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 13, 2024
@Exempt-Medic Exempt-Medic added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Aug 13, 2024
worlds/AutoWorld.py Outdated Show resolved Hide resolved
worlds/AutoWorld.py Show resolved Hide resolved
state.prog_items[self.player][name] -= 1
if state.prog_items[self.player][name] < 1:
del (state.prog_items[self.player][name])
return True # Backwards compatibility, this used to denote whether a progression item was picked up
Copy link
Member Author

@NewSoupVi NewSoupVi Aug 13, 2024

Choose a reason for hiding this comment

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

The only part I'm not happy with - All the overridden world collects are relying on the World collect returning "True" on an advancement, so I can't get rid of return True in favor of return, despite the fact Python is happy with an overridden function returning something different

If someone has some idea of how to get around this, let me know

worlds/AutoWorld.py Outdated Show resolved Hide resolved
@NewSoupVi NewSoupVi marked this pull request as ready for review August 13, 2024 21:46
@NewSoupVi NewSoupVi requested a review from SunnyBat August 13, 2024 21:48
@NewSoupVi
Copy link
Member Author

@lordlou

@NewSoupVi NewSoupVi added is: refactor/cleanup Improvements to code/output readability or organizization. and removed is: enhancement Issues requesting new features or pull requests implementing new features. labels Aug 13, 2024
qwint added a commit to qwint/Archipelago that referenced this pull request Sep 5, 2024
NewSoupVi pushed a commit that referenced this pull request Sep 5, 2024
* speed up collect, will be obsolete after #3786

* vi's a meanie
@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Nov 25, 2024
@NewSoupVi NewSoupVi force-pushed the never_collect_non_advancements branch from 398fffb to 71ccc9d Compare November 25, 2024 18:48
@NewSoupVi NewSoupVi removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Nov 25, 2024
Comment on lines +519 to +520
if state.prog_items[self.player][name] < 1:
del (state.prog_items[self.player][name])
Copy link
Member

Choose a reason for hiding this comment

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

3.10 should mean this is fine now?

Suggested change
if state.prog_items[self.player][name] < 1:
del (state.prog_items[self.player][name])

@NewSoupVi NewSoupVi added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Nov 29, 2024
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: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants