Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Optimize handleMove #142

Open
wants to merge 3 commits into
base: v3
Choose a base branch
from
Open

Feature: Optimize handleMove #142

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c3cf62
Optimize handleMove with chunk caching and movement tick skipping
adabugra Jan 25, 2025
61047ef
Merge branch 'juliarn:v3' into feature/optimize-handle-move
adabugra Feb 18, 2025
4efe6b9
Use Chunk#getChunkKey
adabugra Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 59 additions & 2 deletions bukkit/src/main/java/com/github/juliarn/npclib/bukkit/BukkitActionController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,14 @@
import com.github.juliarn.npclib.bukkit.util.BukkitPlatformUtil;
import com.github.juliarn.npclib.common.CommonNpcActionController;
import com.github.juliarn.npclib.common.flag.CommonNpcFlaggedBuilder;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import org.bukkit.Chunk;
import org.bukkit.Location;
import org.bukkit.World;
import org.bukkit.entity.Player;
Expand All @@ -52,13 +57,18 @@
import org.bukkit.event.player.PlayerMoveEvent;
import org.bukkit.event.player.PlayerQuitEvent;
import org.bukkit.event.player.PlayerToggleSneakEvent;
import org.bukkit.event.world.ChunkLoadEvent;
import org.bukkit.event.world.ChunkUnloadEvent;
import org.bukkit.inventory.ItemStack;
import org.bukkit.plugin.Plugin;
import org.jetbrains.annotations.NotNull;

public final class BukkitActionController extends CommonNpcActionController implements Listener {

private final NpcTracker<World, Player, ItemStack, Plugin> npcTracker;
private final Map<World, Set<String>> loadedChunks = new HashMap<>();
private final Map<UUID, Long> playerCooldowns = new HashMap<>();
private static final int COOLDOWN_TICKS = 10;

// based on the given flags
private final int spawnDistance;
Expand Down Expand Up @@ -135,14 +145,31 @@ public void handleMove(@NotNull PlayerMoveEvent event) {
boolean changedWorld = !Objects.equals(from.getWorld(), to.getWorld());
boolean changedOrientation = from.getYaw() != to.getYaw() || from.getPitch() != to.getPitch();
boolean changedPosition = from.getX() != to.getX() || from.getY() != to.getY() || from.getZ() != to.getZ();
boolean significantMovement = Math.abs(to.getX() - from.getX()) > 0.5 || Math.abs(to.getY() - from.getY()) > 0.5 || Math.abs(to.getZ() - from.getZ()) > 0.5;

if (!significantMovement) return;

Player player = event.getPlayer();
UUID playerId = player.getUniqueId();
long currentTick = System.currentTimeMillis() / 50; // Convert current time to ticks

// Cooldown check
Long lastProcessedTick = playerCooldowns.get(playerId);
if (lastProcessedTick != null && currentTick - lastProcessedTick < COOLDOWN_TICKS) {
return; // Skip processing if still within the cooldown period
}
playerCooldowns.put(playerId, currentTick);

// check if any movement happened (event is also called when standing still)
if (changedPosition || changedOrientation || changedWorld) {
Player player = event.getPlayer();
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())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

World npcWorld = npc.world();

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

distance shouldn't be too expensive as it's just some subtraction and multiplication

Copy link
Contributor

@Cryptite Cryptite Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distance method computes a squared distance - it's just not in the name

Copy link
Contributor

@Cryptite Cryptite Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I overlooked that it's your own BukkitPlatformUtil.distance method, not the vanilla Location.distance() method. Nevermind then!

Expand Down Expand Up @@ -170,6 +197,32 @@ public void handleMove(@NotNull PlayerMoveEvent event) {
}
}

@EventHandler
public void onChunkLoad(ChunkLoadEvent event) {
World world = event.getWorld();
Chunk chunk = event.getChunk();

// Add the chunk to the cache
loadedChunks
.computeIfAbsent(world, w -> new HashSet<>())
.add(chunkKey(chunk.getX(), chunk.getZ()));
Copy link
Contributor

@Cryptite Cryptite Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think getChunkKey is an api that is available since 1.8, let alone in spigot/bukkit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}

@EventHandler
public void onChunkUnload(ChunkUnloadEvent event) {
World world = event.getWorld();
Chunk chunk = event.getChunk();

// Remove the chunk from the cache
Set<String> chunks = loadedChunks.get(world);
if (chunks != null) {
chunks.remove(chunkKey(chunk.getX(), chunk.getZ()));
if (chunks.isEmpty()) {
loadedChunks.remove(world); // Clean up if no chunks are left
}
}
}

@EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
public void handleSneak(@NotNull PlayerToggleSneakEvent event) {
Player player = event.getPlayer();
Expand Down Expand Up @@ -216,6 +269,10 @@ public void handleQuit(@NotNull PlayerQuitEvent event) {
}
}

private String chunkKey(int chunkX, int chunkZ) {
return chunkX + "," + chunkZ;
}

private static final class BukkitActionControllerBuilder
extends CommonNpcFlaggedBuilder<NpcActionController.Builder>
implements NpcActionController.Builder {
Expand Down