EntityItemPickupEvent + PlayerEvent#ItemPickupEvent #101
Conversation
|
:concern: |
ZNixian
left a comment
ZNixian
left a comment
There was a problem hiding this comment.
If there's a good reason for sending the EntityItemPickupEvent from it's own function then go ahead and merge anyway, but IMO it would make the relevant code significantly easier to read.
| public void onPlayerCollision(PlayerEntity player) { | ||
| if (!this.world.isClient && this.pickupDelay <= 0) { | ||
| final ItemEntity entity = (ItemEntity) (Object) this; | ||
| int result = EntityEvents.onItemPickup(entity, player); |
There was a problem hiding this comment.
Would it be easier just to move the event stuff in here? Without checking two different files it's not obvious what the result represents - event.getResult() == Result.ALLOW is much clearer than result == 1?
There was a problem hiding this comment.
It will be easier to move from EntityEvents back to its respective Hook class to prevent breaking compatibility with mods that use the provided hooks to fire the event.
patchwork-events-entity/src/main/java/net/patchworkmc/mixin/event/entity/MixinItemEntity.java
Outdated
Show resolved
Hide resolved
TheGlitch76
added
the
in progress
|
On further review I'd like the threadlocal version to be used until we have a better alternative. |
|
Sure I'll revert to the ThreadLocal commit. |
- Added entity.item events, ItemExpireEvent is implemented but ItemTossEvent is a stub right now. - Implemented 'lifespan', Forge exposed this to modders via IForgeItem/IForgeItemStack. So the injections are commented out at the moment. - Another injection is commented out, as IForgeItem/IForgeItemStack#onEntityItemUpdate isn't implemented yet. -NOTE: I changed EntityItemPickupEvent's call's signature, it now returns a boolean as per request (can use Result too). As it seems like -1 and 1 is returned, and 0 is never returned anywhere.
|
Just updated, reverted Overwrite, and implemented rest of the entity.item events. More info see latest commit msg. |
| * removed from the inventory - and thus removed from the system. | ||
| */ | ||
| // TODO: Call Location - ForgeHook#onPlayerTossEvent -> PlayerEntity#dropSelectedItem + PlayerEntity#dropItem | ||
| public class ItemTossEvent extends ItemEvent { |
There was a problem hiding this comment.
May as well leave this out if you're not going to implement the hooks for it
There was a problem hiding this comment.
I can do it in the same PR if its cool :^)
- Scope-creep'd a little out of the PR
|
|
||
| @Redirect(method = "onPlayerCollision", at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/PlayerEntity;sendPickup(Lnet/minecraft/entity/Entity;I)V")) | ||
| private void onPlayerItemPickup(PlayerEntity entity, Entity item, int count) { | ||
| EntityEvents.firePlayerItemPickupEvent(entity, (ItemEntity) item, getStack().copy()); |
There was a problem hiding this comment.
I also implemented this event in #118.
patchwork-events-entity/src/main/java/net/patchworkmc/mixin/event/entity/MixinItemEntity.java
I created PlayerEvents to fire all types of PlayerEvent, so that EntityEvents can be less messy. I would suggest one of us should drop his PlayerEvent.ItemPickupEvent implementation. Sorry for the inconvenience, I didn't know this PR before I started #118.
Something I would like to discuss is that, in 1.14.4 (The yarnforge snapshot), Forge removed the entityIn.sendPickup(this, i); before if (itemstack.isEmpty()) { but in the later version they didnt. However, I don't think firing the PlayerEvent.ItemPickupEvent before or after entityIn.sendPickup(this, i); make any noticable difference.
|
Merge conflicts |
4 participants
Used
@Overwritein latest commit as injections and redirects got crazy.Roughly checked, and everything should be working.
These events are so similarly named, don't blame me for the EntityEvents methods, if you have any good ones pls say.