-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Command Warmups #6332
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.
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
base: 2.x
Are you sure you want to change the base?
Command Warmups #6332
Changes from all commits
67b1a56
ea3aae5
bcbf86a
5c57964
652f672
c4d0821
df0dae4
8540399
8922378
e061363
ec84c82
4832d09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,119 @@ | ||||||||||||||||||||||||||||||||||||
| package com.earth2me.essentials; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import net.ess3.api.IEssentials; | ||||||||||||||||||||||||||||||||||||
| import net.ess3.api.IUser; | ||||||||||||||||||||||||||||||||||||
| import org.bukkit.Bukkit; | ||||||||||||||||||||||||||||||||||||
| import org.bukkit.Location; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import java.util.UUID; | ||||||||||||||||||||||||||||||||||||
| import java.util.regex.Pattern; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| public class AsyncTimedCommand implements Runnable { | ||||||||||||||||||||||||||||||||||||
| private static final double MOVE_CONSTANT = 0.3; | ||||||||||||||||||||||||||||||||||||
| private final IUser commandUser; | ||||||||||||||||||||||||||||||||||||
| private final IEssentials ess; | ||||||||||||||||||||||||||||||||||||
| private final UUID timer_userId; | ||||||||||||||||||||||||||||||||||||
| private final long timer_started; | ||||||||||||||||||||||||||||||||||||
| private final long timer_delay; | ||||||||||||||||||||||||||||||||||||
| private final long timer_initX; | ||||||||||||||||||||||||||||||||||||
| private final long timer_initY; | ||||||||||||||||||||||||||||||||||||
| private final long timer_initZ; | ||||||||||||||||||||||||||||||||||||
| private final String timer_command; | ||||||||||||||||||||||||||||||||||||
| private final Pattern timer_pattern; | ||||||||||||||||||||||||||||||||||||
| private final boolean timer_canMove; | ||||||||||||||||||||||||||||||||||||
| private int timer_task; | ||||||||||||||||||||||||||||||||||||
| private double timer_health; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| private double timer_health; | |
| private volatile double timer_health; |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor accesses user.getBase().getLocation().getX() (and Y, Z) without first checking if the location is null. If a player's location is null at construction time (e.g., during server startup edge cases), this will throw a NullPointerException. Consider adding a null check before accessing location coordinates, similar to the check in the run() method at line 46.
| this.timer_initX = Math.round(user.getBase().getLocation().getX() * MOVE_CONSTANT); | |
| this.timer_initY = Math.round(user.getBase().getLocation().getY() * MOVE_CONSTANT); | |
| this.timer_initZ = Math.round(user.getBase().getLocation().getZ() * MOVE_CONSTANT); | |
| Location initLocation = user.getBase().getLocation(); | |
| if (initLocation == null) { | |
| // Defensive: set to zero if location is null (could also throw or cancel) | |
| this.timer_initX = 0; | |
| this.timer_initY = 0; | |
| this.timer_initZ = 0; | |
| } else { | |
| this.timer_initX = Math.round(initLocation.getX() * MOVE_CONSTANT); | |
| this.timer_initY = Math.round(initLocation.getY() * MOVE_CONSTANT); | |
| this.timer_initZ = Math.round(initLocation.getZ() * MOVE_CONSTANT); | |
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions "Execute as server command" but the code executes it as the player using Bukkit.dispatchCommand(user.getBase(), ...). This doesn't bypass permission checks as the comment suggests - the command is still executed as the player. If the intent is to bypass warmup checks, this is handled by clearing the warmup first, but the comment is misleading.
| // Execute as server command to bypass the warmup check | |
| // Execute the command as the player (not as server); warmup check is bypassed because it was cleared above |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warmup is cleared on line 84, but the message "commandWarmupComplete" is sent on line 92 after scheduling the command. If an exception occurs during command execution (line 87-90), the user will still see the "complete" message even though the command may have failed. Consider moving the success message inside the lambda on line 87, or handling exceptions more gracefully.
| // Execute as server command to bypass the warmup check | |
| Bukkit.dispatchCommand(user.getBase(), timer_command.substring(1)); // Remove the leading '/' | |
| }); | |
| user.sendTl("commandWarmupComplete"); | |
| } catch (final Exception ex) { | |
| ess.showError(user.getSource(), ex, "\\ command warmup"); | |
| } | |
| try { | |
| // Execute as server command to bypass the warmup check | |
| Bukkit.dispatchCommand(user.getBase(), timer_command.substring(1)); // Remove the leading '/' | |
| user.sendTl("commandWarmupComplete"); | |
| } catch (final Exception ex) { | |
| ess.showError(user.getSource(), ex, "\\ command warmup"); | |
| } | |
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -868,6 +868,66 @@ public void handlePlayerCommandPreprocess(final PlayerCommandPreprocessEvent eve | |||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Command warmups | ||||||||||||||||||||||||||||||||||||||
| if (ess.getSettings().isDebug()) { | ||||||||||||||||||||||||||||||||||||||
| ess.getLogger().info("Warmup check - enabled: " + ess.getSettings().isCommandWarmupsEnabled() | ||||||||||||||||||||||||||||||||||||||
| + ", bypass: " + user.isAuthorized("essentials.commandwarmups.bypass") | ||||||||||||||||||||||||||||||||||||||
| + ", command: " + effectiveCommand); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if (ess.getSettings().isCommandWarmupsEnabled() | ||||||||||||||||||||||||||||||||||||||
| && !user.isAuthorized("essentials.commandwarmups.bypass") | ||||||||||||||||||||||||||||||||||||||
| && (pluginCommand == null || !user.isAuthorized("essentials.commandwarmups.bypass." + pluginCommand.getName()))) { | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+873
to
+881
|
||||||||||||||||||||||||||||||||||||||
| if (ess.getSettings().isDebug()) { | |
| ess.getLogger().info("Warmup check - enabled: " + ess.getSettings().isCommandWarmupsEnabled() | |
| + ", bypass: " + user.isAuthorized("essentials.commandwarmups.bypass") | |
| + ", command: " + effectiveCommand); | |
| } | |
| if (ess.getSettings().isCommandWarmupsEnabled() | |
| && !user.isAuthorized("essentials.commandwarmups.bypass") | |
| && (pluginCommand == null || !user.isAuthorized("essentials.commandwarmups.bypass." + pluginCommand.getName()))) { | |
| if (ess.getSettings().isCommandWarmupsEnabled() | |
| && !user.isAuthorized("essentials.commandwarmups.bypass") | |
| && (pluginCommand == null || !user.isAuthorized("essentials.commandwarmups.bypass." + pluginCommand.getName()))) { | |
| if (ess.getSettings().isDebug()) { | |
| ess.getLogger().info("Warmup check - enabled: " + ess.getSettings().isCommandWarmupsEnabled() | |
| + ", bypass: " + user.isAuthorized("essentials.commandwarmups.bypass") | |
| + ", command: " + effectiveCommand); | |
| } |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This code block for calculating args and fullCommand is duplicated from the command cooldowns section above (lines 836-839). Consider extracting this logic to avoid duplication and ensure consistency if the logic needs to change.
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -134,6 +134,7 @@ public class Settings implements net.ess3.api.ISettings { | |||||||||||
| private boolean isCustomNewUsernameMessage; | ||||||||||||
| private List<String> spawnOnJoinGroups; | ||||||||||||
| private Map<Pattern, Long> commandCooldowns; | ||||||||||||
| private List<Entry<Pattern, Long>> commandWarmups; // Optimized: List is 19% faster than Map for sequential lookup | ||||||||||||
|
||||||||||||
| private List<Entry<Pattern, Long>> commandWarmups; // Optimized: List is 19% faster than Map for sequential lookup | |
| // Note: List was chosen over Map for commandWarmups due to observed ~19% faster sequential lookup in local profiling | |
| // with fewer than 10 entries. For larger configuration files with many warmup entries, a Map would provide O(1) lookup | |
| // vs O(n) for List. If the number of warmup entries is expected to grow, consider switching to a Map for scalability. | |
| private List<Entry<Pattern, Long>> commandWarmups; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
timer_taskfield is not marked asvolatile, but it's accessed from multiple threads (async timer thread and sync cancellation). This could lead to visibility issues where one thread doesn't see the update from another thread. Consider marking it asvolatileto ensure thread-safe visibility, similar to how it's handled in concurrent scenarios.