-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Show resolved
Hide resolved
...combat-plugin/src/main/java/com/eternalcode/combat/fight/trident/FightTridentController.java
Outdated
Show resolved
Hide resolved
| public class FightTridentServiceImpl implements FightTridentService { | ||
| private final FightTridentSettings tridentSettings; | ||
| private final Cache<UUID, Instant> pearlStartTimes; | ||
|
|
||
| public FightTridentServiceImpl(FightTridentSettings pearlSettings) { | ||
| this.tridentSettings = pearlSettings; | ||
| this.pearlStartTimes = Caffeine.newBuilder() | ||
| .expireAfterWrite(pearlSettings.tridentThrowDelay) | ||
| .build(); | ||
| } | ||
|
|
||
| @Override | ||
| public void markDelay(UUID uuid) { | ||
| this.pearlStartTimes.put(uuid, Instant.now()); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean hasDelay(UUID uuid) { | ||
| return this.pearlStartTimes.getIfPresent(uuid) != null; | ||
| } | ||
|
|
||
| @Override | ||
| public Duration getRemainingDelay(UUID uuid) { | ||
| Instant startTime = this.pearlStartTimes.getIfPresent(uuid); | ||
| if (startTime == null) { | ||
| return Duration.ZERO; | ||
| } | ||
|
|
||
| Duration elapsed = Duration.between(startTime, Instant.now()); | ||
| Duration remaining = this.tridentSettings.tridentThrowDelay.minus(elapsed); | ||
|
|
||
| return remaining.isNegative() ? Duration.ZERO : remaining; | ||
| } | ||
|
|
||
| @Override | ||
| public Instant getDelay(UUID uuid) { | ||
| Instant startTime = this.pearlStartTimes.getIfPresent(uuid); | ||
| return startTime != null ? startTime.plus(this.tridentSettings.tridentThrowDelay) : Instant.MIN; | ||
| } | ||
| } |
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 class appears to have been copied from a pearl-related implementation, but some names were not updated. The field pearlStartTimes and constructor parameter pearlSettings should be renamed to tridentStartTimes and tridentSettings respectively to avoid confusion and improve maintainability. I've applied these changes in the suggestion.
public class FightTridentServiceImpl implements FightTridentService {
private final FightTridentSettings tridentSettings;
private final Cache<UUID, Instant> tridentStartTimes;
public FightTridentServiceImpl(FightTridentSettings tridentSettings) {
this.tridentSettings = tridentSettings;
this.tridentStartTimes = Caffeine.newBuilder()
.expireAfterWrite(tridentSettings.tridentThrowDelay)
.build();
}
@Override
public void markDelay(UUID uuid) {
this.tridentStartTimes.put(uuid, Instant.now());
}
@Override
public boolean hasDelay(UUID uuid) {
return this.tridentStartTimes.getIfPresent(uuid) != null;
}
@Override
public Duration getRemainingDelay(UUID uuid) {
Instant startTime = this.tridentStartTimes.getIfPresent(uuid);
if (startTime == null) {
return Duration.ZERO;
}
Duration elapsed = Duration.between(startTime, Instant.now());
Duration remaining = this.tridentSettings.tridentThrowDelay.minus(elapsed);
return remaining.isNegative() ? Duration.ZERO : remaining;
}
@Override
public Instant getDelay(UUID uuid) {
Instant startTime = this.tridentStartTimes.getIfPresent(uuid);
return startTime != null ? startTime.plus(this.tridentSettings.tridentThrowDelay) : Instant.MIN;
}
}| public class FightTridentSettings extends OkaeriConfig { | ||
|
|
||
| @Comment({ | ||
| "# Set true, If you want to disable throwing trident during combat", | ||
| "# This will work globally, but can be overridden by region settings" | ||
| }) | ||
| public boolean tridentThrowDisabledDuringCombat = true; | ||
|
|
||
| @Comment("# Set true, If you want add cooldown to pearls") | ||
| public boolean tridentCooldownEnabled = false; | ||
|
|
||
| @Comment("# Set true, If you want to reset timer when player uses trident") | ||
| public boolean tridentResetsTimerEnabled = true; | ||
|
|
||
| @Comment({ | ||
| "# Block throwing trident with delay?", | ||
| "# If you set this to for example 3s, player will have to wait 3 seconds before throwing another pearl" | ||
| }) | ||
| public Duration tridentThrowDelay = Duration.ofSeconds(3); | ||
|
|
||
| @Comment("# Message sent when player tries to throw trident, but are disabled") | ||
| public Notice tridentThrowBlockedDuringCombat = BukkitNotice.builder() | ||
| .chat("<red>Throwing trident is prohibited during combat!") | ||
| .build(); | ||
|
|
||
| @Comment("# Message sent when player tries to throw ender pearl, but has delay") | ||
| public Notice tridentThrowBlockedDelayDuringCombat = BukkitNotice.builder() | ||
| .chat("<red>You must wait {TIME} before next throw!") | ||
| .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.
Several comments in this file seem to be copied from a pearl-related settings file and were not updated. They incorrectly refer to "pearls" or "ender pearls" instead of "tridents". I've corrected them in the suggestion below.
public class FightTridentSettings extends OkaeriConfig {
@Comment({
"# Set true, If you want to disable throwing trident during combat",
"# This will work globally, but can be overridden by region settings"
})
public boolean tridentThrowDisabledDuringCombat = true;
@Comment("# Set true, If you want add cooldown to tridents")
public boolean tridentCooldownEnabled = false;
@Comment("# Set true, If you want to reset timer when player uses trident")
public boolean tridentResetsTimerEnabled = true;
@Comment({
"# Block throwing trident with delay?",
"# If you set this to for example 3s, player will have to wait 3 seconds before throwing another trident"
})
public Duration tridentThrowDelay = Duration.ofSeconds(3);
@Comment("# Message sent when player tries to throw trident, but are disabled")
public Notice tridentThrowBlockedDuringCombat = BukkitNotice.builder()
.chat("<red>Throwing trident is prohibited during combat!")
.build();
@Comment("# Message sent when player tries to throw trident, but has delay")
public Notice tridentThrowBlockedDelayDuringCombat = BukkitNotice.builder()
.chat("<red>You must wait {TIME} before next throw!")
.build();
}|
|
||
| if (this.settings.pearlResetsTimerEnabled) { | ||
| Duration combatTime = this.config.settings.combatTimerDuration; | ||
| this.fightManager.tag(playerId, combatTime, CauseOfTag.CUSTOM); |
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.fightManager.tag(playerId, combatTime, CauseOfTag.CUSTOM); | |
| this.fightManager.tag(playerId, combatTime, CauseOfTag.NON_PLAYER); |
Refer to the javadocs for CauseOfTag
| public FightTridentServiceImpl(FightTridentSettings pearlSettings) { | ||
| this.tridentSettings = pearlSettings; |
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.
| public FightTridentServiceImpl(FightTridentSettings pearlSettings) { | |
| this.tridentSettings = pearlSettings; | |
| public FightTridentServiceImpl(FightTridentSettings tridentSettings) { | |
| this.tridentSettings = tridentSettings; |
| import java.time.Duration; | ||
| import java.util.UUID; | ||
|
|
||
| public class FightTridentController implements Listener { |
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 functionality of this class does not correspond to what we settled at in issue discussion. Your implementation will prevent throwing all tridents whatsoever, while we want to simply stop throwing tridents with the Riptide enchantment. Requesting changes.
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
| public FightTridentServiceImpl(FightTridentSettings pearlSettings) { | ||
| this.tridentSettings = pearlSettings; | ||
| this.tridentStartTimes = Caffeine.newBuilder() | ||
| .expireAfterWrite(pearlSettings.tridentThrowDelay) |
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.
pearlSettings -> tridentSettings
| .chat("<red>Throwing trident is prohibited during combat!") | ||
| .build(); | ||
|
|
||
| @Comment("# Message sent when player tries to throw trident, but has delay") |
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.
| @Comment("# Message sent when player tries to throw trident, but has delay") | |
| @Comment("# Message marking delay between trident throws") |
| }) | ||
| public Duration tridentThrowDelay = Duration.ofSeconds(3); | ||
|
|
||
| @Comment("# Message sent when player tries to throw trident, but are disabled") |
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.
| @Comment("# Message sent when player tries to throw trident, but are disabled") | |
| @Comment("# Message sent to the player when throwing trident is disabled") |
| "# Block throwing trident with delay?", | ||
| "# If you set this to for example 3s, player will have to wait 3 seconds before throwing trident" |
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.
| "# Block throwing trident with delay?", | |
| "# If you set this to for example 3s, player will have to wait 3 seconds before throwing trident" | |
| "# Should throwing trident be on cooldown?", | |
| "# Setting this option to 3s will make players wait 3 seconds between trident throws" |
| public boolean tridentCooldownEnabled = false; | ||
|
|
||
| @Comment("# Set true, If you want to reset timer when player uses trident") | ||
| public boolean tridentResetsTimerEnabled = true; |
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.
default could be false - meaning throwing trident without pvp will not result in punishment
| @Comment("# Set true, If you want add cooldown to trident") | ||
| public boolean tridentCooldownEnabled = false; | ||
|
|
||
| @Comment("# Set true, If you want to reset timer when player uses trident") |
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.
| @Comment("# Set true, If you want to reset timer when player uses trident") | |
| @Comment("# Set to true, so the users will get combat log when they throw tridents") |
| }) | ||
| public boolean tridentThrowDisabledDuringCombat = true; | ||
|
|
||
| @Comment("# Set true, If you want add cooldown to trident") |
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.
| @Comment("# Set true, If you want add cooldown to trident") | |
| @Comment("# Set to true so throwing trident will result in cooldown - delay between throws") |
| "# Set true, If you want to disable throwing trident during combat", | ||
| "# This will work globally, but can be overridden by region settings" | ||
| }) | ||
| public boolean tridentThrowDisabledDuringCombat = true; |
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.
Could be set to false as default
| public class FightTridentSettings extends OkaeriConfig { | ||
|
|
||
| @Comment({ | ||
| "# Set true, If you want to disable throwing trident during 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.
| "# Set true, If you want to disable throwing trident during combat", | |
| "#Set to true to disable throwing tridents during combat.", |
| public boolean pearlCooldownEnabled = false; | ||
|
|
||
| @Comment("# Set true, If you want to reset timer when player throws ender pearl") | ||
| public boolean pearlResetsTimerEnabled = true; |
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.
| public boolean pearlResetsTimerEnabled = true; | |
| public boolean pearlResetsTimer = true; |
maybe just it?
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 :)
I've added new options to Pearl and Trident that reset the timer when the player uses them.