Skip to content

Use result of setItemStack in PlayerBucketEmptyEvent (#2651)#2857

Closed
Draycia wants to merge 6 commits into
PaperMC:masterfrom
Draycia:master
Closed

Use result of setItemStack in PlayerBucketEmptyEvent (#2651)#2857
Draycia wants to merge 6 commits into
PaperMC:masterfrom
Draycia:master

Conversation

@Draycia
Copy link
Copy Markdown

@Draycia Draycia commented Jan 14, 2020

Resolves the weird behaviour where modifications to itemStack in PlayerBucketEmptyEvent (through setItemStack in PlayerBucketEvent) are ignored.

Tested with #2651's code snippet and it seems to be functional with this change, things seem fine with the plugin omitted as well.

@Draycia
Copy link
Copy Markdown
Author

Draycia commented Jan 14, 2020

Seems this inadvertently causes creative player buckets to empty when they wouldn't otherwise without the patch

@Draycia
Copy link
Copy Markdown
Author

Draycia commented Jan 14, 2020

Tested the latest change, creative players no longer have their buckets mutated on use (as is the case without the patch) and survival player buckets are properly mutated as well.


public static PlayerBucketEmptyEvent callPlayerBucketEmptyEvent(World world, EntityHuman who, BlockPosition changed, BlockPosition clicked, EnumDirection clickedFace, ItemStack itemstack, EnumHand enumHand) {
- return (PlayerBucketEmptyEvent) getPlayerBucketEvent(false, world, who, changed, clicked, clickedFace, itemstack, Items.BUCKET, enumHand);
+ return (PlayerBucketEmptyEvent) getPlayerBucketEvent(false, world, who, changed, clicked, clickedFace, itemstack, itemstack.getItem(), enumHand); // Paper - don't forcefully set to bucket
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes the creative case but breaks survival - only use the item in hand Item if the player is creative, else Bucket will do

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed in latest commit

@Spottedleaf
Copy link
Copy Markdown
Member

Spottedleaf commented Jan 16, 2020

It looks like the fill event is also broken - for creative players. The fill event is broken because the method called at line 57 in ItemBucket ignores the event item if creative, and that the event item is wrong for creative players too - so watch out for that when fixing it I'll get this nevermind - not relevant for this pr

((EntityPlayer) entityhuman).getBukkitEntity().updateInventory(); // SPIGOT-4541
return false;
}
+ entityhuman.setItemInHand(enumhand, org.bukkit.craftbukkit.inventory.CraftItemStack.asNMSCopy(event.getItemStack())); // Paper - 2651: set item in hand if event isn't cancelled
Copy link
Copy Markdown
Member

@Spottedleaf Spottedleaf Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be passed to the caller so it can set the item result in the interact wrapper - the wrapper is the proper way to handle changes to the item

@Draycia
Copy link
Copy Markdown
Author

Draycia commented Jan 30, 2020

I'm too retarded to continue atm, and I'm busy with other things. Maybe in the future if @Spottedleaf doesn't snag it

@Draycia Draycia closed this Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants