Skip to content
Merged
21 changes: 8 additions & 13 deletions src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java
Original file line number Diff line number Diff line change
Expand Up @@ -351,30 +351,25 @@ 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)) {
for (ItemStack stack : ((InventoryHolder) fac.getState()).getInventory().getContents()) {
if (InventoryUtil.hasGenericInventory(facType)) {
org.bukkit.block.BlockState facState = fac.getState();
for (ItemStack stack : ((InventoryHolder) facState).getInventory().getContents()) {

if (!ItemUtil.isStackValid(stack))
continue;

if(!ItemUtil.doesItemPassFilters(stack, filters, exceptions))
continue;

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

if (!items.isEmpty())
InventoryUtil.syncDisplayedContainer(facState);

PipeSuckEvent event = new PipeSuckEvent(block, new ArrayList<>(items), fac);
Bukkit.getPluginManager().callEvent(event);
items.clear();
Expand Down
74 changes: 65 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);
//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);

return leftovers;
}
Expand Down Expand Up @@ -339,13 +343,70 @@ 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 state The container's state, from the caller that mutated it.
*/
public static void syncDisplayedContainer(org.bukkit.block.BlockState state) {
Material type = state.getType();
if (type == Material.CHISELED_BOOKSHELF) {
if (!(state instanceof ChiseledBookshelf))
return;
Inventory inv = ((ChiseledBookshelf) state).getInventory();
Block block = state.getBlock();
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)) {
// update() writes the state's captured contents back into the
// world, so this one must be captured after the mutation; the
// caller's state predates it and would restore the old contents.
state.getBlock().getState().update(true, false);

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'm a little confused by this bit here, this should theoretically be a no-op. What's it actually doing?

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.

Data-wise it is, the point is the side effect: update() ends with chunkSource.blockChanged(pos), which resends the block entity to clients. Vanilla only rebroadcasts shelf contents on player interaction, so without this line clients keep rendering the moved items until the chunk reloads. A/B tested on a live server: same setup, without the line the shelf keeps showing the items after they left, with it the display clears immediately.

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.

But wouldn't the initial update that's actually setting the block contents do that too? This feels like it's a Paper bug that should be reported there, as-is this would substantially increase the overhead of the inventory interactions

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.

You were right, it was a Paper bug. Removing items through the Bukkit inventory skipped the client update, fixed in PaperMC/Paper#13567 a couple of weeks ago. Verified on a current build and removed the resync from this PR.

}
}

/**
* 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 +424,10 @@ 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;
// Shelves are matched by tag because new wood types keep adding materials.
return Tag.WOODEN_SHELVES.isTagged(type);
}
}

Expand Down
Loading