Ensure destination chunk of movement is loaded regardless of option being set.

[pseudospace0]

Is your feature request related to a problem?

While the option to prevent players from moving into unloaded chunks does exist. I think its reasonable for a single check to made regarding weather or not the chunk which contains the destination of the movement is loaded or not. As I think its agreeable that having a player moving into a chunk which is supposed to loaded but hasn't yet is almost certainly unwanted behavior.

In high latency conditions where this is most likely to occur, as a player personally I would way rather be rubberbanded back to my previous location than be allowed to move into a chunk which the server hasn't been able to load yet.

Describe the solution you'd like.

What I'm proposing is a singular check which is done only on the destination chunk of the movement, regardless of the weather or not the server owner has explicitly turned on prevent-moving-into-unloaded-chunks in the config which will rubberband the player to their previous position on attempt to move into a yet-to-be loaded chunk.

Describe alternatives you've considered.

While the config option to prevent movement into unloaded chunks exists. I believe that regardless of weather a server admin decides to enable this or not, players should at the very least be unable to move into a chunk directly infront of them which is not yet loaded. Hence this feature.

Other

No response

[lynxplay]

I don't see the point here ? The reason this is a config option is explicitly to allow moving into unloaded chunks if wanted.
This issue just asks for this option to be effectively useless ? Server owners that do not want players to move into unloaded chunks can simply enable the configuration option.

Can you please elaborate what exactly the usecase of this issue is beyond just, well, effectively making the configuration option useless and just enabling its behaviour by default without an option to turn it off ?

[pseudospace0]

@lynxplay The config option will take any and all chunks between the clients current position and their final movement position and continue checking them until a chunk is unloaded from my understanding the goal of this behavior is to trade more rubberbanding for a more stable TPS. Some server owners might not want to make this tradeoff and hence leave the option in its default state. What I'm proposing is a singular check done not on all chunks in between but rather only the destination chunk, rubberbanding if its unloaded, what I believe this will remedy is during lag spikes where from the perspective of a player, I believe it would be much better to get rubberbanded back into a loaded chunk than be left hanging in one which is not yet loaded. Basically I see this issue as being aimed primarily at players walking/sprinting through the world as a way to ensure a more playable experience during periods of high latency, whereas I see the config option at being more aimed at benefiting server owners (if they are willing to make the tradeoff), for combating low TPS due to (most likely more late game) players moving at faster speeds (elytra/ boat on ice etc) and hence moving fast into unloaded chunks, possibly at angles which result in traveling over multiple chunks in the same movement packet.

[lynxplay]

I mean, this seems like an edge case right ?
For this to actually have any effect you'd need to

1. be fast enough to skip an entire chunk in a single move packet. Elytra and boats might do this.
2. the chunk you skip into has to be loaded while the chunk you skip has to not be loaded.
3. 1 & 2 need to consistently occur, a single move packet landing you in an unloaded chunk would still rubberband you back.
   At these speeds, it'll still be a noticeable rubberbanding effect if you are fast enough to travel into unloaded chunks, as
   you'd certainly not be consistently hitting the few potentially earlier loaded chunks.

All in all, this "option", which would also certainly need to be configurable and disabled by default, similar to the existing unloaded chunk checks, only prevents rubber-banding for players fast enough to skip an entire chunk in a single move packet and that are lucky enough that the chunk they are landing is loaded faster than the unloaded chunk they skipped and then repeat this already uncommon cycle multiple times, jumping into loaded chunks that are surrounded by unloaded ones.

This would not particularly save CPU time over the existing one either (beyond I presume a few less checks against a Long2Object map). The only time this would really end up saving a lot is in scenarios where the client sends bugus move packets, placing them in chunks far far away from their current chunk. But that obviously does not happen without the implication of malicious clients :)

[pseudospace0]

Yeah I guess it is a bit of an edge case, although I think it raises the question of why we even need to construct an AABB which encompasses all chunks in between the player and destination, if its very unlikely that more than one check is needed to determine if the player lands up in an unloaded chunk, while at the same time opening up an opportunity for players with malicious intent to create lag by spamming out movement packets in chunks far away from their current location, racking up large numbers of checks for the server to perform.

Nevertheless I still think there is room for some sort of check, perhaps different to the one I originally proposed to be performed in order to stop an average player from landing in a chunk which due to server lag, is yet to be loaded even when the server admin has not explicitly enabled this. To clarify, i'm thinking here the difference between a player flying with an elytra (this is where the option might be suitable) and a player just walking around who happens to end up in a chunk which isn't loaded yet (this is where a permanent stoppage could be handy)

[lynxplay]

I mean, I agree with your first block, limiting the full AABB chunk loading search and simply deeming a move packet invalid if the player were to skip too many chunks might be a worth concept to explore, spigot already has the "moved too fast" checks, this could simply live after instead of before said checks.

However, I disagree with an option for this that server owners do not have to enable. Adding checks like that without owners specifically enabling them would break existing setups for no apparent reason. An informed decision about this option is always needed, so having sever administrators opt in instead of opt out seems like the more sensible solution.

[pseudospace0]

Ah okay I understand your points of concern.

Although I am interested in exploring this AABB thing further. Thinking about it more, I would argue that there are more cases where simply checking the destination chunk is better than expanding an AABB, obviously there's the micro optimization of having physically having to do less work, however I believe this also ties into a point I made in this PR - #9717 about having messages logged to server console. If players are maliciously causing lag by purposefully creating a large number of chunks which must be checked, first of all this lag would be mitigated by only doing one check, but furthermore as a server admin, it would be useful to see logs created as a byproduct of this activity ie "Player X moved too quickly!" With the current way things work, someone could spam chunk checks, up until they reach an unloaded chunk, at which point the method would be quietly exited, however with only checking the destination, its more likely with check will fall through and instead be caught by the inevitable logging.

[lynxplay]

I mean, the point of the current check is to prevent what you proposed in this issue: Skipping unloaded chunks.

This is intended behaviour right now and I'd argue it is expected behaviour. Being able to teleport through unloaded chunks when the option to prevent movement through unloaded chunks enabled, seems odd.

The fact that these single move chunk skips exists, is merely a result of the fact that networking exists and we only get these packets in a given interval, not continuously. From a gameplay point of view and a client point of view, the player did move through the chunks, if they end up in a loaded chunk does not matter, they did cross the unloaded ones, even if it was just on the client side.

As stated early, I think you are right that logging potentially malicious packets might be a valid idea, which could simply be done by letting the spigot move-to-quickly checked, currently tested after the chunk test, run first.

That way, a malicious client would always get yielded the same result, (a rubber band back) when they exceed the spigot configured speed limit via some crafted packet content, skipping the entire AABB check.

[pseudospace0]

To me it feels like the placement of this check above all other checks if kind of to act as a short cut out of the other checks, I don't know I could be wrong about this but intuitively it feels like doing a check to see if the chunk the player is moving into is even loaded is an obvious first step to determining if a movement is valid or not. To me it just feels like a check like this is an easy to to circumvent needing to do other checks later down the line, and therefore should be done first. Not to mention that I still feel using AABB is overkill for the majority of legitimate use cases where the client is moving and only realistically needs to be checking if the next chunk they move into is loaded or not.

[lynxplay]

I mean, the first check should just be the distance check imposed by spigots moved to quickly.
The chunk checks are stupidly fast given they evaluated the ticking thread and don't need to acquire locks.

The AABB usecase is certainly more than most people will need when they move a single chunk, but it is not much overhead for it either. Moving a single chunk will realistically only end up with the single check anyway + the few calculations to get there, which is a couple multiplications and two bitshifts.

I guess we can first ensure that the to-be-entered chunk is unloaded via a direct check and, if it is loaded, do the "more expensive" AABB run, which well, would have also rubber banded if the chunk was unloaded in the first place. If that optimisation is worth the time, is another questions, requesting that chunk certainly has to be after the initial spigot distance check anyway so we don't request any chunks the player could not have walked/flown/boat cruised into in the first place 😉

[pseudospace0]

Ah okay I understand (sorry it was quite late last night while writing and I think I slightly misunderstood your proposal) I think what you have suggested offers a good compromise between all the different ideas which have been thrown around here.

[lynxplay]

Closing this, after further thought the performance optimization here is probably not worth spending time on a potential PR.