Rework the movement system

[EvoLandEco]

fixes #524 To locate the tests please search FIX_ISSUE_524 I would like to recommend you to build and try the new movement to see whether it is a good idea

[richelbilderbeek]

BTW, I put some other comments at https://github.com/tresinformal/game/issues/524#issuecomment-1095206050, but let's do those in a next PR :-)

[TheoPannetier]

Hey, sorry but I won't find much time for reviewing PR's in the next three weeks, I'll have to un-assign myself from here 😬 May I suggest @sebmader as a reviewer? He's well acquainted with the movement system :)

[EvoLandEco]

BTW, I put some other comments at #524 (comment), but let's do those in a next PR :-)

Yeah, finally, it is time to make player_options come true

[EvoLandEco]

Hey, sorry but I won't find much time for reviewing PR's in the next three weeks, I'll have to un-assign myself from here 😬 May I suggest @sebmader as a reviewer? He's well acquainted with the movement system :)

Thanks for your suggestion😀, good luck!

[codecov-commenter]

Codecov Report

Merging #525 (73ce84b) into develop (a0df559) will increase coverage by 0.71%. The diff coverage is 100.00%.

:exclamation: Current head 73ce84b differs from pull request most recent head 1930eea. Consider uploading reports for the commit 1930eea to get more accurate results

@@             Coverage Diff             @@
##           develop     #525      +/-   ##
===========================================
+ Coverage    92.80%   93.52%   +0.71%     
===========================================
  Files           46       46              
  Lines         2392     2594     +202     
  Branches       148        0     -148     
===========================================
+ Hits          2220     2426     +206     
+ Misses         172      168       -4     

Impacted Files	Coverage Δ
key_action_map.h	100.00% <ø> (ø)
action_type.cpp	100.00% <100.00%> (ø)
color.cpp	98.16% <100.00%> (-0.02%)	:arrow_down:
game.cpp	96.89% <100.00%> (+1.15%)	:arrow_up:
game.h	100.00% <100.00%> (ø)
key_action_map.cpp	99.33% <100.00%> (ø)
player.cpp	99.05% <100.00%> (+0.32%)	:arrow_up:
player.h	100.00% <100.00%> (ø)
player_state.cpp	100.00% <100.00%> (ø)
sound_type.cpp	95.65% <100.00%> (+2.31%)	:arrow_up:
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a0df559...1930eea. Read the comment docs.

[EvoLandEco]

Run ./game_on_gha
game_on_gha: game.cpp:1919: void test_game(): Assertion `abs(g.get_player(0).get_max_speed_backward()) >= g.get_player(0).get_deceleration_backward()' failed.
/home/runner/work/_temp/a4325ad5-43ab-4c38-af96-bc81e1ea32eb.sh: line 1:  4547 Aborted                 (core dumped) ./game_on_gha
Error: Process completed with exit code [13](https://github.com/tresinformal/game/runs/5992780621?check_suite_focus=true#step:8:13)4.

@richelbilderbeek strange that assertion fails on GitHub Action but passes locally

[richelbilderbeek]

@EvoLandEco,

@richelbilderbeek strange that assertion fails on GitHub Action but passes locally

Run ./game_on_gha
game_on_gha: game.cpp:1919: void test_game(): Assertion `abs(g.get_player(0).get_max_speed_backward()) >= g.get_player(0).get_deceleration_backward()' failed.
/home/runner/work/_temp/a4325ad5-43ab-4c38-af96-bc81e1ea32eb.sh: line 1:  4547 Aborted                 (core dumped) ./game_on_gha
Error: Process completed with exit code [13](https://github.com/tresinformal/game/runs/5992780621?check_suite_focus=true#step:8:13)4.

Yup, too bad (yes, I really mean 'bad' here :-) ) your operating system does not detect this access violation. It is a property of access violations to be triggered irregularly.

In our context, this is usually caused bu v[x], where v is a std::vector, and x is an index that is not present in that vector. In the context of the error message, g.get_player(0) is probably the cause, where the player at index 0 is absent.

Tips: add asserts (e.g. assert(index >= 0); and assert(index < static_cast<int>(v.size()); to document your assumptions (see C++ Core Guideline I.6: Prefer Expects() for expressing preconditions, read assert instead of Expects :-) )

[EvoLandEco]

@EvoLandEco,

@richelbilderbeek strange that assertion fails on GitHub Action but passes locally

Run ./game_on_gha
game_on_gha: game.cpp:1919: void test_game(): Assertion `abs(g.get_player(0).get_max_speed_backward()) >= g.get_player(0).get_deceleration_backward()' failed.
/home/runner/work/_temp/a4325ad5-43ab-4c38-af96-bc81e1ea32eb.sh: line 1:  4547 Aborted                 (core dumped) ./game_on_gha
Error: Process completed with exit code [13](https://github.com/tresinformal/game/runs/5992780621?check_suite_focus=true#step:8:13)4.

Yup, too bad (yes, I really mean 'bad' here :-) ) your operating system does not detect this access violation. It is a property of access violations to be triggered irregularly.

In our context, this is usually caused bu v[x], where v is a std::vector, and x is an index that is not present in that vector. In the context of the error message, g.get_player(0) is probably the cause, where the player at index 0 is absent.

Tips: add asserts (e.g. assert(index >= 0); and assert(index < static_cast<int>(v.size()); to document your assumptions (see C++ Core Guideline I.6: Prefer Expects() for expressing preconditions, read assert instead of Expects :-) )

I don't quite understand this violation. Doesn't vector index in C++ start from 0? According to my debug console, player 0 did exist and Assertion abs(g.get_player(0).get_max_speed_backward()) >= g.get_player(0).get_deceleration_backward() shouldn't fail because abs(g.get_player(0).get_max_speed_backward()) returned 0.5 and g.get_player(0).get_deceleration_backward() returned 0.1.

I cannot reproduce any error locally, I also tried to change g.get_player(1) to g.get_player(0) in other tests, but they did not trigger the same error on GitHub Action.

[richelbilderbeek]

Hi @EvoLandEco, the computer is not making things up. Indeed C++ vectors start at zero and if the things you claim are true, it must be something else. Or one of your assumptions is wrong :innocent:

Well, somehow: congratulations with your first access violation, I predict we'll solve it tonight in the first half together. Would you mind if I and another team member hunt for the problem the first 30 mins?

[EvoLandEco]

Hi Richel,

I have an R&O meeting which might end at 17:30, I may not be able to attend in time, please do so even if I am absent.

Tianjian From mobile phone

---

From: Richel Bilderbeek @.> Sent: Wednesday, April 13, 2022 1:46:19 PM To: tresinformal/game @.> Cc: Tianjian @.>; Mention @.> Subject: Re: [tresinformal/game] Rework the movement system (PR #525)

Hi @EvoLandEcohttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FEvoLandEco&data=04%7C01%7C%7Ceea8bf944ab54006e57d08da1d433a48%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637854471819703535%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=4HxhV00tEQPYTTfBTe9immL0h5szqOG%2BNN9ikaAEWps%3D&reserved=0, the computer is not making things up. Indeed C++ vectors start at zero and if the things you claim are true, it must be something else.

Congratulations with your first access violation, I predict we'll solve it tonight in the first half together. Would you mind if I and another team member hunt for the problem the first 30 mins?

— Reply to this email directly, view it on GitHubhttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftresinformal%2Fgame%2Fpull%2F525%23issuecomment-1097955102&data=04%7C01%7C%7Ceea8bf944ab54006e57d08da1d433a48%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637854471819703535%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bhPee%2FQ%2FHXVfHujNzQfZ%2FwxTbZuSOXFbRCq7foOxUl4%3D&reserved=0, or unsubscribehttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FANVRGREPOFAZSR6KTK64LP3VE2XYXANCNFSM5TDDK6FQ&data=04%7C01%7C%7Ceea8bf944ab54006e57d08da1d433a48%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637854471819703535%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=szPtCt0E2sUdYxN%2BjRErtc%2Bna95TB9iXnpd3TTrYXic%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

[richelbilderbeek]

@EvoLandEco awesome, thanks! No worries about being late; we'll show how we found the error if we find it on time :-)

[EvoLandEco]

@richelbilderbeek this bug may be due to an inconsistent behavior insides game::tick(). I set up a ubuntu subsystem on my laptop to be able to reproduce this error.

The only difference according to two debug consoles between two platforms was the variable after, if you set a break point at line 1936 (please pull all the changes). On windows after was -0.4, this is as expected and is what apply_inertia() should do. However on Linux, after was 0.

I also tested some other members of the player 0 after game::tick() was executed, all tests passed which I believe may indicate that player 0 is not an empty vector element. If it was an access violation, why there was only problem reading the speed? Can we assume that game::tick() or something within it behaves differently across the platforms, which might give us different player speeds?

[EvoLandEco]

@richelbilderbeek @ollyturner the problem is the same: on Linux we should use std::abs() instead of abs(), or a double will be coerced into an int. The problematic code was located in player::decelerate(). I just pushed a fix, now all tests are passing.