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

GER: Only consider usable exits when calculating dead-ends #4701

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

Conversation

Ars-Ignis
Copy link

What is this fixing or adding?

Previously, when making a call to randomize_entrances and passing a specific list of entrances and exits to actually randomize, it would consider all possible exits from a region when determining whether or not an entrance should be considered a dead-end.
However, that meant it would consider exits it had not been told to shuffle, which resulted in these entrances getting placed early and causing the algorithm to fail. This change now correctly ignores exits it is not going to shuffle, and as such, correctly determines that entrances that only connect to exits that won't be shuffled should actually be considered dead-ends.

How was this tested?

Tested with my in-progress GER implementation for my Crystalis .apworld, which contains the following example:
image
(Visualized using the improvements in #4685.)
In this example, my first call to GER shuffles the red entrances leading into and out of Windmill Exterior, but not the black entrances. This region therefore should be considered a dead-end, which I confirmed with a breakpoint in _can_expand_graph.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Mar 3, 2025
@BadMagic100
Copy link
Collaborator

Would like to have a test added for this since we have an easy to represent test case (namely, the one diagrammed here) and entrance classification is not dependent on any random processes.

@Ars-Ignis
Copy link
Author

Would like to have a test added for this since we have an easy to represent test case (namely, the one diagrammed here) and entrance classification is not dependent on any random processes.

Seems easy enough. That last commit was just to fix the broken tests that were using EntranceLookup directly, but I can definitely add a new unit test as well.

Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

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

Change and test looks good.

for dead_end in dead_end_region.entrances:
if dead_end.name == "region20_top":
break
# there should be only this one dead-end
Copy link
Collaborator

Choose a reason for hiding this comment

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

It wasn't immediately clear to me why this should be the case (because this region has only a top and right exit and the right has been pulled out of the set) so perhaps additional exposition may be in order

@ScipioWright ScipioWright added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Mar 3, 2025
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

Discussed in the thread, looks reasonable to me, code review came up fine.

@ScipioWright ScipioWright 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 Mar 3, 2025
based on review feedback
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. 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.

4 participants