Sophisticated storage bug

RevBeast commented 7 months ago

Issue type: Bug

:bug: Bug

Short description:

Potentially two bugs: 1) Storage terminal does not view the items inside of sophisticated storage chests/barrels (found testing these two mods alone), 2) IntegratedTerminals does not recognize the basic chest from the "sophisticated storage" mod as storage (in chosen's modded adventures).

Steps to reproduce the problem:

bug 1:get a chest/barrel (warped barrel and a diamond chest were used by me) from sophisticated storage mod, put items in it, attach an item interface to it and link that to a storage terminal with a logic cable. The storage terminal will work fine and show the items of the inventory. Break the inventory and place it back again in the same spot and put items inside. The storage terminal will not show the items inside the chest anymore (even if the terminal/cable/interface were broken and placed back again). I found this while testing these two mods alone as instructed below.

bug 2: on chosen's modded adventures modpack, the original bug is that when you use a base tier of a chest/barrel (ex. "oak chest") The storage terminal will NOT show the items inside. If you try this with any other tier of the chest (iron+), the mod has no issues. I tested bug 1 in this modpack and it occurs aswell. Bug 2 might be a byproduct of bug 1.

Expected behaviour:

For this mod to view the contents of the chests/barrels from that mod with no issues regardless of the tier and inventory replacement.

Versions:

This mod: IntegratedTerminals-1.20.1-1.4.11 (played on the "chosen's modded adventures" modpack v.0.4.3). Sophisticated storage used: sophisticatedstorage-1.20.1-0.8.58.644.
Minecraft: 1.20.1
Forge: 47.1.3

rubensworks commented 7 months ago

Thanks for reporting!

rubensworks commented 7 months ago

Based on what you describe, there may be some issues with Sophisticated storage's item handler capability implementation. Could you report it to their issue tracker? Happy to make changes here if the mod author deems it necessary.

Could you add a link to the issue here?

RevBeast commented 7 months ago

Sorry for the inconvenience, reported: https://github.com/P3pp3rF1y/SophisticatedStorage/issues/316

rubensworks commented 7 months ago

Thanks! :-)

P3pp3rF1y commented 7 months ago

I took a look at this and the problem is down to the combinations of two things

integrated pulls item handler only once as long as block that is in place still continues to have item handler capability
sophisticated storage setting block to changed as part of some of the initialization in a few places

So as the item handler is getting initialized Integrated gets notified of block change, tries to get capability and pulls a different instance of item handler as initialization hasn't completed yet. So block ends up with initialized instance of handler that is different from the one that Integrated now uses, but because Integrated doesn't refresh the handler as long as the block still supports item handler capability it doesn't refresh the handler on any subsequent change notifications.

I tried making it so that setChanged would get called only once but got to the point where the code was getting already very bloated and still had to work with additional cases. Also there should be no reason to work with actual item handler instead of item handler cap and if you work with item handler cap you will always get the correct item handler and you will get notified when that gets invalidated so that new cap can be retrieved from the block.

So basically I feel this needs to be fixed on Integrated side by either supporting item handler cap as it is designed or by updating item handler when attached block notifies it has changed.

rubensworks commented 7 months ago

Hmm, I don't remember implementing any caching of capabilities. But I could be just forgetting it...

Thanks for looking into this @P3pp3rF1y! I'll have a look at it on my end.

Note to self: check if IT is caching caps.

P3pp3rF1y commented 7 months ago

Actually I take it back. I guess I spent too much time debugging that I felt the cache was on your side when in fact it was a wrapper on my side that was getting incorrect handler cached (instead of just getting a Supplier to always get a current one). I have this fixed in dev and will be releasing with next release. Tested with Integrated and the flow described above works correctly.

rubensworks commented 7 months ago

Thanks for checking again @P3pp3rF1y! Looking forward to the next release :-)

CyclopsMC / IntegratedTerminals