-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Initial implementation of Entity Interaction Events #183
base: 1.19
Are you sure you want to change the base?
Conversation
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.
Some generic nitpicks and ideas.
...ty/interaction/src/main/java/org/quiltmc/qsl/entity/interaction/mixin/LivingEntityMixin.java
Outdated
Show resolved
Hide resolved
.../main/java/org/quiltmc/qsl/entity/interaction/mixin/ServerPlayerInteractionManagerMixin.java
Outdated
Show resolved
Hide resolved
...interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseItemCallback.java
Outdated
Show resolved
Hide resolved
* <li>{@link ActionResult#FAIL} cancels further processing and does not send a packet to the server.</li> | ||
* </ul> | ||
*/ | ||
public interface AttackBlockCallback { |
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 know this is based on the QM name, but I don't like calling it "attacking" a block, because it's really the beginning of a player breaking (destroying) a block.
Would this maybe make more sense if it was part of BreakBlockEvents
?
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 think they have different purposes... its kinda hard to describe. The action of left clicking vs the event of breaking.
Break block events to me are designed to be acted upon the block's breaking more than the player's action while this is more the player's action than the block breaking, if that makes any sense.
I'm fine with moving it over tho if people agree it fits better with the block breaking events.
...nteraction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/BreakBlockEvents.java
Outdated
Show resolved
Hide resolved
...nteraction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/BreakBlockEvents.java
Outdated
Show resolved
Hide resolved
...nteraction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/BreakBlockEvents.java
Outdated
Show resolved
Hide resolved
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 like this! Just some nitpicks.
Also, an alternative for the DamageContext
is to have a separate MODIFY_DAMAGE
event that fires if the before event is not canceled. That way all the events are cohesive.
* An encapsulation of several arguments relevant to a damage context with a | ||
* mutable damage value and cancellation that is shared across all listeners. | ||
*/ | ||
public class DamageContext { |
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.
Annotating members with @Nullable
and @NotNull
would be really helpful!
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.
Imo, nothing in it should ever be null. Should I just annotate everything with @notNull
?
* @param source the {@link DamageSource} of the attack | ||
* @param damage the damage that was dealt | ||
*/ | ||
void afterDamage(LivingEntity attacker, ItemStack stack, LivingEntity target, DamageSource source, float damage); |
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 wonder if there should be two DamageContext
s. A MutableDamageContext
for the before event, and an ImmutableDamageContext
(names very much up for debate) for the after event. Then these interfaces can even be merged into one.
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.
So two more classes extending a abstract DamageContext
? I think that can work.
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 separation of the Before
and After
functional interfaces I believe are more for clarification purposes however. The documentation could become a bit confusing if they are merged.
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.
Coming back to this, I think a separate MODIFY_DAMAGE
event would be better. That way you have the ability to cancel it, modify it, or react to it in clear, separate events.
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 believe this would create a lot of duplicate code on the user's part. I think logic that determines cancellation or modifying damage would align a lot of the time and I'd rather not force users to duplicate code to perform these tasks.
...raction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/AttackBlockCallback.java
Show resolved
Hide resolved
* <p> | ||
* Is hooked before the spectator check, so make sure to check the player's game mode! | ||
* <ul> | ||
* <li>{@link ActionResult#SUCCESS} cancels further processing and, on the client, sends a packet to the server.</li> |
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.
linking the class of the packet would be useful! (also in all the other places that have similar docs)
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.
Easy enough to do, but I'm not so sure why that would be helpful. All the packet really does is tell the logical server to process the event on their side.
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.
Just lets devs jump around the codebase a little easier, in case they want to look at how the interaction is processed
.../interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseBlockEvents.java
Outdated
Show resolved
Hide resolved
...interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseEntityEvents.java
Outdated
Show resolved
Hide resolved
.../interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseBlockEvents.java
Show resolved
Hide resolved
...interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseEntityEvents.java
Show resolved
Hide resolved
...y/interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseItemEvents.java
Outdated
Show resolved
Hide resolved
...y/interaction/src/main/java/org/quiltmc/qsl/entity/interaction/api/player/UseItemEvents.java
Show resolved
Hide resolved
return damage; | ||
} | ||
|
||
@Inject(method = "damage", at = @At(value = "HEAD", shift = At.Shift.AFTER), cancellable = 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.
I'd say give this method priority so other @Inject
s at HEAD
get run after this and it can cancel before other mods do funky things.
* @param source the {@link DamageSource} of the attack | ||
* @param damage the damage that was dealt | ||
*/ | ||
void afterDamage(LivingEntity attacker, ItemStack stack, LivingEntity target, DamageSource source, float damage); |
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.
Coming back to this, I think a separate MODIFY_DAMAGE
event would be better. That way you have the ability to cancel it, modify it, or react to it in clear, separate events.
Adds an
interaction
module containing (as of now) the basic Player interaction events similar to Fabric's implementation, and a LivingEntityAttack event.TODO:
BEFORE
andAFTER
events