Potsanity 3.0

[rrealmuto]

This is still a WIP and needs more testing but wanted to put it on people's radar.

Stuff the player will notice:

• Shuffle empty pots/crates and fairy pots This PR adds previously unshuffled pots and crates to the location pool. These are pots that are either empty or contain a fairy. Empty pots/crates have a "Nothing" item shuffled into the item pool, which, as the name suggests, does literally nothing. Fairy pots introduce a "stray fairy" item to the pool, which gives a full heal when collected.

• Enhanced pot major item effect When collecting a shuffled major item from freestanding/pot/crate/etc. checks, it used to turn invisible upon collecting it. Now, the item will display above links head until the message box is closed.

https://github.com/OoTRandomizer/OoT-Randomizer/assets/5086838/25ccdba4-f4cb-4e65-a79a-3b226070ebd0

The under the hood stuff: This PR completely reworks the custom actor flag system that was introduced with potsanity.

• Flag storage within an actor/Extending Actor struct: The old system worked by storing unique flags for each actor into sort-of unused home rotation variables within the actor instance. This new system adds a hack that effectively increases the base Actor structure in order to store this flag information. While only being used for a handful of actors currently, this hack in an enabler for pretty much any other shuffle setting imaginable.
• Flag info Flags were previously a unique 16 bit identifier which consisted of the scene setup, room, and actor index for each actor. This PR extends the flag to 32 bits in order to include the scene index, and more importantly a "sub-flag". This is used by certain actors which spawn multiple shuffled items (goron pot, rupee towers, etc.). Those items previously had to be handled as special cases within the patcher. Now they are treated the same as any other shuffled item utilizing the new flags.
• Flag tables/SRAM storage This PR also changes how these flags are stored in SRAM to be significantly more compact. Previously, a large amount of the SRAM used by these new flags were unused bits which represented actors within a room that weren't shuffled. This new system ensures that there are no wasted bits stored in SRAM (at the expense of storing some more data in tables within the code payload).

[fenhl]

The changes to override keys will require changes to the co-op context.

[r0bd0g]

Here's a funny logic thing I thought of just now that I should probably post in case I forget about it later.

I assume you have to collect the item in the pot before the fairy will spawn from it on the next load? This causes a problem with the logic of the fairy pot in the bk chest room of MQ Fire, which previously expected you could obtain the fairy without having physical access to the pot. So you'll actually have to check whether that pot is shuffled, and if so, hard require the hookshot to obtain the fairy. In addition to needing to check whether dungeon pots are shuffled, because of the way pots revert to vanilla behaviour when disabled, this means the logic will need to be able to check whether the shuffled pot location is disabled, which I'm not sure is currently possible. [I think the actual logic file fix is something like, "... and (hookshot or not dungeonpots or disabled)"]

[TreZc0]

Reminder for maintainers to heavily test encryption with this once it has been merged 🙂

Great job!

[rrealmuto]

The changes to override keys will require changes to the co-op context.

So I'm assuming that on the game's side, all that needs to happen is incrementing the COOP_VERSION and increasing the size of OUTGOING_KEY?

Something will need to happen on the multiworld client side too, right?

[fenhl]

So I'm assuming that on the game's side, all that needs to happen is incrementing the COOP_VERSION and increasing the size of OUTGOING_KEY?

I would also recommend moving OUTGOING_KEY to 0x0C1C instead of shifting everything else. Whatever changes are made will also need to be documented at Notes/coop-ctx.md.

Something will need to happen on the multiworld client side too, right?

Yes, each individual multiworld client will have to add support for the new co-op context version. In the past, the maintainers have required sending a corresponding PR to bizhawk-co-op to add minimal support for the new co-op context version, not sure if that's still the case.

[r0bd0g]

I have some reservations about which crates are shuffled in which times of day. I think also some changes are probably needed regarding under what conditions the shield pot is shuffled, and what item it adds to the item pool. None of that really relates to the logic though. I have a couple of ideas for tricks but they're a bit weird b/c of how the drops tend move a bit randomly, so I'll just jot them down and maybe handle them another time.

Logic Review:

1. The crates at the top of vanilla Fire are behind a bombable wall, so explosives are also required. (In truth, the hammer alone can be used, but you have to reload the room after hammering the fake door, since the door and the bombable wall are tied to the same flag. But you can see with the logic for the hammer chest, that we have opted not to include that in logic, and even if we did, it would have to be a trick.)

2. The logic for fairy pot in the BK Chest Room of MQ Fire relies on destroying the pot from afar using a charged magic spin, and then waiting for the fairy to fly over to you (which it always does). If the pot is shuffled, you'll have to collect the drop first, which means the Hookshot will be required to get over there and pick it up. So the logic for the fairy drop needs to check that either the relevant settings for shuffling the fairy pot are not enabled, or that you have the Hookshot. The way that pots work when they are disabled is different from how other checks work -- they revert fully to vanilla behaviour, rather than having the check guaranteed to be a useless item. So you can additionally check for the shuffled fairy pot location being disabled as another alternative to Hookshot. We might not currently have a means of checking whether a location is disabled inside the logic files. It's kind of up to you whether you want to actually implement this, or maybe just leave a comment explaining the situation.

3. The pots near the boss room of vanilla Water shouldn't check for Longshot, as Longshot is not required if you were to enter the dungeon from the boss door. The checks are already in a region that checks for Longshot when coming into it from the rest of the dungeon.

[rrealmuto]

Now that wonderitems is merged I've rebased this and will start incorporating the changes identified here

[fenhl]

There's a bug on this branch where if you break one of the pots in room 1 of Dodongo's Cavern (the baby dodongo corridor), the game freezes. Tested with this plando:

{
    "settings": {
        "logic_rules": "none",
        "spawn_positions": ["child"],
        "dungeon_shortcuts_choice": "all",
        "starting_items": {
            "Deku Shield": 1,
            "Bomb Bag": 1
        }
    },
    "entrances": {
        "Child Spawn -> KF Links House": "Death Mountain"
    }
}

[fenhl]

The freeze does not occur on Dev-Rob.

[rrealmuto]

So the last commit here isn't up on my branch, and likely what caused the crash. I'll test tonight and depending on what's going on, might just revert that last commit.

[fenhl]

Can confirm the freeze does not happen on commit 17ae3a5.

[fenhl]

Another bug on commit 17ae3a5: A shuffled item in the pots in Dodongos Cavern Lower Lizalfos Pot 1 can't be picked up. Attempting to save the game after attempting to pick up the item breaks things to a point where soft/hard reset no longer work, and BizHawk crashes on core reboot. This bug also doesn't occur on Dev-Rob.

[rrealmuto]

Next thing you know you'll pick up the item from the pot and get teleported to ganon 🤔

[rrealmuto]

That should hopefully fix it

[fenhl]

Looks good! Maybe we could add a CI check to make sure this array is large enough?

[rrealmuto]

Haha I have a check in the patcher, but it was checking the wrong size (which is currently hard-coded into patches.py, and I probably copied that block over from my branch). Perhaps that could be made to dynamically determine the size of the array from get_items.c or from asm_symbols.txt and included in CI

[rrealmuto]

Doing some more testing and found that the alt overrides are still a bit broken. The table itself is fine but some scenes aren't using it correctly. Looking into a fix.

Example. The child kak crates will drop their item at night but not during the day.

This should be fixed now.

[rrealmuto]

Looks good! Maybe we could add a CI check to make sure this array is large enough?

Done. I updated the format of data/generated/symbols.json to include the size of data symbols which seems to work well. I updated the runtime tests to compare against these sizes, and added a check in CI.py as well.

[fenhl]

Another bug report: If crate shuffle is disabled, all crates drop green rupees. Tested in the Kakariko backyard where 3 of the crates are supposed to drop nothing and the 4th is supposed to drop a red rupee:

{
    "settings": {
        "logic_rules": "none",
        "starting_age": "adult",
        "spawn_positions": ["adult"]
    },
    "entrances": {
        "Adult Spawn -> Temple of Time": "Kak Backyard"
    }
}

This bug also doesn't occur on Dev-Rob.

[rrealmuto]

Another bug report: If crate shuffle is disabled, all crates drop green rupees. Tested in the Kakariko backyard where 3 of the crates are supposed to drop nothing and the 4th is supposed to drop a red rupee:

{
    "settings": {
        "logic_rules": "none",
        "starting_age": "adult",
        "spawn_positions": ["adult"]
    },
    "entrances": {
        "Adult Spawn -> Temple of Time": "Kak Backyard"
    }
}

This bug also doesn't occur on Dev-Rob.

Should be fixed now.

[r0bd0g]

I didn't realize the shuffle empty pots setting would put the items into the dmc pots as child... Are the day crates in market still separate checks from the night crates?

[rrealmuto]

Yes

[rrealmuto]

I just merged with the (almost) latest main Dev. Probably just needs a bit of testing to make sure I didn't overlook anything during the merge. Main conflicts were just gi_draw IDs

[rrealmuto]

Nothing seems broken after the testing I've done + other folks that have been playing on my branch since I merged. I'm sure I'll eat those words right after it gets merged

[fenhl]

This is failing the test_build_room_xflags test after resolving merge conflicts.

[rrealmuto]

weird I thought I fixed that unit test the other day. It had been broken for a while

[rrealmuto]

hmm I seem to not have committed it 😅