-
-
Notifications
You must be signed in to change notification settings - Fork 9
GH-266 New features for Trident and Pearls #303
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: master
Are you sure you want to change the base?
GH-266 New features for Trident and Pearls #303
Conversation
Summary of ChangesHello @Pawelusze, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the combat system by introducing new, highly configurable mechanics for both Ender Pearls and Tridents. The core addition is the ability to reset a player's combat timer when they throw an Ender Pearl or use a Trident, providing more dynamic and strategic combat scenarios. Furthermore, it establishes a comprehensive set of combat-specific settings for Tridents, including options to restrict their use during combat and implement cooldowns, aligning them with existing pearl-related features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces new features for handling tridents in combat and adds an option to reset the combat timer upon using ender pearls or tridents. The implementation is largely correct, but I've identified a critical issue: the fightTridentService is used without being initialized, which will lead to a NullPointerException. I've also found several copy-paste errors in the new trident-related classes, including incorrect naming and comments that refer to 'pearls' instead of 'tridents'. My review includes suggestions to fix these issues to ensure the code is correct and maintainable. Additionally, I've pointed out an unused import that should be removed.
eternalcombat-plugin/src/main/java/com/eternalcode/combat/CombatPlugin.java
Outdated
Show resolved
Hide resolved
...combat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentController.java
Outdated
Show resolved
Hide resolved
...ombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentServiceImpl.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlController.java
Outdated
Show resolved
Hide resolved
...ombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentServiceImpl.java
Outdated
Show resolved
Hide resolved
...combat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentController.java
Outdated
Show resolved
Hide resolved
CitralFlo
left a comment
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.
Resolve reviews and the Riptide issue mentioned by Jakubek
...ombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentServiceImpl.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/pearl/FightPearlSettings.java
Outdated
Show resolved
Hide resolved
igoyek
left a comment
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.
Follow friend's instructions :)
f4bdbb7 to
e48a01f
Compare
|
Add right-click check to inflict the cooldown only on throw, not on melee attack |
eternalcombat-plugin/src/main/java/com/eternalcode/combat/CombatPlugin.java
Outdated
Show resolved
Hide resolved
|
Also closes: #296 |
36cf3cb to
ab4e804
Compare
...alcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentSettings.java
Outdated
Show resolved
Hide resolved
...combat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentController.java
Outdated
Show resolved
Hide resolved
eternalcombat-plugin/src/main/java/com/eternalcode/combat/fight/trident/TridentController.java
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| if (this.pluginConfig.trident.tridentRiptideDisabledDuringCombat) { |
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.
this could be higher before isInCombat - when the option is disable we dont need to check if the players is in combat
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.
Exactly, so why is this marked as resolved? #303 (comment)
e41590e to
68fb2ef
Compare
CitralFlo
left a comment
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.
Would be cool if @Rollczi could take a look at reloading the plugin - cache settings thing. Ref needed but using supplier in my case did not work.
|
@Jakubk15 re-review requested due to massive changes |
Jakubk15
left a comment
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.
https://streamable.com/prq6pq Trident cooldown still applies when leaving combat. Up to you if you want to clear it after combat tag has passed.
| import java.time.Instant; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public class Delay<T> { |
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.
How about we wait until GH-76/EternalCodeCommons is merged first?
| return; | ||
| } | ||
|
|
||
| if (this.pluginConfig.trident.tridentRiptideDisabledDuringCombat) { |
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.
Exactly, so why is this marked as resolved? #303 (comment)
| if (this.tridentService.hasDelay(uniqueId)) { | ||
| Duration remainingDelay = this.tridentService.getRemainingDelay(uniqueId); | ||
|
|
||
| this.noticeService.create() | ||
| .player(uniqueId) | ||
| .notice(this.pluginConfig.trident.tridentRiptideOnCooldown) | ||
| .placeholder("{TIME}", DurationUtil.format(remainingDelay)) | ||
| .send(); | ||
|
|
||
| return; | ||
| } |
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.
Is this code reachable? Is it possible to somehow bypass the vanilla delay? Do cheat clients have that feature? Looks like dead code to me, but feel free to reproduce.
| if (this.pluginConfig.trident.tridentRiptideDelay.isZero()) { | ||
| return; | ||
| } |
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.
Move this to the top perhaps?
| public boolean riptideResetsTimerEnabled = false; | ||
|
|
||
| @Comment({ | ||
| "# Should riptide enchantment be on cooldown?", |
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.
| "# Should riptide enchantment be on cooldown?", | |
| "# Should riptide enchantment be on cooldown during combat?", |
| }) | ||
| public Duration tridentRiptideDelay = Duration.ofSeconds(10); |
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.
what if the value is negative? Specify this also in the comment
| @Comment({ | ||
| "# Message sent to the player when riptide is on cooldown", | ||
| "# Available placeholder: {TIME} - remaining time left to use riptide again" | ||
| }) | ||
| public Notice tridentRiptideOnCooldown = BukkitNotice.builder() | ||
| .chat("<red>You must wait {TIME} before next riptide!") | ||
| .build(); | ||
| } |
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.
potential dead code
|
|
||
| if (event.getFrom().distanceSquared(event.getTo()) == 0) { | ||
| return; | ||
| } |
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.
Can we maybe improve this check?
2026-02-08.11-51-08.mp4
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.
I saw it once! And couldn't reproduce it, do You have any tip how?
I've added new options to Pearl and Trident that reset the timer when the player uses them.