EngineHub / CraftBook

🔧 Machines, ICs, PLCs, and more!
https://enginehub.org/craftbook/
GNU General Public License v3.0
304 stars 163 forks source link

Adjacent parallel stained pipes with different colours get added to the visitedPipes set despite not being relevant to the pipe system (making adjacent attached loops not work) #1313

Open ANRAR4 opened 1 year ago

ANRAR4 commented 1 year ago

CraftBook Version

3.10.8

Platform Version

paper

Confirmations

Bug Description

Adjacent parallel stained pipes with different colours get added to the visitedPipes set despite not being relevant to the pipe system, causing adjacent loops (attached on one end of the current pipe) of a different colour to be ignored in further processing of the pipe system. The same behaviour would occur when using (stained) glass panes instead of stained glass blocks.

Expected Behavior

In reference to the reproduction steps: The items are introduced to the pipe system in the black stained glass block. Now CraftBook checks all surrounding blocks. The only block relevant to the pipe system is the unstained glass block beneath. After reaching the unstained glass block CraftBook should find any adjacent stained or unstained glass block (in this case the adjacent orange stained glass block). After that it should find the orange stained glass block adjacent to the black stained glass block. With the current behavior this block isn't found as it was marked as visited when checking adjacent blocks to the black stained glass block. After which it should find the piston and reach the left chest.

Reproduction Steps

  1. CraftBook_Pipe_Bug1
  2. Try to send items from the input chest to the output chest

Anything Else?

In https://github.com/EngineHub/CraftBook/blob/master/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L277 the adjacent glass blocks are added to the list of visitedPipes. The check whether these glass blocks are relevant to the current pipe occurs 2 lines later in https://github.com/EngineHub/CraftBook/blob/master/src/main/java/com/sk89q/craftbook/mechanics/pipe/Pipes.java#L279. These codeblocks would need to swap positions. To incorporate the expected behaviour for stained glass panes the code would need further modifications.

me4502 commented 1 year ago

I can move adding to the visitedPipes list to after the check to allow this, it was originally just done for performance reasons. What are the further changes you're referring to?

ANRAR4 commented 1 year ago

The further changes were refering to

The same behaviour would occur when using (stained) glass panes instead of stained glass blocks.

But I just noticed that glass panes (at least the first one) are not checked regularly, so the check for adjacent pipe blocks is skipped, so this problem wouldn't occur there.

While testing i also noticed that the following setup works for transfering items(which might not be intended). If this were to be fixed(or rather if glass_panes could bridge more than one block retaining their functionality, the original problem would also occur for them, as they aren't handled by ItemUtil.isStainedGlass() in Line 279. CraftBook_Pipe_Bug2