Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove itemstack mutation on player drop method #11831

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove itemstack mutation on player drop method #11831

wants to merge 2 commits into from
+1 −35

Conversation

Owen1212055
Copy link
Member

@Owen1212055 Owen1212055 commented Dec 26, 2024 •
edited by Machine-Maker
Loading

This appears to cause alot of issues, as the general assumption when calling this method is that the itemstack would not be mutated.

Common issues that this seems to cause is item related actions that later increment a statistic, which is no longer true because the item type changes since its now considered empty.
The dupe that this was meant to resolve was patched.

Fixes #11520
Fixes #11765
Supercedes #11521

@Owen1212055
Remove itemstack mutation on player drop method
6cc87f8 
This appears to cause alot of issues, as the general assumption when calling this method is that the itemstack would not be mutated.

The dupe that this was meant to resolve was patched.

Fixes #11520 / #11765
@Owen1212055 Owen1212055 requested a review from a team as a code owner December 26, 2024 22:36
@kennytv kennytv added the type: bug Something doesn't work as it was intended to. label Jan 5, 2025
@Owen1212055
Copy link
Member Author

I have tried to look deeply into all of the changes that may be caused by this and I have seen the following, note that I would like a second opinion from some of these. Note that for analyizing these you must look at the parameter passed and ensure that the itemstack is either cloned or removed from the container properly before being called.

Some interesting calls:

  • ResultSlot.onTake(Player, ItemStack) (net.minecraft.world.inventory) — This looks like where the above bug fix occurs?
  • Inventory.placeItemBackInInventory(ItemStack, boolean) (net.minecraft.world.entity.player) — looked suspicious, altough is fine
  • StonecutterMenu.quickMoveStack(Player, int) (net.minecraft.world.inventory) — This is sus, there is defo mutation in that slot occuring here. Do we cause another bug here?
  • Bucketable.bucketMobPickup(Player, InteractionHand, T) (net.minecraft.world.entity.animal) — behavior change, bucketItemStack will now respect in advancement
  • AbstractContainerMenu#614 -mutation affects outer loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
type: bug Something doesn't work as it was intended to.
Projects
Paper PR Queue
Status: Awaiting review
Milestone
No milestone
2 participants
@Owen1212055 @kennytv