Skip to content

Fix #5149: Items disappearing from shulkers #8219

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

Open
wants to merge 1 commit into
base: mc1.20.1/dev
Choose a base branch
from

Conversation

Xicx99
Copy link

@Xicx99 Xicx99 commented Apr 7, 2025

The item wand of symmetry was not handling the NBT information from the items.
The fix consisted on making sure to also take into account this information when looking for the item in the inventory, when placing the new item and when removing the item.
Created a simple test to confirm the fix.

@VoidLeech VoidLeech added pr type: fix PR fixes a bug pr status: wrong target PR is targeting the wrong branch labels Apr 7, 2025
Copy link

github-actions bot commented Apr 7, 2025

@Xicx99, this pull request is targeting the wrong branch. Pull requests should target the branch corresponding to the earliest supported Minecraft version unless the changes are specific to code that only exists for a newer Minecraft version. Please change the target branch, resolve any merge conflicts, and leave a message here so we can continue with the process of reviewing and merging this pull request. Thanks!

@Xicx99
Copy link
Author

Xicx99 commented Apr 7, 2025

Hello,
Since Mojang changed the way NBT tags work from version 1.20.1 to 1.21.1, I can't simply change the target branch because the code wouldn't work.
Should I update the code to the right version and open a new PR?

@VoidLeech
Copy link
Collaborator

Opening a new PR is fine. Alternatively you could make changes from 1.20.1 and force-push that to this branch.

@@ -122,7 +123,7 @@ public static BlockState setZeroAge(BlockState blockState) {
return blockState;
}

public static int findAndRemoveInInventory(BlockState block, Player player, int amount) {
public static int findAndRemoveInInventory(BlockState block, String nbtString, Player player, int amount) {
Copy link
Contributor

@RaymondBlaze RaymondBlaze Apr 8, 2025

Choose a reason for hiding this comment

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

Is there any specific reason for using String here instead of just pass in the ItemStack and use ItemStack#isSameItemSameComponents?

Also please don't remove the original method, leave it as an overload so no breaking changes are introduced.

Copy link
Author

Choose a reason for hiding this comment

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

I was simply not aware that this method existed...
Now developing for 1.20.1 I will change that and use the equivalent method for it ItemStack#isSameItemSameTags.

Thank you for pointing these out.

The item wand of symmetry was not handling
the NBT information from the items.
The fix consisted on making sure to also take into account
this information when placing and removing the items.
@Xicx99 Xicx99 changed the base branch from mc1.21.1/dev to mc1.20.1/dev April 13, 2025 22:10
@Xicx99
Copy link
Author

Xicx99 commented Apr 13, 2025

@VoidLeech
Hello, I've force-pushed the code for 1.20.1 to here and changed the target branch.
Hopefully everything is good now.
Thank you.

@VoidLeech VoidLeech removed the pr status: wrong target PR is targeting the wrong branch label Apr 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr type: fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants