Skip to content

Support for result ItemStack in CraftItemEvent#8443

Closed
Doc94 wants to merge 2 commits into
PaperMC:masterfrom
Doc94:feature/craftitem-result-ro
Closed

Support for result ItemStack in CraftItemEvent#8443
Doc94 wants to merge 2 commits into
PaperMC:masterfrom
Doc94:feature/craftitem-result-ro

Conversation

@Doc94
Copy link
Copy Markdown
Member

@Doc94 Doc94 commented Oct 8, 2022

This PR close #8439 and #7383 adding a few changes for possible list result item from craft event, this based in the quick move generated when a player use Shift+Click for get all the possible items crafted.

A little example with.

    @EventHandler
    public void onCraft(CraftItemEvent event) {
        HumanEntity humanEntity = event.getWhoClicked();
        humanEntity.sendMessage("You craft is a list of " + event.getItemResults().size());
        event.getItemResults().forEach(itemStack -> {
            humanEntity.sendMessage("You crafted " + itemStack);
        });
    }

@Doc94 Doc94 requested a review from a team as a code owner October 8, 2022 16:16
Copy link
Copy Markdown
Member

@kennytv kennytv left a comment

Choose a reason for hiding this comment

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

I'm a bit iffy on this, as when you for example craft pickaxes, you don't craft an itemstack of a pickaxe with the amount of 5, but 5 different items

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Oct 9, 2022

I'm a bit iffy on this, as when you for example craft pickaxes, you don't craft an itemstack of a pickaxe with the amount of 5, but 5 different items

Ohhh i forget this...
Then what can be? Replace the result method with a list maybe? And need check uf nms handke this too in the same way...

@Doc94 Doc94 force-pushed the feature/craftitem-result-ro branch from 2988cfc to 5c0dd5c Compare October 9, 2022 15:05
@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Oct 9, 2022

Okay i make a few changes...

  • Change the result to a list of items for the case if items with the not default 64 stack
  • The result only respect the items in max stack, the result not represent how inventory handle the split of items (in case if you have more than 1 slot where merge items)

a little example added.
https://youtu.be/V0fupWtk3A4

Comment thread patches/api/0399-Support-for-results-of-ItemStacks-in-CraftItemEvent.patch Outdated
Comment thread patches/server/0925-Support-for-results-of-ItemStacks-in-CraftItemEvent.patch Outdated
Comment thread patches/server/0925-Support-for-results-of-ItemStacks-in-CraftItemEvent.patch Outdated
+ int itemResultCount = baseResult.getAmount();
+ if (packet.getClickType() == net.minecraft.world.inventory.ClickType.QUICK_MOVE && action == InventoryAction.MOVE_TO_OTHER_INVENTORY) {
+ for (itemstack = this.player.containerMenu.quickMoveStack(this.player, packet.getSlotNum()); !itemstack.isEmpty() && ItemStack.isSame(this.player.containerMenu.slots.get(packet.getSlotNum()).getItem(), itemstack); itemstack = this.player.containerMenu.quickMoveStack(player, packet.getSlotNum())) {
+ if (this.player.containerMenu.slots.get(packet.getSlotNum()).mayPickup(this.player) && (this.player.getInventory().getSlotWithRemainingSpace(this.player.containerMenu.slots.get(packet.getSlotNum()).getItem()) != -1 || this.player.getInventory().getFreeSlot() != -1)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not needed the quick move stack already check that see the boolean result of #moveItemStackTo

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i use quickmove based in how NMS handle this calling the same method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh nvm i have missed this nms logic my bad

@Doc94 Doc94 force-pushed the feature/craftitem-result-ro branch from 5c0dd5c to 19b7277 Compare October 12, 2022 01:17
@Doc94 Doc94 requested a review from kennytv October 13, 2022 12:11
@Doc94 Doc94 requested review from Lulu13022002 and kennytv and removed request for Lulu13022002 and kennytv November 2, 2022 14:03
Copy link
Copy Markdown
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

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

I don't really like shoving all that logic into the packet listener class. Also, this issue applies to all the "crafts" in the game. I feel like we need a new system of events listening to "recipe results" for all the recipe types that takes into account shift clicks and everything instead of relying on the inventory click event.

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Mar 20, 2023

I don't really like shoving all that logic into the packet listener class. Also, this issue applies to all the "crafts" in the game. I feel like we need a new system of events listening to "recipe results" for all the recipe types that takes into account shift clicks and everything instead of relying on the inventory click event.

But then is not correct add tbe correct result in CraftItemEvent?

About the code in packet maybe move this to the CraftEvent or dont remember the class with many methods to call event?

@Machine-Maker
Copy link
Copy Markdown
Member

Machine-Maker commented Mar 25, 2023

The CraftItemEvent doesn't have a "result" field, so it can't be incorrect. It has a "clicked item" field, and that is correct, you only clicked on a single item stack for 1 recipe. You didn't click on anything else.

So this is the wrong event to use to get this. A different event(s) should be added to handle "recipe use" in a general sense easily applies to all recipe types, not just 1.

This should be done without mangling the vanilla logic for handling shift clicks.

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Mar 25, 2023

The CraftItemEvent doesn't have a "result" field, so it can't be incorrect. It has a "clicked item" field, and that is correct, you only clicked on a single item stack for 1 recipe. You didn't click on anything else.

So this is the wrong event to use to get this. A different event(s) should be added to handle "recipe use" in a general sense easily applies to all recipe types, not just 1.

This should be done without mangling the vanilla logic for handling shift clicks.

Then you mean...

  • Make a RecipeUseEvent for handle when a recipe is used and have the list of items resulting of this operation like i tried here
  • Move the logic from the packet.... exists another place or you mean move the code to the CraftEvent?

@Machine-Maker
Copy link
Copy Markdown
Member

The event should probably function like shift clicking multiple stacks. Instead of trying to aggregate all the stacks crafted, just call the event once per craft. So maybe it'll be called 64 times, but that similar behavior already exists with the inventory click event.

@Machine-Maker
Copy link
Copy Markdown
Member

Machine-Maker commented Mar 25, 2023

This is smth along the lines of what I had in mind. https://github.com/Machine-Maker/Paper/tree/feature/RecipeCraftEvent. Not 100% satisfied, its just a mockup pretty much. And it only handles quickmoves atm, not other types of clicks

@XLordalX
Copy link
Copy Markdown

What's the status on this?

@Ayouuuu
Copy link
Copy Markdown

Ayouuuu commented May 1, 2024

This is smth along the lines of what I had in mind. https://github.com/Machine-Maker/Paper/tree/feature/RecipeCraftEvent. Not 100% satisfied, its just a mockup pretty much. And it only handles quickmoves atm, not other types of clicks

Not working in 2×2 crafting (InventoryMenu)

@indyteo
Copy link
Copy Markdown
Contributor

indyteo commented Sep 29, 2024

Hi, any update on this?

I'm currently running roughly the same logic as in this PR in my code to compute the number of crafted items, but having to implement it myself can lead to errors (I already had to fix my code twice...), and I would love to see it implemented officially by Paper instead!

Thx

@orang3i orang3i mentioned this pull request Mar 10, 2025
@kennytv kennytv added the pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch label Mar 23, 2025
@kennytv kennytv deleted the branch PaperMC:master March 23, 2025 19:15
@kennytv kennytv closed this Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch pre-softspoon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Final CraftItemEvent result

9 participants