TUNIC: ER Refactor for better plando connections, fewer shops improvement

[ScipioWright]

What is this fixing or adding?

Rewriting the dependent regions concept to work better with plando connections, as an issue with doing plando connections in a restrictive ways was discovered.

I'm modifying the existing PR to be this since it somewhat relies on some changes that were in this PR already, and so this PR shouldn't be merged without this anyway.

Also makes it so the Fewer Shops option creates 1 shop in a fixed location rather than 1 in a fixed location and a second one elsewhere.

Description of the refactor for entrance rando for reviewers/curious people:

Previously, there was a dependent regions structure. This was a hacky way to say "if you have access to RegionA, you can be assumed to have access to RegionB and RegionC". While this was quick, it was a pain to maintain, and wasn't easily modifiable mid-gen, which makes it a huge issue for plando connections.

Now, it is a big list of what are essentially regions and their entrances. I do plan to try reworking this into something I can use to generate the actual entrances, but I'm going to leave that out of the scope of this PR. It's not worth scope creeping this further. Not yet, anyway.

Another thing this change deals with is the Laurels Location - 10 Fairies option and how it interacts with entrance rando. The laurels are your biggest traversal item, and the 10 fairies check is in a scene that is not the Overworld. There is a well in the Overworld, so no special handling was needed for the other options.

So essentially, I needed a way to have it pair the portals together such that the 10 fairies check can be accessed without having laurels. And so, it checks the "fake" entrance rules on the traversal requirements in er_data to determine which paths it can take. Currently, it only looks if you have laurels (hyperdash), your logic difficulty, and if you have already gotten access to certain regions (you need to have access to the event region before you can access the portal that/those event(s) unlock).

And finally, there's the added bonus that this is pretty much exactly how the standalone randomizer does entrance rando, so now I can update the logic in standalone and write a little bit of code to reformat it into a python dict for AP and vice versa.

Description of the overall idea here regarding the fewer shops change for reviewers/curious people:

Currently, there is an option called Fewer Shops (or fixed shop internally) that modifies entrance placement in Entrance Rando (and does nothing outside of ER). This forces a Shop to an Overworld entrance that leads to a shop in vanilla (the Windmill), and instead of ER placing a total of 6 shops, it places 2 shops instead (including the one forced to the Windmill entrance).

The reason why it was 2 shops instead of 1 shop is to maintain an even number of entrances, as there would end up being one unpaired entrance otherwise.

I realized recently that we could use the Zig Skip Exit to add a single extra entrance to the pool, meaning we could go down to 1 shop and still have an even number of entrances in the pool. In vanilla, Zig Skip (or Kevin Skip) is a secret portal that lets you skip a long elevator sequence, which requires forcing the player off a ledge to get to (a player is not meant to find this normally, it was explicitly added for speedrunners). So, the Zig Skip Entrance isn't very viable to place in the pool. The Zig Skip Exit is viable since it doesn't require any tricks or anything like that -- it just drops you into the front of Rooted Ziggurat Lower.

So, this PR adds the Zig Skip Exit to the pool with Fewer Shops on.

If a player decides to get into the Zig Skip Entrance with the PR'd version of Fewer Shops on, it'll just straight up kill them. I'm eventually planning to either make it so the player gets forced back onto the ledge or put a secret there, but for now it's fine imo. They know they weren't supposed to do that.

How was this tested?

Test gens, made sure the client worked.

If this makes graphical changes, please attach screenshots.

N/A

[NewSoupVi]

Found something suspicious, would like an answer on it before I approve

[NewSoupVi]

Looks good, my ~concerns~ have been addressed, don't know what any of the code does though lol, probably should be looked over by someone who plays the game as well

[ScipioWright]

Found a very non-serious but still annoying bug with plando connections, going to fix that before undrafting

[ScipioWright]

I have refactored dependent_regions into traversal_requirements. This is to not have 3 very similar dictionaries anymore, make plando connections work much better when getting fairly restrictive, and allow the laurels at 10 fairies option to put secret gathering place at something other than its vanilla entrance.

[Ixrec]

don't know what any of the code does though lol, probably should be looked over by someone who plays the game as well

I do play Tunic rando, but entrance rando plus connection plando is a special kind of headache far removed from any .yaml I've ever used. At a high level this all makes sense to me, but I have not attempted to audit any of the logic, and there are several changes that seem right but I'm not confident about only because it would probably take me weeks to get confident with this code.

(come to think of it, act rando plus act plando was also by far the hardest part of AHiT's code to review)

[Ixrec]

Seems like any() and all() could make this loop more self-explanatory

[ScipioWright]

How do you mean?

[NewSoupVi]

any(
    all(
        req == "Hyperdash" and has_laurels or req in connected_regions
        for req in reqs
    )
    for reqs in reqs_list
)

[ScipioWright]

I think I'm gonna have to decline for now at least -- maybe later on when I'm a bit more comfortable with stuff like this

[NewSoupVi]

I'm not gonna force the any(all()) thing, but even with for loops, I'm not happy with this particular implementation

I would much prefer not explicitly checking for the 0 case because it already evaluates correctly without it, I don't think the speedup is worth it personally

Ontop of that, the "met count" thing is not great either imo.

What you're currently asking is "are as many parts of this valid as there are parts". The much simpler thing (imo) to ask is "is any part of this invalid"
Here's what I would do.

met_traversal_reqs = False
for reqs in req_lists:
    current_reqs_is_valid = True
    for req in reqs:
        # will need to change this if item plando gets evaluated earlier
        if req == "Hyperdash" and not has_laurels:
            current_reqs_is_valid = False
            break
        if req == "NMG" and not logic:
            current_reqs_is_valid = False
            break
        if req == "UR" and logic != 2:
            current_reqs_is_valid = False
            break
        if req not in connected_regions:
            current_reqs_is_valid = False
            break
    if current_reqs_is_valid:
        met_traversal_reqs = True
        break

or, if you're willing to go slightly technical, this would be the shortest for loop version that keeps the requirement checks individual

met_traversal_reqs = False
for reqs in req_lists:
    for req in reqs:
        # will need to change this if item plando gets evaluated earlier
        if req == "Hyperdash" and not has_laurels:
            break
        if req == "NMG" and not logic:
            break
        if req == "UR" and logic != 2:
            break
        if req not in connected_regions:
            break
    else:  # an "else" after a "for" evaluates only if for loop was not broken out of
        met_traversal_reqs = True
        break

[NewSoupVi]

I'm going to be honest, it felt a little strange clicking "unresolve" on this. I am 100% sympathetic to "I'm not comfortable using this suggestion because it's not intuitive to me", list comprehensions are a higher level concept to wrap your head around. But I do wish you would've let Ixrec say "that's fair, what you currently have is fine", or make a counter-suggestion

Unless Ixrec is the one who clicked "Resolve", or there was no further response after a long time, etc.
in which case, disregard this

[NewSoupVi]

I don't want to make a huge deal out of it though, it was one harmless (in the grand scheme of things) button click. It just gave me pause and I wanted to express my feelings on it.

[Ixrec]

FWIW, I did not click resolve, or do anything else to the status of this comment chain. Since there's no risk of harm beyond code readability, I considered my reviewer duty complete once it was clear Scipio understood the suggestion and had made the informed choice to not adopt it. My previous approval was meant to imply I was satisfied with the state of all review comment chains, without needing to individually reply/react/resolve each one.

[ScipioWright]

I only hit resolve after a couple days, not immediately. And I believe the original reviewer can un-resolve it. I see hitting resolve as "I have made a decision regarding this review comment, and now want to collapse it so that it doesn't look like it's an open item that I need to act upon". Which I wish I could do with the suggestion at the bottom here since I've already acted upon it, since it's making me anxious looking at it even though it's done

[NewSoupVi]

I like this a lot more, the "traversal_requirements" paradigm makes a lot more sense to me, it's what I use for Witness to figure out reachable regions as well.

I have two things I want to see changed.

I can't actually speak to whether this works. It seems very very specific. But, this PR will be in PR hell forever if we hold it to a high standard, so I'm gonna say "This only breaks things in ER plando" and say, that's ok with me

[NewSoupVi]

I'm not gonna force the any(all()) thing, but even with for loops, I'm not happy with this particular implementation

I would much prefer not explicitly checking for the 0 case because it already evaluates correctly without it, I don't think the speedup is worth it personally

Ontop of that, the "met count" thing is not great either imo.

What you're currently asking is "are as many parts of this valid as there are parts". The much simpler thing (imo) to ask is "is any part of this invalid"
Here's what I would do.

met_traversal_reqs = False
for reqs in req_lists:
    current_reqs_is_valid = True
    for req in reqs:
        # will need to change this if item plando gets evaluated earlier
        if req == "Hyperdash" and not has_laurels:
            current_reqs_is_valid = False
            break
        if req == "NMG" and not logic:
            current_reqs_is_valid = False
            break
        if req == "UR" and logic != 2:
            current_reqs_is_valid = False
            break
        if req not in connected_regions:
            current_reqs_is_valid = False
            break
    if current_reqs_is_valid:
        met_traversal_reqs = True
        break

or, if you're willing to go slightly technical, this would be the shortest for loop version that keeps the requirement checks individual

met_traversal_reqs = False
for reqs in req_lists:
    for req in reqs:
        # will need to change this if item plando gets evaluated earlier
        if req == "Hyperdash" and not has_laurels:
            break
        if req == "NMG" and not logic:
            break
        if req == "UR" and logic != 2:
            break
        if req not in connected_regions:
            break
    else:  # an "else" after a "for" evaluates only if for loop was not broken out of
        met_traversal_reqs = True
        break