GER: Prefer expanding reachable regions in stage 1

[bbernardoni]

What is this fixing or adding?

Changes the behavior of GER to prefer expanding reachable regions in stage 1 (when placing non-dead-end entrances to reach new regions). As long as there is a valid placement and there are less reachable randomized exits than to be placed non-dead-end entrances, it will try to run test_speculative_connection to make sure the reachable randomized exit count doesn't decrease. However, it is possible to run out of valid placements and need to place entrances that decrease reachable exits in order to satisfy future entrance access rules.

These changes were motivated by trying to solve an issue with my world where GER was often painting itself into a corner. Some of these issues I was able to solve with complicated can_connect_to function but that required playing a lot of whack-a-mole and the resulting code is not very maintainable. With these generic GER changes, I was able to greatly simplify my can_connect_to function while improving my ER failure rate. Here are some cases that these changes handle generically that I previously had to specifically account for in my can_connect_to function.

1. Regions where only one entrance is randomized and the other is a non randomized entrance that is impassible until it's been unlocked from the other end
2. Making sure expanding dead end regions are not placed if they would cap off that area randomization group
3. Regions where only one entrance is randomized and the other is a non randomized entrance that requires another region to be reachable (I was only allowing these to be placed if their requirements where met to make the entrance passable)

Additionally, this change uncovered a bug where GER wasn't updating reachable regions correctly in test_speculative_connection and do_placement. It was calling update_reachable_regions followed by sweep_for_advancements. The latter has the chance to collect items and stale the collection state, requiring more rounds of update_reachable_regions followed by sweep_for_advancements until nothing is collected.

How was this tested?

Fuzzing for GER failures and benchmark times.

I tested four worlds that implement GER: Toem (my world), Stardew Valley (core), The Messenger (core), and Hollow Knight (beta). The latter three worlds already had a 0% GER failure rate which was maintained by these changes. On the worst case settings with no retries, Toem has a 11% GER failure rate with the complicated can_connect_to before these changes, a 57% GER failure rate with the simple can_connect_to before these changes, and a 0% GER failure rate with the simple can_connect_to after these changes. The failure rate before these changes can be improved by retrying GER, but obviously that comes at the cost of taking more time.

However, these changes are slower per GER run (around 3-4 times worse in some cases). This is due to more aggressively running test_speculative_connection which is reasonably expensive. Here is a table of benchmark times for ER for all the worlds. Note that Toem v1 is with the complicated can_connect_to and v2 is the simple can_connect_to. These times include retries for Toem as well.

World	Avg. entrance count	Avg. time before	Avg. time after
Toem v1	162	0.0273 s	N/A
Toem v2	162	0.0548 s	0.0695 s
Stardew Valley	80	0.0573 s	0.0558 s
The Messenger	37	0.0061 s	0.0285 s
Hollow Knight	868	2.2350 s	8.0563 s

[BadMagic100]

Some of these issues I was able to solve with complicated can_connect_to function but that required playing a lot of whack-a-mole and the resulting code is not very maintainable. With these generic GER changes, I was able to greatly simplify my can_connect_to function while improving my ER failure rate

I would also add for reviewers' sake, this is not a unique problem to this world, several worlds in the past have exhibited similar symptoms (I don't remember which they are but I have recommended people to implement retries many times, something I would also like in GER in a future change)

[BadMagic100]

It would probably be good to add one or more test covering this case to prevent a regression in the various places it was observed

[BadMagic100]

I would consider updating this error message for additional clarity to distinguish between when we are chasing entrances vs just regions

[bbernardoni]

The error message never triggers when in the more exits state

[BadMagic100]

I had mentioned this in discord but due to the performance cost, some worlds may wish to opt out of this new behavior. I think it is a sensible default though so I would add it as an optional input, default true. Given that the arg list to randomize_entrances is already quite long, it might be a good idea to introduce this as a kw-only arg so that it (and future optional args) can be safely rearranged for organizational purposes in the future