Ellpeck / PrettyPipes

Pretty Pipes is a simple to use, all-inclusive item transport mod featuring simple pipes that can be upgraded using modules to accomplish much more advanced tasks.
https://modrinth.com/mod/pretty-pipes
MIT License
21 stars 19 forks source link

[Bug] Drawer Controllers from Storage Drawers don't work reliably with insertion. #131

Closed wompking closed 2 years ago

wompking commented 2 years ago

On my Create: Above & Beyond server, I have a storage system that hinges partially on a row of drawers powered by a single Drawer Controller. When trying to insert items into the Drawer Controller, items can be only added to drawers that do not have a Creative Storage Upgrade attached to them. Any drawers that do have the Creative Storage Upgrade are not recognised as valid inventories that items can be pushed to; however, if you connect them directly instead of over the Controller, they are recognised.

UPDATE: This bug extends solely (???? i think????) to adding items via the Item or Crafting terminals. Extraction via pipes works fine. (still a good idea to check insertion in general, though, my tests could be terrible)

wompking commented 2 years ago

I'm not even sure if this is the problem, but I don't see any other correlations.

Ellpeck commented 2 years ago

Unfortunately, Storage Drawers has been poorly maintained and issue-ridden for a very long time now. It makes improper use of Forge's IItemHandler system and, as such, works incorrectly with a lot of pipe mods. For future (1.18+) modpacks, I recommend checking out Functional Storage instead.

Quinteger commented 2 years ago

@Ellpeck Hi, I'd gladly hear which parts of drawer IItemHandlers are causing problems. I skimmed over the code and it looked fine, except for proper LazyOptional invalidation, which I added in the upcoming update for 1.18.

Ellpeck commented 2 years ago

Hi! Last time this was investigated (which, granted, was a while ago now), there were issues/inconsistencies with how Storage Drawers handles stack sizes over 64. The Forge documentation/team recommends that getStackInSlot returns a single stack/slot for this, and the documentation explicitly states that The result's stack size may be greater than the itemstack's max size. Previously, Storage Drawers would do some custom handling that involved splitting up the items into multiple stacks and slots that changed based on the amounts and items in the drawers (or maybe just outright returning 64 for getStackInSlot when, in reality, there were a lot more items of that type in a drawer?), which meant that inserting and extracting into drawers caused items in pipes to be sent back or not accepted unexpectedly.

I also want to note that there have been much fewer issues with pipe mod interaction with Mekanism's bins (which are essentially one-item drawers), so comparing/cross-referencing with their code might yield some more information.

The LazyOptional invalidation shouldn't be a problem with Pretty Pipes at all, since it doesn't currently cache them (yet).

Ellpeck commented 2 years ago

don't worry i only edited that message like 37 times

wompking commented 2 years ago

relatable

unikmhz commented 2 years ago

Maybe it's the same bug, or maybe it is completely different, but here goes:

Insertion via "non-overflowing" extraction modules to storage drawer controllers does not work also. Before the latest update the only modules with this problem were "high" ones. After the upgrade "medium" ones started to behave similarly, which is expected given the changelog.

This behaviour only manifests for me if the actual drawer has void upgrade installed. If I remove the void upgrade -- then extraction modules of any type start to work normally.

unikmhz commented 2 years ago

I'd like to propose a new augmenting module: to disable overflow detection logic in modules in the same block. That way I can easily make pretty pipes network work with storage drawers and other "interesting" storage mods, without the need to sift through other mod internals and search for incompatibilities.

Ellpeck commented 2 years ago

@Quinteger Hi! I did some more investigation about over-sending prevention for drawers, and I found out something odd that I don't think I understand. When checking for how many items of a certain type can fit into a drawer, two slots on a single-size drawer can be checked: One, which returns a limit of int.MAX_VALUE, and one, which returns the actual limit of the slot (for example, 2048 for a non-upgraded drawer). Because of this, Pretty Pipes thinks that there are actually two slots available for inputting items into, and as such, the over-sending prevention fails.

I found this in the code: https://github.com/jaquadro/StorageDrawers/blob/1.19/src/main/java/com/jaquadro/minecraft/storagedrawers/capabilities/DrawerItemHandler.java#L150 What does this virtual slot mean? How am I supposed to deal with it? Why is there an additional slot that pretends to have an infinite max stack size, when that's not the case?

Thanks so much for any help on this, I'd love to get this finally figured out.

Ellpeck commented 2 years ago

@unikmhz I can't reproduce the issue you're describing in 1.19. The only issue that Pretty Pipes interaction seems to have in 1.19 is that the over-sending prevention fails due to the fact that Pretty Pipes thinks twice as many items can be inserted into the drawer as can actually be inserted. Can you try to reproduce the issue you described with just Pretty Pipes and Storage Drawers installed, and, if you can, let me know if there are any differences in behavior between the 1.18 and 1.19 versions of the mods? Thanks!

Quinteger commented 2 years ago

@Ellpeck Hi! I asked Texelsaur about this a while ago and it turns out that there is indeed a virtual slot at the very beginning. It's supposed to be interacted with when depositing items and will answer on the behalf of the entire network. The reason why this was done is because SD employs a system of priorities for storage slots, like for example a fully-packed slot with a void upgrade, despite being able to intake more items, should be the last one to get them, since they will be simply deleted. It's not perfect and I did want to rewrite the item handlers into a different system that would just collapse slots with the same item into one, but haven't gotten down to it yet.

Ellpeck commented 2 years ago

@wompking @unikmhz Sorry for the many pings today, but I just made a change to Pretty Pipes that should hopefully mitigate any over-sending issues, but also possibly solve the issue that was initially described. It would be very useful if you downloaded the dev version and tried it out with your setups that originally caused these issues and let me know your findings. Thanks so much!! https://ci.ellpeck.de/job/PrettyPipes/job/main/99/artifact/build/libs/PrettyPipes-1.13.5.99.jar

unikmhz commented 2 years ago
  1. Sorry, forgot to add, I'm running 1.18.2.
  2. Tried functional storage mod, with similar results (extractors with oversend-protection refuse to send items to storage controllers with attached storage with voiding upgrade. Immediately after removing voiding upgrade, the items are successfully sent.

Versions:

Ellpeck commented 2 years ago

@unikmhz I also built a 1.18 version for you. Same as with the 1.19 version, the fix I implemented fixes too many items being sent to drawers even though they cannot accept them, and I also cannot reproduce the issues you're describing on the 1.18 version either (although I have not attempted to before implementing this fix, so that might also have fixed it). Please let me know your findings.

https://ci.ellpeck.de/job/PrettyPipes/job/1.18/9/artifact/build/libs/PrettyPipes-1.12.7.9.jar

unikmhz commented 2 years ago

With latest Pretty Pipes mod dev build on 1.18.2 (1.12.7.9) storage drawers mod drawers/controllers work with any type of extraction module, with or without voiding upgrades.

Functional storage mod (1.0.6) behaviour though remains erratic -- working some times, and not working at another, given a controller with attached drawers with voiding upgrades. Non-voiding behaviour is always ok. Maybe it depends on size and orientation of storage drawer blocks around a controller.