Swordless Link behavior is incorrect

[vaguerant]

This block of code is not doing what the CVar name implies it is and separately, the result of toggling that CVar is broken. https://github.com/HarbourMasters/Shipwright/blob/40f13ff2e6467e9c2e5d802d8b91e88cb9dbdadd/soh/src/overlays/gamestates/ovl_file_choose/z_file_choose.c#L1499-L1512

Sub-Issue 1

In most cases, the code inside the if statement never runs. if (CVar_GetS32("gSwordlessLink", 0) != 0) means that the original code inside the if statement only runs if gSwordlessLink is true, and there is no GUI toggle for that CVar so you'd only know about it by reading this particular C file. This obviously diverges from the GC debug ROM and the upstream decomp, where this code is always run.

This shouldn't be a) the default behavior, or b) named gSwordlessLink since its effect is the opposite of the CVar's name. In theory, the obvious fix is to turn that != into a == which will make the code run by default but not run when gSwordlessLink is set, but this runs into Sub-Issue 2. It may in fact be that that secondary issue is the reason this behavior was made default, since it causes a relatively minor bug in exchange for fixing a more severe one.

While we're at it, I'd argue that it'd be cleaner to just use one if statement this way instead of two nested:

    if ((CVar_GetS32("gSwordlessLink", 0) == 0) &&
        (gSaveContext.equips.buttonItems[0] != ITEM_SWORD_KOKIRI) && 
        (gSaveContext.equips.buttonItems[0] != ITEM_SWORD_MASTER) && 
        (gSaveContext.equips.buttonItems[0] != ITEM_SWORD_BGS) && 
        (gSaveContext.equips.buttonItems[0] != ITEM_SWORD_KNIFE)) { 

But that probably falls to preference.

Since there seems to be some sensitivity around this, I am not reporting this because I'm a "purist"--I literally found this while trying to add the Swordless Link bug. It's been stated by Kenix in the Discord that default behavior should match the upstream decomp by default and any changes to that should be user-selected. That's not even the view that I hold personally; regardless of my opinion, I am pointing this out because it doesn't match the project's goal.

Sub-Issue 2

If a user does know about that CVar and decides to set it manually (via the console or cvars.cfg), it has unexpected and incorrect behavior, mangling the user's equipment inventory when they load their save with the "wrong" item on B, which does not match the decomp/debug ROM. Using glitches, I got an Empty Bottle onto my B button, one of the scenarios which triggers the above code. Upon saving and reloading, that code ran and did this to my inventory:

I assume something is going wrong somewhere in the byte order swaps and bitshifts, but this goes over my head; I don't know my bitwise operations.

Reproducing

Here's a save file you can use to reproduce the above situation. I have an empty bottle on my B button.

oot_save.sav.bottleonb.zip

Save slots 1 and 2 have the same data, just in case you're running with debug mode enabled and want to load with or without the map select.

With gSwordlessLink set to 0 (current default behavior):

• Link loads in with the Empty Bottle on B (not accurate to debug ROM)
• the inventory is intact

With gSwordlessLink set to 1:

• Link loads in with the Deku Stick on B (sounds weird, but this is the expected behavior and matches decomp/debug ROM)
• the inventory is corrupted

EDIT: I'm now quite convinced that the place where Swordless Link is implemented above is not actually where the bug/s responsible for the original Swordless Link behavior actually occurs. Rather, this is where Swordless Link should be implemented:

https://github.com/HarbourMasters/Shipwright/blob/d1a2f98524428c6b476b4994429884fd046b2a25/soh/src/code/z_sram.c#L158-L163

I'm pretty sure this would be the correct approach:

    if (LINK_AGE_IN_YEARS == YEARS_ADULT && !CHECK_OWNED_EQUIP(EQUIP_SWORD, 1)) {
        gSaveContext.inventory.equipment |= gBitFlags[1] << gEquipShifts[EQUIP_SWORD];
        // SWORDLESS LINK IS BACK BABY
        if (CVar_GetS32("gSwordlessLink", 0) == 0) {
            gSaveContext.equips.buttonItems[0] = ITEM_SWORD_MASTER;
            gSaveContext.equips.equipment &= ~0xF;
            gSaveContext.equips.equipment |= 2;
        }
    }

This should match 1.0 behavior, where Link's sword does get added back to the inventory gSaveContext.inventory.equipment |= gBitFlags[1] << gEquipShifts[EQUIP_SWORD]; but the button and inventory equipment setups are not changed.

However, this doesn't help the fact that the current "Swordless Link" code is broken. Running that code ruins the inventory contents and I haven't been able to figure out why that is through trial and error. Part of what that code is supposed to do--and it works this way on all versions (1.0 through to GameCube), is if you have a non-sword item on B but have a sword equipped in the inventory, that sword gets removed from the inventory gSaveContext.inventory.equipment ^= (gBitFlags[swordEquipMask - 1] << BOMSWAP16(gEquipShifts[EQUIP_SWORD]));.

I don't think those BOMSWAP16s belong there at all--nowhere else that the inventory is manipulated are you flipping byte order, so maybe somebody else was trying to fix this before me, but this all comes from the pre-history of the repo. Removing the swaps makes it almost work, except that gBitFlags[swordEquipMask - 1] seems to be returning 3 (1 Kokiri + 2 Master Sword) when it should only be returning a single bit, since you can't have multiple swords equipped at the same time.

[garrettjoecox]

@Archez what is your take on the issues remaining, can we close this and maybe open up a separate issue on what is remaining?