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

Start Player Item Docs #1396

Merged
merged 8 commits into from
Sep 30, 2023
Merged

Start Player Item Docs #1396

merged 8 commits into from
Sep 30, 2023

Conversation

engineer124
Copy link
Collaborator

Once again using research from OoT/Fig that also applies to MM to import player docs. Source of the docs comes from here:
zeldaret/oot#1523

@@ -210,7 +210,7 @@ void Player_Action_97(Player* this, PlayState* play);

s32 Player_UpperAction_0(Player* this, PlayState* play);
s32 Player_UpperAction_1(Player* this, PlayState* play);
s32 Player_UpperAction_2(Player* this, PlayState* play);
s32 Player_UpperAction_ChangeHeldItem(Player* this, PlayState* play);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Worth noting that in OoT, these functions are called _IA_ or "ItemAction" functions, which seems to be the use case for most of these, but not in all cases, such as carrying an actor above your head. UpperAction is still an option to name these, so I left it alone for now. But it can always change in a later PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I am heavily leaning toward changing oots to upper action as well. Some desync between games here is fine in the interim until that all gets sorted

@engineer124 engineer124 added documentation Improvements or additions to documentation Needs-second-approval Second approval Needs-first-approval First approval labels Sep 27, 2023
@@ -570,7 +570,7 @@ s32 func_801235DC(PlayState* play, f32 arg1, s16 arg2) {
return false;
}

ItemId func_8012364C(PlayState* play, Player* player, s32 arg2) {
ItemId Player_GetItemOnButton(PlayState* play, Player* player, s32 arg2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the usage of this function it seems like arg2 is always passed a value equivalent to EquipSlot.

Suggested change
ItemId Player_GetItemOnButton(PlayState* play, Player* player, s32 arg2) {
ItemId Player_GetItemOnButton(PlayState* play, Player* player, EquipSlot slot) {

thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was about to do this before PR'ing, but the one call to this function in z_player_lib.c made me hesitate, i.e. in function func_80123810 since the arg i seems shifted up by 1 from C btns. Is i suppose to be EquipSlot when it's fed into this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I think I see how it works. It just shifts i from an enum of just C buttons to an enum with B and C buttons. EquipSlot has buttons B, C, A so I'm not sure if it's suppose to be all 3 or not, so it's worth revisiting this later. See zeldaret/oot#1228 for this kind of discussion

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't say it is shifting from an enum of just C buttons to another enum, but instead it is just accounting for the fact that sCItemButtons doesn't include the B button. That array could have comments like the following considering how it is used

u16 sCItemButtons[] = {
    BTN_CLEFT, // EQUIP_SLOT_C_LEFT
    BTN_CDOWN, // EQUIP_SLOT_C_DOWN
    BTN_CRIGHT, // EQUIP_SLOT_C_RIGHT
};

Copy link
Collaborator Author

@engineer124 engineer124 Sep 27, 2023

Choose a reason for hiding this comment

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

I would argue sCItemButtons is more of a map from:

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;

to controller buttons, then i++ is really the macro #define INTERACT_C_BTN_TO_BC_BTN(btnsC) ((btnsC) + 1) being applied to i and storing the result back into i. So I'll leave the comments out for now until these different enums are introduced in MM.

For reference, equip slot would be InteractBCAButton from that PR, but then it might make more sense for Player_GetItemOnButton to take InteractBCButton as it's enum arg and not InteractBCAButton. Just something to be careful of

src/overlays/actors/ovl_player_actor/z_player.c Outdated Show resolved Hide resolved
@engineer124 engineer124 removed the Needs-second-approval Second approval label Sep 29, 2023
Copy link
Collaborator

@tom-overton tom-overton left a comment

Choose a reason for hiding this comment

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

One thing I'm not 100% sure about

if ((anim == &gPlayerAnim_link_normal_fighter2free) && (this->currentShield == PLAYER_SHIELD_NONE)) {
anim = &gPlayerAnim_link_normal_free2fighter_free;
}

endFrame = Animation_GetLastFrame(anim);

if (var_v1 >= 0) {
if (itemChangeType >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be:

Suggested change
if (itemChangeType >= 0) {
if (itemChangeType >= PLAYER_ITEM_CHG_0) {

Copy link
Contributor

Choose a reason for hiding this comment

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

its a check for being negative or not, so not really imo

Copy link
Contributor

Choose a reason for hiding this comment

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

say you shift the enum for whatever reason and type 0 becomes 1, it ruins the check for positive or negative

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah exactly, it's the negative value that's important, not it being within a range of enum values

@tom-overton tom-overton added Merge-ready All reviewers satisfied, just waiting for CI and removed Needs-first-approval First approval labels Sep 30, 2023
@engineer124 engineer124 merged commit 33aaaea into zeldaret:main Sep 30, 2023
@engineer124 engineer124 deleted the item-docs branch September 30, 2023 07:28
petrie911 added a commit to Decompollaborate/mm that referenced this pull request Oct 3, 2023
* z_message_nes (1 NON_MATCHING) (zeldaret#1394)

* Bring over progress

* Some docs

* scratch urls + format

* Fixes

* Match Message_DrawTextNES

* String macros

* color structs

* Fix struct

* 20/79 OK

* BossHakugin_Draw OK

* func_80B0D69C OK

* Document object_mk (zeldaret#1389)

* Updated object_mk animation, limb, and skeleton names

* Updated object_mk texture and DList names

* Added comment to object_mk.xml

* Remove "ing" from Anim names

Co-authored-by: engineer124 <[email protected]>

* Update z_en_mk.c to not use "ing" in names

* Running formatter

---------

Co-authored-by: engineer124 <[email protected]>

* func_80B0D2B8 OK

* func_80B0D750 OK

* Shadow function docs

* Fix some non-matchings

* Apparently this matches now? It didn't before

* OverrideLimbDraw OK

* BossHakugin_PostLimbDraw

* Animation Cleanup: En_G* (zeldaret#1395)

* wip

* more cleanup

* more cleanup

* one more thing

* one more default case

* missed brackets

* PR Review

* add more matches and fill stuct

* 3 more Draw functions

* func_80B0E5A4

* Finshed the Draw functions

* Small cleanup

* Player Docs: Initial framework for "Action Change Lists" (zeldaret#1397)

* copy fig docs

* adjust wording

* another fix

* fix bool

* PR Suggestion

* Animation Cleanup: En_H* (zeldaret#1399)

* begin H

* more cleanup

* Animation Cleanup: En_I* (zeldaret#1400)

* cleanup I

* oops

* Most of the death-handling code done

* Start Player Item Docs (zeldaret#1396)

* Copy Fig Docs

* small cleanup

* ItemChangeType comment

* bool

* item change comments

* PR Review

* rm comments

* more matching

* Animation Cleanup: En_K* (zeldaret#1402)

* cleanup k

* one more name

* Animation Cleanup: En_J* (zeldaret#1401)

* cleanup J

* small fix

---------

Co-authored-by: Derek Hensley <[email protected]>
Co-authored-by: Tom Overton <[email protected]>
Co-authored-by: Zach North <[email protected]>
Co-authored-by: engineer124 <[email protected]>
Co-authored-by: Parker B <[email protected]>
Co-authored-by: petrie911 <[email protected]>
petrie911 added a commit to Decompollaborate/mm that referenced this pull request Oct 3, 2023
* Animation Cleanup: En_G* (zeldaret#1395)

* wip

* more cleanup

* more cleanup

* one more thing

* one more default case

* missed brackets

* PR Review

* z_message_nes (1 NON_MATCHING) (zeldaret#1394)

* Bring over progress

* Some docs

* scratch urls + format

* Fixes

* Match Message_DrawTextNES

* String macros

* color structs

* Document object_mk (zeldaret#1389)

* Updated object_mk animation, limb, and skeleton names

* Updated object_mk texture and DList names

* Added comment to object_mk.xml

* Remove "ing" from Anim names

Co-authored-by: engineer124 <[email protected]>

* Update z_en_mk.c to not use "ing" in names

* Running formatter

---------

Co-authored-by: engineer124 <[email protected]>

* Player Docs: Initial framework for "Action Change Lists" (zeldaret#1397)

* copy fig docs

* adjust wording

* another fix

* fix bool

* PR Suggestion

* Animation Cleanup: En_H* (zeldaret#1399)

* begin H

* more cleanup

* Animation Cleanup: En_I* (zeldaret#1400)

* cleanup I

* oops

* Start Player Item Docs (zeldaret#1396)

* Copy Fig Docs

* small cleanup

* ItemChangeType comment

* bool

* item change comments

* PR Review

* rm comments

* Animation Cleanup: En_K* (zeldaret#1402)

* cleanup k

* one more name

* Animation Cleanup: En_J* (zeldaret#1401)

* cleanup J

* small fix

* functions

---------

Co-authored-by: engineer124 <[email protected]>
Co-authored-by: Derek Hensley <[email protected]>
Co-authored-by: Zach North <[email protected]>
Co-authored-by: petrie911 <[email protected]>
engineer124 added a commit that referenced this pull request Feb 2, 2024
…sorted documentation (#1599)

* Fix struct

* 20/79 OK

* BossHakugin_Draw OK

* func_80B0D69C OK

* func_80B0D2B8 OK

* func_80B0D750 OK

* Shadow function docs

* Fix some non-matchings

* Apparently this matches now? It didn't before

* OverrideLimbDraw OK

* BossHakugin_PostLimbDraw

* Animation Cleanup: En_G* (#1395)

* wip

* more cleanup

* more cleanup

* one more thing

* one more default case

* missed brackets

* PR Review

* add more matches and fill stuct

* 3 more Draw functions

* func_80B0E5A4

* Finshed the Draw functions

* Small cleanup

* Most of the death-handling code done

* more matching

* functions

* Goht collab (#9)

* Animation Cleanup: En_G* (#1395)

* wip

* more cleanup

* more cleanup

* one more thing

* one more default case

* missed brackets

* PR Review

* z_message_nes (1 NON_MATCHING) (#1394)

* Bring over progress

* Some docs

* scratch urls + format

* Fixes

* Match Message_DrawTextNES

* String macros

* color structs

* Document object_mk (#1389)

* Updated object_mk animation, limb, and skeleton names

* Updated object_mk texture and DList names

* Added comment to object_mk.xml

* Remove "ing" from Anim names

Co-authored-by: engineer124 <[email protected]>

* Update z_en_mk.c to not use "ing" in names

* Running formatter

---------

Co-authored-by: engineer124 <[email protected]>

* Player Docs: Initial framework for "Action Change Lists" (#1397)

* copy fig docs

* adjust wording

* another fix

* fix bool

* PR Suggestion

* Animation Cleanup: En_H* (#1399)

* begin H

* more cleanup

* Animation Cleanup: En_I* (#1400)

* cleanup I

* oops

* Start Player Item Docs (#1396)

* Copy Fig Docs

* small cleanup

* ItemChangeType comment

* bool

* item change comments

* PR Review

* rm comments

* Animation Cleanup: En_K* (#1402)

* cleanup k

* one more name

* Animation Cleanup: En_J* (#1401)

* cleanup J

* small fix

* functions

---------

Co-authored-by: engineer124 <[email protected]>
Co-authored-by: Derek Hensley <[email protected]>
Co-authored-by: Zach North <[email protected]>
Co-authored-by: petrie911 <[email protected]>

* hotfix

* all functions decomped

* all functions decomped

* Now with more data

* small fix

* another small fix

* ok last small fix I swear

* cleanup

* func_80B0A8C4 OK

* func_80B0D9CC OK

* Use generated reloc for Goht + remove Goht's undefined_syms

* Bodyparts + other clean up

* cleanup

* oops

* fix merge

* begin docs

* fix merge

* cutscene docs

* effects docs

* small docs

* Document colliders

* Odds and ends in the struct

* Name some functions

* Name two functions

* Some action functions and other odds and ends

* Name the lightning segments and electric ball functions correctly

* Names and other cleanup

* Start on hakurock + more cleanup

* Name all hakurock functions

* Name hakurock effects + create a GET_TYPE macro for it

* Finish documenting Hakurock

* Name one more function in BossHakugin

* Name a few more things

* Name a lot of struct vars

* Wall-related stuff

* Fix some fake matches

* Name a couple more struct vars

* Document some more of the struct

* Name the limb flag stuff

* Name the last unnamed struct vars

* Name the horn colliders correctly

* Some initial work on documenting data

* Some function documentation

* A couple of cleanups

* Finish documenting BossHakugin_UpdateBaseRot

* Name BossHakugin_SetLightningSegmentColliderVertices

* Document BossHakugin_AddLightningSegments

* Document BossHakugin_RunUpdateCommon and related functions

* Document BossHakugin_AddMalfunctionEffects

* Document BossHakugin_StepVectorToTarget

* Rename to BossHakugin_StepVector

* Finish documenting BossHakugin_AddMalfunctionEffects for now, might come back later though

* Document BossHakugin_UpdateSubCam

* Document BossHakugin_Thaw

* Document BossHakugin_ChargeUpAttack

* Some scattered docs

* Finish documenting the top "library" functions

* Document intro cutscene functions

* Document running and charging

* Document downed

* Document throwing

* Document remaining non-death action functions

* Document most of the death cutscene

* Get started on documenting crushing rocks

* Separate out the crushing rocks into their own struct

* Document the ExplosionLimbHideInfo

* Finish documenting BossHakugin_DeathCutsceneCrushedByRocks

* Almost done with rocks I think

* Port over Odolwa shadow documentation

* Finish documenting draw functions

* Finish documenting BossHakugin_Update

* Name variables in BossHakugin_UpdateElectricBalls

* Document BossHakugin_CheckForBodyColliderHit

* Finish documenting BossHakugin_UpdateDamage

* I've convinced myself these names are fine

* Almost finish documenting every update function

* Remove TODOs from the header

* Finish documenting the C file

* Delete duplicate define

* Finish variables.txt for Goht

* Undo change that desync'd z_eff_ss_fhg_flash.h from other effect headers

* Missed a THIS

* Some low-hanging review responses

* Add comment explaining something in BossHakugin_ShouldWait

* Match BossHakugin_SpawnLargeStalactiteWalls using a for-loop

* Create GOHT_ELECTRIC_BALL_COUNT_MAX constant

* vector -> norm and targetVector -> targetNorm

* Add TODO comments for dynamic shadow functions

---------

Co-authored-by: Derek Hensley <[email protected]>
Co-authored-by: engineer124 <[email protected]>
Co-authored-by: Parker B <[email protected]>
Co-authored-by: petrie911 <[email protected]>
Co-authored-by: petrie911 <[email protected]>
Co-authored-by: Zach North <[email protected]>
Co-authored-by: engineer124 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Merge-ready All reviewers satisfied, just waiting for CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants