Document Player Params #2307

fig02 · 2024-11-24T05:44:26Z

Creates a getter for Player's other param, and a packing macro to set params.

AngheloAlf · 2024-11-24T10:59:49Z

src/code/z_play.c

+    playerStartBgCamIndex = PLAYER_GET_START_BG_CAM_INDEX(&player->actor);
+
    if (playerStartBgCamIndex != 0xFF) {


I see you are passing -1 the bgcamindex arg of PLAYER_PARAMS, but that behavior makes it inconsistent when reading that parameter because you have to compare it against 0xFF instead of -1.
Or maybe this check could be rewritten somehow to actually compare to -1?

Yeah I tried using -1 in the comparison but it doesnt match. If there is a way to match it then I would gladly change it, but I dont know if there is

Did you try casting to u8?
if (playerStartBgCamIndex != (u8)-1) {

It does match.
Maybe #define PLAYER_START_BG_CAM_DEFAULT (u8)-1 should exist. I hate how long and clunky it is, but I think this is inherently a problem with the name bgCamIndex (which I want to explore renaming)

yeah, technically without the cast -1 == 0xFFFFFFFF because of s32 playerStartBgCamIndex

Unless you plan on dropping BG from the name, I don't see how a rename wouldn't just make the name longer.

well, the original name for these is "cam id", which is alot shorter than "bg cam index".
I think in general it shouldnt be called an "index", the fact that it indexes an array is not helpful to have in the name.

Not that "cam id" is the best name, but something like it seems appropriate.
But anyway, this conversation should happen separately.

Anyway, I added PLAYER_START_BG_CAM_DEFAULT. The word "index" is omitted for brevity and as a personal protest against it being in the name :)

it would be more consistent if either PLAYER_GET_START_BG_CAM_INDEX uses PARAMS_GET_S and PLAYER_START_BG_CAM_DEFAULT is (s8)-1, or PLAYER_START_BG_CAM_DEFAULT is 0xFF (which is maybe not bad since it's hidden in a macro)

(also macro definition should be in parens like ((u8)-1) here)

AngheloAlf · 2024-11-24T11:01:50Z

src/overlays/actors/ovl_player_actor/z_player.c

@@ -10747,7 +10747,7 @@ void Player_Init(Actor* thisx, PlayState* play2) {
    }

    if (func_80845C68(play, (respawnFlag == 2) ? 1 : 0) == 0) {
-        gSaveContext.respawn[RESPAWN_MODE_DOWN].playerParams = PARAMS_GET_S(thisx->params, 0, 8) | 0xD00;
+        gSaveContext.respawn[RESPAWN_MODE_DOWN].playerParams = PLAYER_PARAMS(PLAYER_START_MODE_IDLE, thisx->params);


I guess it would be a bit more correct to wrap the second argument withPLAYER_GET_START_BG_CAM_INDEX

Suggested change

gSaveContext.respawn[RESPAWN_MODE_DOWN].playerParams = PLAYER_PARAMS(PLAYER_START_MODE_IDLE, thisx->params);

gSaveContext.respawn[RESPAWN_MODE_DOWN].playerParams = PLAYER_PARAMS(PLAYER_START_MODE_IDLE, PLAYER_GET_START_BG_CAM_INDEX(thisx));

I did this at first, but it doesnt match. It uses the wrong register and throws the function off. Maybe there is a way for it to match, I didnt really try much. I copied what MM currently has, since the same problem apparently existed there too.

Oh, sounds like I probably had this very same thought and this very same issue years ago.
It's fine then

Its pretty odd that it doesnt match, considering that it did match with PARAMS_GET_S(thisx->params, 0, 8) | 0xD00;
I guess it has to do with how thisx is being passed/accessed but I dont really know

considered that PLAYER_PARAMS() already packs startBgCamIndex by itself, it might be expected that

... | PARAMS_PACK(thisx->params, 0, 8)

and

... | PARAMS_PACK(PLAYER_GET_START_BG_CAM_INDEX(thisx), 0, 8) // or with expanded macro... ... | PARAMS_PACK(PARAMS_GET_S(thisx->params, 0, 8), 0, 8)

won't match

(there's no shift, but still, better to asm-diff it)

I think PARAMS_PACK_NOMASK sounds good. I wouldn't want to do away with masking altogether since it's not really as safe from a user perspective as bits from one field could overflow into others for values that are too large

I pushed my attempt at doing NOMASK + the change in the review comment above. Something about this is wrong but I cannot look into it right now. If anyone does please let me know

PARAMS_PACK(startMode, 8, 4) is the important one that should be PARAMS_PACK_NOMASK (When I tested this I replaced both PARAMS_PACKs with PARAMS_PACK_NOMASK, but maybe it's possible to keep the bg cam one as PARAMS_PACK)

(also I'm not sure if the PARAMS_GET_U/PARAMS_GET_S change has anything to do with it either)

it only matches if PLAYER_START_BG_CAM_DEFAULT is casted to u8, not s8 (even if using PARAMS_GET_S)

(.venv) fig@LAPTOP-UOUVEO0H:~/repo/oot$ ./first_diff.py First difference at ROM addr 0xAA70B8, 0x8002FF18 is at 0x36C bytes inside 'Actor_DrawFaroresWindPointer' (VRAM: 0x8002FBAC, VROM: 0xAA6D4C, SIZE: 0x8DC, build/gc-eu-mq-dbg/src/code/z_actor.o) Bytes: 240AFFFF vs 240A06FF addiu $t2, $zero, -0x1 vs addiu $t2, $zero, 0x6FF

I still think I kinda like making it clear to the reader that the value is -1, but if that isnt liked I will change it to 0xFF

src/code/z_play.c

cadmic · 2024-11-27T20:33:51Z

(made another comment in a resolved thread)

cadmic · 2024-11-29T17:45:47Z

include/z64actor.h

@@ -752,6 +752,10 @@ typedef struct NpcInteractInfo {
 #define PARAMS_PACK(p, s, n) \
    (((p) & NBITS_TO_MASK(n)) << (s))

+// Pack all bits in `p` by `s`. There is no masking of specific bits.


I'm not sure what packing "by s" means. Maybe something like

// Moves the value `p` to bit position `s` for building actor parameters by OR-ing these together.

fig02 added 2 commits November 24, 2024 00:33

document player params

eaf9127

better bgcamindex comment

1a0d352

fig02 added Needs first approval Needs second approval labels Nov 24, 2024

fig02 added 2 commits November 24, 2024 02:20

cant use -1 for condition

e8183b4

fix match

ce924d4

AngheloAlf reviewed Nov 24, 2024

View reviewed changes

Feacur reviewed Nov 24, 2024

View reviewed changes

src/code/z_play.c Show resolved Hide resolved

define for default bgcam

77af3f1

fig02 added 2 commits November 27, 2024 22:23

doesnt work

5ed13a6

matches

d79bca6

cadmic approved these changes Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document Player Params #2307

Document Player Params #2307

fig02 commented Nov 24, 2024

AngheloAlf Nov 24, 2024

fig02 Nov 24, 2024

AngheloAlf Nov 24, 2024

fig02 Nov 24, 2024

Feacur Nov 24, 2024 •

edited

Loading

mzxrules Nov 24, 2024

fig02 Nov 24, 2024 •

edited

Loading

fig02 Nov 25, 2024

cadmic Nov 27, 2024

AngheloAlf Nov 24, 2024

fig02 Nov 24, 2024

AngheloAlf Nov 24, 2024

fig02 Nov 24, 2024

Feacur Nov 24, 2024 •

edited

Loading

Thar0 Nov 27, 2024

fig02 Nov 28, 2024 •

edited

Loading

cadmic Nov 28, 2024

cadmic Nov 28, 2024 •

edited

Loading

fig02 Nov 28, 2024

cadmic commented Nov 27, 2024

cadmic Nov 29, 2024

		playerStartBgCamIndex = PLAYER_GET_START_BG_CAM_INDEX(&player->actor);

		if (playerStartBgCamIndex != 0xFF) {

	gSaveContext.respawn[RESPAWN_MODE_DOWN].playerParams = PLAYER_PARAMS(PLAYER_START_MODE_IDLE, thisx->params);
	gSaveContext.respawn[RESPAWN_MODE_DOWN].playerParams = PLAYER_PARAMS(PLAYER_START_MODE_IDLE, PLAYER_GET_START_BG_CAM_INDEX(thisx));

Document Player Params #2307

Are you sure you want to change the base?

Document Player Params #2307

Conversation

fig02 commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feacur Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fig02 Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feacur Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fig02 Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadmic Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadmic commented Nov 27, 2024

Choose a reason for hiding this comment

Feacur Nov 24, 2024 •

edited

Loading

fig02 Nov 24, 2024 •

edited

Loading

Feacur Nov 24, 2024 •

edited

Loading

fig02 Nov 28, 2024 •

edited

Loading

cadmic Nov 28, 2024 •

edited

Loading