Malformed Jukebox-logic allows to duplicate discs indefinitely

[BlvckBytes]

CraftBook Version

craftbook-3.10.13-SNAPSHOT

Platform Version

Paper version 1.21.10-101-main@893ea74

Confirmations

• [x] I am using the most recent Minecraft release.
• [x] I am using a version of WorldEdit compatible with my Minecraft version.
• [x] I am using a version of CraftBook compatible with my Minecraft version.
• [x] I am using the latest or recommended version of my platform software.
• [x] I am NOT using a hybrid server, e.g. a server that combines Bukkit and Forge. Examples include Arclight, Mohist, and Cardboard.
• [x] I am NOT using a fork of WorldEdit, such as FastAsyncWorldEdit (FAWE) or AsyncWorldEdit (AWE)

Bug Description

Since the logic of Jukeboxes is malformed both while sucking and while putting, one can duplicate discs indefinitely. Here's a short preview outlining the situation:

https://streamable.com/po7ze6

This also works if the pipe simply had either no containers attached, or if all of these containers couldn't hold another of the disc in question.

Expected Behavior

The disc should simply be transported to the other Jukebox.

Reproduction Steps

1. Set up two empty Jukeboxes, one on a sticky- and one on a normal piston
2. Connect the pistons with glass-blocks
3. Add an input-clock (or clock manually)
4. Put a disc into the input - notice how it is dropped over and over again
5. Remove the disc manually - now the cycle stops

Anything Else?

No response

[me4502]

This might just require jukeboxes to be removed as a possible pipe target tbh. I had similar issues with API inconsistencies with the redstone jukebox mechanic using the Spigot APIs as well, where it'd report the wrong "fullness" state and lead to weird results. Using the Paper APIs in CraftBook 5 for the redstone jukebox mechanic worked fine however. This might just need to be disabled in CB3, and then re-enabled in CB5 to utilise Paper APIs.

[BlvckBytes]

I considered simply removing them too, but then I discovered that the API works just fine - the logic within Pipes is itself malformed.

Items are only ever put into Jukeboxes if they are not already empty (wrong way around):

https://github.com/EngineHub/CraftBook/blob/5ac877c76d67a7bb85b02e9efc54ba1989d4e76b/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L185

And after the item has been sucked out of the Jukebox, the record is not immediately cleared from it:

https://github.com/EngineHub/CraftBook/blob/5ac877c76d67a7bb85b02e9efc54ba1989d4e76b/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L432

It is only cleared if the disc found a container to be stored in after walking the pipe:

https://github.com/EngineHub/CraftBook/blob/5ac877c76d67a7bb85b02e9efc54ba1989d4e76b/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L450

So the above exactly describes the bug we're facing. I've fixed this in my fork and am going to further test Jukeboxes in all kinds of situations manually to ensure that there really isn't anything wrong with the API itself. I do understand why there's a lot of confusion around this mechanic, seeing how messy the code-base really is... I gave my best to untangle it, and only afterwards have I been able to build a proper mental model.

[me4502]

A PR would be appreciated. Although it seems like it's meant to be only cleared if a container is found, Pipes in general don't modify the "starting" container until the item has actually found a place to move to.

The issues with the redstone Jukebox mechanic might be on the other end, where "getting" the currently playing item was inverted upon the record actually completing. It's been a bit since I touched it, but there was something Paper did that made it work again (which is what CraftBook 5 does)- as that's more about actually triggering play/pause, it might not really matter here

[BlvckBytes]

Pipes most definitely do modify the input-container ahead of time, before even starting to walk the pipe; just check out the examples for chest-like inventories as well as furnaces:

https://github.com/EngineHub/CraftBook/blob/5ac877c76d67a7bb85b02e9efc54ba1989d4e76b/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L373

https://github.com/EngineHub/CraftBook/blob/5ac877c76d67a7bb85b02e9efc54ba1989d4e76b/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L407

The containers above are backed by live inventories, so the removals take immediate effect. Of course, one could also only remove them if they actually found a slot to be put in, but this seems to be the easier and safer option overall.

I would like to open a PR, but it is hard for me to just fix this issue in isolation, seeing how my fork is putting lots of effort into readability and especially performance, and I didn't exactly start by fixing the Jukebox. In general, I believe that the only way to rescue this mechanic long-term is by taking stronger measures like these - we cannot simply continue to put band-aids on what is effectively a legacy codebase. I will present my approach ASAP, including a summary of my changes, and hope that we can discuss it in a constructive manner that will benefit the parts of the community who had this mechanic grow onto them as much as it did onto me.