[codex] Fix combat runtime issues#393
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several bug fixes, scheduler adjustments, and compatibility improvements, including a reflection-based implementation for handling entity mount events across different server platforms. Key feedback focuses on ensuring items are dropped at the correct respawn location during player respawn, using the correct max health attribute value that accounts for active modifiers, and caching the reflected getMount method in KnockbackRegionController to avoid performance overhead on every event execution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| HashMap<Integer, ItemStack> leftover = playerInventory.addItem(itemsToGive); | ||
| leftover.values().forEach(item -> player.getWorld().dropItemNaturally(player.getLocation(), item)); |
There was a problem hiding this comment.
In PlayerRespawnEvent, the player is in the process of respawning and may not be fully teleported to their respawn location yet. Using player.getLocation() and player.getWorld() can cause leftover items to drop at their death location or an incorrect world (e.g., if they respawn in a different world). Use event.getRespawnLocation() and its corresponding world to ensure items are dropped at the actual respawn location.
| HashMap<Integer, ItemStack> leftover = playerInventory.addItem(itemsToGive); | |
| leftover.values().forEach(item -> player.getWorld().dropItemNaturally(player.getLocation(), item)); | |
| HashMap<Integer, ItemStack> leftover = playerInventory.addItem(itemsToGive); | |
| leftover.values().forEach(item -> event.getRespawnLocation().getWorld().dropItemNaturally(event.getRespawnLocation(), item)); |
| int dropPercent = MathUtil.clamp(100 - percentHealth, this.settings.playersHealthPercentClamp, 100); | ||
| int keepPercent = 100 - dropPercent; |
There was a problem hiding this comment.
Using player.getAttribute(Attribute.GENERIC_MAX_HEALTH).getBaseValue() retrieves the base maximum health (usually 20.0), which ignores any active modifiers such as health boost effects, absorption, or attribute modifiers from equipment. Since logout.health() represents the player's actual health at logout (which can exceed the base max health), this can result in incorrect percentage calculations (e.g., greater than 100%). Use getValue() instead of getBaseValue(), and safely handle potential null values.
| int dropPercent = MathUtil.clamp(100 - percentHealth, this.settings.playersHealthPercentClamp, 100); | |
| int keepPercent = 100 - dropPercent; | |
| double correctedMaxHealth = Optional.ofNullable(player.getAttribute(org.bukkit.attribute.Attribute.GENERIC_MAX_HEALTH)) | |
| .map(org.bukkit.attribute.AttributeInstance::getValue) | |
| .orElse(20.0); | |
| int dropPercent = MathUtil.clamp(100 - MathUtil.getRoundedCountPercentage(health, correctedMaxHealth), this.settings.playersHealthPercentClamp, 100); | |
| int keepPercent = 100 - dropPercent; |
| private static final String[] ENTITY_MOUNT_EVENT_CLASSES = { | ||
| "org.bukkit.event.entity.EntityMountEvent", | ||
| "org.spigotmc.event.entity.EntityMountEvent" | ||
| }; |
There was a problem hiding this comment.
To avoid performing expensive reflection lookups (getMethod) on every single EntityMountEvent execution, we should resolve and cache the getMount method once during initialization.
| private static final String[] ENTITY_MOUNT_EVENT_CLASSES = { | |
| "org.bukkit.event.entity.EntityMountEvent", | |
| "org.spigotmc.event.entity.EntityMountEvent" | |
| }; | |
| private static final String[] ENTITY_MOUNT_EVENT_CLASSES = { | |
| "org.bukkit.event.entity.EntityMountEvent", | |
| "org.spigotmc.event.entity.EntityMountEvent" | |
| }; | |
| private Method getMountMethod; |
| private void registerEntityMountEvent(Plugin plugin) { | ||
| Optional<Class<? extends Event>> eventClass = this.findEntityMountEventClass(); | ||
| if (eventClass.isEmpty()) { | ||
| plugin.getLogger().fine("EntityMountEvent is not available. Mount region protection will be disabled."); | ||
| return; | ||
| } | ||
|
|
||
| EventExecutor executor = this::onEntityMount; | ||
| plugin.getServer().getPluginManager().registerEvent(eventClass.get(), this, EventPriority.HIGHEST, executor, plugin, true); | ||
| } |
There was a problem hiding this comment.
Resolve and cache the getMount method during event registration so it can be reused efficiently.
private void registerEntityMountEvent(Plugin plugin) {
Optional<Class<? extends Event>> eventClass = this.findEntityMountEventClass();
if (eventClass.isEmpty()) {
plugin.getLogger().fine("EntityMountEvent is not available. Mount region protection will be disabled.");
return;
}
try {
this.getMountMethod = eventClass.get().getMethod("getMount");
} catch (NoSuchMethodException exception) {
plugin.getLogger().warning("Failed to find getMount method on " + eventClass.get().getName());
return;
}
EventExecutor executor = this::onEntityMount;
plugin.getServer().getPluginManager().registerEvent(eventClass.get(), this, EventPriority.HIGHEST, executor, plugin, true);
}| private Entity getMount(Event event) { | ||
| try { | ||
| Method getMount = event.getClass().getMethod("getMount"); | ||
| Object mount = getMount.invoke(event); | ||
|
|
||
| if (mount instanceof Entity entity) { | ||
| return entity; | ||
| } | ||
|
|
||
| return null; | ||
| } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException exception) { | ||
| throw new IllegalStateException("Cannot read mount from " + event.getClass().getName(), exception); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the cached getMountMethod to invoke the method. Additionally, handle any reflection exceptions gracefully by returning null instead of throwing an IllegalStateException which would crash the event handler and spam the console.
| private Entity getMount(Event event) { | |
| try { | |
| Method getMount = event.getClass().getMethod("getMount"); | |
| Object mount = getMount.invoke(event); | |
| if (mount instanceof Entity entity) { | |
| return entity; | |
| } | |
| return null; | |
| } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException exception) { | |
| throw new IllegalStateException("Cannot read mount from " + event.getClass().getName(), exception); | |
| } | |
| } | |
| private Entity getMount(Event event) { | |
| if (this.getMountMethod == null) { | |
| return null; | |
| } | |
| try { | |
| Object mount = this.getMountMethod.invoke(event); | |
| if (mount instanceof Entity entity) { | |
| return entity; | |
| } | |
| return null; | |
| } catch (IllegalAccessException | InvocationTargetException exception) { | |
| return null; | |
| } | |
| } |
Summary
Fixes several combat runtime and logic issues found during review:
PlaceBlockBlockerregistrationheadDropOnlyInCombatbefore head-drop chance rollsMONITORpriorityRoot Cause
The plugin mixed API-version-specific event classes, async callbacks, and Bukkit state access in several listener paths. A few drop calculations also treated the percentage to drop as the percentage to keep/remove.
Validation
:eternalcombat-plugin:compileJavagradle checkNote: the project currently has no test sources, so Gradle reports
test NO-SOURCE.