oot icon indicating copy to clipboard operation
oot copied to clipboard

Generic actor params getters

Open Thar0 opened this issue 3 years ago • 6 comments
trafficstars

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.

Thar0 avatar Aug 21 '22 19:08 Thar0

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 :fearful:)

Dragorn421 avatar Nov 21 '22 12:11 Dragorn421

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?

hensldm avatar Sep 14 '23 01:09 hensldm

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)

fig02 avatar Sep 14 '23 01:09 fig02

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.

Thar0 avatar Sep 14 '23 01:09 Thar0

(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 avatar Sep 14 '23 01:09 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.

fig02 avatar Jan 17 '24 16:01 fig02