z_player Death functions, some helper functions - #2767
Conversation
| * A "reset" function when changing between different actions/states. | ||
| * Often called together with Player_DetachHeldActor (by Player_ResetStatesHeldActor). | ||
| */ | ||
| void Player_ResetStates(PlayState* play, Player* this) { |
There was a problem hiding this comment.
Yea I don't like the name. ResetStates is extremely broad... reading it you could imagine it potentially modifying literally thousands of states, whereas here we're only touching a handful of them.
You have to come up with a name that greatly narrows down the scope.
There was a problem hiding this comment.
I realize I didn't go into much detail in the PR post.
I'm open to alternatives, but disagree that it by itself is a inadequate name and disagree that it needs to be further narrowed down, so I won't be not be the one to try to rename it if needed. I'm by no means a decomp or programming master, but by now I've named all action and upper action functions as well as many helper functions and looked at far more functions beyond that where I more or less understand their purpose but don't feel confident enough to name them.
There is no function that you can confuse Player_ResetStates with. It is the only helper function that does so many different kinds of resets. It's not like Player_RemoveGetItemState, Player_InactivateMeleeWeapon, Player_ExitCutsceneFirstPerson, Player_ResetForwardFootWeight, etc. Furthermore, it is so common (as part of Player_ResetStatesHeldActor) that anyone that works a bit with z_player will quickly recognize it and know that whatever function they're dealing with needs to reset a lot of things, even if they can't tell from the name itself exactly what - they can go look at the function if they need specifics.
As I did write, I was hesitant at first because it is so non-specific, but working with the code over these months I've come to the conclusion that it actually works very well - I know exactly what function is being referred to and have never confused it for anything else. Rather, personally I would be confused if it did have a more narrow name, because it would put focus only on what can fit in the name, and I'd forget there is more to it.
Player_SetupAction resets more playerStates than Player_ResetStates and is a far more important function (that doesn't even have a comment), but I would expect of someone who wants to understand or modify action functions to study Player_SetupAction and realize what it does beyond setting this->actionFunc, so it doesn't need to be called Player_SetupActionResetStatesAndActionVarsMaybeCheckItemActionAndShielding for clarity. Likewise I don't believe Player_ResetStates needs to be called Player_InactivateMeleeCollidersSetNormalCameraEndOnePointCutsceneRemoveClimbHangJumpPushGrabbedCrawlingZeroTripleSlashCountSpinAttackStartTimerShapeXrotShapeYoffset*.
* This is actually incomplete because in addition to this, Player_EndOnePointCutscene removes PLAYER_STATE2_DEEP_WATER and PLAYER_STATE2_DIVING, and this did trip me up once while researching some water-related bug
Death related functions and some small and big helper functions.
When I first set the name
Player_ResetStatesHeldActorI was concerned it's a bit dense yet diffuse, but I've come to appreciate it as it reflects what it does and there isn't any other standalone function or function combination that accomplishes the same thing - reset some player states and flags together with detaching held actor - to confuse it with.It could be argued that
Player_ZeroXZNormalCamerashouldn't be named "normal camera" because that implies some camera setting, but it's hard to nameunk_6ADtoo which should be the base for the function name. (My note on it: "0 = normal, 1 = first person without weapon, 2 = first person with weapon, 3 = cutscene action, 4 = cutscene items")For
SurfaceType_DamageIsNotLavaI empirically tested whatunk_27does.Suggestions for
Inventory_FairyReviveare welcome, it really should be "check if player has bottled fairy for revive".