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

Fill: allow for single player fill restrictive placement and sweeping #2415

Merged
merged 9 commits into from
May 4, 2024

Conversation

alwaysintreble
Copy link
Collaborator

@alwaysintreble alwaysintreble commented Nov 2, 2023

What is this fixing or adding?

title

How was this tested?

generated the same seed for 300 players under 4 separate conditions. existing emerald code with and without using the all_state cache, and then the same after merging in the single player optimization.
emerald cache single player opt.txt
emerald cache.txt
emerald no cache.txt
emerald no cache single player opt.txt

If this makes graphical changes, please attach screenshots.

@alwaysintreble alwaysintreble changed the title Fille: allow for single player state sweeping Fill: allow for single player state sweeping Nov 2, 2023
@ThePhar ThePhar 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 2, 2023
@alwaysintreble alwaysintreble marked this pull request as ready for review November 2, 2023 14:09
@espeon65536
Copy link
Collaborator

Tested on main's OoT, observed ~20% speedup on 10 worlds default settings and ~35% speedup on 20 worlds, presumably would get even better with more worlds but I don't have infinite processing power to test it.

BaseClasses.py Outdated Show resolved Hide resolved
@alwaysintreble alwaysintreble added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 14, 2024
@alwaysintreble alwaysintreble changed the title Fill: allow for single player state sweeping Fill: allow for single player fill restrictive placement and sweeping Feb 14, 2024
Copy link
Collaborator

@Zunawe Zunawe left a comment

Choose a reason for hiding this comment

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

Changes look about like what I did a while ago when the conversation that sparked this PR was had. I don't think my approval can speak for whether these changes forget behaviors that I don't know about, but I can say that it works very well for Emerald's purposes. Emerald is starting to hurt quite badly in pre_fill due to having to collect and sweep from every world in order to place a handful of its own items. Aside from creating a patch file, this is by far the slowest aspect of Emerald right now.

Shuffling both badges and HMs (16 items among 16 locations, split into two groups) on my current dev branch for Emerald takes

Without this PR:

  • ~1.1 seconds per pre_fill in a 10-player game
  • ~10 minutes per pre_fill in a 2000-player game

With this PR

  • ~0.3 seconds per pre_fill in a 10-player game
  • ~2 minutes per pre_fill in a 2000-player game with this PR merged

It's another good chunk of time knocked off if there's an api to create single-player all_states, though not nearly as much.

@alwaysintreble
Copy link
Collaborator Author

It's another good chunk of time knocked off if there's an api to create single-player all_states, though not nearly as much.

i have this #2427 but i'm not happy with it atm so would definitely like some feedback on it on how it can be improved/made more usable.

@ScipioWright ScipioWright added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Apr 26, 2024
@alwaysintreble alwaysintreble removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Apr 26, 2024
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Seems good & necessary

@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 May 1, 2024
@NewSoupVi
Copy link
Member

Tested a bunch of seeds with multiple games between main and this PR branch

  • made sure some of them entered swap
  • some singleplayer, some multiworld
  • Some involving ALTTP (the changed game here)

Everything was identical.

@NewSoupVi NewSoupVi merged commit 005fc4e into ArchipelagoMW:main May 4, 2024
16 checks passed
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
…ArchipelagoMW#2415)

* Core: allow for single player state sweeping

* Fill: have distribute items use single_player fill when it can

* oop

* pass locations to sweep_for_events instead of the player

* finally found the diff that was breaking swap

* LTTP fills everyone's dungeons at once, not just a single player's

---------

Co-authored-by: NewSoupVi <[email protected]>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
…ArchipelagoMW#2415)

* Core: allow for single player state sweeping

* Fill: have distribute items use single_player fill when it can

* oop

* pass locations to sweep_for_events instead of the player

* finally found the diff that was breaking swap

* LTTP fills everyone's dungeons at once, not just a single player's

---------

Co-authored-by: NewSoupVi <[email protected]>
Mysteryem added a commit to Mysteryem/Archipelago-ahit that referenced this pull request Sep 24, 2024
ArchipelagoMW#2415 removed the ability to provide items/locations for multiple
players simultaneously with single_player_placement=True. Now that this
is removed, the old behaviour should be considered deprecated so that
developers using the old behaviour don't run into unexpected problems.

With the requirement in place that all items in the provided item pool
have to belong to the same player, that singular player can be retrieved
once in advance rather than getting the player from the most recent item
set into the `item` variable. This reduces the used scope of the `item`
variable to just the loops it is part of and removes IDE warnings about
the variable possibly not being initialized.
Mysteryem added a commit to Mysteryem/Archipelago-ahit that referenced this pull request Sep 28, 2024
…layers

ArchipelagoMW#2415 removed the ability to provide items/locations for multiple
players simultaneously with single_player_placement=True. Now that this
is removed, the old behaviour should be considered deprecated so that
developers using the old behaviour don't run into unexpected problems.

With the requirement in place that all items in the provided item pool
have to belong to the same player, that singular player can be retrieved
once in advance rather than getting the player from the most recent item
set into the `item` variable. This reduces the used scope of the `item`
variable to just the loops it is part of and removes IDE warnings about
the variable possibly not being initialized.
Mysteryem added a commit to Mysteryem/Archipelago-ahit that referenced this pull request Sep 28, 2024
…layers

ArchipelagoMW#2415 removed the ability to provide items/locations for multiple
players simultaneously with single_player_placement=True. Now that this
is removed, the old behaviour should be considered deprecated so that
developers using the old behaviour don't run into unexpected problems.

With the requirement in place that all items in the provided item pool
have to belong to the same player, that singular player can be retrieved
once in advance rather than getting the player from the most recent item
set into the `item` variable. This reduces the used scope of the `item`
variable to just the loops it is part of and removes IDE warnings about
the variable possibly not being initialized.
AustinSumigray pushed a commit to AustinSumigray/Archipelago that referenced this pull request Jan 4, 2025
…ArchipelagoMW#2415)

* Core: allow for single player state sweeping

* Fill: have distribute items use single_player fill when it can

* oop

* pass locations to sweep_for_events instead of the player

* finally found the diff that was breaking swap

* LTTP fills everyone's dungeons at once, not just a single player's

---------

Co-authored-by: NewSoupVi <[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: 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