From 485f573955fdfda52395e15fc497066d6ef77040 Mon Sep 17 00:00:00 2001 From: Dylan T Date: Sun, 31 May 2020 15:51:30 +0100 Subject: [PATCH 1/2] =?UTF-8?q?Player:=20remove=20move=20buffering,=20impl?= =?UTF-8?q?ement=20simple=20rate=20limited=20movement=E2=80=A6=20(#3167)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduction This PR is a second attempt at improving movement processing to fix #1215 , #2730 and more. This is significantly less complex than the previous attempt #2646 -- it gets rid of the movement buffering system entirely and instead relies on a simple rate limit counter to restrict on-the-fly movement processing. Movement is rate limited to a max average of 2 per tick. It allows up to 5 seconds' backlog to accommodate network lag. The rate limit counter is increased by 2 per tick and decreased once for every movement processed. This prevents movement processing being abused for denial of service attacks. Changes API changes This PR, while obviously highly beneficial for most current users, poses some BC-breaking issues because of changes to the internal Player API. Player->processMovement() (protected) has been removed. This is a BC concern for custom player classes which overrode it and called it as a parent. In addition, child implementations won't be called every tick any more, which could break some custom movement processing systems. Player->newPosition (protected) has been removed. This internal field may have been accessed by custom movement implementations. Player->isTeleporting (protected) has been removed. BC concern is same as previous point. Player->getNextPosition() (public) has been @deprecated. Added the following protected Player class members: int $moveRateLimit ?Vector3 $forceMoveSync handleMovement() processMostRecentMovements() revertMovement() Behavioural changes Player movement is now subject to less rubberbanding and has more reliable behaviour. --- src/pocketmine/Player.php | 131 +++++++++++++++++++++----------------- 1 file changed, 74 insertions(+), 57 deletions(-) diff --git a/src/pocketmine/Player.php b/src/pocketmine/Player.php index 002d3fe4f..ad3f14399 100644 --- a/src/pocketmine/Player.php +++ b/src/pocketmine/Player.php @@ -217,6 +217,9 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ public const SPECTATOR = 3; public const VIEW = Player::SPECTATOR; + private const MOVES_PER_TICK = 2; + private const MOVE_BACKLOG_SIZE = 100 * self::MOVES_PER_TICK; //100 ticks backlog (5 seconds) + /** * Validates the given username. */ @@ -329,10 +332,11 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ /** @var bool[] map: raw UUID (string) => bool */ protected $hiddenPlayers = []; + /** @var int */ + protected $moveRateLimit = 10 * self::MOVES_PER_TICK; /** @var Vector3|null */ - protected $newPosition; - /** @var bool */ - protected $isTeleporting = false; + protected $forceMoveSync = null; + /** @var int */ protected $inAirTicks = 0; /** @var float */ @@ -865,8 +869,11 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $this->lastPingMeasure = $pingMS; } + /** + * @deprecated + */ public function getNextPosition() : Position{ - return $this->newPosition !== null ? Position::fromObject($this->newPosition, $this->level) : $this->getPosition(); + return $this->getPosition(); } public function getInAirTicks() : int{ @@ -1517,23 +1524,19 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ } } - /** - * @return void - */ - protected function processMovement(int $tickDiff){ - if(!$this->isAlive() or !$this->spawned or $this->newPosition === null or $this->isSleeping()){ + protected function handleMovement(Vector3 $newPos) : void{ + $this->moveRateLimit--; + if($this->moveRateLimit < 0){ return; } - assert($this->x !== null and $this->y !== null and $this->z !== null); - assert($this->newPosition->x !== null and $this->newPosition->y !== null and $this->newPosition->z !== null); - - $newPos = $this->newPosition; - $distanceSquared = $newPos->distanceSquared($this); + $oldPos = $this->asLocation(); + $distanceSquared = $newPos->distanceSquared($oldPos); $revert = false; - if(($distanceSquared / ($tickDiff ** 2)) > 100){ + if($distanceSquared > 100){ + //TODO: this is probably too big if we process every movement /* !!! BEWARE YE WHO ENTER HERE !!! * * This is NOT an anti-cheat check. It is a safety check. @@ -1545,7 +1548,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ * asking for help if you suffer the consequences of messing with this. */ $this->server->getLogger()->debug($this->getName() . " moved too fast, reverting movement"); - $this->server->getLogger()->debug("Old position: " . $this->asVector3() . ", new position: " . $this->newPosition); + $this->server->getLogger()->debug("Old position: " . $this->asVector3() . ", new position: " . $newPos); $revert = true; }elseif(!$this->level->isInLoadedTerrain($newPos) or !$this->level->isChunkGenerated($newPos->getFloorX() >> 4, $newPos->getFloorZ() >> 4)){ $revert = true; @@ -1559,7 +1562,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $this->move($dx, $dy, $dz); - $diff = $this->distanceSquared($newPos) / $tickDiff ** 2; + $diff = $this->distanceSquared($newPos); if($this->isSurvival() and $diff > 0.0625){ $ev = new PlayerIllegalMoveEvent($this, $newPos, new Vector3($this->lastX, $this->lastY, $this->lastZ)); @@ -1570,7 +1573,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ if(!$ev->isCancelled()){ $revert = true; $this->server->getLogger()->debug($this->getServer()->getLanguage()->translateString("pocketmine.player.invalidMove", [$this->getName()])); - $this->server->getLogger()->debug("Old position: " . $this->asVector3() . ", new position: " . $this->newPosition); + $this->server->getLogger()->debug("Old position: " . $this->asVector3() . ", new position: " . $newPos); } } @@ -1579,13 +1582,25 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ } } + if($revert){ + $this->revertMovement($oldPos); + } + } + + /** + * Fires movement events and synchronizes player movement, every tick. + */ + protected function processMostRecentMovements() : void{ + $exceededRateLimit = $this->moveRateLimit < 0; + $this->moveRateLimit = min(self::MOVE_BACKLOG_SIZE, max(0, $this->moveRateLimit) + self::MOVES_PER_TICK); + $from = new Location($this->lastX, $this->lastY, $this->lastZ, $this->lastYaw, $this->lastPitch, $this->level); $to = $this->getLocation(); $delta = (($this->lastX - $to->x) ** 2) + (($this->lastY - $to->y) ** 2) + (($this->lastZ - $to->z) ** 2); $deltaAngle = abs($this->lastYaw - $to->yaw) + abs($this->lastPitch - $to->pitch); - if(!$revert and ($delta > 0.0001 or $deltaAngle > 1.0)){ + if($delta > 0.0001 or $deltaAngle > 1.0){ $this->lastX = $to->x; $this->lastY = $to->y; $this->lastZ = $to->z; @@ -1597,41 +1612,47 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $ev->call(); - if(!($revert = $ev->isCancelled())){ //Yes, this is intended - if($to->distanceSquared($ev->getTo()) > 0.01){ //If plugins modify the destination - $this->teleport($ev->getTo()); - }else{ - $this->broadcastMovement(); - - $distance = sqrt((($from->x - $to->x) ** 2) + (($from->z - $to->z) ** 2)); - //TODO: check swimming (adds 0.015 exhaustion in MCPE) - if($this->isSprinting()){ - $this->exhaust(0.1 * $distance, PlayerExhaustEvent::CAUSE_SPRINTING); - }else{ - $this->exhaust(0.01 * $distance, PlayerExhaustEvent::CAUSE_WALKING); - } - } + if($ev->isCancelled()){ + $this->revertMovement($from); + return; } - } - if($revert){ + if($to->distanceSquared($ev->getTo()) > 0.01){ //If plugins modify the destination + $this->teleport($ev->getTo()); + return; + } - $this->lastX = $from->x; - $this->lastY = $from->y; - $this->lastZ = $from->z; + $this->broadcastMovement(); - $this->lastYaw = $from->yaw; - $this->lastPitch = $from->pitch; + $distance = sqrt((($from->x - $to->x) ** 2) + (($from->z - $to->z) ** 2)); + //TODO: check swimming (adds 0.015 exhaustion in MCPE) + if($this->isSprinting()){ + $this->exhaust(0.1 * $distance, PlayerExhaustEvent::CAUSE_SPRINTING); + }else{ + $this->exhaust(0.01 * $distance, PlayerExhaustEvent::CAUSE_WALKING); + } - $this->setPosition($from); - $this->sendPosition($from, $from->yaw, $from->pitch, MovePlayerPacket::MODE_RESET); - }else{ - if($distanceSquared != 0 and $this->nextChunkOrderRun > 20){ + if($this->nextChunkOrderRun > 20){ $this->nextChunkOrderRun = 20; } } - $this->newPosition = null; + if($exceededRateLimit){ //client and server positions will be out of sync if this happens + $this->server->getLogger()->debug("Player " . $this->getName() . " exceeded movement rate limit, forcing to last accepted position"); + $this->sendPosition($this, $this->yaw, $this->pitch, MovePlayerPacket::MODE_RESET); + } + } + + protected function revertMovement(Location $from) : void{ + $this->lastX = $from->x; + $this->lastY = $from->y; + $this->lastZ = $from->z; + + $this->lastYaw = $from->yaw; + $this->lastPitch = $from->pitch; + + $this->setPosition($from); + $this->sendPosition($from, $from->yaw, $from->pitch, MovePlayerPacket::MODE_RESET); } public function fall(float $fallDistance) : void{ @@ -1703,7 +1724,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $this->timings->startTiming(); if($this->spawned){ - $this->processMovement($tickDiff); + $this->processMostRecentMovements(); $this->motion->x = $this->motion->y = $this->motion->z = 0; //TODO: HACK! (Fixes player knockback being messed up) if($this->onGround){ $this->inAirTicks = 0; @@ -2259,8 +2280,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ public function handleMovePlayer(MovePlayerPacket $packet) : bool{ $newPos = $packet->position->subtract(0, $this->baseOffset, 0); - if($this->isTeleporting and $newPos->distanceSquared($this) > 1){ //Tolerate up to 1 block to avoid problems with client-sided physics when spawning in blocks - $this->sendPosition($this, null, null, MovePlayerPacket::MODE_RESET); + if($this->forceMoveSync !== null and $newPos->distanceSquared($this->forceMoveSync) > 1){ //Tolerate up to 1 block to avoid problems with client-sided physics when spawning in blocks $this->server->getLogger()->debug("Got outdated pre-teleport movement from " . $this->getName() . ", received " . $newPos . ", expected " . $this->asVector3()); //Still getting movements from before teleport, ignore them }elseif((!$this->isAlive() or !$this->spawned) and $newPos->distanceSquared($this) > 0.01){ @@ -2268,9 +2288,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $this->server->getLogger()->debug("Reverted movement of " . $this->getName() . " due to not alive or not spawned, received " . $newPos . ", locked at " . $this->asVector3()); }else{ // Once we get a movement within a reasonable distance, treat it as a teleport ACK and remove position lock - if($this->isTeleporting){ - $this->isTeleporting = false; - } + $this->forceMoveSync = null; $packet->yaw = fmod($packet->yaw, 360); $packet->pitch = fmod($packet->pitch, 360); @@ -2280,7 +2298,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ } $this->setRotation($packet->yaw, $packet->pitch); - $this->newPosition = $newPos; + $this->handleMovement($newPos); } return true; @@ -3794,12 +3812,14 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $pk->onGround = $this->onGround; if($targets !== null){ + if(in_array($this, $targets, true)){ + $this->forceMoveSync = $pos->asVector3(); + } $this->server->broadcastPacket($targets, $pk); }else{ + $this->forceMoveSync = $pos->asVector3(); $this->dataPacket($pk); } - - $this->newPosition = null; } /** @@ -3817,11 +3837,8 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{ $this->resetFallDistance(); $this->nextChunkOrderRun = 0; - $this->newPosition = null; $this->stopSleep(); - $this->isTeleporting = true; - //TODO: workaround for player last pos not getting updated //Entity::updateMovement() normally handles this, but it's overridden with an empty function in Player $this->resetLastMovements(); From 484557935e8fd72e6ca5d8332193d644ef185cbc Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 31 May 2020 16:06:48 +0100 Subject: [PATCH 2/2] Level: remove dead block placement code (player movements are now always processed immediately, just not immediately broadcasted) --- src/pocketmine/level/Level.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/pocketmine/level/Level.php b/src/pocketmine/level/Level.php index 61c2c7ffc..03b5b29b7 100644 --- a/src/pocketmine/level/Level.php +++ b/src/pocketmine/level/Level.php @@ -1897,15 +1897,6 @@ class Level implements ChunkManager, Metadatable{ if(count($this->getCollidingEntities($collisionBox)) > 0){ return false; //Entity in block } - - if($player !== null){ - if(($diff = $player->getNextPosition()->subtract($player->getPosition())) and $diff->lengthSquared() > 0.00001){ - $bb = $player->getBoundingBox()->offsetCopy($diff->x, $diff->y, $diff->z); - if($collisionBox->intersectsWith($bb)){ - return false; //Inside player BB - } - } - } } }