Move player movement code into Player class #7220

bendeutsch · 2018-04-06T18:15:25Z

This allows running the same code on the server, for example for server-side movement checks.

This set of changes is actually a large part of pull request #6219 , "server side movement". But this pull request does not contain any of the network code or server-side checking or reconciliation. All it does is move the main movement methods from LocalPlayer to Player (and thus RemotePlayer), without changing any functionality.

There are just two points that are not "pure" in this sense:

the player attribute control has been changed to m_control, to avoid overshadowing by a function parameter of this name
the Android "autojump via stepheight" has, yet again, been removed and needs a replacement

SmallJoker · 2018-04-06T18:23:20Z

src/network/serverpackethandler.cpp

-	player->control.sneak = (keyPressed & 64);
-	player->control.LMB = (keyPressed & 128);
-	player->control.RMB = (keyPressed & 256);
+	player->m_control.up = (keyPressed & 1);


m_* class member variables are supposed to be private

Oh, that explains why it didn't have the m_ before – I could change it back (it's just confusing with the step and move parameters), or remove the const from the (pre-existing) accessor?

Since it has multiple variables .. -> key_controls ?

Technically it's keys, mouse, and joystick 😉

I'll try it with a private member and a non-const accessor-by-reference, sec…

src/localplayer.h

src/player.cpp

src/client.cpp

src/clientenvironment.cpp

paramat · 2018-04-07T14:26:21Z

the Android "autojump via stepheight" has, yet again, been removed and needs a replacement

Yes good :]

stujones11 · 2018-04-08T14:17:09Z

I am not sure how making the game unplayable for many people can be a good thing. IMO it just makes them more likely to use unofficial clients.

paramat · 2018-04-09T15:44:36Z

I agree, the current code is a nasty hardcoded hack though, it's good that it goes but it certainly needs a replacement. However, a per-player optional stepheight of 1.1 (either chosen by server or server allows each player to choose) is already possible using player object properties, it could be lua code added to builtin or done another way. I won't let 0.5.0 go out without autojump.

The issue for this is #6183

bendeutsch · 2018-04-09T18:36:22Z

@stujones11 : please take a look at PR #7228 as a suggestion for replacing the Android step height hack.

bendeutsch · 2018-04-14T16:08:57Z

I've reenabled the Android step height hack! 😱

There's another PR, #7228 , which replaces the step height hack with a real autojump. Originally, the plan was to merge that one, then this (and then adjust the original server side movement, #6219 , and then start on my actual plans for player movement). But then I might as well just not touch the step height hack here, so that this PR can be merged immediately without any change in behaviour.

bendeutsch · 2018-04-19T19:59:59Z

Rebased again, mainly incorporating changes from #7243 .

Could you please reconsider the "WIP" label? I consider this PR finished and mergeable, even if it does not directly add a feature or fix a bug.

src/localplayer.cpp

src/player.cpp

src/remoteplayer.h

sfan5 · 2018-05-15T09:41:49Z

src/player.h

-	{}
+	void step(float dtime, const PlayerControl &control,
+			const PlayerSettings &player_settings, Environment *env,
+			std::vector<CollisionInfo> *player_collisions = NULL);


The code doesn't seem to account for player_collisions == NULL at all, why does it default to this?

also: should be nullptr

Not sure, I'll need to check.

Got it: the Player::step function does not care about the CollisionInfo, but this is used as a parameter when calling Player::move. This function does account for a null pointer.

Please keep in mind that all this PR is trying to do is lift the existing code and algorithms from LocalPlayer to Player. So answering for, or even changing, the existing movement code and call chain is outside of the scope of this PR.

bendeutsch · 2018-06-30T20:14:24Z

Oops; I just noticed several requested changes – sorry, @sfan5 , I must have missed the notifications :-(

I'll get on these over the next few days, I hope.

This allows running the same code on the server, for example for server-side movement checks.

sfan5

I don't remember if I'd tested this at the time of my last review, code looks good to me either way.

nerzhul · 2019-03-05T07:13:50Z

rebase needed

paramat · 2019-03-16T05:20:33Z

Bump for review attention, this is a high priority issue as it leads to #6219

ClobberXD · 2019-03-16T16:18:20Z

What should I expect while testing this? There should be no difference in test results whatsoever, right?

nerzhul · 2019-03-18T10:29:04Z

src/client.cpp

@@ -406,7 +406,8 @@ void Client::step(float dtime)
 	// Control local player (0ms)
 	LocalPlayer *player = m_env.getLocalPlayer();
 	assert(player);
-	player->applyControl(dtime, &m_env);
+	player->applyControl(dtime, player->getPlayerControl(),


why we pass already owned objects to the player class ?

It's been a while, but some of these things are because the same code (the same methods) need to be called on the server and the client side, where different things are available.

Or it really is unnecessary, but I'll need to get into the code again first.

bendeutsch · 2019-03-21T20:06:24Z

Hi, sorry, I had just about abandoned this PR; it's a deep change for no visible benefit, after all. No, there should be no changes in tests or behaviour (that I'm aware of).

As a reminder, the goal of this PR is just to make the player movement code available to the server, so that some kind of server-side movement check or replay can be developed. My first attempt was all in one branch, which got out of hand as other work was done on the movement code.

Rebasing will take some time, I fear, but I'll pick it up again. Maintainers can edit, or anyone else can just grab the code and run with it, it's all good.

paramat · 2019-03-21T21:14:59Z

I'd like to see indication of potential approval from a second core dev before you are required to rebase.
Personally i don't expect contributors to constantly rebase unless there is good chance of a merge.

ClobberXD · 2019-03-22T01:04:42Z

it's a deep change for no visible benefit, after all. No, there should be no changes in tests or behaviour (that I'm aware of).

Thought so, but just wanted to confirm. I've tested this PR, and I did not notice any oddities or glitches. I ran around and flew around for around 5 minutes each, and didn't encounter any issues. I guess this PR works. :)

👍

paramat · 2019-12-06T23:33:15Z

This is necessary for #6219
Seems to be waiting for more review and support, so that the author sees rebasing as worthwhile (if they are still interested in working on it, see below).

@bendeutsch i seem to have marked all your PRs as 'adoption needed' but cannot remember why, is it the case that you are not interested in working on them? (which is fine by the way).

rubenwardy · 2020-02-02T18:50:51Z

@bendeutsch Please work on this, it has one approval and would be a very nice addition

SmallJoker reviewed Apr 6, 2018

View reviewed changes

bendeutsch mentioned this pull request Apr 6, 2018

Server side movement #6219

Closed

SmallJoker reviewed Apr 6, 2018

View reviewed changes

src/localplayer.h Outdated Show resolved Hide resolved

SmallJoker reviewed Apr 6, 2018

View reviewed changes

src/player.cpp Outdated Show resolved Hide resolved

SmallJoker added @ Server / Client / Env. Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Apr 6, 2018

bendeutsch force-pushed the lift-player-move branch from d8f5760 to 6e7831f Compare April 6, 2018 19:08

nerzhul requested changes Apr 7, 2018

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

src/clientenvironment.cpp Outdated Show resolved Hide resolved

src/clientenvironment.cpp Outdated Show resolved Hide resolved

bendeutsch force-pushed the lift-player-move branch 2 times, most recently from 5e519e7 to 5b8b873 Compare April 7, 2018 13:49

rubenwardy added the WIP label Apr 8, 2018

bendeutsch force-pushed the lift-player-move branch 2 times, most recently from 7cd4c0c to db796a8 Compare April 14, 2018 16:03

bendeutsch mentioned this pull request Apr 15, 2018

PlayerSettings struct for player movement code #7243

Merged

bendeutsch force-pushed the lift-player-move branch from db796a8 to 273ba7d Compare April 19, 2018 19:50

SmallJoker removed the WIP label Apr 19, 2018

bendeutsch force-pushed the lift-player-move branch from 273ba7d to f96195c Compare April 22, 2018 12:21

sfan5 requested changes May 15, 2018

View reviewed changes

bendeutsch force-pushed the lift-player-move branch from f96195c to 5633391 Compare May 29, 2018 03:48

SmallJoker added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 19, 2018

bendeutsch force-pushed the lift-player-move branch from 5633391 to ded2688 Compare July 2, 2018 04:53

paramat added the Rebase needed The PR needs to be rebased by its author label Jul 2, 2018

Move player movement code into Player class

6bc2437

This allows running the same code on the server, for example for server-side movement checks.

bendeutsch force-pushed the lift-player-move branch from ded2688 to 6bc2437 Compare July 3, 2018 04:43

sfan5 approved these changes Jul 3, 2018

View reviewed changes

sfan5 added One approval ✅ ◻️ and removed Rebase needed The PR needs to be rebased by its author Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Jul 3, 2018

paramat added this to the 5.1.0 milestone Dec 30, 2018

nerzhul added Rebase needed The PR needs to be rebased by its author and removed One approval ✅ ◻️ labels Mar 5, 2019

paramat added the High priority label Mar 16, 2019

paramat mentioned this pull request Mar 16, 2019

Detection of dirty clients #6114

Closed

paramat added the One approval ✅ ◻️ label Mar 16, 2019

nerzhul reviewed Mar 18, 2019

View reviewed changes

paramat removed this from the 5.1.0 milestone May 8, 2019

paramat added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Jun 14, 2019

rubenwardy force-pushed the master branch 2 times, most recently from 0e3b135 to 39c54e1 Compare June 21, 2019 00:46

rubenwardy added the Possible close label Dec 6, 2019

rubenwardy closed this Mar 6, 2020

Poikilos mentioned this pull request Sep 6, 2021

Consider some type of movement security. Poikilos/EnlivenMinetest#425

Open

Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label May 1, 2022

Zughy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move player movement code into Player class #7220

Move player movement code into Player class #7220

bendeutsch commented Apr 6, 2018

SmallJoker Apr 6, 2018

bendeutsch Apr 6, 2018

SmallJoker Apr 6, 2018

bendeutsch Apr 7, 2018

paramat commented Apr 7, 2018

stujones11 commented Apr 8, 2018

paramat commented Apr 9, 2018

bendeutsch commented Apr 9, 2018

bendeutsch commented Apr 14, 2018

bendeutsch commented Apr 19, 2018

sfan5 May 15, 2018

bendeutsch Jun 30, 2018

bendeutsch Jul 2, 2018

bendeutsch commented Jun 30, 2018

sfan5 left a comment

nerzhul commented Mar 5, 2019

paramat commented Mar 16, 2019

ClobberXD commented Mar 16, 2019

nerzhul Mar 18, 2019

bendeutsch Mar 21, 2019

bendeutsch commented Mar 21, 2019

paramat commented Mar 21, 2019

ClobberXD commented Mar 22, 2019

paramat commented Dec 6, 2019 •

edited

Loading

rubenwardy commented Feb 2, 2020

Move player movement code into Player class #7220

Move player movement code into Player class #7220

Conversation

bendeutsch commented Apr 6, 2018

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

paramat commented Apr 7, 2018

stujones11 commented Apr 8, 2018

paramat commented Apr 9, 2018

bendeutsch commented Apr 9, 2018

bendeutsch commented Apr 14, 2018

bendeutsch commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendeutsch commented Jun 30, 2018

sfan5 left a comment

Choose a reason for hiding this comment

nerzhul commented Mar 5, 2019

paramat commented Mar 16, 2019

ClobberXD commented Mar 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendeutsch commented Mar 21, 2019

paramat commented Mar 21, 2019

ClobberXD commented Mar 22, 2019

paramat commented Dec 6, 2019 • edited Loading

rubenwardy commented Feb 2, 2020

paramat commented Dec 6, 2019 •

edited

Loading