Fix double chests double-counting their viewers - #604
Open
KostiaFed wants to merge 1 commit into
Open
Conversation
The Bukkit-level viewer hooks on CompoundContainer called the vanilla startOpen/stopOpen, which drive ContainerOpenersCounter. ChestMenu already calls startOpen once per open and CompoundContainer fans it out to both halves, so every viewer of a large chest was counted twice. The five-tick recheck corrects the count to 1 while the chest is still open, so the two decrements on close take the counter to -1: the first plays the close sound, and the pending recheck then sees 0 openers against a stored -1 and fires onClose a second time. Held open for less than five ticks the sequence happens to balance, which is why the symptom is erratic - a second close sound, or a lid and redstone output stuck in the wrong state, since a counter left negative never makes the 0 -> 1 transition that fires onOpen and schedules the next recheck. Delegate to the halves' onOpen/onClose instead, as CraftBukkit does, so each half still tracks the viewer for getViewers() while only vanilla touches the opener count. Also fix the InventoryOpenEvent cancel path, which leaked an opener per half on large chests: a large chest arrives as the MenuProvider that DoubleBlockCombiner builds, so neither the Container nor the CompoundContainer branch matched and the constructor's startOpen was never undone. The CompoundContainer branch was unreachable anyway - CompoundContainer is a Container, so the first branch would have claimed it - and it only stopped container1. Take the container from the menu that was created instead, and let CompoundContainer.stopOpen fan out to both halves. Follow-up to CardboardPowered#570, which fixed the same double count on the other path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #570, which fixed the same double count on the other path — that is why shulker boxes got better and chests did not.
Problem
The Bukkit-level viewer hooks on
CompoundContainercalled the vanillastartOpen/stopOpen, which driveContainerOpenersCounter:CraftBukkit calls
container1.onOpen(who)there — transaction tracking only.ChestMenu's constructor already callsstartOpenonce, andCompoundContainer.startOpenfans that out to both halves, so every viewer of a large chest was counted twice. Single chests are unaffected:ChestBlockEntity'sonOpenonly adds totransaction, so the count stays balanced. Large chests are affected becauseCraftInventoryDoubleChestwraps theCompoundContainer.Counted against 26.1.2 bytecode (
ChestMenuctor → onestartOpen;removed→ onestopOpen):ChestMenuctor →startOpenscheduleRecheck(+5 ticks)transferTo→onOpen→startOpenrecheckOpenersfive ticks latertransferTo→onClose→stopOpenChestMenu.removed→stopOpendecrementOpenerstestsopenCount == 0)actual == 0branch → second close soundHence the erratic behaviour. Closed within five ticks, the sequence happens to balance (2→1→0) and everything is correct. Held open longer — nearly always — the counter ends at −1 and a second close sound arrives. While it sits negative, the next open never makes the 0→1 transition, so
onOpendoes not fire and no recheck is scheduled: the lid animation and the trapped-chest redstone output are left in the wrong state.Changes
CompoundContainerMixin—onOpen/onClosedelegate to the halves'ContainerBridge.onOpen/onClose, as CraftBukkit does. Each half still records the viewer in its owntransaction(needed for per-halfgetViewers()), and only vanilla touches the opener count, so it moves exactly +1/−1.ServerPlayerMixin— theInventoryOpenEventcancel path leaked an opener per half on large chests. A large chest arrives as the anonymousMenuProviderthatDoubleBlockCombinerbuilds, so neitherfactory instanceof Containernorfactory instanceof CompoundContainermatched and the constructor'sstartOpenwas never undone (+1 per half, lid stuck open). TheCompoundContainerbranch was unreachable regardless —CompoundContaineris aContainer, so the first branch would have claimed it — and it only stoppedcontainer1. Now, whenfactoryis not aContainer, the container is taken from theChestMenuthat was created, andCompoundContainer.stopOpenfans out to both halves.Scope and testing
gradlew compileJavapasses. This is code and bytecode analysis, not an in-game test — worth verifying on a live server.I found no counting asymmetry on the pure single-chest path: block click,
player.openInventory, client close, server-sidecloseContainer(), and player disconnect all balance +1/−1. Reports of single chests misbehaving are most likely halves of large chests, or a half whose counter was already driven negative and stays broken after the pair is split.