Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documet bottle using actions #2360

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Pepe20129
Copy link
Collaborator

Document Player_Action_8084EAC0 as Player_Action_DrinkFromBottle, Player_Action_8084EED8 as Player_Action_UseFairyFromBottle, and Player_Action_8084EFC0 as Player_Action_DropActorFromBottle.

Also documents func_8083C0E8 as Player_SetupIdleAndPlayIdleAnim (could maybe be named something like Player_ResetToIdle instead as that's how it's used from what I can see) as it was used in Player_Action_DrinkFromBottle.

@fig02
Copy link
Collaborator

fig02 commented Dec 13, 2024

Thanks.
Would personally prefer to wait on the idle setup function. There are so many idle action setup wrappers, and I think we need to figure them out together. Plan to do so when their usages are a bit more clear and we can more easily discern a difference.

@Pepe20129
Copy link
Collaborator Author

Undid the func_8083C0E8 rename.

@@ -927,6 +927,7 @@ typedef struct Player {
s16 csDelayTimer; // Player_Action_WaitForCutscene: Number of frames to wait before responding to a cutscene
s16 playedLandingSfx; // Player_Action_BlueWarpArrive: Played sfx when landing on the ground
s16 appearTimer; // Player_Action_FaroresWindArrive: Counts up, appear at 20 frames (1 second)
s16 drinkingState; // Player_Action_DrinkFromBottle: Uses values 0-2 to determine which part of the drinking action is currently active
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mention DrinkingState enum here?

if (sp2C >= 0) {
if (sp2C == 0xC) {
Player_SetupActionPreserveItemAction(play, this, Player_Action_8084EED8, 0);
#define ACTION_TO_BOTTLE_ACTION(action) (action - PLAYER_IA_BOTTLE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really prefer if this define was at top-level instead of in the middle of a function. Maybe in z64player.h? Then it could be used in Player_ActionToBottle as well.

(PS: I'm not really sure what to do about these "sub-enums" like with Player_ActionToMagicSpell and Player_ActionToMeleeWeapon too. Maybe they deserve a separate enum? I think we can figure that out later though)

(PPS: maybe it should be called ACTION_TO_BOTTLE to match the function name?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I would prefer to handle all these in a consistent manner at some point in the future. But if we want to introduce this now I guess we can change other examples of it later.

But there are so many of these id offset situations in player.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there are so many of these id offset situations in player.

yeah, I kinda think this is a bigger project and not too relevant to this PR (where the goal is just to name some action funcs). Could also revert ACTION_TO_BOTTLE_ACTION here to really punt on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants