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

Use CommonHooks.getCraftingRemainder #1585 #1672

Open
wants to merge 5 commits into
base: 1.21.x
Choose a base branch
from

Conversation

ewoudje
Copy link

@ewoudje ewoudje commented Nov 8, 2024

CommonHooks.getCraftingRemainder doesn't get used, and 'destroyed' items were not being destroyed.

I split it over 2 commits, one fixing the actual bug, and the other one making all patches call CommonHooks instead.
I wasn't sure how it would be preferred.

Fixes #1585

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2024

CLA assistant check
All committers have signed the CLA.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@Shadows-of-Fire
Copy link
Contributor

The patch changes in this PR are unnecessary, you've updated ItemStack#getCraftingRemainder to use the hook method, but then patched all the call sites to the hook method anyway. Undo the patch changes.

@Shadows-of-Fire
Copy link
Contributor

Actually, based on further review (and looking at the levels of indirection here), the CommonHooks method should just be removed and flattened into the ItemStack method entirely.

@Matyrobbrt Matyrobbrt added bug A bug or error 1.21.3 Targeted at Minecraft 1.21.3 labels Nov 9, 2024
@neoforged-compatibility-checks

@ewoudje, this PR introduces breaking changes.
Fortunately, this project is currently accepting breaking changes, but if they are not intentional, please revert them.
Last checked commit: 95dd711ebc9d514e00c540393885a2279cb1e8d3.

neoforge (:neoforge)

  • net/neoforged/neoforge/common/CommonHooks
    • getCraftingRemainder(Lnet/minecraft/world/item/ItemStack;)Lnet/minecraft/world/item/ItemStack;: ❗ API method was removed

@embeddedt
Copy link
Member

Something feels weird about a method named getCraftingRemainder having side effects like firing a destroy item event.

@Shadows-of-Fire
Copy link
Contributor

A little bit, yes; it's not entirely different than how we handle other itemstack extension methods, since the class is final. There is one call site where we don't want the event to be fired, in DataComponentIngredient#display, so I guess we need to take a different approach here. Prior to the SlotDisplay stuff the only call sites for these methods were in recipe remainder calculation.

@ewoudje
Copy link
Author

ewoudje commented Nov 11, 2024

Something feels weird about a method named getCraftingRemainder having side effects like firing a destroy item event.

I could rename it to computeCraftingRemainder or something alike? makes it more clear that it doesn't just get something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.3 Targeted at Minecraft 1.21.3 bug A bug or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CommonHooks.getCraftingRemainingItem not used
5 participants