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: fix swap error found in CI #2397

Merged
merged 5 commits into from
Nov 11, 2023

Conversation

black-sliver
Copy link
Member

https://discord.com/channels/731205301247803413/731214280439103580/1167195750082560121

Swap now assumes the unplaced items can be placed before the to-be-swapped item.
Unsure if that is safe or unsafe.

@ScootyPuffJr1 ScootyPuffJr1 added is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. labels Oct 29, 2023
@black-sliver black-sliver added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. and removed is: refactor/cleanup Improvements to code/output readability or organizization. labels Oct 29, 2023
@Berserker66
Copy link
Member

I thought the remaining item pool is already in state, since in reverse fill they're yet to be placed into earlier spheres. IF the bug was just that it wasn't correctly in swap state, then this should be safe.

Copy link
Member

@Berserker66 Berserker66 left a comment

Choose a reason for hiding this comment

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

So approving that if the above theory is correct, this should be good. But this is one where people testing it would be nice.

@black-sliver
Copy link
Member Author

black-sliver commented Oct 29, 2023

The assumption that it can place all of item_pool before the to-be-swapped-out item may not be true. While this is fine for the reverse fill, because it'll swap later, I'm not sure that's always fine for swap, because swap is what should "fix" it.

Testing/comparing failures would be good and if we find issues with this, we can filter item_pool based on reachable locations with the old swap_state (i.e. sweep without item_pool). Something like any(location.can_fill(...) for ...) or a simple len() for it to be able to continue swapping maybe.

@black-sliver
Copy link
Member Author

Comparing my old set of yamls, I get

{
    "success": {
      "github-com_ArchipelagoMW-Archipe_main/venv": 4920,
      "github-com_black-sliver-Archipelago_fill-swap-fix-2/venv": 4921
    },
    "failed": {
      "github-com_ArchipelagoMW-Archipe_main/venv": 80,
      "github-com_black-sliver-Archipelago_fill-swap-fix-2/venv": 79
    }
}

Mixing the SM64EX yaml that fails every other seed with others, I see a real improvement:

{
    "success": {
      "github-com_ArchipelagoMW-Archipe_main/venv": 1236,
      "github-com_black-sliver-Archipelago_fill-swap-fix-2/venv": 1295
    },
    "failed": {
      "github-com_ArchipelagoMW-Archipe_main/venv": 164,
      "github-com_black-sliver-Archipelago_fill-swap-fix-2/venv": 105
    }
}

The failing yaml itself is not fixed by this though, only when combined with others.

@black-sliver black-sliver merged commit e670ca5 into ArchipelagoMW:main Nov 11, 2023
12 checks passed
@black-sliver black-sliver deleted the fill-swap-fix-2 branch November 11, 2023 09:55
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* Fill: add test for swap error with item rules

https://discord.com/channels/731205301247803413/731214280439103580/1167195750082560121

* Fill: fix swap error found in CI

Swap now assumes the unplaced items can be placed before the to-be-swapped item.
Unsure if that is safe or unsafe.

* Test: clarify docstring and comments in fill swap test

* Test: clarify comments in fill swap test more
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* Fill: add test for swap error with item rules

https://discord.com/channels/731205301247803413/731214280439103580/1167195750082560121

* Fill: fix swap error found in CI

Swap now assumes the unplaced items can be placed before the to-be-swapped item.
Unsure if that is safe or unsafe.

* Test: clarify docstring and comments in fill swap test

* Test: clarify comments in fill swap test more
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants