fixes#3655, fixes#3241
instead of guessing where the end of the transaction is, we attempt validation after every piece of the transaction, with the assumption being that a crafting transaction will not validate until it's complete.
phpstan 0.12.26 starts reporting errors about the result of array_search() being given to some constructor or another because of the lack of key type specification.
while this is slightly less bandwidth efficient (1 in 92 datagrams not full vs 1 in 733), this is significantly less memory-hard.
I made this decision looking at the memory pressures that 1MB chunks exert - especially on RakNet. Client-side, these resource pack chunks all hang around in RakNet memory until the whole thing is received, and it's a lot more costly to receive 733 datagrams than it is to receive 92, especially since it's much more likely that some of the 733 will disappear along the way.
If, for example, the first couple of hundred KB split parts arrived out of 1MB, and then one of the parts got lost, all the already-received parts would hang around in memory not getting processed. With smaller chunks this is much less of a problem.
I explored taking the chunk size all the way down to 1KB to reduce the bandwidth waste caused by split packets (split headers), but this made resource pack downloading unbearably slow, so it wasn't acceptable.
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.
these were not in the usage search because PhpStorm decided to refer to ChunkLoader->getLevel() for any Player references, which caused them to only show when that was searched.
There's also an undetected LSP violation with ChunkLoader because it requires returning Level and Position->getLevel() returns Level|null. I don't know why PHPStan doesn't complain about that.
this allows assuming that a position has a valid world in places where it's never expected to not be valid. Since this is the vast majority of usages, it eliminates a lot of possible null-pointer warnings given by static analysers.
TODO: Consider whether we can make Position->getLevel/World use this behaviour out of the box in the next major version.
