Document Player Knockback related functions #1601
Conversation
|
Contributions made in this pr are licensed under CC0 |
| void Player_SetIFrameInvincibilityTimer(Player* this, s32 timer) { | ||
| if (this->invincibilityTimer > timer) { | ||
| this->invincibilityTimer = timer; |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
my only suggestion is Player_SetInvincibilityTimer and Player_SetInvincibilityTimerFlashing
There was a problem hiding this comment.
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_80837AE0will 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_80837AFCwill 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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->invincibilityTimeris of far greater importance gameplay wise than the effect of flashing the player.func_80837AE0is making sure the player isn't under IFrames before setting up a fixed number of damage framesfunc_80837AFCis 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->invincibilityTimerand 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.
There was a problem hiding this comment.
Sorry to butt in: I agree with fig that "IFrameInvincibilityTimer" is awkward (damage invincibility gives I frames too) but I agree with mzxrules that "flashing" is not a good way to distinguish the two types of invincibility. Maybe Player_SetDamageInvincibilityTimer and Player_SetNaturalInvincibilityTimer based on the wording in the discussion?
There was a problem hiding this comment.
Yea the naming is probably not ideal. Arguably, invincibilityTimer could be shortened to iFrameTimer.
But I also I don't particularly like using a neutral sounding word like "natural", I'd prefer something more descriptive like Blocking or Evasion.
There was a problem hiding this comment.
This is still the only blocker for me. I suggest a totally new direction
func_80837AE0 -> Player_SetIntangibility
Definition from google: "incapable of being touched"
In this state, Player does not only avoid taking damage, but damage also has no effect on him, he cannot be touched. See video: https://youtu.be/1m2hkeNQ1Es
func_80837AFC -> Player_SetInvulnerability
Definition from google: "incapable of being wounded, injured, or harmed"
In this state, player avoids taking damage but still experiences damage knockback, and makes noises like he is getting hit.
See video: https://youtu.be/r0g561yKbw8
The only thing Im not sure of with this approach is what to call the timer value (currently invincibilityTimer) that represents both intangibility and invulnerability. Suggestions welcome. But I feel these names to be incredibly good for differentiating between the two.
| void Player_SetIFrameInvincibilityTimer(Player* this, s32 timer) { | ||
| if (this->invincibilityTimer > timer) { | ||
| this->invincibilityTimer = timer; |
There was a problem hiding this comment.
Sorry to butt in: I agree with fig that "IFrameInvincibilityTimer" is awkward (damage invincibility gives I frames too) but I agree with mzxrules that "flashing" is not a good way to distinguish the two types of invincibility. Maybe Player_SetDamageInvincibilityTimer and Player_SetNaturalInvincibilityTimer based on the wording in the discussion?
There was a problem hiding this comment.
Im not happy with quite a few nitpicky documentation details, but Ive decided it would be easier if I just make the changes on my own and PR them later.
The knockback stuff specifically is important to have and comes up for me often.
In the future we should try to limit scope creep a bit for easier reviewing and merging.
|
can merge after conflicts resolved |
No description provided.