Add hopper Logic by IcyEndymion004 · Pull Request #5869 · pmmp/PocketMine-MP

IcyEndymion004 · 2023-07-02T20:23:50Z

Follow up and update to #4416

(This code is all done by Colin and i dont want credit for it at all just want this added)

This is testing and working!

ShockedPlot7560

Logically, if we want to fully implement the hopper logic, the TODOs for the events and the remaining logics would have to be made.

ShockedPlot7560 · 2023-07-12T10:41:45Z

src/block/Hopper.php

+		if($origin instanceof Container){
+			$success = $this->pull($inventory, $origin->getInventory()) || $success;
+		}else{
+			$success = $this->pickup($inventory) || $success;


To avoid retrieving entities from above when a block is obstructing, it would be interesting to check whether the block above is full, and therefore that no entity can be above it.

You still have to check whether an item entity is inside that full block:
It is also possible for a hopper to collect items from inside a full, solid block, a situation that might come from items rising up through solid blocks or being summoned. wiki

This very specific case may not need to be taken into account in pocketmine. It may increase the performance impact of hoppers, when in fact you should just place a full block on top to prevent entities from being checked.

And this is what some java engines use to optimize performance.

Of course, it's an increased performance impact. Blocking this is basically just disabling a - mostly pretty useless - feature, making PM less-matching with vanilla behaviour.
In the end, it's your decision on what you want to implement into the core. But imo, before removing functionality for performance's sake, there should be taken a look at other, more important hot paths.

I agree that pocketmine wouldn't exactly follow vanilla's behavior in this situation, but is that so important?
It's something to think about, as the feature is not very useful, but it could greatly improve performance as soon as the number of hoppers becomes too large.

To me it doesn't seem like a situation that would come up very often anyway, so the performance gain from changing it is likely to be close to zero. I'd classify this as premature optimisation.

I think @ShockedPlot7560's suggestion was, to skip the entire pickup logic (calling getNearbyEntities(), etc.) if there is a full block above the hopper, or even don't reschedule the hopper if he also has nowhere to push items too.
That should actually help and not just be premature optimization. But as I said, it's not really an optimization, since it removes vanilla behaviour.

ShockedPlot7560 · 2023-07-12T10:43:40Z

src/block/Hopper.php

+			//TODO: Composter
+			//TODO: Brewing Stand
+			//TODO: Jukebox (improve)
+			if($destination instanceof \pocketmine\block\tile\Furnace){


Each push logic should be implemented in the different tiles to allow greater flexibility / addition in the future.

Better implement it in the block classes (maybe identifiable with an HopperInteractable interface), not just to remove the dependence on tiles, but also don't force yourself only to allow the implementation of hopper logic for tiles: There could be blocks, that hoppers can interact with, without them being a tile. (I don't know the Bedrock internals well enough, so I don't know if composters would also need a tile. But if not, they would be a counterexample.)

That's indirectly what I was explaining, but you did it better than I did. I mentioned tile because that's what was checked, but implementing it in blocks seems much more coherent.

ShockedPlot7560 · 2023-07-12T10:44:19Z

src/block/Hopper.php

+		//TODO: Composter
+		//TODO: Brewing Stand
+		//TODO: Jukebox
+		if($origin instanceof FurnaceInventory){


Each pull logic should be implemented in the different tiles to allow greater flexibility / addition in the future.

Same argument as for the push logic.

ShockedPlot7560 · 2023-07-12T10:45:16Z

src/block/tile/Hopper.php

+	public const DEFAULT_TRANSFER_COOLDOWN = 10;
+
+
+


Unnecessary extra spaces.

ShockedPlot7560 · 2023-07-12T10:47:51Z

src/block/Hopper.php


 	public function onScheduledUpdate() : void{
-		//TODO
+		$this->position->getWorld()->scheduleDelayedBlockUpdate($this->position, 1);


If the block above is not a container, but a full block, there's no point in ticking the hopper all the time while the transfer is in cooldown for x amount of time if the block below is a container.
In addition, if the hopper has a full block above it but no container in front of it, the tick should not be activated to improve performance.

As said above, there still could be item entities in that full block.

dktapps · 2023-07-12T11:06:11Z

This is a copy pasta of #4416 and none of the issues in the review comments appear to have been addressed.

ColinHDev · 2023-07-12T11:48:09Z

This is a copy pasta of #4416 and none of the issues in the review comments appear to have been addressed.

I don't seem to remember any non-addressed comments.

ColinHDev · 2023-07-12T11:51:43Z

src/block/tile/Hopper.php


 	private const TAG_TRANSFER_COOLDOWN = "TransferCooldown";

+	public const DEFAULT_TRANSFER_COOLDOWN = 10;


Where did you get that number from? The original pr contained the correct value:
A hopper waits for 4 redstone ticks = 8 game ticks, according to the wiki

dktapps · 2023-07-12T11:56:50Z

This is a copy pasta of #4416 and none of the issues in the review comments appear to have been addressed.

I don't seem to remember any non-addressed comments.

There were some design issues that you said you didn't know how to address, I thought that's why you closed it.

Considering my past experience with copy-pasta PRs I don't usually have high hopes for them.

ColinHDev · 2023-07-12T13:02:30Z

This is a copy pasta of #4416 and none of the issues in the review comments appear to have been addressed.

I don't seem to remember any non-addressed comments.

There were some design issues that you said you didn't know how to address, I thought that's why you closed it.

Considering my past experience with copy-pasta PRs I don't usually have high hopes for them.

Not necessarily. I already discussed it here on the pm discord, when @IcyEndymion004 told me he created the pr.
tl;dr: There were some significant performance problems, especially with idling hoppers, so I wrote the plugin where I had a freer hand regarding certain approaches that are not suitable for the core.

dktapps · 2023-07-13T08:24:02Z

I expect the performance issues had to do with constantly ticking all the hoppers to actively search for nearby items?

ShockedPlot7560 · 2023-07-13T08:38:17Z

I expect the performance issues had to do with constantly ticking all the hoppers to actively search for nearby items?

Entities do, and hopper ticking also adds load when ticking to move items (even if it's tiny).

dktapps · 2023-07-13T12:07:59Z

Perhaps this is an area where an area API like the one described in #5799 would be useful.

ColinHDev · 2023-07-13T12:30:04Z

I expect the performance issues had to do with constantly ticking all the hoppers to actively search for nearby items?

It is actually more problematic to have hundreds of hoppers scheduled every tick. Hoppers are not set on cooldown when they couldn't do anything, so even if they are connected to a container above (which means they don't pick up item entities), they will still be scheduled for the next tick, e.g. if the container is empty / the hopper full.
But I don't have any timing reports anymore from back then, to support my claim

dktapps · 2023-07-13T12:34:57Z

Sounds like a lava/water type problem.

ShockedPlot7560 · 2023-07-13T12:44:13Z

If ticking is correctly implemented, not necessarily. We can use locks to avoid ticking the block unnecessarily:

If the block above is not valid at pulling: this check is not useful until the block above is updated.
If the hopper's inventory is full: there's no need to check above.
If the inventory is empty, we don't need to look for a block to send the items. (As long as there is no change in the inventory, a lock can be used to limit the number of checks.)
(to be discussed as it removes vanilla functionality) If the block above is full and solid, there's no need to check the entities.

In the future, if an API is implemented that allows things to be executed when an entity enters an area, hopper ticking may be reduced to nothing if no action is possible.
This could be premature optimization in a sense. But on the other hand, with large maps and big servers with lots of loaded hoppers, it can save performance.

dktapps · 2023-07-13T13:15:02Z

Any kind of block ticking becomes a problem if you add enough blocks. That's why most servers ban lava, water, fire etc.

ColinHDev · 2023-07-13T21:46:36Z

If ticking is correctly implemented, not necessarily. We can use locks to avoid ticking the block unnecessarily:

If the block above is not valid at pulling: this check is not useful until the block above is updated.

If the hopper's inventory is full: there's no need to check above.

If the inventory is empty, we don't need to look for a block to send the items. (As long as there is no change in the inventory, a lock can be used to limit the number of checks.)

(to be discussed as it removes vanilla functionality) If the block above is full and solid, there's no need to check the entities.

In the future, if an API is implemented that allows things to be executed when an entity enters an area, hopper ticking may be reduced to nothing if no action is possible. This could be premature optimization in a sense. But on the other hand, with large maps and big servers with lots of loaded hoppers, it can save performance.

As you can see in the push() method, it returns early if the hopper's inventory is empty.
Checking if each slot contains an item, would not satisfy the requirement of the inventory being full, since you would have to check if each item's count is at its max stack size. And I figured it would be premature optimization to additionally loop over the hopper's contents just to return early. Especially since it doesn't save you from rescheduling the block: If the hopper is empty, you still need to check for entities / pullable blocks and for pushable blocks if it's full.
And you can't really check the nearby blocks, don't reschedule the hopper if they don't match and then just rely on the onNearbyBlockChange() method, since there is no guarantee that it's called. You would probably be right most times, but there are likely to be some edge cases: E.g. a hopper on a chunk border facing to the other chunk. onNearbyBlockChange would not be called if that other chunk is changed via. setChunk(). (I just noticed that this case is not even addressed by the plugin... Oops.)

I am not sure if that area system would be a good use for hoppers since you would have to register a separate area for each of them. I'm just theorizing, so it could just be premature optimization, but you could end up having multiple moving item instances checking each tick if they entered any of the 100 hopper areas. (But that depends on how that API is designed. It would probably be less dramatic if you index them a certain way so that you have a reduced loop-up cost.)

dktapps · 2023-07-14T08:47:24Z

But that depends on how that API is designed. It would probably be less dramatic if you index them a certain way so that you have a reduced loop-up cost.

Yeah, I'm aware of the cost. My current thought is to use an octree for this.

ColinHDev · 2023-07-14T09:14:51Z

But that depends on how that API is designed. It would probably be less dramatic if you index them a certain way so that you have a reduced loop-up cost.

Yeah, I'm aware of the cost. My current thought is to use an octree for this.

Seems like a decent choice if there is a good way to implement them in php.

ShockedPlot7560 · 2023-07-31T09:34:56Z

I think #5906 is a better active alternative, could we close this one?

dktapps · 2023-07-31T10:17:25Z

This seems like a copy pasta of #4416 anyway, and I've rarely seen a copy pasta by anyone other than the code author be successful, as they lack sufficient understanding of the code to address reviews

IcyEndymion004 added 2 commits July 2, 2023 13:20

Update Hopper.php

deb0fd7

Update Hopper.php

a8a5212

jasonw4331 added Category: Gameplay Related to Minecraft gameplay experience Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Jul 11, 2023

ShockedPlot7560 requested changes Jul 12, 2023

View reviewed changes

ColinHDev reviewed Jul 12, 2023

View reviewed changes

ShockedPlot7560 mentioned this pull request Jul 17, 2023

World hopper interactions #5906

Open

dktapps closed this Jul 31, 2023

dktapps added the Resolution: Duplicate label Jul 31, 2023


		private const TAG_TRANSFER_COOLDOWN = "TransferCooldown";

		public const DEFAULT_TRANSFER_COOLDOWN = 10;

Uh oh!

Conversation

IcyEndymion004 commented Jul 2, 2023

Uh oh!

ShockedPlot7560 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColinHDev Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dktapps commented Jul 12, 2023

Uh oh!

ColinHDev commented Jul 12, 2023

Uh oh!

ColinHDev Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dktapps commented Jul 12, 2023

Uh oh!

ColinHDev commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dktapps commented Jul 13, 2023

Uh oh!

ShockedPlot7560 commented Jul 13, 2023

Uh oh!

dktapps commented Jul 13, 2023

Uh oh!

ColinHDev commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dktapps commented Jul 13, 2023

Uh oh!

ShockedPlot7560 commented Jul 13, 2023

Uh oh!

dktapps commented Jul 13, 2023

Uh oh!

ColinHDev commented Jul 13, 2023

Uh oh!

dktapps commented Jul 14, 2023

Uh oh!

ColinHDev commented Jul 14, 2023

Uh oh!

ShockedPlot7560 commented Jul 31, 2023

Uh oh!

dktapps commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

ColinHDev Jul 12, 2023 •

edited

Loading

ColinHDev Jul 12, 2023 •

edited

Loading

ColinHDev commented Jul 12, 2023 •

edited

Loading

ColinHDev commented Jul 13, 2023 •

edited

Loading