Document Player Knockback related functions

[mzxrules]

No description provided.

[mzxrules]

Contributions made in this pr are licensed under CC0

[hensldm]

Seems good to me, though some nits/questions around adjacent areas.

[fig02]

apologies for delayed review, got distracted with retail matching

[fig02]

Dont really think this name works. "i frame invincibility timer" means "invincibility frame invincibility timer", it doesnt distinguish it from the function above.

Really we need to name this based on the fact that one is visible with flashing, while the other invisible. I dont really have any good ideas for that though, and think the word "invisible" is both clunky and kind of confusing next to the word "invincible"

[fig02]

my only suggestion is Player_SetInvincibilityTimer and Player_SetInvincibilityTimerFlashing

[mzxrules]

Hard disagree. Nothing with these functions creates that flashing effect, that's just a superficial indicator to the underlying state of the player.

With player->invincibilityTimer, remember that negative values are specifically tied with natural invulnerability (what I opted to call IFrames), while positive values are specifically tied to invulnerability after taking damage. We can see this in action for both z_en_ik.c and z_en_wallmas.c.

In z_en_ik.c, the iron knuckle tests that player->invincibilityTimer is less than or equal to 0 in order to override the invincibility from e.g. rolling, allowing it to knock back the player and deal damage, while still maintaining invincibility if the player was already damaged.

In z_en_wallmas.c, the wall master tests that player->invincibilityTimer is greater or equal to 0 to allow the player the ability to dodge being grabbed when rolling.

Now if you follow the logic for func_80837AE0 and func_80837AFC...

• func_80837AE0 will not behave in a sensible way if passed in a negative value. Firstly, it won't enable the flashing effect you want to attribute to the function, and secondly if player->invincibilityTimer is already negative and another negative value is passed in, the new value won't take unless the result reduces the number of invincibility frames.
• Similarly func_80837AFC will not function correctly when passed a positive value. player->invincibilityTimer will never be updated to a positive value unless player->invincibilityTimer is already a larger positive value, which would have that same weird effect of reducing the number of invincibility frames.

So that's why I went with damaged/IFrames.

[fig02]

I dont follow your reasoning for why it cant be tied to flashing. As you mention func_80837AE0 only intends to receive positive values, and positive values for invincibilityTimer are what make the player flash (after taking damage).
If you think its more important to emphasize the fact that its for damage, thats fine, but you make it seem like the flashing isnt relevant at all, when it definitely is.

[mzxrules]

A few reasons:

• There is no "one function sets up flashing, one doesn't", like your suggestion proposed. unk_88F is the value that tracks the flashing effect, and zeroing it simply resets the animation, and again neither function are directly responsible for creating that flashing animation.
• The state change to player->invincibilityTimer is of far greater importance gameplay wise than the effect of flashing the player.
  • func_80837AE0 is making sure the player isn't under IFrames before setting up a fixed number of damage frames
  • func_80837AFC is either overriding damage frames/neutral with IFrames, or overriding IFrames with a longer IFrame timer
• The flashing is a visual feedback effect reflecting the state of player->invincibilityTimer and it just doesn't need to be explicitly stated that a flashing effect will trigger, similar to how ChangeRupees does not explicitly state that it will trigger money making sound effects since it updates the rupee accumulator variable instead of the rupee variable directly.

[fig02]

For a while ive been meaning to call this "damage reaction". I think both words convey the same thing so its a matter of preference. The term "damage reaction" was used in the gamecube titles to mean something similar, but thats a small point.

Really, I think "damage response/reaction" would be a good term for what we currently call "damage effect" in colliders. So using it here for this player specific thing may hurt if we wanted to use that terminology with colliders. But I wouldnt know what to call this value in that case. they really mean the same thing

[Dragorn421]

Can you elaborate on

"damage response/reaction" would be a good term for what we currently call "damage effect" in colliders

Which fields specifically are you referring to?

[fig02]

Its easier to explain it from the perspective of a damage table:
/* Fire arrow */ DMG_ENTRY(4, DEKUBABA_DMGEFF_FIRE),
/* Fire magic */ DMG_ENTRY(4, DEKUBABA_DMGEFF_FIRE)
The value currently called "damage effect" describes how an actor should react or respond to the damage source, like by burning up, getting frozen, or dropping certain items, as a few examples. There are a few fields within colliders that would need to change as well, but the important one is collider->elem.toucher/bumper.effect (using masters current names)

These values here in player mean the same thing. But link has his own translation layer on top of the values he receives from a collider, for seemingly no reason. Other than to combine the collider "effects" with knockback types. Here you can see player mapping the values received from a collider to his internal "reaction" values (using code from mzx's branch for readability):

if (this->stateFlags1 & PLAYER_STATE1_27) {
    sp4C = PLAYER_DAMAGE_RESPONSE_NONE;
} else if (this->actor.colChkInfo.acHitEffect == 2) {
    sp4C = PLAYER_DAMAGE_RESPONSE_ICE_TRAP;
} else if (this->actor.colChkInfo.acHitEffect == 3) {
    sp4C = PLAYER_DAMAGE_RESPONSE_ELECTRIC_SHOCK;
} else if (this->actor.colChkInfo.acHitEffect == 4) {
    sp4C = PLAYER_DAMAGE_RESPONSE_KNOCKBACK_LARGE;
} else {
    // (im adding the comment below for clarity, this is not in mzx's branch)
    // Handle fire/burning (acHitEffect == 1) or the different kinds of knockback
    func_80838280(this);
    sp4C = PLAYER_DAMAGE_RESPONSE_NONE;
}

So basically I am suggesting that instead of calling players extra translation layer as "damage response/reaction", the field in the collider/damage system should probably get that name instead. And we would have to call Links something else.

[fig02]

The value currently called "damage effect" describes how an actor should react or respond to the damage source

To add, of course, these values are actor specific, and do not have global meaning. But this does not change the fact that in general, it does describe the response/reaction to the damage (or sometimes not damage, just getting stunned)

[fig02]

as discussed on discord, I think its best if we revert these name changes for now and consider these names later at a broader scale with the at/acHitEffect fields in colliders.

[Dragorn421]

https://discord.com/channels/688807550715560050/838852326231769089/1214214286340128869
to
https://discord.com/channels/688807550715560050/838852326231769089/1214230605353586748

[Dragorn421]

#1925

[Dragorn421]

#1925 (currently) names this->actor.colChkInfo.acHitEffect -> this->actor.colChkInfo.playerACHitReaction / PlayerACHitReaction / PLAYER_AC_HIT_REACTION_

func_808382DC as highlighted by fig above maps PLAYER_AC_HIT_REACTION_ values to this PR's current PLAYER_DAMAGE_RESPONSE_

[Dragorn421]

Can you elaborate on

"damage response/reaction" would be a good term for what we currently call "damage effect" in colliders

Which fields specifically are you referring to?