Add "interact button" enums: C, BC, BCA

[Dragorn421]

Add three "interact button" enums:

• one for C buttons,
• one for B + C buttons,
• one for B + C buttons + A

and macros for converting between them (ft. bottle adventure)
see z64save.h

(to get confirmation of the order of c buttons left/down/right, look at pauseCtx->equipTargetCBtn, or D_80854388)

[Roman971]

I'm not opposed to this idea and I have considered it myself many times in the past, but I still don't know if it would be an improvement overall considering the amount of iteration/computation being done with these indices. That plus the A button only being in buttonStatus, and the 1-off mapping with C button indexes.

One thing I'll say is that if we were to have this enum, names like SAVE_BTN_B would probably be good enough imo

[Dragorn421]

Yeah so because there's no way to have a single "save button" enum, I'm now thinking there could be one enum per array basically

So maybe:

gSaveContext.equips.buttonItems[SAVE_EQUIPS_BUTTON_ITEM_(B / C_LEFT / C_DOWN / C_RIGHT)]
gSaveContext.equips.cButtonSlots[SAVE_EQUIPS_C_BUTTON_SLOT_(LEFT / DOWN / RIGHT)]
gSaveContext.buttonStatus[SAVE_BUTTON_STATUS_(B / C_LEFT / C_DOWN / C_RIGHT / A)]

?

[Dragorn421]

I was inspired

[mzxrules]

Can't say that it makes much sense to me to have InteractButtonBCA and InteractButtonBC exist as separate enumerations. The only thing I see that it does is it allows you define IBTN_BC_MAX / IBTN_BCA_MAX without coupling it to a specific value, but I don't feel it is essential to define things in this way simply to achieve this effect.

[Dragorn421]

well, where a IBTN_BC_ value is used, not all IBTN_BCA_ values can be used (i.e. not A)

All three enums overlap, IBTN_C_ is also contained in both others, it just happens the two others start the same

The purpose of this change is documentation and making the code easier to read, may as well not introduce confusion by conflating two enums that share values

The PR as it stands isn't my first and only attempt at documenting the button array indices, I've tried several things before settling on this solution, so I'm past the improve-by-tinkering phase and would need concrete suggestions to improve it

[mzxrules]

Still seems unnecessary and bloaty to split the one enumeration into two just to avoid using IBTN_A outside of gSaveContext.buttonStatus usages, especially given that the two enums are coupled anyway.

[Dragorn421]

IBTN_C_ is also coupled to the two others

Using IBTN_BCA_B instead of IBTN_BC_B would hint IBTN_BCA_A can be used there, when it would be an OOB access
IBTN_BCA_TO_BC(IBTN_BCA_B) was an option I considered to have a single enum and still document what of BC/C/BCA indices were expected, but it looked bad imo

And yeah it's definitely more bloaty to use names like IBTN_C_C_LEFT instead of 0, it's a whole 12 characters longer, but that's the point, being easier to understand at the cost of being less terse. It's a common tradeoff when documenting anything including one's own code

[mzxrules]

not talking about simple constant replacements, I'm talking about the duplicated code (in the doubled enum values) and the magic macro functions that make it a little tougher to follow the logic, and use/expand upon correctly. that sort of bloat.

[Dragorn421]

is the magic 0/1/2/... better?

[mzxrules]

genuinely don't understand why you would ask me that.

[Dragorn421]

I refered to

the magic macro functions that make it a little tougher to follow the logic

I assume "tougher" is "tougher than the magic 0/1/2/..."

[mzxrules]

still don't understand; why would I want no enumeration? I've never argued for such a thing.

[Dragorn421]

cool, that's nice to hear

I would need a recap of what we're arguing about then

[AngheloAlf]

I think he wants only one enum instead of two because those two enums overlap.
Personally I think merging those two enums would make stuff confusing and error prone (OoB accesses and such), so two enums sounds good to me

[mzxrules]

To me, the purpose of an enum is to define an ordering to some set of things, and not necessarily to index an array, and so splitting the larger set into a smaller set just to support indexing different sized arrays just doesn't feel clean. In this case it just happens to feel reasonable to do because there isn't a whole lot of code dependent on the enums, the full set doesn't have a NONE value to cause oobs access, and the duplication isn't adding tremendous amounts of code.

In any case, I await to see your split of SCENE_ into at least 4 enums.

[Dragorn421]

In any case, I await to see your split of SCENE_ into at least 4 enums.

which splits? I can only think of the dungeons one

and yeah enums overlap. that's just how OoT was coded. For example I think we have like 10 enums that are contained in the ItemID enum (including like player item action, exchange item, item slots, sword/tunic/boots (and their player counterparts), upgrades, quest items)

So at this point I have given up on having everything properly shifting on its own. Unfortunately, I think you just have to learn the codebase and know what to change if you wanted to edit this stuff, as imo favouring that in decomp would be too big of a hit to readability

splitting the larger set into a smaller set just to support indexing different sized arrays just doesn't feel clean

yeah it isn't, cf
... RBA https://github.com/Dragorn421/oot/blob/6fe01efdc88149271244f6242154b119da2fd54c/src/code/z_parameter.c#L2060
... and BA https://github.com/Dragorn421/oot/blob/6fe01efdc88149271244f6242154b119da2fd54c/src/code/z_parameter.c#L1231

where IBTN_BC_TO_C(...) evaluates to -1 with a bottle item on B :)

again it's just how oot is coded

[engineer124]

I think it's too many abbreviations compact in a small place that throws me off for this. I'd have no issues fully spelling out INTERACT_BTN. What about:

typedef enum {
    /* 0 */ INTERACT_BC_BTN_B,
    /* 1 */ INTERACT_BC_BTN_C_LEFT,
    /* 2 */ INTERACT_BC_BTN_C_DOWN,
    /* 3 */ INTERACT_BC_BTN_C_RIGHT,
    /* 4 */ INTERACT_BC_BTN_MAX
} InteractBCButton;

#define INTERACT_BC_BTN_C_FIRST INTERACT_BC_BTN_C_LEFT
#define INTERACT_BC_BTN_C_LAST INTERACT_BC_BTN_C_RIGHT

typedef enum {
    /* 0 */ INTERACT_C_BTN_C_LEFT,
    /* 1 */ INTERACT_C_BTN_C_DOWN,
    /* 2 */ INTERACT_C_BTN_C_RIGHT,
    /* 3 */ INTERACT_C_BTN_MAX
} InteractCButton;

#define INTERACT_C_BTN_TO_BC_BTN(btnsC) ((btnsC) + 1)
#define INTERACT_BC_BTN_TO_C_BTN(btnsBC) ((btnsBC) - 1)

typedef enum {
    /* 0 */ INTERACT_BCA_BTN_B,
    /* 1 */ INTERACT_BCA_BTN_C_LEFT,
    /* 2 */ INTERACT_BCA_BTN_C_DOWN,
    /* 3 */ INTERACT_BCA_BTN_C_RIGHT,
    /* 4 */ INTERACT_BCA_BTN_A,
    /* 5 */ INTERACT_BCA_BTN_MAX
} InteractBCAButton;

#define INTERACT_C_BTN_TO_BCA_BTN(btnsC) ((btnsC) + 1)
#define INTERACT_BC_BTN_TO_BCA_BTN(btnsBC) (btnsBC)

[Dragorn421]

I like this yeah, and I think it works with "interact"

the thing with "interact" is "there are other buttons you use to 'interact'", but this way e.g. INTERACT_C_BTN_ could read as "the interact C button(s)"

[Dragorn421]

I went ahead with this at least for now

[Dragorn421]

link to short discussion on discord about this PR
https://discord.com/channels/688807550715560050/688851317593997489/1154095781679210587
to
https://discord.com/channels/688807550715560050/688851317593997489/1154168746865344623