Core: Add guard to start of fill to early exit if it is impossible to create a beatable game.

[Severencir]

What is this fixing or adding?

Two additions.

One is a guard to early exit if no beatable world can be made so you dont have to waste the time filling and getting to accessibility to be told it failed. This was added because during testing for other features, i realized that fairly frequently with options set to random, i'd see some seeds fail to generate. I investigated a bit and found that they are simply impossible because of logic that happened before fill, but they made it through fill. One such example is my fix in #6318. I decided a check of assuming all items that fill is responsible for are collected, can the game be beaten is quick and can occasionally save a half hour to hour of generation for large multiworlds.
The guard is just testing against a most permissive possible state so it can't reject anything that accessibility wouldn't also reject, and has consistently guarded known problematic seeds and permitted known good seeds compared to the previous version in all tests.

The other is a caching system. The idea being to prewarm a state with most items either collected or swept, and from that state, copy and sweep/collect the remaining items for a window. That way most of the work is done already most of the time for the placement attempts. During testing i found a window size of 4 batches to be optimal, but it was very close and only gradually increasing as i widened the window.
This method is byte equivalent to the existing output and is 1.75x to 2.25x faster according to my tests.

How was this tested?

Stock unit tests were all green.

Also tested 80 core games in groups of 5, 10, 20, 40 (in batches so all games were tested), 80, and 160 players with both random options and default across 20 seeds

[Mysteryem]

Looks like you've reimplemented the main part of #3872
"pre-warmed state" -> "partial exploration state"
"window size" -> "batch size"

[Severencir]

I think you're right from skimming over it. I can reduce this to just the pre-fill guard.

[duckboycool]

Probably makes sense to update the title as well.

[GinjaNinja32]

used this to try to figure out why a large gen kept failing

the pre-check consistently pointed to one particular Super Metroid slot as being un-goalable

testing that slot on its own though, it generates fine on both main and 0.6.7, but fails the pre-check on this PR's branch and 0.6.7+cherry-pick.

minimal yaml displaying same behaviour:

name: Player{number}
game: Super Metroid
requires:
  version: 0.6.7
Super Metroid:
  max_difficulty: easy

unsure if it's a this-PR problem or a Super Metroid problem, but given SM is core it's this repo either way.

[Severencir]

That's an interesting case. SM generation actually fails to generate a beatable seed under easy difficulty in every case ive tried, and it's a legitimate logical failure (degenerate case. It's actually quite beatable). Its because the difficulty requirement for ridley and mother brain (and any boss) is diff = 1 but the item pool can only ever give as low as a diff = 1.04 for mother brain and diff = 1.61 for ridley with everything collected on any seed

Stock generation solves this by just ignoring the conflict. In post fill it sets the diff = 1 for all bosses if any are unreachable, so it never fails on arbitrarily hard fights. Realistically this only triggers like this because no set of items can satisfy the requirements.

I could trivially add handling for sm that just sets diff to 1 for the check like the post fill does, but it's technically an issue with sm that the randomizer logic just explicitly and deliberately ignores difficulty for validation. and diff = 1.61 for ridley with everything collected on any seed

Stock generation solves this by just ignoring the conflict. In post fill it sets the diff = 1 for these bosses if they're unreachable, so it never fails on arbitrarily hard fights. Realistically this only triggers like this because no set of items can satisfy the requirements.

I could trivially add handling for sm that just sets diff to 1 for the check like the post fill does, but it's technically an issue with sm that the randomizer logic just explicitly and deliberately ignores difficulty for validation.

[duckboycool]

If it's not possible for this check to avoid false positives for situations like this, then I don't think it can really be added, at least as an error. As far as I'm aware, what SM does here is valid use of the world API, and I think it'd be a bad idea to make a breaking change for this. Even if it was made a warning instead, this might cause it to not be useful for the case of large gens which are more likely to have such a slot in them, which is where there'd really be a benefit to it.

Maybe it could work to run post_fill for this test first by putting everything in start inventory and then placing Nothing on all locations in the world? Not sure if there's potential for that to cause issues as well though.

[Severencir]

I was thinking of your post fill idea as well, i hadn't done anything yet because i wasn't sure that fixing it in post is a valid approach or just something that was overlooked. Since it is a valid approach, ill look more into how the post fill workflow works. Can you explain what's wrong with a non-stopping warning though?

I see the possibility of failing to generate post fill as a rather significant issue as it has come up in several asyncs i have been a part of, leading to multiple people running multiple concurrent generations and even dropping progression balancing to reduce the wait time just to get something to generate after hours of trying in the worst cases.

I feel like at the minimum having the precheck print a warning with the offending players would go a long way to helping avoid such an experience by allowing the organizer to at least know what might have been the source of any failures to generate an unbeatable game.

[Mysteryem]

If it's not possible for this check to avoid false positives for situations like this, then I don't think it can really be added, at least as an error. As far as I'm aware, what SM does here is valid use of the world API, and I think it'd be a bad idea to make a breaking change for this. Even if it was made a warning instead, this might cause it to not be useful for the case of large gens which are more likely to have such a slot in them, which is where there'd really be a benefit to it.

Maybe it could work to run post_fill for this test first by putting everything in start inventory and then placing Nothing on all locations in the world? Not sure if there's potential for that to cause issues as well though.

I'd argue that SM is leaving the multiworld in an invalid state (multiworld unbeatable with an 'all state'), so I would say this is a bug in SM. The test.general.test_reachability.TestBase.test_default_all_state_can_reach_everything test expects the goal to be reachable with an 'all state' after the pre_fill step, and I think that test is applicable here.

Particularly consider an SM player trying to play with accessibility: minimal, if their goal is never reachable for the duration of progression fill, then the player is not going to see the expected effects of choosing accessibility: minimal.

[Severencir]

that is a finding i have encountered, but to be fair, in the case that you are running minimal accessibility with easy difficulty, having every progressive that affects difficulty is genuinely required, so there isn't really much of a difference. i.e. what's logically required to pass the difficulty is a greater set than all progressive items, so nothing can be skipped

the reason this conflict exists is that varia uses all ammo collections in it's difficulty calculation, but the ap version of it only makes a few ammo upgrades progressive with the rest being filler. because of how fill restrictive works, it can't use filler in its logic because it's not actually placing them yet. so it undercounts the actual difficulty. realistically, the player will actually have the filler ammo available so the fight will be genuinely easy enough because exactly one check is behind the failing difficulty checks iirc, but fill is blind to that. i think the simplest solution is to move the difficulty override from post-fill to the reachability logic by just assuming that if all available progressives/advancements are collected, difficulty is 1 in the same way post fill does. that would solve any beatability issues without drastically changing how the logic currently works

that said. if there is a genuine reason to support post-fill fixes like sm does, i can look in to using post-fill calls or setting it up as a warning to accommodate that. im just holding off so far for discussion on the subject

[duckboycool]

I guess with no response striking down the idea of making a change to SM for this when I mentioned in in discord (even if no approval either), this can go back to just being on peer review. (Although it does currently also have conflicts still.)

And also it'd need @lordlou to make/approve such a change in that case.

[Severencir]

sorry for the delay. i've been busy. i tried seeing if calling post fill in the check was a good solution, but i decided it was unsafe because post fill can mutate the multiworld in ways that aren't trivially accounted for. what i decided to do, since we haven't seemed to make a decision on a change to sm and i am assuming that post fill corrections is deliberately supported for now, is add a special case for sm where we just set the maxDiff arbitrarily high for the guard state so this issue doesn't alone cause a failure. i also demoted it to a warn so that it never interferes with other games that might use a similar pattern, but i feel the notice that generation could fail and the players that are causing the concern is still valuable to have for async hosts from my experience.