Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request refactors NPC movement and rotation mechanics. Changes include physics-based vertical movement (gravity, jumping, ground detection) in path following, persistent yaw tracking in paths, improved waypoint detection using horizontal distance with bounded Y delta, and a shift from packet-based head rotation to entity-based rotation. Hologram positioning now uses entity parenting instead of manual Y-offset teleportation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.java`:
- Around line 250-256: The head rotation currently always uses
npc.getPath().getYaw() which ignores npc.getOptions().isLookAtPath(); update the
call to entity.rotateHead(...) in NPCMovement to use the same conditional as the
body teleport (i.e. choose npc.getPath().getYaw() when
npc.getOptions().isLookAtPath() is true, otherwise use current.getYaw()) so the
head honors the isLookAtPath() option; keep the same second argument (0) as
before.
In `@spaceNPC/src/main/java/me/tofaa/entitylib/npc/path/NPCPath.java`:
- Around line 72-85: The cached yaw is only recomputed in advanceToNext(),
causing processPathFollowing() to read a stale or default yaw after reset() or
setIndex(); update the code so that whenever the active waypoint index changes
(including in reset(), setIndex(), and any method that directly sets
currentIndex) you recompute this.yaw using the same calculation currently in
advanceToNext() (use currentIndex, previousIndex/currentWaypoint,
waypoints.get(currentIndex) and Math.atan2 to compute degrees) so getYaw()
returns the correct heading before the first advance; keep the existing
advanceToNext() logic but extract or duplicate the yaw-compute step into a
shared helper invoked whenever the active waypoint is set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f96dd48-7382-4f74-bcc9-73993f592054
📒 Files selected for processing (2)
spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.javaspaceNPC/src/main/java/me/tofaa/entitylib/npc/path/NPCPath.java
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.java`:
- Around line 94-96: The hologram sync currently calls npc.getEntity().get()
again inside the npc.getHologram().ifPresent(...) lambda, risking a
NoSuchElementException if the Optional changes; capture the entity Optional once
(e.g., via a local variable like entityOpt or entity) before the
hologram.ifPresent and use that local reference when calling
hologram.setParent(...) so the code uses the previously read entity instead of
re-fetching npc.getEntity().
- Around line 215-262: The async path processing is calling thread-unsafe Bukkit
APIs (world.getBlockAt, entity.teleport, entity.rotateHead,
Bukkit.getOnlinePlayers, player.getLocation) from methods like
processPathFollowing, processAllNPCHeadRotation, and processViewerSync; change
the code to perform only thread-safe computations asynchronously and move any
Bukkit API interactions to the main server thread by scheduling synchronous
tasks (e.g., using the BukkitScheduler runTask/submit to main) for: block/world
queries (replace direct world.getBlockAt calls with a sync-block that reads
needed block/passable data), entity operations (teleport, rotateHead,
follower.jump/land/setOnGround and verticalVelocity writes), and viewer
iteration (Bukkit.getOnlinePlayers and player.getLocation). Identify and wrap
the sections around world.getBlockAt,
follower.jump/applyVerticalPhysics/land/setOnGround/verticalVelocity,
entity.teleport, entity.rotateHead, and any player location reads so they
execute on the main thread while keeping path math async.
- Around line 264-269: The vertical (Y) interpolation uses fullLen that was
computed after dx/dz were normalized, so Y stepping is no longer true 3D
interpolation; in NPCMovement fix this by computing the 3D distance using the
original deltas (capture origDx = target.getX() - current.getX() and origDz =
target.getZ() - current.getZ(), keep dy = target.getY() - current.getY()), then
compute fullLen3D = Math.sqrt(origDx*origDx + dy*dy + origDz*origDz) and use
fullLen3D when calculating newY (newY = current.getY() + (dy / fullLen3D) *
speed), leaving the X/Z normalization and movement logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59791095-4409-43af-a31c-51832c4d515f
📒 Files selected for processing (3)
spaceNPC/build.gradle.ktsspaceNPC/src/main/java/me/tofaa/entitylib/npc/NPC.javaspaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.java
Comment on lines
+215
to
+262
| if (npc.getOptions().isClampToGround()) { | ||
| var world = npc.getWorld(); | ||
| if (world != null) { | ||
| int bx = (int) Math.floor(newX); | ||
| int bz = (int) Math.floor(newZ); | ||
| int feetY = (int) Math.floor(current.getY()); | ||
|
|
||
| // Check if there's a solid block ahead at feet level (obstacle) | ||
| var blockAtFeet = world.getBlockAt(bx, feetY, bz); | ||
| var blockAboveFeet = world.getBlockAt(bx, feetY + 1, bz); | ||
|
|
||
| boolean obstacleAhead = !blockAtFeet.isPassable() | ||
| && blockAboveFeet.isPassable(); | ||
|
|
||
| // If we hit a 1-block obstacle and we're on the ground, jump | ||
| if (obstacleAhead && follower.isOnGround()) { | ||
| follower.jump(); | ||
| } | ||
| } | ||
|
|
||
| Location newLoc = new Location( | ||
| newX, | ||
| newY, | ||
| newZ, | ||
| yaw, | ||
| 0 | ||
| ); | ||
| // Apply vertical physics (gravity + velocity) | ||
| newY = follower.applyVerticalPhysics(current.getY()); | ||
|
|
||
| // Resolve ground collision: find solid ground below new position | ||
| int newFeetY = (int) Math.floor(newY); | ||
| double groundLevel = findGroundLevel(world, bx, newFeetY, bz, feetY + 2); | ||
|
|
||
| entity.teleport(newLoc); | ||
| if (newY <= groundLevel) { | ||
| // Landed on ground | ||
| newY = groundLevel; | ||
| follower.land(groundLevel); | ||
| } else { | ||
| // Still airborne | ||
| follower.setOnGround(false); | ||
| } | ||
|
|
||
| entity.rotateHead(yaw, 0); | ||
| // If jumping into a ceiling (block above head), stop upward velocity | ||
| int headY = (int) Math.floor(newY + 1.8); | ||
| var blockAtHead = world.getBlockAt(bx, headY, bz); | ||
| if (!blockAtHead.isPassable() && follower.verticalVelocity > 0) { | ||
| follower.verticalVelocity = 0; | ||
| } | ||
|
|
||
| // If obstacle is 2+ blocks tall, don't move horizontally into it | ||
| if (!blockAtFeet.isPassable() && !blockAboveFeet.isPassable()) { | ||
| newX = current.getX(); | ||
| newZ = current.getZ(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show async scheduler setup and affected processing methods"
rg -n -C4 'runTaskTimerAsynchronously|processPathFollowing|processViewerSync|processAllNPCHeadRotation' \
spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.java
echo
echo "Show Bukkit world/entity API usage inside these methods"
rg -n -C3 'getBlockAt|Bukkit.getOnlinePlayers|player.getLocation|entity.teleport|rotateHead' \
spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.javaRepository: Tofaa2/EntityLib
Length of output: 5668
Thread-unsafe Bukkit operations in async scheduler context.
The processPathFollowing() method (invoked from runTaskTimerAsynchronously) executes multiple non-thread-safe Bukkit API calls: world.getBlockAt() (lines 223–224, 252), entity.teleport() (line 281), and entity.rotateHead() (line 282). Additionally, processAllNPCHeadRotation() and processViewerSync() also call thread-unsafe operations (entity.rotateHead(), Bukkit.getOnlinePlayers(), player.getLocation()) from async tasks. These operations must run on the main thread only and will cause crashes or data corruption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.java` around lines
215 - 262, The async path processing is calling thread-unsafe Bukkit APIs
(world.getBlockAt, entity.teleport, entity.rotateHead, Bukkit.getOnlinePlayers,
player.getLocation) from methods like processPathFollowing,
processAllNPCHeadRotation, and processViewerSync; change the code to perform
only thread-safe computations asynchronously and move any Bukkit API
interactions to the main server thread by scheduling synchronous tasks (e.g.,
using the BukkitScheduler runTask/submit to main) for: block/world queries
(replace direct world.getBlockAt calls with a sync-block that reads needed
block/passable data), entity operations (teleport, rotateHead,
follower.jump/land/setOnGround and verticalVelocity writes), and viewer
iteration (Bukkit.getOnlinePlayers and player.getLocation). Identify and wrap
the sections around world.getBlockAt,
follower.jump/applyVerticalPhysics/land/setOnGround/verticalVelocity,
entity.teleport, entity.rotateHead, and any player location reads so they
execute on the main thread while keeping path math async.
Comment on lines
+264
to
+269
| // No ground clamping: use linear Y interpolation (original behavior) | ||
| double dy = target.getY() - current.getY(); | ||
| double fullLen = Math.sqrt(dx * dx + dy * dy + dz * dz); | ||
| if (fullLen > 0.01) { | ||
| newY = current.getY() + (dy / fullLen) * speed; | ||
| } |
There was a problem hiding this comment.
Non-clamp vertical interpolation currently uses normalized X/Z components.
fullLen is computed after dx/dz may have been normalized, so Y stepping is no longer true 3D interpolation toward the waypoint.
Proposed fix
- double dx = target.getX() - current.getX();
- double dz = target.getZ() - current.getZ();
+ double rawDx = target.getX() - current.getX();
+ double rawDz = target.getZ() - current.getZ();
+ double dx = rawDx;
+ double dz = rawDz;
double hLen = Math.sqrt(dx * dx + dz * dz);
@@
} else {
// No ground clamping: use linear Y interpolation (original behavior)
double dy = target.getY() - current.getY();
- double fullLen = Math.sqrt(dx * dx + dy * dy + dz * dz);
+ double fullLen = Math.sqrt(rawDx * rawDx + dy * dy + rawDz * rawDz);
if (fullLen > 0.01) {
newY = current.getY() + (dy / fullLen) * speed;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spaceNPC/src/main/java/me/tofaa/entitylib/npc/NPCMovement.java` around lines
264 - 269, The vertical (Y) interpolation uses fullLen that was computed after
dx/dz were normalized, so Y stepping is no longer true 3D interpolation; in
NPCMovement fix this by computing the 3D distance using the original deltas
(capture origDx = target.getX() - current.getX() and origDz = target.getZ() -
current.getZ(), keep dy = target.getY() - current.getY()), then compute
fullLen3D = Math.sqrt(origDx*origDx + dy*dy + origDz*origDz) and use fullLen3D
when calculating newY (newY = current.getY() + (dy / fullLen3D) * speed),
leaving the X/Z normalization and movement logic unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
should fix look at path behaviour by using the cached value in NPCPath computed on waypoint change.
Summary by CodeRabbit
New Features
Changes