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

Potsanity 3.0 #2069

Draft
wants to merge 19 commits into
base: Dev
Choose a base branch
from
Draft

Potsanity 3.0 #2069

wants to merge 19 commits into from

Conversation

rrealmuto
Copy link

@rrealmuto rrealmuto commented Aug 12, 2023

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.

GLideN64_THE_LEGEND_OF_ZELDA_019

GLideN64_THE_LEGEND_OF_ZELDA_020

  • 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.
THE.LEGEND.OF.ZELDA.-.Project64.3.0.1.5664-2df3434.2023-08-11.23-41-54.mp4

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
Copy link
Collaborator

fenhl commented Aug 12, 2023

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

@fenhl fenhl added Type: Enhancement New feature or request Component: Logic Non-trivial changes to the JSON logic files Component: ASM/C Changes some internals of the ASM/C libraries labels Aug 12, 2023
@r0bd0g
Copy link

r0bd0g commented Aug 12, 2023

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
Copy link
Collaborator

TreZc0 commented Aug 12, 2023

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

Great job!

@rrealmuto
Copy link
Author

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
Copy link
Collaborator

fenhl commented Aug 22, 2023

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.

Copy link
Collaborator

@fenhl fenhl left a comment

Choose a reason for hiding this comment

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

The changes to the co-op context need to be documented in Notes/coop-ctx.md, and the changes to the save data in Notes/extra-save-data.md (might be a good opportunity to start documenting the contents of the extended save context).

break;
}
if (z64_file.scene_flags[key_scene].unk_00_ == override.key.all) {
// Prevent duplicate entries
if (extended_savectx.incoming_queue[i].key.all == override.key.all) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep the comment.

ASM/src/coop_state.asm Outdated Show resolved Hide resolved
@r0bd0g
Copy link

r0bd0g commented Nov 2, 2023

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.

ASM/c/item_table.c Outdated Show resolved Hide resolved
@@ -142,6 +140,8 @@ TRIFORCE_ICON_TEXTURE:

.align 0x10

.skip 0x200 ; Temporary address bump to avoid audio issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

A Minecraft screenshot showing a simple bridge crossing a narrow river. In front of the bridge, there are signs reading “Temporary Bridge (don't judge)”, “Temporary means Permanent (not judging)”, and “h*ck”

LocationList.py Outdated Show resolved Hide resolved
SettingsList.py Outdated Show resolved Hide resolved
SettingsList.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go in the Notes directory.

data/World/Bottom of the Well MQ.json Outdated Show resolved Hide resolved
"GF Crate 4": "can_break_crate",
"GF Crate 5": "can_break_crate",
"GF Crate 6": "can_break_crate",
"GF HBA Crate 1": "((is_adult and Gerudo_Membership_Card) or is_child) and can_break_crate",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the archery range should be a separate region?

Copy link

Choose a reason for hiding this comment

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

There are maybe a couple other cases along these lines but I figured, it wasn't broke. If these kinds of things are still there next time I go through it all, I'll clean up some things with extra regions at that time, whenever that happens to be.

@fenhl fenhl added the Changes Item Table Adds/removes items from the item table. Allows coordination of multiple PRs which add to the table. label Dec 5, 2023
@rrealmuto
Copy link
Author

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

rrealmuto and others added 3 commits December 7, 2023 23:50
Co-authored-by: Fenhl <fenhl@fenhl.net>
Co-authored-by: Fenhl <fenhl@fenhl.net>
@fenhl
Copy link
Collaborator

fenhl commented Mar 28, 2024

The freeze does not occur on Dev-Rob.

@rrealmuto
Copy link
Author

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
Copy link
Collaborator

fenhl commented Mar 28, 2024

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

@fenhl
Copy link
Collaborator

fenhl commented Mar 28, 2024

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
Copy link
Author

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

@rrealmuto
Copy link
Author

That should hopefully fix it

@fenhl
Copy link
Collaborator

fenhl commented Mar 28, 2024

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

@rrealmuto
Copy link
Author

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
Copy link
Author

rrealmuto commented Mar 29, 2024

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
Copy link
Author

rrealmuto commented Mar 31, 2024

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
Copy link
Collaborator

fenhl commented Mar 31, 2024

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
Copy link
Author

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.

default = False,
gui_tooltip = '''\
Enabling this will include empty crates into the location
pool based on the Shuffle Pots setting chosen.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pool based on the Shuffle Pots setting chosen.
pool based on the Shuffle Crates setting chosen.

@r0bd0g
Copy link

r0bd0g commented Apr 28, 2024

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
Copy link
Author

Yes

HintList.py Outdated
@@ -524,6 +524,8 @@ def tokens_required_by_settings(world: World) -> int:
'Ocarina C down Button': (["a low note"], "the Ocarina C down Button", 'item'),
'Ocarina C left Button': (["a somewhat high note"], "the Ocarina C left Button", 'item'),
'Ocarina C right Button': (["a middle note"], "the Ocarina C right Button", 'item'),
'Fairy Drop': (["an annoying companion"], "a Stray Fairy", 'item'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make sure the cryptic hints make sense between bottled and stray fairies. I think “Navi's cousin” sounds more like a stray fairy than a bottled one, so I suggest adding it here and removing it from the Bottle with Fairy hints.

Suggested change
'Fairy Drop': (["an annoying companion"], "a Stray Fairy", 'item'),
'Fairy Drop': (["an annoying companion", "Navi's cousin"], "a Stray Fairy", 'item'),

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember to also remove this hint from the bottled fairy item.

Co-authored-by: Fenhl <fenhl@fenhl.net>
@fenhl fenhl removed the Status: Needs Testing Probably should be tested label May 17, 2024
@rrealmuto
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Item Table Adds/removes items from the item table. Allows coordination of multiple PRs which add to the table. Component: ASM/C Changes some internals of the ASM/C libraries Component: Logic Non-trivial changes to the JSON logic files Status: Needs Review Someone should be looking at it Status: Under Consideration Developers are considering whether to accept or decline the feature described Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants