Offline player tp lag fix + allow getting offline players. #352
Conversation
|
Thank you for opening a pull request to Essence. Someone will review your request shortly. If you don't hear from us within 5 working days, please feel free to ping Lewis or email dev@lewmc.net Thank you! |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Rate limit exceeded@lewmilburn has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPrioritizes live Player data when loading player records (name/IP), adds OfflinePlayer-based nickname resolution, switches Folia scheduling calls to use Changes
Sequence DiagramsequenceDiagram
participant Sender as Command Sender
participant Cmd as CommandTeleport
participant FoliaLib as FoliaLib Scheduler
participant UtilPlayer as UtilPlayer / DataStore
participant MainThread as Main Thread / Teleport Executor
participant WorldMgr as World Manager
Sender->>Cmd: /teleport <offlineName>
Cmd->>FoliaLib: runAsync(load UUID & player data)
FoliaLib->>UtilPlayer: loadPlayer(UUID) (async)
UtilPlayer-->>FoliaLib: player data (location, nickname, world, ip)
FoliaLib->>Cmd: async callback with data
Cmd->>WorldMgr: resolve world by name
WorldMgr-->>Cmd: World instance (or null)
Cmd->>FoliaLib: runAtEntity(...) to schedule teleport on main thread
FoliaLib->>MainThread: execute teleport to location
MainThread-->>Sender: send success/failure message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
lewmilburn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (1)
184-224: Add early return after starting async task.After initiating the asynchronous offline player teleportation at line 186, execution continues to line 223, which immediately sends the "playernotfound" message. This causes the user to receive the error message even when the async task successfully finds and teleports to the offline player.
🔄 Proposed fix
if (permission.has("essence.teleport.offline")) { FoliaLib flib = new FoliaLib(this.plugin); flib.getScheduler().runAsync(task -> { OfflinePlayer offline = getOfflinePlayer(args[0]); if (offline == null || !offline.hasPlayedBefore()) { flib.getScheduler().runAtEntity((Player) cs, t -> message.send("teleport", "offlineplayernodata")); return; } Files opf = new Files(this.plugin.foundryConfig, this.plugin); if (!opf.exists(opf.playerDataFile(offline.getUniqueId()))) { flib.getScheduler().runAtEntity((Player) cs, t -> message.send("teleport", "offlineplayernodata")); return; } opf.load(opf.playerDataFile(offline.getUniqueId())); double x = opf.getDouble("location.last-known.x"); double y = opf.getDouble("location.last-known.y"); double z = opf.getDouble("location.last-known.z"); String worldName = opf.getString("location.last-known.world"); opf.close(); flib.getScheduler().runAtEntity((Player) cs, t -> { World world = Bukkit.getWorld(worldName); if (world == null) { message.send("teleport", "offlinenoworld"); return; } Location offlineLoc = new Location(world, x, y, z); tp.doTeleport((Player) cs, offlineLoc, 0, true); message.send("teleport", "to", new String[]{offline.getName() != null ? offline.getName() : offline.getUniqueId().toString()}); message.send("teleport", "tooffline"); }); }); + return true; } message.send("generic", "playernotfound");
🤖 Fix all issues with AI agents
In @src/main/java/net/lewmc/essence/core/UtilPlayer.java:
- Around line 178-184: In UtilPlayer, the block that reads
p.getAddress().getAddress().getHostAddress() can NPE because p.getAddress() may
be null; update the null-check to verify p.getAddress() and
p.getAddress().getAddress() before calling getHostAddress(), e.g., if p != null
&& p.getAddress() != null && p.getAddress().getAddress() != null then extract
the ip and set f.set(KEYS.USER_IP_ADDRESS.toString(), ip) and
player.user.ipAddress = ip, otherwise fall back to player.user.ipAddress =
f.getString(KEYS.USER_IP_ADDRESS.toString()) as currently done.
- Around line 141-149: The current block misuses Player.getPlayer(), returns
early when a player is offline, and uses confusing naming; fix by obtaining an
OfflinePlayer instead of Player (use Bukkit.getOfflinePlayer(uuid)), rename the
variable (e.g., offline) for clarity, then if offline.isOnline() set Player p =
offline.getPlayer(); otherwise leave p null and proceed to file-based loading
logic so offline data can be loaded; remove any calls to Player.getPlayer() and
ensure subsequent code handles a null p by loading from disk or the existing
offline-data loader.
- Around line 555-563: Replace the unsafe
f.get(KEYS.USER_NICKNAME.toString()).toString() call in UtilPlayer (inside the
OfflinePlayer handling block) with a null-safe retrieval: use
f.getString(KEYS.USER_NICKNAME.toString()) or first check that f.get(...) is
non-null before calling toString(), then close the Files instance and return the
value only if non-null; this prevents the NullPointerException when the nickname
key is absent.
In @src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java:
- Line 183: In CommandTeleport, the permission check is inverted: change the
condition around the permission.has("essence.teleport.offline") call so players
who lack the permission are denied and those who have it are allowed;
specifically, locate the check using permission.has("essence.teleport.offline")
(currently negated) and remove the leading '!' or flip the branching logic so
the branch that denies offline teleport runs when permission.has(...) returns
false and the branch allowing offline teleport runs when it returns true.
🧹 Nitpick comments (2)
src/main/java/net/lewmc/essence/environment/UtilEnvironment.java (2)
175-177: Consider caching the FoliaLib instance.Creating a new
FoliaLibinstance on everysetTime()call could be inefficient. Consider initializing it once in the constructor and reusing it.♻️ Proposed refactor to cache FoliaLib instance
Add a field to cache the FoliaLib instance:
public class UtilEnvironment { private final Plugin plugin; + private final FoliaLib flib; /** * Constructor for UtilEnvironment with plugin instance * @param plugin The plugin instance */ public UtilEnvironment(Plugin plugin) { this.plugin = plugin; + this.flib = plugin != null ? new FoliaLib(plugin) : null; }Then update the
setTimemethod:public boolean setTime(World wo, long t) { if (wo != null) { - if (this.plugin != null) { - FoliaLib flib = new FoliaLib(this.plugin); - if (flib.isFolia()) { - flib.getScheduler().runNextTick(task -> wo.setTime(t)); + if (this.flib != null) { + if (this.flib.isFolia()) { + this.flib.getScheduler().runNextTick(task -> wo.setTime(t)); } else { wo.setTime(t); } } else { wo.setTime(t); } return true; } else { return false; } }
176-177: FoliaLib API usage is correct. ThegetScheduler().runNextTick()pattern is consistent with all other scheduler calls in the codebase. As an optional optimization, consider caching the FoliaLib instance (line 175) rather than creating a new one on each invocation if this method is called frequently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/net/lewmc/essence/core/UtilPlayer.javasrc/main/java/net/lewmc/essence/environment/UtilEnvironment.javasrc/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.javasrc/main/java/net/lewmc/essence/teleportation/tp/UtilTeleport.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/net/lewmc/essence/core/UtilPlayer.java (2)
src/main/java/net/lewmc/essence/core/TypePlayer.java (1)
TypePlayer(10-55)src/main/java/net/lewmc/essence/economy/UtilVaultEconomy.java (1)
Override(143-151)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTprandom.java (3)
checkChunkLoaded(130-158)CommandTprandom(22-167)CommandTprandom(32-36)
🔇 Additional comments (5)
src/main/java/net/lewmc/essence/core/UtilPlayer.java (1)
165-171: Conditional approval: logic is sound but depends on fixing lines 141-149.The logic to prefer the online player's current name over the stored value is correct. However, this depends on properly initializing
pat lines 141-149.Ensure the fix at lines 141-149 is applied before this logic will work correctly.
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (3)
207-219: LGTM! Proper thread-safe teleportation.The callback correctly schedules the teleportation on the entity thread using
runAtEntity, includes a null check for the world, and provides appropriate user feedback.
288-288: LGTM! Documentation typo fixed.
201-204: Data file key migration is properly implemented.The
migratePlayerFilemethod inUtilPlayer.javacorrectly handles the key migration fromlast-location.*tolocation.last-known.*. The migration logic checks for old keys, transfers all coordinate and world data to the new format, and removes old keys after migration. Existing player data files will be properly updated when loaded.src/main/java/net/lewmc/essence/teleportation/tp/UtilTeleport.java (1)
223-223: FoliaLib 0.4.4 API is correctly implemented.The
getScheduler()method is the correct API for FoliaLib v0.4.4. The migration fromflib.getImpl().runAtEntityLater(...)toflib.getScheduler().runAtEntityLater(...)aligns with the current FoliaLib 0.4.4 API and is consistently used throughout the codebase (e.g., inCommandTeleport.java,CommandTprandom.java,UtilEnvironment.java, andEssence.java).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (1)
184-216: Critical: Premature "playernotfound" message and control flow issue.Line 215 executes immediately after the async task starts (line 186), not after completion. This means:
- The "playernotfound" message is ALWAYS sent, even when the offline teleport succeeds
- Users see a confusing "player not found" message followed by a successful teleport
The async block should
return trueto prevent fall-through, and line 215 should only execute when the player is truly not found.🔧 Fix: Restructure control flow
if (permission.has("essence.teleport.offline")) { FoliaLib flib = new FoliaLib(this.plugin); flib.getScheduler().runAsync(task -> { UtilPlayer up = new UtilPlayer(this.plugin); UUID uuid = Bukkit.getOfflinePlayer(args[0]).getUniqueId(); if (!up.loadPlayer(uuid)) { flib.getScheduler().runAtEntity((Player) cs, t -> message.send("teleport", "offlineplayernodata")); return; } double x = (double) up.getPlayer(uuid, UtilPlayer.KEYS.LAST_LOCATION_X); double y = (double) up.getPlayer(uuid, UtilPlayer.KEYS.LAST_LOCATION_Y); double z = (double) up.getPlayer(uuid, UtilPlayer.KEYS.LAST_LOCATION_Z); String worldName = (String) up.getPlayer(uuid, UtilPlayer.KEYS.LAST_LOCATION_WORLD); flib.getScheduler().runAtEntity((Player) cs, t -> { World world = Bukkit.getWorld(worldName); if (world == null) { message.send("teleport", "offlinenoworld"); return; } Location offlineLoc = new Location(world, x, y, z); tp.doTeleport((Player) cs, offlineLoc, 0, true); message.send("teleport", "to", new String[]{up.getPlayer(uuid, UtilPlayer.KEYS.USER_NICKNAME) != null ? up.getPlayer(uuid, UtilPlayer.KEYS.USER_NICKNAME).toString() : uuid.toString()}); message.send("teleport", "tooffline"); }); }); + return true; } message.send("generic", "playernotfound"); return true;
🤖 Fix all issues with AI agents
In @src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java:
- Line 190: In CommandTeleport's async scheduler callbacks (the block using
flib.getScheduler().runAtEntity and similar calls), replace all unsafe casts of
(Player) cs with the already-defined and null-checked player variable to avoid
repeated casting and potential ClassCastExceptions; update each occurrence
(e.g., the call that currently does flib.getScheduler().runAtEntity((Player) cs,
...), and the other instances noted around the same async block) to pass player
instead and ensure any closures capture that variable rather than casting cs.
- Line 183: The permission check in CommandTeleport (variable permission and
method permission.has) is inverted causing users with essence.teleport.offline
to be denied and others allowed; invert the conditional so that it grants the
offline-teleport path when permission.has("essence.teleport.offline") is true
and only sends the "playernotfound" / deny response when it is false, ensuring
the branch that looks up or teleports to offline players is executed under
permission.has("essence.teleport.offline").
- Around line 194-197: The code in CommandTeleport.java is casting values from
up.getPlayer(...) for UtilPlayer.KEYS.LAST_LOCATION_X/Y/Z and
LAST_LOCATION_WORLD directly to double/String which will throw
NullPointerException if any are null; update the teleport logic to fetch each
value into an Object (or use Optional), check for null before casting (or
provide a safe default and log an error), and abort with a user-friendly message
if any coordinate or worldName is missing so you don't cast null into
primitives; specifically revise the retrieval of x, y, z and worldName (the
up.getPlayer calls) to include null checks and handle the failure path safely.
🧹 Nitpick comments (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (1)
209-209: Use last-known name instead of UUID for better UX.When the nickname is unavailable, falling back to the UUID string is not user-friendly. The
USER_LAST_KNOWN_NAMEfromUtilPlayerwould provide a more recognizable identifier.♻️ Improve fallback logic
- message.send("teleport", "to", new String[]{up.getPlayer(uuid, UtilPlayer.KEYS.USER_NICKNAME) != null ? up.getPlayer(uuid, UtilPlayer.KEYS.USER_NICKNAME).toString() : uuid.toString()}); + Object nickname = up.getPlayer(uuid, UtilPlayer.KEYS.USER_NICKNAME); + Object lastKnownName = up.getPlayer(uuid, UtilPlayer.KEYS.USER_LAST_KNOWN_NAME); + String displayName = nickname != null ? nickname.toString() : + (lastKnownName != null ? lastKnownName.toString() : args[0]); + message.send("teleport", "to", new String[]{displayName});
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (2)
src/main/java/net/lewmc/essence/core/UtilMessage.java (1)
UtilMessage(17-186)src/main/java/net/lewmc/essence/core/UtilPlayer.java (1)
UtilPlayer(21-652)
🔇 Additional comments (1)
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java (1)
3-3: LGTM: Import additions support the async offline teleport workflow.The new imports (FoliaLib, UtilPlayer, UUID) are correctly added to support the asynchronous offline player data loading and teleportation.
Also applies to: 7-7, 18-18
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java
Outdated
Show resolved
Hide resolved
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java
Outdated
Show resolved
Hide resolved
src/main/java/net/lewmc/essence/teleportation/tp/CommandTeleport.java
Outdated
Show resolved
Hide resolved
|
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.