InventoryTransaction's source can't be anything except a player

ColinHDev commented 3 years ago

Description

Currently, the source of an InventoryTransaction and therefore the only reason an InventoryTransactionEvent is called is a Player. But how seen in the code, the comment at the beginning of the InventoryTransactionEvent class suggests that it could also be called on inventory transactions by non-player entities or hoppers. https://github.com/pmmp/PocketMine-MP/blob/master/src/event/inventory/InventoryTransactionEvent.php#L33 But how seen in the constructor of the InventoryTransaction class it only accepts players. https://github.com/pmmp/PocketMine-MP/blob/master/src/inventory/transaction/InventoryTransaction.php#L72

Justification

A change would be needed to implement that the InventoryTransactionEvent could also be called if no player is involved, for example on an inventory transaction by a hopper.

Possible Solution

As the source could both be an entity or a block we could use an in php8 introduced union type to change the data type from Player $source to Entity | Block $source. However also the data type of most other classes who are related to InventoryTransaction would be needed to change, for example in the InventoryAction classes.

dktapps commented 3 years ago

Right now, Inventory transactions only originate from players anyway (they are just a way to make sure players aren't cheating). I think the doc comment should be updated, rather than changing the source information.

ColinHDev commented 3 years ago

I am planning to make a PR which fully implements hoppers if this PR would be merged https://github.com/pmmp/PocketMine-MP/pull/4402. In preparation I started to look into it and found that comment in the event so I thought that this event should be used for that case. Also because the InventoryTransaction handels the interaction between two inventories which would be the case for hoppers

ColinHDev commented 3 years ago

yikes, the comment I was referencing was made in 2014 in 5e97da2e11270d87c950e3c35709b8bc10a00447 when the event was added sorry, my bad

dktapps commented 3 years ago

I've never much liked InventoryTransactionEvent (or transactions in general) and find them overcomplicated. But changing source type to allow non-players would break a whole bunch of assumptions, so it's not the best idea.

ColinHDev commented 3 years ago

If you don't like the idea anyway, I won't try to use it But I still would like to use events on hoppers but no event would suit at the moment. What do you think about the addition of an ItemChangeInventoryEvent or something for that case?

ColinHDev commented 3 years ago

To listen if a hopper would pick up an item, there would be hopefully, if it gets merged (https://github.com/pmmp/PocketMine-MP/pull/4402), the BlockItemPickupEvent. To listen if a hopper pushed or pulled an item, there could be the ItemChangeInventoryEvent if the InventoryTransactionEvent should only apply to players. That would be two different events with just the single-use on hoppers. They could be called by other classes as well but atm I can't think of any other use except hoppers. I don't know if it is good practice / good coding style to make events with just a single-use like these two would have.

ColinHDev commented 3 years ago

To listen if a hopper would pick up an item, there would be hopefully, if it gets merged (#4402), the BlockItemPickupEvent. To listen if a hopper pushed or pulled an item, there could be the ItemChangeInventoryEvent if the InventoryTransactionEvent should only apply to players. That would be two different events with just the single-use on hoppers. They could be called by other classes as well but atm I can't think of any other use except hoppers. I don't know if it is good practice / good coding style to make events with just a single-use like these two would have.

If that is even a problem. For example, the BlockTeleportEvent also has just one use when teleporting the dragon egg. https://github.com/pmmp/PocketMine-MP/blob/master/src/block/DragonEgg.php#L71

ColinHDev commented 3 years ago

The problem could be: Not every block a hopper can push into is a container. Jukeboxes aren't containers and don't have an inventory a hopper pushes into. If composters were implemented, they probably also wouldn't be a container and would just store the "height" of stuff in them

dktapps commented 1 year ago

Some recent musings on this, after thinking about new API to harness full potential from ItemStackRequest:

The real purpose of InventoryTransaction is to validate that a set of seemingly unrelated inventory slot changes balances out. The only source of such potentially unbalanced transactions is a player interacting with an inventory, so it doesn't make any sense to have a transaction that doesn't originate from a player. With ItemStackRequest this also doesn't apply anymore since InventoryTransaction only remains as a backwards compatible API shim that I plan to replace with a more powerful API. (In fact, it just now occurred to me that we might want to strip out the validation now that ItemStackRequestExecutor already pre-validates everything anyway, to improve performance).

Transactions don't really make sense for internal usage beyond generating events because we expect that internal transactions should always be valid.

Messing with InventoryTransaction for this is an XY problem. I don't think that building transactions (which are already a hassle to interact with and figure out the behaviour of from code) is the right way to deal with stuff like hoppers.

What we really need for this server-sided behaviour is to fire events when item movements happen. For example, hoppers transferring items into a container should have a dedicated HopperSendItemEvent, with a from, a to, and an itemstack being sent. This would be considerably easier for plugins to understand, better for performance (due to specialization and skipping useless validation steps) and also allow more dynamic API for specific circumstances.

pmmp / PocketMine-MP