OoT-Randomizer icon indicating copy to clipboard operation
OoT-Randomizer copied to clipboard

Introduce a `GetItemId` enum

Open AngheloAlf opened this issue 1 year ago • 2 comments

As proposed on discord I made a GetItemId enum, which represents the entries from the item_table table This PR includes an enums for both C and Python code

I tried to replace as many instances of GI raw values and replace them with the enum values. If I missed any please tell me!

There's no "compiled" files diff (bundle.o, asm_symbols.txt, etc) because the enum doesn't impact the codegen in any way.

There are a few places where the code had comments indicating to which GI the raw value corresponds to. This is no longer necessary with the enums, but I left them in case you guys still want them, if not I can remove them.

        case 0: return GI_GORONS_BRACELET; // Goron Bracelet
        case 1: return GI_SILVER_GAUNTLETS; // Silver Gauntlets
        default: return GI_GOLD_GAUNTLETS; // Gold Gauntlets

Advantages of using an enum

  • Improves readability and searchability.
  • No need for comments indicating to which item a magic number corresponds to
    • i.e return GI_GORONS_BRACELET; doesn't need a comment like return 0x54; // Goron Bracelet
  • Using an enum for the GI items helps a lot for cases when you have a branch/PR that adds new GIs and another PR that adds GIs gets merged to upstream, values for the table would automatically not overlap between them, making merging upstream less error-prone.

Disadvantages of using an enum

  • The item_table ends up with way too much spacing. I guess I could try reducing the spacing by category or something if preferred.

Future work

  • Enums themselves can be used as types, improving readability even more.

For example (click on details to expand):

The following code:
    // example 1
    uint16_t resolved_item_id = resolve_upgrades(override);
    switch (resolved_item_id) {
        case GI_OCARINA_FAIRY: { // Fairy Ocarina
            z64_file.items[Z64_SLOT_OCARINA] = 0x07;
            break;
        }
        case GI_OCARINA_OF_TIME: { // Ocarina of Time
            z64_file.items[Z64_SLOT_OCARINA] = 0x08;
            break;
        }
    }

    // example 2
uint16_t strength_upgrade(z64_file_t *save, override_t override) {
    switch ((override.value.base.player == PLAYER_ID || !MW_PROGRESSIVE_ITEMS_ENABLE) ? save->strength_upgrade : MW_PROGRESSIVE_ITEMS_STATE[override.value.base.player].strength) {
        case 0: return GI_GORONS_BRACELET; // Goron Bracelet
        case 1: return GI_SILVER_GAUNTLETS; // Silver Gauntlets
        default: return GI_GOLD_GAUNTLETS; // Gold Gauntlets
    }
}

    // example 3
item_row_t *get_item_row(uint16_t item_id) {
    if (item_id >= GI_RANDO_MAX) {
        return NULL;
    }
    item_row_t *item_row = &(item_table[item_id]);
    if (item_row->base_item_id == 0) {
        return NULL;
    }
    return item_row;
}

would becomes this:

    // example 1
    GetItemID resolved_item_id = resolve_upgrades(override);
    switch (resolved_item_id) {
        case GI_OCARINA_FAIRY: { // Fairy Ocarina
            z64_file.items[Z64_SLOT_OCARINA] = 0x07;
            break;
        }
        case GI_OCARINA_OF_TIME: { // Ocarina of Time
            z64_file.items[Z64_SLOT_OCARINA] = 0x08;
            break;
        }
    }

    // example 2
GetItemID strength_upgrade(z64_file_t *save, override_t override) {
    switch ((override.value.base.player == PLAYER_ID || !MW_PROGRESSIVE_ITEMS_ENABLE) ? save->strength_upgrade : MW_PROGRESSIVE_ITEMS_STATE[override.value.base.player].strength) {
        case 0: return GI_GORONS_BRACELET; // Goron Bracelet
        case 1: return GI_SILVER_GAUNTLETS; // Silver Gauntlets
        default: return GI_GOLD_GAUNTLETS; // Gold Gauntlets
    }
}

    // example 3
item_row_t *get_item_row(GetItemID item_id) {
    if (item_id >= GI_RANDO_MAX) {
        return NULL;
    }
    item_row_t *item_row = &(item_table[item_id]);
    if (item_row->base_item_id == 0) {
        return NULL;
    }
    return item_row;
}

AngheloAlf avatar Oct 16 '23 13:10 AngheloAlf

Looks good to me from a code perspective, I'd like to see someone play through a seed on this branch though, just to make sure everything's still working correctly

cjohnson57 avatar Nov 13 '23 23:11 cjohnson57

I've played through most of a seed and everything seemed fine. So I'd say this should be good to merge after the upcoming release, a rebase or merge of latest Dev probably couldn't hurt though.

fenhl avatar Aug 31 '24 04:08 fenhl