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.

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

[Regression] Fix for MC-168573 potentially deletes "broken" item stacks which are not empty #1916

Open
KnightMiner opened this issue Jan 25, 2025 · 0 comments
Labels
triage Needs triaging and confirmation

Comments

@KnightMiner
Copy link
Contributor

Minecraft Version: Confirmed on 1.20.x and 1.21.x branches.

NeoForge Version: After commit 52733f5

Description of issue:

It is often desired for a mod to have an item that instead of disappearing simply enters a broken state. To accomplish this, you usually just need to ensure ItemStack#getDamageValue never exceeds ItemStack#getMaxDamage. To accomplish this with maximum vanilla compatibility, you often override IForgeItem#damageItem to return 0 for its damage value, then handle the damaging yourself including calling the callback passed into the consumer.

In this patch, shields were fixed to call LivingEntity#stopUsingItem in LivingEntity#hurtCurrentlyUsedShield when the shield breaks via the callback to fix a vanilla bug that causes the wrong item to be used visually and prevents the forge IItemExtension#stopUsingItem hook from being fired.

The problem is LivingEntity#stopUsingItem causes LivingEntity#useItem to be set to ItemStack#EMPTY, and directly after the callback vanilla has code to delete the mainhand or offhand item if LivingEntity#useItem is empty, which is normally meant to cleanup stack size 0 stack instances that are not ItemStack#EMPTY along with a queue to play the breaking sound. In this case, it overwrites a stack that is "broken" but not empty.

Steps to Reproduce:

  1. Create a shield which in its IItemExtension#damageItem hook calls the consumer without having its damage > max damage.
  2. Observe that breaking it while blocking will cause the item to be deleted.
  3. Create an item with the same logic in IItemExtension#damageItem that is damaged through other means.
  4. Observe that it will play the breaking animation without the stack being lost.

Proposed Fix:

Simplest fix is to cache the use item stack before the hurt callback in LivingEntity#hurtCurrentlyUsedShield in a local variable, and use that cache for the if statement. This has the sideeffect that the shield sound will not play for items that are broken without being made empty.

Alternatively, you could protecting the this.setItemSlot code in LivingEntity#hurtCurrentlyUsedShield below the callback to only set the slot if the instance is the same as the original item stack (by shallow comparison).

Another potential fix would be to no-op the entire if statement and move the sound up to the callback, but this has the side-effect of leaving empty instances that are not the same as ItemStack#EMPTY in the player hand.

Alternatives considered:

I could in my code simply bypass calling the broken callback, but that breaks a lot of other usages of damaging items, and prevents my code from calling the forge event.

I could also reintroduce the vanilla bug on my items by starting to use the item after it stops to prevent the item from being deleted as well, but that leads to undesired behavior when using a shield in the mainhand.

@KnightMiner KnightMiner added the triage Needs triaging and confirmation label Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs triaging and confirmation
Projects
None yet
Development

No branches or pull requests

1 participant