Treat shelves as containers in pipes - #1376
Conversation
The shelf block entity implements Container, so vanilla hoppers insert into and extract from its three slots. This lets pipes do the same: wooden_shelves joins the container check and the pipe pull list, and insertion goes through the ordinary inventory path.
Adding shelves meant editing the same generic-container list in two places, the pipe pull check and doesBlockHaveInventory, which is how the two drift. hasGenericInventory now owns the family (plain item slots, safe to insert or pull generically); doesBlockHaveInventory is that family plus the four role-slot blocks whose callers route them through dedicated branches. The next container type lands in one place.
9d08c5b to
02ad3f1
Compare
Two findings from testing shelves on a live 26.2 server. Some containers hand out a mirror of the slot from getContents, so removing the slot contents also empties the stack the pipe is carrying and the pull delivers air; pulled stacks are cloned before removal. And shelves render their contents but vanilla only resyncs the display on player interaction, so pipe mutations leave clients showing stale items until the chunk reloads; a state captured after the mutation is written back for shelf pulls and inserts. Capture order matters: updating a state from before the removals writes the old contents back into the world.
|
Added a third commit after testing shelves on a live 26.2 server. Two things came up: pulled stacks need to be cloned before the slot is cleared (the stack handed out mirrors the slot on shelves, so the pipe was carrying air), and shelf displays need a post-mutation state update since vanilla only resyncs them on player interaction. The state has to be captured after the removal; updating one from before it writes the old contents back. |
Comparing the shelf handling against its nearest neighbour showed the same bug class has been sitting in chiseled bookshelves: a pipe edit lands in the inventory but the slot_X_occupied blockstate never moves, so the shelf renders wrong and the comparator signal lies. The two publish mechanisms differ (bookshelves use blockstate properties, shelves block-entity data), so syncDisplayedContainer owns both and the pull path, the shared insert path and the bookshelf insert helper all call it. Verified both ways on a 26.2 server: occupancy set on insert, cleared on pull, including non-contiguous slots and enchanted books.
|
Fourth commit: while comparing the shelf handling against chiseled bookshelves I found the same bug class has been there all along, pipe edits update the bookshelf inventory but never the slot_X_occupied blockstate, so the block renders wrong and the comparator signal lies. The shelf resync is now a shared syncDisplayedContainer that handles both publish mechanisms, called from the pull path and both insert paths. Verified both directions on a 26.2 server, including non-contiguous slots and enchanted books. |
| } | ||
|
|
||
| /** | ||
| * Checks whether a material is a container whose whole inventory is plain |
There was a problem hiding this comment.
This javadoc seems to be in the wrong spot, not attached to the actual method
| } | ||
|
|
||
| public static boolean hasGenericInventory(Material type) { | ||
| if (Tag.SHULKER_BOXES.isTagged(type) || Tag.WOODEN_SHELVES.isTagged(type)) { |
There was a problem hiding this comment.
This is probably worthwhile moving into the default branch of the switch statement, as the switch lookup is much faster than a tag check
There was a problem hiding this comment.
Done, moved into the default branch.
There was a problem hiding this comment.
The shulker colours are switch cases again too, so only the shelf family pays a tag lookup, and only on a switch miss.
… the tags Review feedback: the javadoc had drifted away from its method, and the enum switch is a faster first check than the tag lookups, which now sit in the default branch.
Benchmarked the three shapes: enumerating every stable material as a switch case and leaving only the open-ended shelf family in the default branch is fastest on every input class, and the dominant caller is a non-container block that previously fell through to two tag lookups. The shulker colour set has been fixed since 1.11, so the case list cannot go stale the way a wood-type list would.
| ((Chest) ((DoubleChestInventory) container.getInventory()).getRightSide().getHolder()).update(true); | ||
| } | ||
| if (container instanceof org.bukkit.block.BlockState) | ||
| syncDisplayedContainer(((org.bukkit.block.BlockState) container).getBlock()); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| || facType == Material.DECORATED_POT | ||
| || Tag.SHULKER_BOXES.isTagged(facType)) { | ||
| if (InventoryUtil.hasGenericInventory(facType)) { | ||
| for (ItemStack stack : ((InventoryHolder) fac.getState()).getInventory().getContents()) { |
There was a problem hiding this comment.
Can probably extract the state here onto the above line, and then pass it in to syncDisplayedContainer later on here
| return true; | ||
| default: | ||
| return false; | ||
| // Only the open-ended family stays a tag lookup: shelf materials |
There was a problem hiding this comment.
This comment shouldn't be here, it just describes how it's changed since the last commit
Creating a state is expensive on Spigot, so the pull loop hoists one and hands it on, and the insert paths pass the holder they already have. The shelf arm is the one exception and re-captures internally: update() writes the state's captured contents back into the world, so it needs a capture from after the mutation.
| // 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); |
There was a problem hiding this comment.
I'm a little confused by this bit here, this should theoretically be a no-op. What's it actually doing?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
All eight variants run through ChestBlock and the ordinary chest state, so doubles and insertion already behave like chests; the only thing missing was family membership. The set is four oxidation stages waxed or not, closed, so they are switch cases rather than a tag.
|
Added copper chests as a last commit since they are the same concern, new containers joining the family predicate. All eight variants run through ChestBlock, so membership is all they needed; tested on a 26.2 server with a pipe pull between two copper chests. Closed #1377 in favour of this. |
2 participants
The shelf block entity implements Container, so vanilla hoppers insert into and extract from its three slots. Pipes should match: this adds the wooden_shelves tag to the container checks. Insertion needs nothing special since shelves are plain item slots.
Doing that meant editing the same generic-container list in two places (the pipe pull check and doesBlockHaveInventory), so the second commit extracts one hasGenericInventory predicate both call: the family of plain-slot containers, with furnaces, smokers, blast furnaces and brewing stands kept separate since their role-specific slots have dedicated branches at the call sites. The next container type lands in one place.
Tested the same change on a shelf-aware server in my fork: pipes push into a shelf and pull from one, other containers unaffected.