Skip to content
Merged
19 changes: 8 additions & 11 deletions src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java
Original file line number Diff line number Diff line change
Expand Up @@ -351,16 +351,7 @@ private void startPipe(Block block, List<ItemStack> items, boolean request) {
Block fac = block.getRelative(p.getFacing());
Material facType = fac.getType();

if (facType == Material.CHEST
|| facType == Material.TRAPPED_CHEST
|| facType == Material.DROPPER
|| facType == Material.DISPENSER
|| facType == Material.HOPPER
|| facType == Material.BARREL
|| facType == Material.CHISELED_BOOKSHELF
|| facType == Material.CRAFTER
|| facType == Material.DECORATED_POT
|| Tag.SHULKER_BOXES.isTagged(facType)) {
if (InventoryUtil.hasGenericInventory(facType)) {
for (ItemStack stack : ((InventoryHolder) fac.getState()).getInventory().getContents()) {

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.

Can probably extract the state here onto the above line, and then pass it in to syncDisplayedContainer later on here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.


if (!ItemUtil.isStackValid(stack))
Expand All @@ -369,12 +360,18 @@ private void startPipe(Block block, List<ItemStack> items, boolean request) {
if(!ItemUtil.doesItemPassFilters(stack, filters, exceptions))
continue;

items.add(stack);
items.add(stack.clone());
((InventoryHolder) fac.getState()).getInventory().removeItem(stack);
if (pipeStackPerPull)
break;
}

// syncDisplayedContainer captures its state AFTER the removals;
// updating a state from before them writes the old contents
// back into the world.
if (!items.isEmpty())
InventoryUtil.syncDisplayedContainer(fac);

PipeSuckEvent event = new PipeSuckEvent(block, new ArrayList<>(items), fac);
Bukkit.getPluginManager().callEvent(event);
items.clear();
Expand Down
73 changes: 64 additions & 9 deletions src/main/java/com/sk89q/craftbook/util/InventoryUtil.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.sk89q.craftbook.util;

import org.bukkit.Material;
import org.bukkit.Tag;
import org.bukkit.block.Block;
import org.bukkit.block.BrewingStand;
import org.bukkit.block.Chest;
Expand Down Expand Up @@ -67,6 +68,8 @@ public static List<ItemStack> addItemsToInventory(InventoryHolder container, boo
((Chest) ((DoubleChestInventory) container.getInventory()).getLeftSide().getHolder()).update(true);
((Chest) ((DoubleChestInventory) container.getInventory()).getRightSide().getHolder()).update(true);
}
if (container instanceof org.bukkit.block.BlockState)
syncDisplayedContainer(((org.bukkit.block.BlockState) container).getBlock());

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 method already re-gets the state, are you able to make it pass in the state directly please? Creating the state is an extremely expensive operation on Spigot

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. One exception inside the method: the shelf branch re-captures its own state, since update() writes the captured contents back and a state from before the removals would restore the pulled items. Comment in the method explains it.

//if(container instanceof BlockState && update)
// ((BlockState) container).update();
return leftovers;
Expand Down Expand Up @@ -211,6 +214,7 @@ public static List<ItemStack> addItemsToChiseledBookshelf(ChiseledBookshelf chis
stacks = Arrays.stream(stacks).filter(item -> ItemUtil.isAStorableBook(item)).toArray(ItemStack[]::new);

leftovers.addAll(chiseledBookshelf.getInventory().addItem(stacks).values());
syncDisplayedContainer(chiseledBookshelf.getBlock());

return leftovers;
}
Expand Down Expand Up @@ -339,13 +343,67 @@ public static boolean fitsInSlot(ItemStack stack, ItemStack slot) {
public static boolean doesBlockHaveInventory(Block block) {

switch(block.getType()) {
case FURNACE:
case BLAST_FURNACE:
case SMOKER:
case BREWING_STAND:
return true;
default:
return hasGenericInventory(block.getType());
}
}

/**
* Publishes a display container's contents after a plugin-side mutation.
* Player interaction is the only thing vanilla syncs on, so a pipe edit
* otherwise leaves clients rendering stale contents. The two mechanisms
* differ: chiseled bookshelves render and emit comparator signal from
* their slot_X_occupied blockstate properties, while shelves render
* straight from block-entity data, which a fresh post-mutation state
* update rebroadcasts. Every other container is a no-op here.
*
* @param block The container block that was mutated.
*/
public static void syncDisplayedContainer(Block block) {
Material type = block.getType();
if (type == Material.CHISELED_BOOKSHELF) {
org.bukkit.block.BlockState state = block.getState();
if (!(state instanceof ChiseledBookshelf))
return;
Inventory inv = ((ChiseledBookshelf) state).getInventory();
org.bukkit.block.data.BlockData data = block.getBlockData();
if (!(data instanceof org.bukkit.block.data.type.ChiseledBookshelf))
return;
org.bukkit.block.data.type.ChiseledBookshelf occupancy = (org.bukkit.block.data.type.ChiseledBookshelf) data;
for (int i = 0; i < inv.getSize(); i++)
occupancy.setSlotOccupied(i, ItemUtil.isStackValid(inv.getItem(i)));
block.setBlockData(occupancy, false);
} else if (Tag.WOODEN_SHELVES.isTagged(type)) {
block.getState().update(true, false);
}
}

/**
* Checks whether a material is a container whose whole inventory is plain
* item slots, safe to insert into or pull from generically. Furnaces,
* smokers, blast furnaces and brewing stands are containers too but have
* role-specific slots (fuel, ingredient, result), so their callers route
* them through dedicated branches instead of this family.
*
* @param type The material to check.
* @return If the material is a generic container.
*/
public static boolean hasGenericInventory(Material type) {
switch(type) {
case CHEST:
case TRAPPED_CHEST:
case DROPPER:
case DISPENSER:
case FURNACE:
case BREWING_STAND:
case HOPPER:
case BARREL:
case CHISELED_BOOKSHELF:
case DECORATED_POT:
case CRAFTER:
case WHITE_SHULKER_BOX:
case ORANGE_SHULKER_BOX:
case MAGENTA_SHULKER_BOX:
Expand All @@ -363,15 +421,12 @@ public static boolean doesBlockHaveInventory(Block block) {
case BLACK_SHULKER_BOX:
case RED_SHULKER_BOX:
case SHULKER_BOX:
case BLAST_FURNACE:
case SMOKER:
case BARREL:
case CHISELED_BOOKSHELF:
case DECORATED_POT:
case CRAFTER:
return true;
default:
return false;
// Only the open-ended family stays a tag lookup: shelf materials

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 comment shouldn't be here, it just describes how it's changed since the last commit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

// grow with each new wood type, while the shulker colour set has
// been fixed since 1.11.
return Tag.WOODEN_SHELVES.isTagged(type);
}
}

Expand Down
Loading