pmmp / PocketMine-MP

A server software for Minecraft: Bedrock Edition in PHP
https://pmmp.io
GNU Lesser General Public License v3.0
3.28k stars 1.56k forks source link

Setting player's pitch not working #821

Closed mad-hon closed 7 years ago

mad-hon commented 7 years ago

Issue description

Setting player's position ignores whatever is supplied as pitch and resets player's pitch to 0.

Steps to reproduce the issue

$player->teleport(new Vector3($x, $y, $z), 0, 45);

This should set player's pitch to look 45 degrees down but it does not.

Via some trial and error I discovered that adding the pitch float value once more into the move player packet makes this work:

    public function encode(){
        $this->reset();
        $this->putEntityRuntimeId($this->eid);
        $this->putVector3f($this->x, $this->y, $this->z);
        $this->putLFloat($this->pitch);
        $this->putLFloat($this->yaw);
        $this->putLFloat($this->bodyYaw); //TODO
        $this->putLFloat($this->pitch); // <---- Added to make PMMP able to set player's pitch
        $this->putByte($this->mode);
        $this->putBool($this->onGround);
    }

Not sure though that this is 100% correct. I suggest you verify what exactly has changed in the protocol, and then update the MovePlayerPosition class accordingly.

moskadev commented 7 years ago

I can confirm. I have the same problem when I'm doing this.

dktapps commented 7 years ago

That encoding is incorrect. I'm sorry, but we don't do guesswork. I can say for 100% certain that there is no extra float in MovePlayerPacket. It's more likely that you accidentally hit the MODE_ROTATION movement type. The current encoding is correct.

dktapps commented 7 years ago

iTXTech/Genisys#1997

dries-c commented 7 years ago

can not be found

mad-hon commented 7 years ago

Well, but when I tried to use the current encoding, I was not able to change the player's pitch, whatever mode I passed in. I mentioned that I am not sure my fix is correct, but it was the only way I actually made it work.

mad-hon commented 7 years ago

BTW, don't forget that when you only reverse-engineer communication between Minecraft PE client and server, you don't actually get a functional packet where server would be setting the player's pitch, so how can you be sure there's no extra float expected in such case?

dktapps commented 7 years ago

It's more likely that you accidentally hit the MODE_ROTATION movement type. The current encoding is correct.

We know far more about MCPE internals than you give us credit for.

mad-hon commented 7 years ago

Well, that's good then! I was actually hoping that you do have access to more info. But how do you explain that setting mode to rotation does not work and adding that extra float actually does work? That for me at least proves that the current encoding is still missing something.

dktapps commented 7 years ago

Once again:

I can say for 100% certain that there is no extra float in MovePlayerPacket.

The encoding is definitely not missing anything. As I said, you've guessed and hit something that works by accident.

In the long run of things, packets are a sequence of bytes. By giving it a float you happened to give it a byte that makes it behave differently.

mad-hon commented 7 years ago

I confirm I guessed and made something work by accident. I have never said otherwise. The primary reason I was trying that is that something is not right. At least the $player->teleport does not work properly because it ignores the pitch even though it accepts it as parameter. If the packet encoding is right then the bug is somewhere between the Player::teleport method and the MovePlayerPacket::encode method. Sorry for guessing wrong where the bug is. Will keep searching...

dktapps commented 7 years ago

Not necessarily. The referenced issue in Genisys above also described this same issue. The general consensus was that it is a client-sided bug, however I never got around to putting the effort into finding out why the issue was occurring.

mad-hon commented 7 years ago

The fact that I succeeded making the player's pitch change is an indication that it can be solved from within PMMP, regardless of whether it would be a bug fix or workaround.

mad-hon commented 7 years ago

Slight progress - it seems the MODE_ROTATION does indeed work, but it is not used anyware.

mad-hon commented 7 years ago

Hmm, seems like overriding the mode in Player::sendPosition helps to achieve what I need although the behavior indicates it's still not a clear solution...

$pk->mode = ($pitch and ($mode == MovePlayerPacket::MODE_RESET)) ? MovePlayerPacket::MODE_ROTATION : $mode;

It works fine when flying but causes little trouble when standing on ground.

It seems MCPE is able to adjust the player's pitch on server's demand but only does it when receiving the MovePlayerPacket with the MODE_ROTATION mode, and produces some unwanted extra movement in response to it.

ghost commented 7 years ago

@mad-hon You did not succeed. Go decompile MCPE yourself instead of guessing as dktapps said. Also, altering packets like that will cause unexpected client bugs. MiNet's encoding of the packet is virtually the same of how PocketMine does it.

It works fine when flying but causes little trouble when standing on ground.

As I said, perfect example of a unintended side effect.

ghost commented 7 years ago

@Chris-Prime

I never said the encoding was incorrect. I said altering packets has unintended side effects weather good or bad; for example some time ago not using Mode_Reset during teleports caused weird client side glitches. I could care less about anyone's opinions, although most notably I do admire @PEMapModder contributions to PocketMine.

mad-hon commented 7 years ago

Well, I don't have time to decompile and examine MCPE. I thought I was helping as I assumed the main reason @dktapps did not fix this yet was low priority of this issue. Apparently my assumptions were wrong, didn't want to waste anyone's time. Anyway, it's still helpful to know that server can affect player's pitch, even though there's no guarantee it can be done without breaking something else...

TheDiamondYT1 commented 7 years ago

Related to https://github.com/pmmp/PocketMine-MP/commit/716efe25492a19e6ba7bd93b3943378fb1532c24?

dktapps commented 7 years ago

@TheDiamondYT1 no.

This is indeed a client-sided bug. They actually hacked around this by adding a separate MovePlayerPacket mode, purely for setting the player's pitch. I'm just... wtf.