Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
234 changes: 232 additions & 2 deletions src/block/Hopper.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,18 @@

namespace pocketmine\block;

use pocketmine\block\inventory\FurnaceInventory;
use pocketmine\block\inventory\HopperInventory;
use pocketmine\block\tile\Container;
use pocketmine\block\tile\Hopper as TileHopper;
use pocketmine\block\utils\PoweredByRedstoneTrait;
use pocketmine\block\utils\SupportType;
use pocketmine\data\runtime\RuntimeDataDescriber;
use pocketmine\entity\object\ItemEntity;
use pocketmine\inventory\Inventory;
use pocketmine\item\Bucket;
use pocketmine\item\Item;
use pocketmine\item\Record;
use pocketmine\math\AxisAlignedBB;
use pocketmine\math\Facing;
use pocketmine\math\Vector3;
Expand Down Expand Up @@ -92,8 +99,231 @@ public function onInteract(Item $item, int $face, Vector3 $clickVector, ?Player
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$tile = $this->position->getWorld()->getTile($this->position);
if(!$tile instanceof TileHopper){
return;
}

$transferCooldown = $tile->getTransferCooldown();
if($transferCooldown > 0){
$transferCooldown--;
$tile->setTransferCooldown($transferCooldown);
}

if($this->isPowered() || $transferCooldown > 0){
return;
}

$inventory = $tile->getInventory();
$success = $this->push($inventory);
// Hoppers that have a container above them, won't try to pick up items.
$origin = $this->position->getWorld()->getTile($this->position->getSide(Facing::UP));
//TODO: Not all blocks a hopper can pull from have an inventory (for example: Jukebox).
if($origin instanceof Container){
$success = $this->pull($inventory, $origin->getInventory()) || $success;
}else{
$success = $this->pickup($inventory) || $success;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
// The cooldown is only set back to the default amount of ticks if the hopper has done anything.
if($success){
$tile->setTransferCooldown(TileHopper::DEFAULT_TRANSFER_COOLDOWN);
}
}

/**
* This function handles pushing items from the hopper to a tile in the direction the hopper is facing.
* Returns true if an item was successfully pushed or false on failure.
*/
private function push(HopperInventory $inventory) : bool{
if(count($inventory->getContents()) === 0){
return false;
}
$destination = $this->position->getWorld()->getTile($this->position->getSide($this->facing));
if($destination === null){
return false;
}

for($slot = 0; $slot < $inventory->getSize(); $slot++){
$item = $inventory->getItem($slot);
if($item->isNull()){
continue;
}

// Hoppers interact differently when pushing into different kinds of tiles.
//TODO: Composter
//TODO: Brewing Stand
//TODO: Jukebox (improve)
if($destination instanceof \pocketmine\block\tile\Furnace){
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@ColinHDev ColinHDev Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// If the hopper is facing down, it will push every item to the furnace's input slot, even items that aren't smeltable.
// If the hopper is facing in any other direction, it will only push items that can be used as fuel to the furnace's fuel slot.
if($this->facing === Facing::DOWN){
$slotInFurnace = FurnaceInventory::SLOT_INPUT;
$itemInFurnace = $destination->getInventory()->getSmelting();
}else{
if($item->getFuelTime() === 0){
continue;
}
$slotInFurnace = FurnaceInventory::SLOT_FUEL;
$itemInFurnace = $destination->getInventory()->getFuel();
}
if(!$itemInFurnace->isNull()){
if($itemInFurnace->getCount() >= $itemInFurnace->getMaxStackSize()){
return false;
}
if(!$itemInFurnace->canStackWith($item)){
continue;
}
$item->pop();
$itemInFurnace->setCount($itemInFurnace->getCount() + 1);
}else{
$itemInFurnace = $item->pop();
}

//TODO: event on item inventory switch

$destination->getInventory()->setItem($slotInFurnace, $itemInFurnace);
$inventory->setItem($slot, $item);
return true;

}elseif($destination instanceof TileHopper){
$itemToPush = $item->pop();
if(!$destination->getInventory()->canAddItem($itemToPush)){
continue;
}
// Hoppers pushing into empty hoppers set the empty hoppers transfer cooldown back to the default amount of ticks.
if(count($destination->getInventory()->getContents()) === 0){
$destination->setTransferCooldown(TileHopper::DEFAULT_TRANSFER_COOLDOWN);
}

}elseif($destination instanceof \pocketmine\block\tile\Jukebox){
if(!($item instanceof Record)){
continue;
}
//TODO:
// Jukeboxes actually emit a redstone signal when playing a record so nearby hoppers are blocked and
// prevented from inserting another disk. Because neither does redstone work properly nor can we check if
// a jukebox is still playing a record or has already finished it, we can just check if it has already a
// record inserted.
if($destination->getRecord() !== null){
return false;
}

// The Jukebox block is handling the playing of records, so we need to get it here and can't use TileJukebox::setRecord().
$jukeboxBlock = $destination->getBlock();
if($jukeboxBlock instanceof Jukebox){
$jukeboxBlock->insertRecord($item->pop());
$jukeboxBlock->getPosition()->getWorld()->setBlock($jukeboxBlock->getPosition(), $jukeboxBlock);
$inventory->setItem($slot, $item);
return true;
}
return false;

}elseif($destination instanceof Container){
$itemToPush = $item->pop();
if(!$destination->getInventory()->canAddItem($itemToPush)){
continue;
}

}else{
return false;
}

//TODO: event on item inventory switch

$inventory->setItem($slot, $item);
$destination->getInventory()->addItem($itemToPush);
return true;
}
return false;
}

//TODO: redstone logic, sucking logic
/**
* This function handles pulling items by the hopper from a container above.
* Returns true if an item was successfully pulled or false on failure.
*/
private function pull(HopperInventory $inventory, Inventory $origin) : bool{
// Hoppers interact differently when pulling from different kinds of tiles.
//TODO: Composter
//TODO: Brewing Stand
//TODO: Jukebox
if($origin instanceof FurnaceInventory){
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same argument as for the push logic.

// Hoppers either pull empty buckets from the furnace's fuel slot or pull from its result slot.
// They prioritise pulling from the fuel slot over the result slot.
$item = $origin->getFuel();
if($item instanceof Bucket){
$slot = FurnaceInventory::SLOT_FUEL;
}else{
$slot = FurnaceInventory::SLOT_RESULT;
$item = $origin->getResult();
if($item->isNull()){
return false;
}
}
$itemToPull = $item->pop();

//TODO: event on item inventory switch

$origin->setItem($slot, $item);
$inventory->addItem($itemToPull);
return true;

}else{
for($slot = 0; $slot < $origin->getSize(); $slot++){
$item = $origin->getItem($slot);
if($item->isNull()){
continue;
}
$itemToPull = $item->pop();
if(!$inventory->canAddItem($itemToPull)){
continue;
}

//TODO: event on item inventory switch

$origin->setItem($slot, $item);
$inventory->addItem($itemToPull);
return true;
}
}
return false;
}

/**
* This function handles picking up items by the hopper.
* Returns true if an item was successfully picked up or false on failure.
*/
private function pickup(HopperInventory $inventory) : bool{
// In Bedrock Edition hoppers collect from the lower 3/4 of the block space above them.
$pickupCollisionBox = new AxisAlignedBB(
$this->position->getX(),
$this->position->getY() + 1,
$this->position->getZ(),
$this->position->getX() + 1,
$this->position->getY() + 1.75,
$this->position->getZ() + 1
);

foreach($this->position->getWorld()->getNearbyEntities($pickupCollisionBox) as $entity){
if($entity->isClosed() || $entity->isFlaggedForDespawn() || !$entity instanceof ItemEntity){
continue;
}
// Unlike Java Edition, Bedrock Edition's hoppers don't save in which order item entities landed on top of them to collect them in that order.
// In Bedrock Edition hoppers collect item entities in the order in which they entered the chunk.
// Because of how entities are saved by PocketMine-MP the first entities of this loop are also the first ones who were saved.
// That's why we don't need to implement any sorting mechanism.
$item = $entity->getItem();
if(!$inventory->canAddItem($item)){
continue;
}

//TODO: event on block picking up an item

$inventory->addItem($item);
$entity->flagForDespawn();
return true;
}
return false;
}
}
12 changes: 12 additions & 0 deletions src/block/tile/Hopper.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ class Hopper extends Spawnable implements Container, Nameable{

private const TAG_TRANSFER_COOLDOWN = "TransferCooldown";

public const DEFAULT_TRANSFER_COOLDOWN = 10;
Copy link
Copy Markdown
Contributor

@ColinHDev ColinHDev Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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




Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary extra spaces.

private HopperInventory $inventory;
private int $transferCooldown = 0;

Expand Down Expand Up @@ -76,4 +80,12 @@ public function getInventory() : HopperInventory{
public function getRealInventory() : HopperInventory{
return $this->inventory;
}

public function getTransferCooldown() : int{
return $this->transferCooldown;
}

public function setTransferCooldown(int $transferCooldown) : void{
$this->transferCooldown = $transferCooldown;
}
}