tresinformal / drakkar

The tresinformal video game called 'Drakkar'
GNU General Public License v3.0
11 stars 4 forks source link

Implement invulnerability after shrinking #695

Closed EvoLandEco closed 1 year ago

EvoLandEco commented 1 year ago

fixes #535 The logic is:

  1. When a player loses, it shrinks and becomes passive for a while
  2. When a player is passive, it cannot grow or shrink, it cannot make other players grow or shrink either, it will be like a ghost
  3. After a while, an passive player will become active again
codecov-commenter commented 1 year ago

Codecov Report

Merging #695 (46d00f4) into develop (8dd1863) will increase coverage by 0.94%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #695      +/-   ##
===========================================
+ Coverage    86.96%   87.91%   +0.94%     
===========================================
  Files           37       37              
  Lines         1634     1737     +103     
  Branches       124      133       +9     
===========================================
+ Hits          1421     1527     +106     
+ Misses         213      210       -3     
Impacted Files Coverage Δ
src/game.h 100.00% <ø> (ø)
src/game.cpp 97.36% <100.00%> (+0.60%) :arrow_up:
src/player.cpp 99.61% <100.00%> (+0.78%) :arrow_up:
src/player.h 100.00% <100.00%> (ø)
src/player_state.cpp 100.00% <100.00%> (ø)
src/game_functions.cpp 80.00% <0.00%> (+1.66%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TheoPannetier commented 1 year ago

I've assigned you to #381 #382 #463; please make sure that you assign yourself to issues before you start working on them! Otherwise other people may decide to work on it too, come up with a different solution, eventually causing a merge conflict (and some wasted time)!

EvoLandEco commented 1 year ago

I've assigned you to #381 #382 #463; please make sure that you assign yourself to issues before you start working on them! Otherwise other people may decide to work on it too, come up with a different solution, eventually causing a merge conflict (and some wasted time)!

Yes, I should have done it before this PR. I also assigned myself #469 😸

EvoLandEco commented 1 year ago

When I was thinking about adding visual effect for invulnerability, I feel like the current implementation is bad. The reason is, we already have a player_state class, a better option would be to expand this enum class, add some states into it, and build the whole thing on top of it, rather than inventing a new flag m_invulnerability in the player class.

@TheoPannetier @swom apologies for inviting you as reviewers prematurely and (maybe) have already wasted a bit of your time. I will re-implement invulnerability,

EvoLandEco commented 1 year ago

@TheoPannetier An open discussion is always helpful, thanks for the opinions, and let's keep talking and find the best solution 😉

TheoPannetier commented 1 year ago

Hello @EvoLandEco ! I think the discussion has been a bit dense and dispersed, so to make your life easier here are the two things I'd like to see addressed before I accept this PR:

  1. When resolving collision and player growth/shrinkage, you check that players involved in the collision are both active before resolving the effects of the collision. This is good, but you do the check inside the growing/shrinking functions. I feel that if either player is not active, there should not be any collision at all (and no need to call the growth/shrinkage functions). So the check for player active/passive state should be done earlier, and has_any_player_collision should return FALSE if either player is passive.
  2. Make passive duration a fixed parameter of either the player or game, and remove the corresponding setter.

(and as usual, resolve any pending merge conflict!)

EvoLandEco commented 1 year ago

Hello @EvoLandEco ! I think the discussion has been a bit dense and dispersed, so to make your life easier here are the two things I'd like to see addressed before I accept this PR:

  1. When resolving collision and player growth/shrinkage, you check that players involved in the collision are both active before resolving the effects of the collision. This is good, but you do the check inside the growing/shrinking functions. I feel that if either player is not active, there should not be any collision at all (and no need to call the growth/shrinkage functions). So the check for player active/passive state should be done earlier, and has_any_player_collision should return FALSE if either player is passive.
  2. Make passive duration a fixed parameter of either the player or game, and remove the corresponding setter.

(and as usual, resolve any pending merge conflict!)

Let's focus on what this PR is meant to be. 😉 I remember I did status check in a later stage due to a limitation from how collision check was implemented, but I will surely revisit this part and see if I can do any better.

For the passive duration, I agree to make it align with what we did in the shoot cooldown system: maybe make it a static variable.

EvoLandEco commented 1 year ago

Hello @EvoLandEco ! I think the discussion has been a bit dense and dispersed, so to make your life easier here are the two things I'd like to see addressed before I accept this PR:

  1. When resolving collision and player growth/shrinkage, you check that players involved in the collision are both active before resolving the effects of the collision. This is good, but you do the check inside the growing/shrinking functions. I feel that if either player is not active, there should not be any collision at all (and no need to call the growth/shrinkage functions). So the check for player active/passive state should be done earlier, and has_any_player_collision should return FALSE if either player is passive.
  2. Make passive duration a fixed parameter of either the player or game, and remove the corresponding setter.

(and as usual, resolve any pending merge conflict!)

@TheoPannetier We have the game_options class here, what about putting passive duration in this class?

TheoPannetier commented 1 year ago

@TheoPannetier We have the game_options class here, what about putting passive duration in this class?

Indeed, it looks appropriate :) (sorry, only seeing this now)

TheoPannetier commented 1 year ago

Merged to not lose progress