Paper
Paper copied to clipboard
PaperMC
Add PlayerInventorySlotChangeEvent
Useful event for detecting user acquired/lost an item in any way (e.g. custom advancement).
Otherwise, it is needed to listen for several events and I am not sure whether it is possible to listen for example for /give.
Originally I wanted it to be InventoryEvent, but since there is no InventoryView and it would have to be null, I thought PlayerEvent would be better.
If this event is going to be a thing, which I’m not 100% sold on yet, it should be in AbstractContainerMenu#triggerSlotListeners so you can call it with the old and new stacks. Also, the new stack should be a mirror I think, not a copy.
Well I know the Paper team generally don't like events, that can be replaced by other events, but I would say this is not the case. If I wanted to only check Player obtained an item, I would have to listen for (and I am sure I forgot about some things):
- Item pickup
- menu click (would have to think also about Bundles)
- menu drag
- bucket fill
- bucket empty (obtaining empty bucket)
- item consume (empty bottle/bowl)
- entity interact (not sure if milking a cow calls bucket fill, also filling bucket of fish)
- block interact (lodestone compass?)
- signing/editing a book
- cloning a block (creative middle clicking on a block)
- /give
- plugins adding items into player's inventory
Most of the stuff can be detected by other events, but I am not sure how to detect last three mentioned. And even if I was, as you can see, for such simple task - detect whether/when player got an item, I would have to check that many events. This event would solve it.
Alternative would be to skim through all players inventories every tick and check for the item, which is solution I really don't like and I try to avoid doing stuff that often...
About your remarks: I also originally though putting it in there, but then the event would be bound to Inventory and not to player, and also I didn't see player instance in there, I guess maybe checking if Inventory is player's inventory and then casting it?
I did not know that mirroring was a option, found this in other event, will definitely fix it. I guess for old itemstack I should use copy though, right?
About your remarks: I also originally though putting it in there, but then the event would be bound to Inventory and not to player, and also I didn't see player instance in there, I guess maybe checking if Inventory is player's inventory and then casting it?
Ah yeah, in that case, I would add a default method to the ContainerListener interface that takes an additional ItemStack argument that delegates to the existing slotChanged method. Then in ServerPlayer, override that method to fire the event.
EDIT: Also, this event can be cancellable, but make clear in the event docs, that you are not cancelling the changing of the itemstacks, but logic that runs after that, such as advancement triggers for inventory changing.
Also, this event can be cancellable, but make clear in the event docs, that you are not cancelling the changing of the itemstacks, but logic that runs after that, such as advancement triggers for inventory changing.
I'm not sure if that is the right way to go about that if the action that gets stopped isn't the one from the name of the event (the slot actually changing) I feel like if it was made cancellable that it should also stop the action that caused the event to be called (the slot changing) from occurring, at least that's how pretty much all events work.
If this should really include a toggle for the actions after the change then I feel like this should be a separate toggle (e.g. "triggerAdvements" or something along that line)
If this should really include a toggle for the actions after the change then I feel like this should be a separate toggle (e.g. "triggerAdvements" or something along that line)
Yeah, that might be better.
If this should really include a toggle for the actions after the change then I feel like this should be a separate toggle (e.g. "triggerAdvements" or something along that line)
Ok, will do that...
Ah yeah, in that case, I would add a default method to the ContainerListener interface that takes an additional ItemStack argument that delegates to the existing slotChanged method. Then in ServerPlayer, override that method to fire the event.
If I add a default method with an additional parameter and override that, then the original method will not be overridden and thus it will be uncompilable. I might override both, and use the new only for the event, but then I can't cancel the other one.
I can make the old one default calling the new one with null as the argument. Would that be sufficient?
Edit: Another option would be to just change the method, it is only referenced twice, when called and overridden. But that might break stuff in the future, I guess...
If I add a default method with an additional parameter and override that, then the original method will not be overridden and thus it will be uncompilable. I might override both, and use the new only for the event, but then I can't cancel the other one.
The original one will still be there, just add an additional override where the instance of ContainerListener is created. Then call that new method from AbstractContainerMenu. that should work fine.
EDIT: see https://paste.gg/p/anonymous/55a51399e00f4de0afc768de37a7f1c1
Rebased and fixed remarks...
Edit: thanks for the paste, noticed it after I committed, so hopefully it is fine this way... :D
I would really like to see this happening. For I project, I work on, I need an event, that detects any kind of inventory change. No matter, if by a pickup or /give command.
Me and my team use that also for making the command blocks and data packs communicate with the plugin, that is responsible for the story quests. Currently, our only solution is tracking item pickups and also /give command executions. When a /give command happens, the hash codes for the slots have to be saved and compared against the now possible updated inventory, one tick after.
Other solutions, like the one in use, are not very performant but the only way without crashing maintainability.
Really far from fond of events which allow mutation of the results from other events with 0 awareness towards other events
This breaks the general bukkit contract that API won't trigger events outside of niche cases, and introduces many risks for things tripping over one another and doesn't allow for many concerns of context, etc
What about some immutable event? Otherwise, there is no good way, to catch all changes to the player's inventory?
only way for it to be immutable would be for all getter methods to return a clone
AFAIK this PR does not allow mutation of the stacks, or does it? It is a while when I did it, but I still miss sine event in this matter...
And if I cloned it? I really need just the information, don't need to modify the item.
And if I cloned it? I really need just the information, don't need to modify the item.
Same for me
If this event is going to be a thing, which I’m not 100% sold on yet, it should be in
AbstractContainerMenu#triggerSlotListenersso you can call it with the old and new stacks. Also, the new stack should be a mirror I think, not a copy.
Just skimmed through the PR, I was changing it to mirror the ItemStack based on Machine's remarks. But yet again, I really don't care whether it will be a mirror, a copy or the ItemStack will be missing at all.
Any updates on this?
Well, now we need to wait for reviews. But since there is quite a lot of PRs and the Paper team works on Paper in their free time, we need to be patient...