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.

Sign up for GitHub

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

Jump to bottom

AHIT: Fix act shuffle test fail #3522

Merged
merged 193 commits into from
Jul 14, 2024
Merged

AHIT: Fix act shuffle test fail #3522

merged 193 commits into from
Jul 14, 2024

Conversation

CookieCat45
Copy link
Contributor

What is this fixing or adding?

Fixes an act shuffle test failure caused by Alpine Free Roam being placed on Time Rift - Balcony

How was this tested?

Running the act shuffle test a few times

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jun 12, 2024
@Exempt-Medic Exempt-Medic added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Jun 12, 2024
Jouramie
Jouramie approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@Jouramie Jouramie left a comment

Choose a reason for hiding this comment

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

LGTM

I'm just unsure why tests would fail with this placement. My understanding from the wiki is that Balcony is accessible from any act in Arctic Cruise, and requires Rock the Boat to be completed. So Rock the Boat must be accessible, and therefore Balcony and therefore Alpine Free Roam would be accessible anyway? I have not play the rando, so I must be missing something.

@Exempt-Medic
Copy link
Member

Exempt-Medic commented Jul 12, 2024 •
edited
Loading

Are the additional indirect conditions supposed to fix the bug here? If so, you can remove the change in Regions.py

Exempt-Medic
Exempt-Medic approved these changes Jul 12, 2024
View reviewed changes
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.

I don't love the pattern of excluding it because it was causing test failures but it works so *shrug*

@Exempt-Medic Exempt-Medic 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 12, 2024
black-sliver
black-sliver approved these changes Jul 14, 2024
View reviewed changes
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I don't understand any of it, but it follows the same pattern as others and fixes the issue, so I assume it's fine.

@black-sliver black-sliver merged commit e76d32e into ArchipelagoMW:main Jul 14, 2024
17 checks passed
@Exempt-Medic Exempt-Medic mentioned this pull request Aug 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Exempt-Medic Exempt-Medic Exempt-Medic approved these changes

@black-sliver black-sliver black-sliver approved these changes

@Jouramie Jouramie Jouramie approved these changes

Assignees
No one assigned
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@CookieCat45 @Exempt-Medic @Jouramie @black-sliver @Berserker66