Feature: Optimize handleMove #142
Conversation
|
Excellent, I too was about to start looking into it as well, as it was often the laggiest method running for me too. |
| // Add the chunk to the cache | ||
| loadedChunks | ||
| .computeIfAbsent(world, w -> new HashSet<>()) | ||
| .add(chunkKey(chunk.getX(), chunk.getZ())); |
There was a problem hiding this comment.
I would advise just using Chunk.getChunkKey() and storing the long this provides instead of a String. It is much more efficient than constructing a string on every single chunk load, unload, and player movement.
There was a problem hiding this comment.
I don't think getChunkKey is an api that is available since 1.8, let alone in spigot/bukkit.
There was a problem hiding this comment.
It's available in the paper-api since I think about 1.12, but all the same, here's the method:
static long getChunkKey(int x, int z) {
return (long)x & 4294967295L | ((long)z & 4294967295L) << 32;
}
| for (Npc<World, Player, ItemStack, Plugin> npc : this.npcTracker.trackedNpcs()) { | ||
| // check if the player is still in the same world as the npc | ||
| Position pos = npc.position(); | ||
| if (!npc.world().equals(player.getWorld()) || !npc.world().isChunkLoaded(pos.chunkX(), pos.chunkZ())) { |
There was a problem hiding this comment.
In all honestly, I think a better call here would be for NPCs to store state as to whether they are in a loaded chunk or not, rather than rely on every single small player movement to check if they are in a loaded chunk.
|
|
||
| // Use cached chunk data to check if the chunk is loaded | ||
| Set<String> loadedChunksInWorld = loadedChunks.get(npcWorld); | ||
| if (loadedChunksInWorld == null || !loadedChunksInWorld.contains(chunkKey(pos.chunkX(), pos.chunkZ()))) { | ||
| // if the player is tracked by the npc, stop that | ||
| npc.stopTrackingPlayer(player); | ||
| continue; |
There was a problem hiding this comment.
(This code isn't visible in this PR, but) there is code below that does: BukkitPlatformUtil.distance(npc, to) too often.
This should only be called when changedPosition or maybe changedWorld is true. If the player changes orientation, their distance to the NPC won't change. This would save on the expensive distance check for players who just turn their heads around.
There was a problem hiding this comment.
distance shouldn't be too expensive as it's just some subtraction and multiplication
There was a problem hiding this comment.
It's less preferable over a pre-calculated distanceSquared for speed (Math.sqrt is a "slow" op), but all the same it's wasteful to do if the only change is orientation, especially since this is extremely hot code.
There was a problem hiding this comment.
The distance method computes a squared distance - it's just not in the name
There was a problem hiding this comment.
Oh, I overlooked that it's your own BukkitPlatformUtil.distance method, not the vanilla Location.distance() method. Nevermind then!
This PR optimizes the handleMove method in the BukkitActionController class by introducing chunk caching and movement tick skipping. These enhancements reduce unnecessary calculations and improves performance.
Tests (~550 online/real players):
Before:
After:
