World hopper interactions

[CoderJoeW]

trafficstars

Introduction

This pull request solves the following issue

https://github.com/pmmp/PocketMine-MP/projects/14#card-67670591

Follow-up

Normally when a hopper is powered it locks the hopper. Due to redstone and powering functionality not currently being implemented I saw no need to implement this in the hopper(yet)

[ShockedPlot7560]

There's already an open PR on the subject: #5869

[CoderJoeW]

@ShockedPlot7560 I have looked over the original pull request and it has some issues

1. The code does not seem to handle input properly(when hopper is on top of furnance) it will throw anything into the input
2. The code has this long if/elseif/else statement

I think I could maybe build a better implementation at least I would like the chance to try to build a better implementation

[ShockedPlot7560]

As mentioned in the other PR, transfer logics should be located in the blocks directly, to allow greater flexibility, or at least be externalized from the hopper class itself : https://github.com/pmmp/PocketMine-MP/pull/5869#discussion_r1261131283

This PR also seems incomplete, as it lacks the logic to retrieve items / check entities.

If it's in progress, convert it into a draft so you can tell when it's ready for review.

[CoderJoeW]

@ShockedPlot7560 How do I convert it into a draft?

[ColinHDev]

@ShockedPlot7560 I have looked over the original pull request and it has some issues

1. The code does not seem to handle input properly(when hopper is on top of furnance) it will throw anything into the input
2. The code has this long if/elseif/else statement

I think I could maybe build a better implementation at least I would like the chance to try to build a better implementation

It does handle the input properly, as you can see in the wiki: A working hopper pointing into top of a furnace deposits only into the ingredient slot. It can push any item, including items that can't be smelted by the furnace.

[CoderJoeW]

@ColinHDev I didnt know that. Its weird thats how it is supposed to work in vanilla mc

[CoderJoeW]

Perhaps this is better off being closed then as I clearly do not know as much as I need to know for this implementation

[ColinHDev]

@ColinHDev I didnt know that. Its weird thats how it is supposed to work in vanilla mc

It matches how the player can interact with furnaces. As a player, you also can put any item in the furnace's smelting slot, but only items like coal into the fuel one.

[ShockedPlot7560]

Should target next-minor

[ColinHDev]

I'm sorry, I didn't notice it earlier: Hoppers pull items from containers that are placed above them. Hoppers push blocks into the container they are facing. And while your method is called pull() it pushes items to other containers instead of pulling them into the hopper.

[CoderJoeW]

@ColinHDev Your right and I was thinking about that yesterday when I was working on it

Going to make that change today

[CoderJoeW]

Ready for review

[ColinHDev]

A few other comments:

• There are some complaints by PHPStan that need to be addressed.
• What is your goal with this pr? Currently, you implemented the hopper push logic for some blocks (a lot are still missing, like brewing stands or barrels), partially the pull logic as far as I could tell and none of the item entity pick-up logic. How much do you plan to implement here?
• You still rely on the hopper tile instead of the block. I know, that the goal is / was to reduce the dependence on the tile objects in these places. Although you have a point, that those methods will have to get the tile themselves, a maintainer should probably just settle that argument.

[CoderJoeW]

Okay made some big changes which I believe puts the code on a good path forward for implementing for other blocks currently the following blocks should be fully implemented

• Hopper
• Chest
• Furnace

There may still be a debate about the cooldown? I am not sure I think it works correctly now

The hopper will push before it pulls If a push or pull is successful the next interaction happens on the 8 tick otherwise it happens on the next tick

Functionality may not be 100% correct for the following

• Blast furnace
• Smoker
• Trapped chest

[ColinHDev]

I think to avoid confusion later. It might be necessary to rename the interface methods to : pullFrom and pushTo

By implementing the other logics, it will surely be possible to factorize the item transfer logic, which seems to be redundant.

I already discussed it on discord: push() or pushTo() is not really self-explanatory without thinking of hoppers. My suggestion would be onHopperPull() / onHopperPush()

[ColinHDev]

Should target next-minor

Since the target branch got changed, might be a good time to merge next-minor into this one, so the "files changes" are not polluted and easier to read

[ColinHDev]

Currently, you have duplicated logic in your code with the transferItem() method. Although this only is implemented twice, you probably have to implement it a few more times when accounting for other storage blocks like barrels or shulker boxes. I propose a HopperInteractableTrait that implements basic doHopperPull() and doHopperPush methods:

• Normal blocks like chests or barrels could just implement the HopperInteractable interface as well as using that trait and be done.
• Certain blocks like the hopper, which are set on an 8 tick cooldown if an item is pushed into them, could still make use of the trait and just also schedule the block update, only requiring them to contain a few lines calling the trait's method than implementing the logic themselves.
• Special blocks like the furnace blocks can still implement their logic and just don't have to use the trait.

[CoderJoeW]

Alright I think that resolves the outstanding issues

[ColinHDev]

Alright I think that resolves the outstanding issues

• There is still no logic for some of the blocks hoppers can interact with, e.g. brewing stands. A complete list would be here in the wiki
• You switched the logic: doHopperPush() should handle the behaviour when a hopper pushes an item into another container. doHopperPull() on the other hand handles the behaviour when a hopper pulls an item from another container into its own inventory. It seems like you switched those in your implementation.
• You always used BaseInventory as the type for your method parameters. From what I've learned so far, is that you should generally use the base /parent type in those cases and not any of its implementations. So that would be using the Ìnventory` interface here for the type declaration. But someone might disagree with that.
• As already discussed in the #internals-chat on Discord, the doHopperPush() and doHopperPull() methods should just get the block as the parameter and not only / or in combination with the inventory. Yes, it would result in some boilerplate code where each implementation would have to get the tile instance, check if it's valid and then get the inventory. But, as discussed on Discord yesterday, the goal is to remove tiles from the core. That boilerplate code could then be simplified to just $inventory = $block->getInventory(). Yes, your code would be fine for now, but in that version where tiles get removed, there would be no point in having two parameters in those methods for block and inventory. So the interface would have to change then, which would have the consequence of every implementation of HopperInteractable being forced to adapt, which is not only the case for classes in the core but also for every plugin that made use of that interface.

[dktapps]

So that would be using the Inventory interface here for the type declaration. But someone might disagree with that.

Yes, that's correct. There's no obvious reason to constrain stuff to BaseInventory since it's merely implementing Inventory. BaseInventory should be considered an implementation detail and shouldn't normally appear in typehints.

[CoderJoeW]

@ColinHDev

1. There is still no logic for some of the blocks hoppers can interact with, e.g. brewing stands. A complete list would be here in the wiki

I am waiting on the base implementation to accepted as the standard before implementing the rest of the blocks it is painful enough testing with 3 blocks and making sure everything works correctly it would be more painful to test every block

2. You switched the logic: doHopperPush() should....

I dont think so?

3. You always used BaseInventory as the type....

This has now been updated to Inventory

4. As already discussed in the #internals-chat on Discord....

This has now been corrected

[ColinHDev]

1. There is still no logic for some of the blocks hoppers can interact with, e.g. brewing stands. A complete list would be here in the wiki

I am waiting on the base implementation to accepted as the standard before implementing the rest of the blocks it is painful enough testing with 3 blocks and making sure everything works correctly it would be more painful to test every block

Fair enough.

2. You switched the logic: doHopperPush() should....

I dont think so?

If you look at your furnace class for example: You check in your doHopperPull() method how the hopper faces the furnace since different faces change the target slot in the furnace. This is done when the hopper pushes an item into the furnace.

[CoderJoeW]

@ColinHDev I made most of the changes

My only thing is im just not understanding your comments about pull vs push

[ColinHDev]

Hopper pushing describes: An Hopper pushes an item from its own inventory into the inventory of the block it is facing. Hopper pulling describes: An Hopper pulls an item from the inventory of the block above it into its own inventory.

The code in your methods doHopperPush() and doHopperPull() do the opposite: doHopperPush() pulls items into the hopper's inventory, doHopperPull() pushes items into the facing block's inventory.

[CoderJoeW]

@ColinHDev That took me a minute to wrap my mind around I was not thinking about it in the sense of the hopper performing the action but the block performing the action

Anyways should be correct now

[ShockedPlot7560]

A lock principle must be implemented to prevent conflicts with cooldowns. It should not be implemented through updates alone.

If a hopper is placed on a tick, it will update in 8 ticks. Suppose another hopper is placed 1 tick later under a chest and facing the previous hopper.

• The first time, it will retrieve the item from the chest above and schedule an update in 8 ticks.
• The first hopper does nothing and reschedules an update.
• The second hopper sends its item to the first hopper and schedules an update for itself and the other hopper. Thus, the first hopper is now subject to two updates, one tick apart.

[ShockedPlot7560]

https://github.com/pmmp/PocketMine-MP/pull/5906#issuecomment-1657174520

Still seems relevant

[ShockedPlot7560]

#5906 (comment)

Still seems relevant

For your information, this PR is waiting for this comment.

[CoderJoeW]

If someone wants to takeover and finish this would be great I am busy atm to finish

[ShockedPlot7560]

If someone wants to takeover and finish this would be great I am busy atm to finish

You haven't authorized the maintaner to push on your branch. I've taken over the PR locally, but I can't update your branch because of this permission problem.

See here for the solution