Enemy Drop Shuffle + Enemy Soul Shuffle

[rrealmuto]

Enemy Drop Shuffle + Enemy Soul Shuffle. PR'ing because r0bd0g worked so hard on it and I wouldn't want him to feel disheartened.

Testing

This has existed on my branch for years now so pretty thoroughly tested. I haven't tested at all since rebasing though other than checking that I could generate a seed. If there is actually plans to merge this, I recommend someone fork this PR branch to make any necessary fixes.

Few known issues that should probably be ironed out:

• Text flowing outside of text boxes for certain soul messages.
• Minimap tracker doesn't seem to work on RetroArch using Parallel core.
• Some arguments about how all locations reachable works (or the fact that it doesn't)
• In text shuffle, souls are added to the base message list even if they are not shuffled. They shouldn't be in the message pool if souls aren't enabled
• May want to treat souls as important for the "Shuffled except important text" option
• Need to add enemy drops to advanced logic

[cjohnson57]

We should definitely get this in. Since r0b already reviewed the logic, maybe @GSKirox and/or @fenhl could do the asm and fenhl and/or I could do the python?

[r0bd0g]

There are a couple of little fixes that I realized shortly after my review, and also, new locations have been added since then, so I want to spend a day or two going over it again, and also see if I can think of anything else that I might have missed before.

[r0bd0g]

I also had a list of things I wanted to talk about. Let me go through my notes and see what all is in there...

There may be some remaining misspellings, "Dinalfos" instead of "Dinolfos" I think is the main one? But there might be some instances of "Like Like" not being spelled as two separate words, or "ReDead" without the capital D. There was also "Dead Hand" as one word or with the H not capitalized. And you'd want to check on the various item names to see if the capitalization looks correct. I'm not sure exactly how many issues along these lines remain.

And there are some long texts that hang out of the text boxes in some cases.

There are two Red Bubbles that are really difficult to kill. We'd have to figure out something for those. I think the least that we could do is warn players about them in the tooltip. Probably a better way would be to disable them by default, though people might see them in the disabled locations list and get confused.

Some of the boss rooms currently behave very poorly if you don't have the boss soul for the room. In Morpha's room, the room textures don't spawn. In Bongo's room, the drum doesn't spawn so you fall out the bottom of the map. I think the Barinade and maybe the Volvagia rooms have some minor issues where certain objects don't load. This is a pretty sharp edge with this feature, but unfortunately these issues sound like they'd be really hard to fix.

I think how you get drops from Deku Babas is in kind of a weird place. The withered ones give you the drop when you would spawn sticks. The more lively type can only be killed by being an adult and using a magical arrow. It's kind of janky to actually get the kill with the magical arrow, and kind of strange that that's the only way to do it -- simply spawning the nuts or sticks isn't enough. Some of these babas are in child-only areas, creating a bit of an ALR violation where some 'junk' item isn't going to be reachable because they're on those checks. There's one in vanilla botw that can never be reachable in any settings. Those babas will also force Deku to be an adult dungeon under dungeon ER.

I think the Spinning Tiles from the Fire Temple should require a soul to spawn. There would be logical relevance for this soul only in the lower loop in MQ.

Enemies that split into more enemies (Baris, Floormasters, and Peahats) can be a bit weird/inconsistent in how you obtain the drops. Baris drop when you split them. Floormasters require you to kill one of the specific mini ones. And Peahats I do not understand at all, I just sometimes get the drops at strange times.

In regional souls, I think that instead of a grotto soul it might be more consistent with how other settings work to have it depend on the soul of the grotto's parent region (but that might be difficult to implement). The enemies in the graves also currently depend on the graveyard soul, despite graves being considered as grottos in rando.

I don't agree that the flying pot soul should be required to trip enemy clear flags in rooms that include them, since they only actually need to be defeated unless you were to get close enough to spawn them. This isn't that big of a deal despite how strongly I disagree with requiring the flying pot soul for this.

It's a bit strange that you have to manually pick up the drops from the skull kids and only the skull kids. The rest spawn on you (though I think you can maybe dodge the drop as it spawns in some cases).

The enemy drops tooltip is so long that on my screen I couldn't even figure out how to read the whole thing.

It's not currently possible to select souls as a starting item in the starting items menu.

All of the enemy drops are given a vanilla drop of a blue rupee. It can be kind of a blue rupee overload. So something that could be good to have in the future, not super important, is some kind of implementation of enemy drop tables, so each enemy puts a random drop into the pool according to what it might normally drop.

EDIT: I thought of a few more things.

When working on this I had to add BGS as an advancement item. I also added Gnife and those fixed some issues with glitched logic at the time. Glitched logic has been added since then and eventually changed those? But I also decided to include Double Defence as an advancement item because I thought it was the last main item not included I guess. I think whether something is advancement or not affects whether it can be the one major item in the dungeon? So because this PR here randomly changes DD to an advancement item I guess you'll want to decide what to do about that. Also, the comments that I wrote when I made these changes for drops/souls might no longer be correct since glitched logic has changed so much since back when I wrote them.

This feature has a souls menu where you can turn the souls you've found on and off. Rob said he wasn't sure whether he wanted this feature to actually be a part of the final product or not. I guess you'll want to make a decision about that. Regardless, I've decided not to consider the disabling of souls using this menu as something you can do in logic.

Even though the enemy drop spawns right on top of you, it might be possible to dodge it? I feel like I've seen it happen once. So you could get into trouble with the MQ Forest Wolfos for example, at least with the way the logic is currently written, if it's actually possible to avoid their drops. I also kinda wonder how certain locations, such as the Forest Temple Bow Stalfos, will behave if you were to actually manage to dodge one of their drops.

I think there are some enemy souls that could be considered foolish in some settings combinations.

The Japanese property.json needs to update some location names from Redead to ReDead.

[JackTriton]

Edited Japanese's property.json for this PR
property.json

[djevangelia]

Pretty sure xflag_t size is 0x08, still fits in the data just the alignment

[djevangelia]

I've gone through parts of the asm/C now and played some enemy shuffle on this build (including with my changes). Got souls and killed enemies. Seems like it's mostly about just separating what is enemy shuffle only and non-shuffle code.

As it seems like the professional devs will check it out I haven't made a serious fork, just put my changes from the comments in one commit here: https://github.com/djevangelia/OoT-Randomizer/tree/rebase_enemy_drops_souls
I'll probably look at the guay assembly but otherwise I'm staying away unless asked to do something.

[djevangelia]

Is this only for enemy randomizing or should it run for everyone?

[djevangelia]

where is this done?

[djevangelia]

Suggested change

	} ActorAdditionalData;
	#include <assert.h>
	#define ACTOR_ADDITIONAL_DATA_SIZE 0x10
	// New data added to the end of every actor.
	// Make sure the size of this struct is equal to ACTOR_ADDITIONAL_DATA_SIZE above and to the amount of space added in Actor_Spawn_Malloc_Hack from actor.asm
	typedef struct {
	/* 0x00 */ uint16_t actor_id; // + padding 0x02
	/* 0x04 */ xflag_t flag;
	/* 0x0C */ uint8_t minimap_draw_flags; // + padding 0x03
	} ActorAdditionalData; // 0x10
	static_assert(sizeof(ActorAdditionalData) <= ACTOR_ADDITIONAL_DATA_SIZE, "Struct ActorAdditionalData size is larger than constant ACTOR_ADDITIONAL_DATA_SIZE");

[djevangelia]

Suggested change

	return (z64_actor_t*)1;
	return (z64_actor_t*)&z64_link;

maybe, or another actual pointer? line 21 as well

[djevangelia]

is this function only for enemy randomizing?

[djevangelia]

Suggested change

	;
	; v0 from return is preserved until beqz v0 check in original function

[djevangelia]

I rewrote the anti-Guay hack to only use normal ra. Tested in Lon Lon Ranch by slaughtering Guays with/without setting on, seems to work fine.

[djevangelia]

Actor_Kill = 0x80020EB4; needs to be put in ootSymbols.ld

; Hack Guays to not respawn when enemy drop shuffle is enabled
; A0 contains the actor
; actionFunc is at 0x01B0(A0) if we care.
en_crow_respawn_hack:
    addiu   sp,sp,-24
    sw      ra,16(sp)
    sw      a0,20(sp)    ; actor
    sw      v0,24(sp)    ; loaded reloc address

; Check if the setting is enabled.
    lb      a1,CFG_PREVENT_GUAY_RESPAWNS
    beqzl   a1,@en_crow_return_to_func
    move    v1,zero     ; return 0 if should continue SetupRespawn/no kill
; Setting is enabled so kill the guay.
    jal     Actor_Kill
    nop
    li      v1,1        ; return 1 if Guay killed = return early
    
    lw      ra,16(sp)
    lw      a0,20(sp)
    lw      v0,24(sp)
@en_crow_return_to_func:
    move    a2,a0       ; displaced
    lw      t6,(v0)     ; displaced
    li      at,10       ; displaced
    li      t7,1        ; displaced
    jr      ra
    addiu   sp,sp,24

(edit: removed storing a1 as it's not needed; maybe not a0 either but as the function has been hooked and replaced, keeping it)

[TreZc0]

+1, see below.

[djevangelia]

Needed to put this in its own hack file due to the branch (couldn't reset the headersize afterwards in hacks.asm)

.headersize(0x80b72bd0 - 0x00eee2f0)

; Hack Guays (en_crow) to not respawn in enemy drop shuffle
.org 0x80b73124     ; Beginning of EnCrow_SetupRespawn 0xEEE844
; replaces
;   or      a2, a0, r0
;   lw      t6, 0x0000(v0)
;   li      at, 10
;   li      t7, 1
    jal     en_crow_respawn_hack
    nop
    bnez    v1,0x80b731f8    ; if v1 = 1, guay was killed = jump to end of function, load ra
    nop

[TreZc0]

+1, see below.

[r0bd0g]

That test fail is I think b/c the Japanese text still has the locations that were named Redead instead of ReDead. So I think you just have to go through that Japanese property.json and change Redead to ReDead three times or whatever.

(I'm still reviewing the logic again. I've found a handful of issues yeah, some new, some I missed before in drops/souls review, and some older than that.)

Edit: Still working on it. A few more dungeons left to go over.

[rrealmuto]

rebased to resolve binary conflicts

[TreZc0]

I'll try my best to review this today and tomorrow, and get the test branch updated to make sure everything plays nicely on ootrdotcom.

[TreZc0]

Is now up and can be tested at https://ootrandomizer.com/generatorDev?version=devEnemyShuffle_ - i would appreciate people helping play test.

[rrealmuto]

This has been tested for many years on my branch 🤨. Didn't need a whole new test branch

[TreZc0]

It's not a new one 🙂 this existed for a long time, just moved it up to the latest rebase to make sure nothing is off with that one. Everything else is obviously field tested and good 😉

[r0bd0g]

I played though a 10hr race seed on wii vc on that branch. (I missed a check that had my 100th token but I figured it and actually finished by some miracle. So I did probably every check in the game.)
I didn't encounter any issues that I didn't know about. (Well, except for one, but that was a known bug unrelated to drops/souls.)

[TreZc0]

I think this is 99% ready for primetime, obviously play tested, and thus ready to be at the very least live in dev until the next release.

The only things I see are cleanup that should be done, as I reviewed. None of this is changing any functionality, just cleanup pre merge in preparation of having it in a release.

I also concur that the normal ra variant of @djevangelia 's Anti Guay hack is a bit cleaner, so I suggest we include that in the PR.

Other than that, long-term, we will have to take a look at the Enable/Disable functionality for souls and review it in regards to race impact. But that is SECONDARY - for now, this setting will only be used competitively in very specific settings where people will be aware of the impact and where rules can be made. Long-term, this should be handled by a race mode flag.

As for the other requested changes, mostly guardrails to check for missing symbols, I agree with rob that we do not really need them right now. Luckily, we are in a tightly integrated eco system for deployment and release, and here, it's fairly easy to keep this under control without the guardrails. Long-term, it might make sense to clean up the entire ASM section to be more type / null proof, but in this case, I say we stick with the code style at hand.

[TreZc0]

This does not account for the functionality that souls can be enabled / disabled through the menu.

[TreZc0]

with how the padding is set up, this should be 0x04 for flag and 0x0c for minimap_draw_flags

[TreZc0]

Most of this file can be removed, as it is no longer the case.
The only things i still see that are relevant are

• the coffin gibdo in Bottom of the Well, which could (after talks with rob) probably be fixed by modifying the coffin actor to call the modified version of actor_spawn assigning flags to an actor afterwards,
• the regular deku baba behavior around their specific drop mechanics.

[TreZc0]

Given that they are now included in the LocationList, this should probably be obsolete?

[TreZc0]

Long term, I do see an open question for competitive racing with this toggle. However, I do not think that this is any reason to delay the PR. It's probably just the kick we need to finally add a proper race mode toggle in the randomizer itself.

[TreZc0]

:) It's cute for sure, but just to not confuse casuals, I suggest we flesh this out.

[djevangelia]

Just want to clarify that my settings check suggestions are not intended to be guardrails, it is purely about human readibility for making the code easier to get into.
I note that my questions regarding if bb_after_init_hack is only relevant for shuffle and where Handle actor's that we've patched out using ID 0xFFFF is done haven't been marked as resolved. These stem from the exact same goal of readibility that resulted in settings checks - difference being I had to ask them as questions because I couldn't figure it out on my own and conclude whether the code is indeed exclusive to the enemy mode. And for those I did figure out, instead of just adding a // Enemy soul/drop shuffle comment I thought that - well, then they don't need to be run at all?

Settings checks or not, I do still think that adding comments in a highly convoluted or core function such as Actor_Spawn that a particular code block is only ever relevant to a specific setting is not a bad idea.
This is from my perspective of not being a year-long contributor of this project, but someone who enters from the outside to find an entangled system of three vastly differrent programming languages that realistically will never be refactored. It's what makes the project fun to me, but I can see why someone else might decide it's too much and just get into SoH instead.

[TreZc0]

I understand. Maybe we should try and make a genuine attempt to make the C a little more approachable. But honestly, I think one of the reasons that's not the case is the longstanding wish to migrate OoTR to sit on top of decomp as a base, which would make a lot of that work obsolete.
I left the two questions open cause I think it would be good for rob to answer them.

[cjohnson57]

Amazing job on this. No problems from me on the Python side other than the settings descriptions things TreZ and I noted.

[cjohnson57]

I think some explanation is needed here on what "regional" means

[flagrama]

• Choose to shuffle Boss souls when generating the seed
• Create a new in-game save file
• Reset without saving (file select menu or in game, doesn't matter)

The souls you are supposed to start with and that do not have soul items in the pool will be lost. Therefore the enemies will not spawn and there is not an item available to make them start spawning. You cannot turn them on in the soul menu because they are greyed out instead of red (turned off).

[rrealmuto]

The issue with the souls getting cleared is likely because of this change

[djevangelia]

Problem is that the extended savecontext isn't included in the checksum on file creation. The extended savecontext does get written to the save when creating a file, but unless game is saved ingame the save gets reverted when verifying saves.

I replaced everything below // Copy extended savectx to the buffer with:

uint8_t* extended_slot = &extended_savectx;
extended_initial_save_entry* entry = &EXTENDED_INITIAL_SAVE_DATA;
while (entry->all != 0) {
    extended_slot[entry->offset] = entry->value;
    entry++;
}

z64_memcopy(dramAddr + slot_offset + SRAM_ORIGINAL_SLOT_SIZE, &extended_savectx, sizeof(extended_savectx));
Sram_WriteSave(&z64_game.sramCtx, NULL);

essentially using the ingame write save for writing the initial save to get the checksum correct. It seems to work. My prior knowledge of saving is zero though so someone else should look at it too.