Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Support for result ItemStack in CraftItemEvent

Open Doc94 opened this issue 2 years ago • 10 comments

This PR close https://github.com/PaperMC/Paper/issues/8439 and https://github.com/PaperMC/Paper/issues/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 avatar Oct 08 '22 16:10 Doc94

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 avatar Oct 09 '22 11:10 Doc94

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

Doc94 avatar Oct 09 '22 15:10 Doc94

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?

Doc94 avatar Mar 20 '23 11:03 Doc94

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.

Machine-Maker avatar Mar 25 '23 06:03 Machine-Maker

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?

Doc94 avatar Mar 25 '23 12:03 Doc94

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 avatar Mar 25 '23 14:03 Machine-Maker

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

Machine-Maker avatar Mar 25 '23 17:03 Machine-Maker

What's the status on this?

XLordalX avatar Feb 23 '24 16:02 XLordalX

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)

Ayouuuu avatar May 01 '24 04:05 Ayouuuu