GH-348 Post death command execution #348
+245
−33
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,4 +23,40 @@ public class CommandSettings extends OkaeriConfig { | |
| "tpa", | ||
| "tpaccept" | ||
| ); | ||
|
|
||
| @Comment({ | ||
| "# List of commands that will be executed from console after player death.", | ||
| "# Use {player} to represent the name of the player who died and {killer} for the killer's name (if applicable)." | ||
| }) | ||
| public List<String> consolePostDeathCommands = List.of( | ||
|
Member
There was a problem hiding this comment. Maybe instead of adding few fields for do it in one list? like: killer:say GG
console:broadcast dupa
player:say dupa
Member
Author
There was a problem hiding this comment. This is giving me multification vibes. I think I'm leaning more towards the multiple list approach that @CitralFlo suggested |
||
| "broadcast {player} has died in combat!" | ||
Jakubk15 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ); | ||
|
|
||
| @Comment("# When this is set to true, the plugin will execute the console commands only after the dead player has respawned.") | ||
| public boolean deferConsoleAfterRespawn = false; | ||
|
|
||
| @Comment({ | ||
| "# List of commands that will be executed from the dead player's perspective after death.", | ||
| "# Use {player} to represent the name of the player who died and {killer} for the killer's name (if applicable)." | ||
| }) | ||
| public List<String> deadPostDeathCommands = List.of( | ||
| "say You have died in combat!" | ||
| ); | ||
|
|
||
| @Comment("# When this is set to true, the plugin will execute the commands above only after the dead player has respawned.") | ||
| public boolean deferDeadAfterRespawn = true; | ||
|
|
||
| @Comment({ | ||
| "# List of commands that will be executed from the killer's perspective after killing a player.", | ||
| "# Use {player} to represent the name of the player who was killed and {killer} for the killer's name (if applicable)." | ||
| }) | ||
| public List<String> killerPostDeathCommands = List.of( | ||
| "say You have killed {player} in combat!" | ||
| ); | ||
|
|
||
| @Comment("# When this is set to true, the plugin will only execute the post-death commands if the players were tagged") | ||
| public boolean onlyExecuteIfTagged = true; | ||
|
|
||
| @Comment("# The returned string when the killer is unknown") | ||
|
Comment on lines
+26
to
+60
Member
There was a problem hiding this comment. maybe add options - suggestions for names to be easily distinguishable:
|
||
| public String unknownKillerPlaceholder = "Unknown"; | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,169 @@ | ||||||||||||
| package com.eternalcode.combat.fight.death; | ||||||||||||
|
Member
There was a problem hiding this comment. this class is in fight.death, why settings are in command settings?
Member
Author
There was a problem hiding this comment. Because that class suits the purpose of the feature
Member
There was a problem hiding this comment. nah we have settings regarding death events I think it would be more suitable there |
||||||||||||
|
|
||||||||||||
| import com.eternalcode.combat.config.implementation.PluginConfig; | ||||||||||||
| import com.eternalcode.combat.fight.FightManager; | ||||||||||||
| import com.eternalcode.combat.fight.FightTag; | ||||||||||||
| import com.eternalcode.combat.fight.event.CauseOfUnTag; | ||||||||||||
| import com.eternalcode.combat.fight.event.FightUntagEvent; | ||||||||||||
| import org.bukkit.Server; | ||||||||||||
| import org.bukkit.entity.Player; | ||||||||||||
| import org.bukkit.event.EventHandler; | ||||||||||||
| import org.bukkit.event.EventPriority; | ||||||||||||
| import org.bukkit.event.Listener; | ||||||||||||
| import org.bukkit.event.entity.PlayerDeathEvent; | ||||||||||||
| import org.bukkit.event.player.PlayerRespawnEvent; | ||||||||||||
|
|
||||||||||||
| import java.util.ArrayList; | ||||||||||||
| import java.util.Collections; | ||||||||||||
| import java.util.List; | ||||||||||||
| import java.util.Map; | ||||||||||||
| import java.util.Set; | ||||||||||||
| import java.util.UUID; | ||||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||||
|
|
||||||||||||
| public class DeathCommandController implements Listener { | ||||||||||||
|
Contributor
There was a problem hiding this comment. This class is doing way too much. It handles three different events, decides when commands should run, resolves killer logic, replaces placeholders, dispatches commands, and also manages internal state (pendingCommands, handledByUntag). That’s multiple responsibilities packed into one place, which makes it harder to read and extend. Any change in command rules, killer resolution or execution timing will require touching this class, which is a red flag. I’d split this into:
Right now this feels like a central orchestrator + domain logic + infrastructure mixed together. It works, but it’s tightly coupled and will get messy as the feature grows. |
||||||||||||
|
|
||||||||||||
| private final PluginConfig config; | ||||||||||||
| private final FightManager fightManager; | ||||||||||||
| private final Server server; | ||||||||||||
|
|
||||||||||||
| private final Map<UUID, List<PendingCommand>> pendingCommands = new ConcurrentHashMap<>(); | ||||||||||||
| private final Set<UUID> handledByUntag = Collections.newSetFromMap(new ConcurrentHashMap<>()); | ||||||||||||
|
|
||||||||||||
| public DeathCommandController(PluginConfig config, FightManager fightManager, Server server) { | ||||||||||||
| this.config = config; | ||||||||||||
| this.fightManager = fightManager; | ||||||||||||
| this.server = server; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true) | ||||||||||||
| void onPlayerUntag(FightUntagEvent event) { | ||||||||||||
| UUID playerUUID = event.getPlayer(); | ||||||||||||
| CauseOfUnTag cause = event.getCause(); | ||||||||||||
|
|
||||||||||||
| if (cause != CauseOfUnTag.DEATH && cause != CauseOfUnTag.DEATH_BY_PLAYER) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| Player deadPlayer = this.server.getPlayer(playerUUID); | ||||||||||||
|
|
||||||||||||
| if (deadPlayer == null) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| this.handledByUntag.add(playerUUID); | ||||||||||||
| this.executeDeathCommands(deadPlayer); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true) | ||||||||||||
| void onPlayerDeath(PlayerDeathEvent event) { | ||||||||||||
| if (this.config.commands.onlyExecuteIfTagged) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+60
to
+62
Contributor
There was a problem hiding this comment. The
Member
Author
There was a problem hiding this comment. I tested it and I wasn't able to produce any bugs, however I encourage other team members to try it nevertheless |
||||||||||||
|
|
||||||||||||
| Player deadPlayer = event.getEntity(); | ||||||||||||
| UUID playerUUID = deadPlayer.getUniqueId(); | ||||||||||||
|
|
||||||||||||
| if (this.handledByUntag.remove(playerUUID)) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| this.executeDeathCommands(deadPlayer); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @EventHandler(priority = EventPriority.MONITOR) | ||||||||||||
| void onPlayerRespawn(PlayerRespawnEvent event) { | ||||||||||||
| Player player = event.getPlayer(); | ||||||||||||
| UUID playerUUID = player.getUniqueId(); | ||||||||||||
|
|
||||||||||||
| this.handledByUntag.remove(playerUUID); | ||||||||||||
|
|
||||||||||||
| List<PendingCommand> commands = this.pendingCommands.remove(playerUUID); | ||||||||||||
|
|
||||||||||||
| if (commands == null) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| for (PendingCommand pending : commands) { | ||||||||||||
| switch (pending.executor()) { | ||||||||||||
| case CONSOLE -> this.server.dispatchCommand(this.server.getConsoleSender(), pending.command()); | ||||||||||||
| case DEAD_PLAYER -> this.server.dispatchCommand(player, pending.command()); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private void executeDeathCommands(Player deadPlayer) { | ||||||||||||
|
Member
There was a problem hiding this comment. I feel like post mortem commands do not need to be stored in this method - when it's called in every listener. |
||||||||||||
| UUID playerUUID = deadPlayer.getUniqueId(); | ||||||||||||
| String deadPlayerName = deadPlayer.getName(); | ||||||||||||
| String killerName = this.resolveKillerName(playerUUID, deadPlayer); | ||||||||||||
|
|
||||||||||||
| List<PendingCommand> deferred = new ArrayList<>(); | ||||||||||||
|
|
||||||||||||
| for (String command : this.config.commands.consolePostDeathCommands) { | ||||||||||||
| String resolved = this.replacePlaceholders(command, deadPlayerName, killerName); | ||||||||||||
| if (this.config.commands.deferConsoleAfterRespawn) { | ||||||||||||
| deferred.add(new PendingCommand(CommandSource.CONSOLE, resolved)); | ||||||||||||
| } else { | ||||||||||||
| this.server.dispatchCommand(this.server.getConsoleSender(), resolved); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| for (String command : this.config.commands.deadPostDeathCommands) { | ||||||||||||
| String resolved = this.replacePlaceholders(command, deadPlayerName, killerName); | ||||||||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||||||||
| if (this.config.commands.deferDeadAfterRespawn) { | ||||||||||||
| deferred.add(new PendingCommand(CommandSource.DEAD_PLAYER, resolved)); | ||||||||||||
| } else { | ||||||||||||
| this.server.dispatchCommand(deadPlayer, resolved); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| Player killer = this.resolveKiller(playerUUID, deadPlayer); | ||||||||||||
|
|
||||||||||||
| if (killer != null) { | ||||||||||||
| for (String command : this.config.commands.killerPostDeathCommands) { | ||||||||||||
| String resolved = this.replacePlaceholders(command, deadPlayerName, killerName); | ||||||||||||
| this.server.dispatchCommand(killer, resolved); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (!deferred.isEmpty()) { | ||||||||||||
| this.pendingCommands.put(playerUUID, deferred); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| private String resolveKillerName(UUID deadPlayerUUID, Player deadPlayer) { | ||||||||||||
| Player killer = this.resolveKiller(deadPlayerUUID, deadPlayer); | ||||||||||||
| return killer != null ? killer.getName() : this.config.commands.unknownKillerPlaceholder; | ||||||||||||
|
Comment on lines
+136
to
+137
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private Player resolveKiller(UUID deadPlayerUUID, Player deadPlayer) { | ||||||||||||
| Player killer = deadPlayer.getKiller(); | ||||||||||||
|
|
||||||||||||
| if (killer != null) { | ||||||||||||
| return killer; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| FightTag tag = this.fightManager.getTag(deadPlayerUUID); | ||||||||||||
|
|
||||||||||||
| if (tag != null && tag.getTagger() != null) { | ||||||||||||
| return this.server.getPlayer(tag.getTagger()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return null; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private String replacePlaceholders(String command, String playerName, String killerName) { | ||||||||||||
| return command | ||||||||||||
| .replace("{player}", playerName) | ||||||||||||
| .replace("{killer}", killerName); | ||||||||||||
Jakubk15 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private enum CommandSource { | ||||||||||||
| CONSOLE, | ||||||||||||
| DEAD_PLAYER | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private record PendingCommand(CommandSource executor, String command) { | ||||||||||||
|
Comment on lines
+162
to
+167
Contributor
There was a problem hiding this comment. Why in one class and also private? |
||||||||||||
| } | ||||||||||||
| } | ||||||||||||
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.
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.
?????
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.
for testing 🤷