Pocketmine-mp: Movement reversion does not revert PlayerMoveEvent::getFrom() value

[Muqsit]

Issue description

PlayerMoveEvent::getFrom() does not consider player movements getting reverted (due to covering large distances at once, or attempting to enter unloaded terrain): https://github.com/pmmp/PocketMine-MP/blob/8d5cc9adc3b881f6960ffe725aa3d14b85be2c52/src/player/Player.php#L1162

• Expected result: What were you expecting to happen?
  If a player were to move from point A(0, 0, 0) to point B(2, 0, 0) to point C(13, 0, 0) at once, their movement between points B and C would be reverted due to excessive movement, and the next PlayerMoveEvent to fire would have getFrom() return point A and getTo() return point B.
• Actual result: What actually happened?
  The next PlayerMoveEvent to fire has getFrom() return point B and getTo() return point B as well.

Steps to reproduce the issue

To move from a point A to point B without having PlayerMoveEvent notify of movement happening between point A and point B:

1. Move to point B
2. Move to point B + 11 (this movement will be reverted)
3. Server fires PlayerMoveEvent(from: B, to: B)

Through code:

$a = $player->getPosition();

/** @var Vector3 $b */
$player->handleMovement($b);
$player->handleMovement($b->add(11, 0, 0));
// server fires PlayerMoveEvent(from: $b, to: $b) instead of PlayerMoveEvent(from: $a, to: $b)

OS and versions

• PocketMine-MP: 3907ae6726fd9f46914832d2608cdb9f5617667d-dirty
• PHP: 7.4.14
• Server OS: Ubuntu 20.04.1 LTS
• Game version: Win10

Plugins

• If the issue is not reproducible without plugins:
  
  
  
  • Can you provide sample, minimal reproducing code for the issue? If so, paste it in the bottom section
    
    A script plugin was involved in testing this bug.
    
    https://gist.github.com/Muqsit/f23647a2c122aaa1d7c3b2f2717dbd01
    
    Example usage: run /pmebp at x=0 while facing a block at x=5. The next PlayerMoveEvent data to be var_dump-ed will show PlayerMoveEvent::getFrom() = (x=5) and PlayerMoveEvent::getTo() = (x=5)
  
  

[dktapps]

This is likely a problem on PM3 also.

[dktapps]

This is a rat's nest of problems which stem from abuse of Entity->lastLocation, which actually has a specialized purpose despite its very generic looking name.

[dktapps]

It looks like there are some bugs with from/to during PlayerMoveEvent during teleportation also, because syncing Player position doesn't sync lastLocation. The first movement after PlayerMoveEvent will report from as being the pre-teleport position and the to as being the actual new position, when it should actually be from = teleport location and to = teleport location +/- a small distance. This might have caused unexpected behaviour in anti-cheat plugins. never mind, seems like I already found and hacked around this several years ago: https://github.com/pmmp/pocketmine-mp/blob/8d5cc9adc3b881f6960ffe725aa3d14b85be2c52/src/player/Player.php#L2229

lastLocation, despite its generic name, has a specific purpose: it tracks what the most recently sent position of the entity was, so that updateMovement() can avoid sending position updates if the entity moved by only a small distance.

Since Player doesn't use this mechanism (it uses the diff between the last received position and the current one to decide whether movement should be sent or not, which, now that I think about it, is basically the same as the one used by the entity base ..............), it abuses these fields to store something else: the last position reported to PlayerMoveEvent. However, those fields also get reset during movement reverts, creating inconsistencies.

It's not obvious to me why the field is reset during movement reverts, since in any case they are not updated anywhere except Player->processMostRecentMovements() and Entity->updateMovement(). Therefore overwriting them for movement reverts seems entirely redundant.