Fix three jukebox item bugs in pipes - #1372
Conversation
The jukebox branch created a new disc ItemStack and only emptied the jukebox if every item was delivered. When the network was full or missing, the copy dropped at the piston while the original kept playing, so a clocked piston printed discs. It was also the only branch that never fed leftovers, so an empty jukebox silently destroyed any items a pipe request delivered into it. Now the disc is removed from the jukebox before the event fires, and undelivered items go through leftovers like the other branches.
The put-path test was inverted: a disc was only inserted when the jukebox was already playing, so an empty jukebox never accepted one, and when it did fire, setPlaying overwrote the playing disc and destroyed it. Now only an empty jukebox accepts a disc, and a disc that does not fit flows on through the pipe like any other undelivered item.
RasmusKD
changed the title
| Jukebox juke = (Jukebox) fac.getState(); | ||
| List<ItemStack> its = new ArrayList<>(event.getItems()); | ||
| if (juke.getPlaying() != Material.AIR) { | ||
| // Only an empty jukebox accepts a disc. The test was inverted, so |
There was a problem hiding this comment.
Are you able to please remove these comments that just comment on how it's changed from the previous state? They don't really add much to the code itself
| // an empty jukebox never took one, and a playing one had its disc | ||
| // overwritten by setPlaying and destroyed. Discs that do not fit | ||
| // stay in the list and flow on through the pipe. | ||
| if (juke.getPlaying() == Material.AIR) { |
There was a problem hiding this comment.
From memory this was done the old way because when getPlaying was AIR, it would fail to actually accept something (and silently eat the item). This might no longer be the case, but worth testing/validating on Spigot
There was a problem hiding this comment.
Tested it on a 1.21 server. Pushing a disc through a pipe into an empty jukebox works: the disc ends up in the jukebox, the block ends up as jukebox[has_record=true], and nothing is dropped or lost. CraftJukebox.setRecord sets HAS_RECORD from the item and update() writes the block back, so the old failure looks like it is gone.
Also checked the other direction, pulling from a playing jukebox into a chest gives exactly one disc, and when the delivery is refused exactly one disc drops at the piston instead of two.
| juke.update(); | ||
| } | ||
|
|
||
| if (!items.isEmpty()) { |
There was a problem hiding this comment.
Does this actually fix anything? From memory it was actually requested by other plugins that we still call this event even when nothing is pulled in, so this is potentially an unwanted behavioural change
There was a problem hiding this comment.
Good point, I dropped that. The event now fires unconditionally, same as the other branches.
2 participants
Three separate bugs in how Pipes handles jukeboxes, all reproducible on a test server.
1. Disc duplication (suck path). The branch creates a new disc ItemStack and only empties the jukebox if every item was delivered. If the network is full or missing, the copy drops at the piston while the original keeps playing. A clocked sticky piston facing a playing jukebox prints discs.
Repro: jukebox with a disc playing, sticky piston facing it, no pipe network. Pulse the piston. A disc drops and the jukebox keeps playing. Repeat for more discs.
2. Item loss (suck path). It is the only branch that never feeds leftovers, so when the jukebox is empty, any items a pipe request delivers into it are cleared at the end of startPipe and destroyed.
3. Item loss (put path). The insert test is inverted: a disc is only inserted when the jukebox is already playing, so an empty jukebox never accepts one. When it does fire, setPlaying overwrites the playing disc and destroys it.
Repro: chest with a disc, pipe into a jukebox that is already playing a different disc. Two discs before, one after.
Fixes. For the suck path, take the disc out of the jukebox before firing the event and route undelivered items through leftovers like the other branches. For the put path, only insert into an empty jukebox and let discs that do not fit flow on through the pipe.
One behavior change worth noting: a disc the network refuses now drops at the piston instead of staying in the jukebox. That is what stops the duplication.