Generic actor params getters

[Thar0]

Creates generic params getter macros that can be built upon to create specific macros for extracting values stored in the params field.
There are unfortunately quite a number of distinct patterns that must be met, so there are 6 different getters:

• Mask then shift
• Shift then mask
• No mask, only shift
• No shift, only mask
• Shift then mask to a bit position other than 0
• Mask then shift to a bit position other than 0

The last two are very rare, are they worth having?

There is also one case that I could not turn into a macro, it was a mask by 0x33 (0b110011). Since this bit range is not continuous and all of the getter macros take a number of bits rather than the mask itself, this mask could not be constructed.

[Dragorn421]

Bump on the comment

Do we still want to use those new macros for single bit checks (flags)?

which was in reaction to

-    invisble = this->actor.params & SPAWN_INVISIBLE;
+    invisble = PARAMS_GET_NOSHIFT(this->actor.params, 15, 1); // SPAWN_INVISIBLE

Personally I'm waiting on solving that to continue reviewing as this comes up often-ish

(also many conflicts in here... I hope it's manageable 😨)

[hensldm]

Is it still (or I guess was it even) the plan for actor's to make their own macros, with descriptive names, that just build off these ones?

[fig02]

Not that this dictates what we do, but Ive been learning about tww/tp lately, and they ended up taking the generic approach in those games:

inline u32 fopAcM_GetParamBit(void* ac, u8 shift, u8 bit) {
    return (fopAcM_GetParam(ac) >> shift) & ((1 << bit) - 1);
}

(also, they actually called them param(s) in those games which I did not know. pretty neat)

[Thar0]

The intention I had going into this is that these generic params macros introduced in this PR would be used to build specific macros for individual actors as they are documented.

There are already some examples of this in this PR, from actors that had already received some params documentation prior. For example z_obj_switch defines:

#define OBJSWITCH_TYPE(thisx) PARAMS_GET((thisx)->params, 0, 3)
#define OBJSWITCH_SUBTYPE(thisx) PARAMS_GET((thisx)->params, 4, 3)
#define OBJSWITCH_SWITCH_FLAG(thisx) PARAMS_GET((thisx)->params, 8, 6)
#define OBJSWITCH_FROZEN(thisx) PARAMS_GET((thisx)->params, 7, 1)

And then uses the specific macros.

[fig02]

(on the topic of that example, Id like to standardize having _GET_ in the name of the specific macro, which would match the generic name as well)

overall I am okay with that approach though

[fig02]

I feel like its about time to get this PR in. It will help greatly with actor documentation.
Ive gone ahead and made all requested changes, with the permission of tharo. My changes summarized below:

• All existing specific params macros are now of the form <actor>_GET_<param-name>.
• PARAMS_GET -> PARAMS_GET_U
• PARAMS_GET2 -> PARAMS_GET_S
• some uneeded comments were removed

This change was not requested above in review, but I made it on my own (in communication with tharo):

• The macro formerly named PARAMS_GET_S has been removed. It was a special case used in one actor
• PARAMS_GET2_S has been removed. It was a special case used in one actor

I dont feel the generalized macro system needs to support this one-off use cases. It keeps the generalized system more clear to only need to worry about PARAMS_GET_S and PARAMS_GET_U

I tried to split my commits to make it easier to review.

[krm01]

LGTM, I was kinda against this until I realized the authors have already done this for every actor, and it wasn't just a prescription for future docs to have to use. since its already been implemented though, LGTM!

[fig02]

This approval is slightly biased since I completed the most recent set of changes. But I'm moreso approving the overall concept and initial implementation :)

[mzxrules]

I hate how overwritten this is.

[fig02]

Its written in the same form as the generic macros. The point is to easily let you see the bit width and position.

[fig02]

Its also 1 of 2 cases where a custom actor-specific actor macro is needed. Imo we can figure out these edge cases another time, like when the actor is documented

[mzxrules]

Personally I don't find that the generic macro easy to follow logically in it's raw form. Probably more important though is that the macro isn't a proper params getter; the bit gets extracted but then you have a secondary operation "baked in" of the value being shifted into the bit 1 spot instead of bit 0 like the majority of params getters. This is an important detail that is being obfuscated by the macro, and makes it hard to follow the subsequent logic.

In this unusual instance, I feel PARAMS_GET_NOSHIFT would be the more appropriate way to extract the value. The right shift needs to be exposed to the user.

[hensldm]

Fwiw I agree with mxz here. PARAMS_GET_NOSHIFT seems better so you can explicitly show the unique shift (probably the same for en_ishi as well which seems to have the same structure). However also agree we could wait until the actors are documented.

[fig02]

can either of you write out how both cases would be used with NOSHIFT? I would be willing to change it now

[hensldm]

#define TWISTED_HALLWAY_GET_PARAM_15(thisx) (PARAMS_GET_NOSHIFT((thisx)->params, 15, 1) >> 14)
Oh wait en_ishi is different. Don't think NOSHIFT works. My bad.

Edit: #define TWISTED_HALLWAY_GET_PARAM_15(thisx) PARAMS_GET_NOMASK(PARAMS_GET_NOSHIFT((thisx)->params, 15, 1), 14) should also work

[hensldm]

en_ishi does work like #define ISHI_GET_SWITCH_FLAG_UPPER(thisx) PARAMS_GET_NOSHIFT(PARAMS_GET_NOMASK((thisx)->params, 12 - 2), 2, 4) but they may be a bit too much.

[mzxrules]

I would do something more like:

#define TWISTED_HALLWAY_GET_TYPE_NOSHIFT(x) PARAMS_GET_NOSHIFT(x, 15, 1)
...
this->dyna.actor.params = TWISTED_HALLWAY_GET_TYPE_NOSHIFT(this->dyna.actor.params) >> 14;

[Dragorn421]

I suggest just reverting this (to main), and we can indeed figure it out (or not) later, but not let it stop the whole thing