Implement invulnerability after shrinking #695

EvoLandEco · 2022-12-23T23:13:11Z

fixes #535
The logic is:

When a player loses, it shrinks and becomes passive for a while
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
After a while, an passive player will become active again

codecov-commenter · 2022-12-23T23:16:33Z

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%)`	⬆️
src/player.cpp	`99.61% <100.00%> (+0.78%)`	⬆️
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%)`	⬆️

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

TheoPannetier · 2022-12-25T16:35:42Z

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 · 2022-12-25T16:49:45Z

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 · 2022-12-25T17:25:46Z

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 · 2022-12-29T17:01:06Z

src/player.cpp

@@ -129,8 +129,8 @@ void add_action(player& p, action_type action) noexcept

 bool are_colliding(const player &lhs, const player &rhs) noexcept
 {
-  if (lhs.get_state() == player_state::active
-     && rhs.get_state() == player_state::active)


Things become more complicated here: how do we define colliding? Simply two players are overlapping? Or they should all be non-passive and/or non-invulnerable? Here I chose to define colliding as they are simply overlapping, so as long as they are not out/dead, they can collide. This will not break the other part of the code. We might need to discuss those mechanics in depth later.

We could add a feature later to make non-passive players unable to overlap with each other

Yeah I think it confused me too the first time I saw this; colliding means overlapping AND active, i.e. capable to interact. If two players (or other objects) are overlapping but one or either is not active, there is no collision. I can't think of a better name for this, unfortunately!

TheoPannetier · 2023-01-18T15:09:20Z

src/game.cpp

+  #define FIX_ISSUE_381
  #ifdef FIX_ISSUE_381
-  ///A player can become invulnerable
+  /// A player can become passive
+  {
+    game g;
+
+    assert(is_active(g.get_player(0)));
+    g.set_player_state_passive(g.get_player(0));
+    assert(is_passive(g.get_player(0)));
+  }
+
+  /// A player can become passive, using player.set_state_passive()
  {
    game g;

    assert(is_active(g.get_player(0)));
-    become_invulnerable(g.get_player(0));
-    assert(is_invulnerable(g.get_player(0)));
+    g.get_player(0).set_state_passive();
+    assert(is_passive(g.get_player(0)));
+  }
+
+  /// A player can become non-passive again
+  {
+    game g;
+
+    assert(is_active(g.get_player(0)));
+    g.set_player_state_passive(g.get_player(0));
+    g.remove_player_state_passive(g.get_player(0));
+    assert(!is_passive(g.get_player(0)));
+  }
+
+  /// A player can become non-passive again, using player.remove_state_passive()
+  {
+    game g;

+    assert(is_active(g.get_player(0)));
+    g.get_player(0).set_state_passive();
+    g.get_player(0).remove_state_passive();
+    assert(!is_passive(g.get_player(0)));
  }
  #endif


I don't understand why you changed the tests here. I see that you're being more exhaustive, systematically asserting that all setters exist. I don't see the necessity for this; the short test I had written was enough to reach the goal of the issue ("A player can become passive/invulnerable"). Being able to turn it back is not a necessity to reach this.
I believe (I'm not sure, can't read it from your commit history) what you are doing here is characteristic of unit-testing, that is writing tests, typically after the code, to ensure that the concerned bit of code keeps working as intended. This is ok, but we prefer test-driven development. The difference is that in TDD, we write the test before the code and even before thinking of the implementation. The test states our intended goal, and will pass when this goal is reached. Then we seek to implement a solution.

This is good work, your tests are functional and do satisfy the end goal, I just want to raise the point that I believe this is excessive, maybe premature testing.

@TheoPannetier Thanks for pointing out this, it was that I feel like the tests I had written beforehand were not enough to detect all the changes in the future that might be done intentionally or unintentionally by me or by others, I am sometimes too obsessive that I force myself to add a lot of probably excessive or premature tests. 😢

I am sometimes too obsessive

I definitely can't blame you for caring about the future of the game! I confirm I was too harsh with my initial comment, and I'd like to apologise for this.

These tests are indeed a bit premature at this stage, but they don't remove anything or add anything dangerous to the game AFAICS and may pave the way for the next steps with the passive state. Take the above as advice on how to better design tests, but what you're doing is an important step too.

I am sometimes too obsessive

I definitely can't blame you for caring about the future of the game! I confirm I was too harsh with my initial comment, and I'd like to apologise for this.

These tests are indeed a bit premature at this stage, but they don't remove anything or add anything dangerous to the game AFAICS and may pave the way for the next steps with the passive state. Take the above as advice on how to better design tests, but what you're doing is an important step too.

This mechanism really involves a lot of worth-thinking issues, I've been a bit busy with my paper recently, I would find a time to rethink about it, and try to improve this PR

TheoPannetier · 2023-01-18T15:10:10Z

src/game.cpp

  #ifdef FIX_ISSUE_382
-  ///An invulnerable player cannot shrink
+  /// An passive player cannot grow


Suggested change

/// An passive player cannot grow

/// A passive player cannot grow

TheoPannetier · 2023-01-18T15:10:28Z

src/game.cpp

+    assert(inv_player_size_after == inv_player_size_before);
+  }
+
+  /// An passive player cannot shrink


Suggested change

/// An passive player cannot shrink

/// A passive player cannot shrink

TheoPannetier · 2023-01-18T15:23:03Z

src/game.cpp

  #ifdef FIX_ISSUE_463
-  // Players lose invulnerability after a short time
+  // Can get and set a player's passive state


Suggested change

// Can get and set a player's passive state

// Can get and set a player's passive state duration

TheoPannetier · 2023-01-18T15:26:01Z

src/game.cpp

+  {
+    game g;
+    player& p = g.get_player(0);
+
+    // Check default duration
+    assert(p.get_duration_passive() == 2000);
+
+    int new_duration = 200;
+    p.set_duration_passive(new_duration);
+    assert(p.get_duration_passive() == new_duration);
+  }


I can't see why you would change the duration of the passive state at the player level; shouldn't it be a fixed parameter of the game?

@TheoPannetier I was thinking about possible future extension of the power-ups etc. For a minimal viable game, it should also be good to just let the duration be a fixed parameter of the game. What is your opinion about this?

I think you've figured out my opinion already 😉
I believe anticipating possible extensions beyond the minimal viable game is premature optimisation, and a wise man once told me it's the root of all evil.

So for now, I'd rather have the duration as a fixed parameter. It will be easy to make it free again when/if we need it :)

I think you've figured out my opinion already 😉 I believe anticipating possible extensions beyond the minimal viable game is premature optimisation, and a wise man once told me it's the root of all evil.

So for now, I'd rather have the duration as a fixed parameter. It will be easy to make it free again when/if we need it :)

I'll change it in the following commits

TheoPannetier · 2023-01-18T15:29:57Z

src/game.cpp

+  // Game cannot increment a player's passive timer unless it is passive
+  {
+    game g;
+    player& p = g.get_player(0);
+    assert(!is_passive(p));
+    assert(p.get_passive_timer() == 0);
+
+    g.increment_passive_timers();
+    assert(p.get_passive_timer() == 0);
+
+    int timer_before = p.get_passive_timer();
+    p.set_state_passive();
+    g.increment_passive_timers();
+    int timer_after = p.get_passive_timer();
+    int difference = timer_after - timer_before;
+    assert(difference == 1);
+  }


Same comment as above for test 381; I don't think this adds anything that is not already (implicitly) required by the test I had written (below).

TheoPannetier · 2023-01-18T15:42:08Z

src/game.cpp

+  // A player's passive timer cannot be bigger than its passive duration,
+  // Otherwise it will be reset to 0
+  {
+    game g;
+    player& p = g.get_player(0);
+    g.set_player_state_passive(p);
+    int duration = p.get_duration_passive();
+    for (int i = 0; i < duration - 1; i++)
+    {
+      assert(p.get_state() == player_state::passive);
+      int timer_before = p.get_passive_timer();
+      g.tick();
+      int timer_after = p.get_passive_timer();
+      int difference = timer_after - timer_before;
+      assert(difference == 1);
+      assert(timer_after <= duration);
+    }
+    g.tick();
+    assert(!p.get_passive_timer());
+    int timer = p.get_passive_timer();
+    assert(timer == 0);
+  }
+
+  // Game cannot reset a player's passive timer if it's less than duration
+  {
+    game g;
+    player& p = g.get_player(0);
+
+    int timer = p.get_passive_timer();
+    assert(timer == 0);
+
+    p.set_state_passive();
+    int duration = p.get_duration_passive();
+
+    for (int i = 0; i < duration - 1; i++)
+    {
+      int timer_before = p.get_passive_timer();
+      g.tick();
+      int timer_after = p.get_passive_timer();
+      int difference = timer_after - timer_before;
+      assert(difference == 1);
+      assert(timer_after <= duration);
+      timer_before = p.get_passive_timer();
+      g.reset_passive_timers();
+      timer_after = p.get_passive_timer();
+      assert(timer_after == timer_before);
+    }
+  }


Same as above, I don't think it's necessary to explicitly test for the timer?
Unless you think the issue needed to be broken down in smaller steps - in which case, sure, go ahead and create smaller intermediate issues towards losing invulnerability after some time :)

I added these tests for debugging purposes as I encountered quite a lot of issues. I was making sure that all the parts I had added were working as I expected.

The main test (if it's good!) should already ensure that everything works as expected. If players lose invulnerability after some time and not earlier, as #463 demands, then we have our feature and we're happy!
I'd argue we only care about the underlying mechanisms and potential issues if a problem indeed manifests after completing the test. Then it's time to make a new issue to signal the problem, and write a test that ensures it is fixed.

That being said, using tests as debug towards completing another test is entirely fine, but then no need to leave it in the code once you have resolved the final test!

The main test (if it's good!) should already ensure that everything works as expected. If players lose invulnerability after some time and not earlier, as #463 demands, then we have our feature and we're happy! I'd argue we only care about the underlying mechanisms and potential issues if a problem indeed manifests after completing the test. Then it's time to make a new issue to signal the problem, and write a test that ensures it is fixed.

That being said, using tests as debug towards completing another test is entirely fine, but then no need to leave it in the code once you have resolved the final test!

Yes that's true, I mean I would add extra debugging tests besides the main tests and remove them right before merging, this could be a better way than what was done in this PR 😉

TheoPannetier · 2023-01-18T15:48:23Z

src/game.cpp

-  winning_player.grow();
+  const int loser_index = get_losing_player_index(g, first_player_index, second_player_index);
+  player& losing_player = g.get_player(loser_index);
+  if (!is_passive(winning_player) && !is_passive(losing_player))


I think passive states shouldn't be caught here; instead if either player is passive, there is no collision (and thus, growing/shrinking won't be called)

TheoPannetier · 2023-01-18T15:53:03Z

src/game.cpp

@@ -143,6 +143,12 @@ void game::tick()
    shrink_losing_player(*this);
  }

+  /// PASSIVE STATE ///
+  increment_passive_timers();


I disagree with the refactoring of invulnerable into passive. I think the former conveys more explicit information than the latter. passive is too general, could refer to the player being inactive for any reason, e.g. having been killed, stunned, or recovering from a defeat, and suggest the player cannot interact with anything, including foods etc. invulnerable simply means the player cannot be attacked, and lose a fight

IMO we do need to have some sound definitions of these states. For the passive state, I was referring to a game state that is the same as in GTA5, which means a player is alive but cannot be killed by the others players, and of course, a passive player cannot hurt the other players. A passive player can collect things and do jobs.

Considering foods or power-ups, it is worthwhile to think thoroughly. Maybe we could find a more proper name for this state I was referring to? As for invulnerable, I still feel like it could be a state after collecting a power-up or triggering some game mechanics.

Fair enough, I agree with the reasoning, although again I prefer invulnerable over passive to refer to what you describe: passive implies you cannot do anything other than move, so no item or task collection. On the other hand, invulnerable does suggest you could still attack other players, which is not true (or is it?).
I'll let you make the final call :)

Fair enough, I agree with the reasoning, although again I prefer invulnerable over passive to refer to what you describe: passive implies you cannot do anything other than move, so no item or task collection. On the other hand, invulnerable does suggest you could still attack other players, which is not true (or is it?). I'll let you make the final call :)

I think what is important is to have two types of invulnerable (not receiving damage) states, one you can still deal damage while the other one you cannot.

To be more specific:

Invulnerable State A: If a player receives damage, it shrinks and becomes State A, in this state it won't receive more damage and won't deal damage or won't collide, it is like passive we've discussed.

Invulnerable State B: If a player collects a power-up or something, it just won't receive any damage, but it still deals damage to the others.

Is there any possibility we find two words to well describe them?

Invulnerable State B: If a player collects a power-up or something, it just won't receive any damage, but it still deals damage to the others

We don't have power-ups are plans to add any before 1.0, so let's not worry about it for now ;)
We can always make changes later if the current implementation becomes a barrier to new desired features.
Let's keep it to what's referred to in the title of this PR.
So for this "invulnerable state A", I guess either passive or invulnerable are ok names; neither is entirely satisfying but they do an ok job.

@TheoPannetier That's true, I will refactor passive back to invulnerable for now

TheoPannetier

I have been a bit harsh and grumpy in my comments, but overall this is good work and this contribution definitely helps the game move forward :)
So there is only one point where I request a change before merging: collision should not happen if either member is passive, and in turn there is no need to check for state in grow and shrink.
I disagree with the name passive instead of invulnerable, but the last word is yours for this 👍

EvoLandEco · 2023-02-15T15:51:17Z

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

TheoPannetier · 2023-02-24T10:08:16Z

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:

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.
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 · 2023-02-24T10:43:09Z

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:

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.

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 · 2023-02-24T11:36:03Z

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:

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.

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 · 2023-03-16T18:23:08Z

@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 · 2023-05-10T15:34:26Z

Merged to not lose progress

EvoLandEco added 3 commits December 23, 2022 22:13

revise the tests prior to implementation

3c5eb19

revise tests

e0cddb2

#535 implement invulnerability after shrinking

5a238e6

EvoLandEco requested review from swom and TheoPannetier December 23, 2022 23:13

EvoLandEco linked an issue Dec 23, 2022 that may be closed by this pull request

[Game Play] Too much 'grow' and 'shrink' #535

Open

EvoLandEco self-assigned this Dec 23, 2022

Merge branch 'develop' into tianjian2

f0c1333

EvoLandEco removed request for swom and TheoPannetier December 25, 2022 17:20

EvoLandEco linked an issue Dec 25, 2022 that may be closed by this pull request

Expand player_state, add a passive state #699

Open

EvoLandEco added 2 commits December 25, 2022 18:41

#699 add passive state to player_state

2d8ea49

reimplement passive state

46d00f4

EvoLandEco commented Dec 29, 2022

View reviewed changes

EvoLandEco requested review from TheoPannetier and richelbilderbeek December 29, 2022 17:03

TheoPannetier reviewed Jan 18, 2023

View reviewed changes

TheoPannetier suggested changes Jan 18, 2023

View reviewed changes

TheoPannetier mentioned this pull request Feb 22, 2023

There is a maximum size a player can reach #726

Closed

TheoPannetier and others added 2 commits May 10, 2023 17:04

Merge branch 'develop' into tianjian2

adc88be

move passivity check to collision

14e7522

TheoPannetier closed this May 10, 2023

TheoPannetier reopened this May 10, 2023

TheoPannetier merged commit 55b0d45 into develop May 10, 2023

	/// An passive player cannot grow
	/// A passive player cannot grow

	/// An passive player cannot shrink
	/// A passive player cannot shrink

	// Can get and set a player's passive state
	// Can get and set a player's passive state duration

Implement invulnerability after shrinking #695

Implement invulnerability after shrinking #695

Conversation

EvoLandEco commented Dec 23, 2022 • edited Loading

codecov-commenter commented Dec 23, 2022 • edited Loading

Codecov Report

TheoPannetier commented Dec 25, 2022

EvoLandEco commented Dec 25, 2022

EvoLandEco commented Dec 25, 2022

EvoLandEco Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheoPannetier Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheoPannetier left a comment

Choose a reason for hiding this comment

EvoLandEco commented Feb 15, 2023 • edited Loading

TheoPannetier commented Feb 24, 2023

EvoLandEco commented Feb 24, 2023

EvoLandEco commented Feb 24, 2023

TheoPannetier commented Mar 16, 2023

TheoPannetier commented May 10, 2023

EvoLandEco commented Dec 23, 2022 •

edited

Loading

codecov-commenter commented Dec 23, 2022 •

edited

Loading

EvoLandEco Dec 29, 2022 •

edited

Loading

TheoPannetier Jan 18, 2023 •

edited

Loading

EvoLandEco commented Feb 15, 2023 •

edited

Loading