-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
base: 1.21.x
Are you sure you want to change the base?
Conversation
|
The patch changes in this PR are unnecessary, you've updated |
…ension" This reverts commit 31e82c3.
Actually, based on further review (and looking at the levels of indirection here), the |
@ewoudje, this PR introduces breaking changes.
|
Something feels weird about a method named |
A little bit, yes; it's not entirely different than how we handle other itemstack extension methods, since the class is |
I could rename it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs to be reassessed, in light of the fact that DataComponentIngredient#display
uses the crafting remainder, which might cause the event to be fired when obtaining the slot display without a player doing any actual crafting.
This probably entails switching uses of getCraftingRemainder()
in methods that do actual crafting to the hook method, and leaving unmodified the method in the item stack extension.
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