Rewrite Item Laser Relay system with support for Slotless Item Handlers
rubensworks opened this issue ยท 19 comments
CyclopsMC/ColossalChests#58 shows that the lasers of this mod have performance issues when interacting with Colossal Chests.
Because of this, I have implemented a simplified variant of the IItemhandler
capability which is exposed by the Common Capabilities mod.
This is required for mods like Colossal Chests that expose a huge amount of slots, which does not scale well for item transfer when a slot-based handler is used. This is not a performance issue of either mods, but simply a direct consequence of the slot-based IItemhandler
capability.
The new ISlotlessItemHandler
capability is slot-agnostic, so it moves the responsibility for finding items when extracting and finding locations when inserting to the capability provider. This makes it possible for mods like Colossal Chest to optimize these actions by indexing its inventories.
Let me know what you think. I'd be happy to assist when implementing this capability.
More details on its usage can be found here: https://github.com/CyclopsMC/CommonCapabilitiesAPI/wiki/Slotless-Item-Handler
The latest dev build of CyclopsMC/CommonCapabilities@2ebd61c provides this new capability.
CyclopsMC/CyclopsCore@f01eb2a and CyclopsMC/ColossalChests@4bf524e have also been updated to provide an efficient implementation of this capability.
CyclopsMC/IntegratedTunnels@034c933 is an item transfer mod that has been updated as well to make use of this capability.
Relevant dev builds:
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/commoncapabilities/CommonCapabilities/1.9.4-1.2.2-74/
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/cyclopscore/CyclopsCore/1.9.4-0.9.0-470/
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/colossalchests/ColossalChests/1.10.2-1.4.2-141/
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/integratedtunnels/IntegratedTunnels/1.10.2-1.0.3-35/
FYI, the slotless item handler capability is now available in the master-1.11
branch of CommonCapabilities.
So where exactly do I need to implement this?
The Item Laser Relay system doesn't input/export items itself, only other machines inputting into and pulling from it do actual item moving work, the Item Lasers just expose their capability.
Also, does implementing your capability require me to have Common Capabilities as a hard dependency? If so, then I can say no to this right away.
Also, if it wouldn't have to be a hard dependency, it would be really cool if you could put this into a Pull Request.
Ideally, you would first check if a slotless item handler exists before checking the regular one. If that exists, you should continue using that one, which can be done very similar to the regular item handler.
And no, Common Capabilities does not have to be a hard dependency, since it's purely capability-based
The easiest way to use it is by adding a git submodule of the API repo.
Alternatively, you could also include it in your build file.
I'm not very familiar with your codebase, so perhaps it's easier if you'd implement this.
But I'm willing to assist you with it if you need any help.
The example on the wiki page I linked should already give you a good basis to start from.
Also, I saw that you've also already updated to 1.11.
I'll update the Common Capabilities API to 1.11 asap, so that you don't have any issues with porting.
I love "already updated" :P
I think 1.10 is outdated and obsolete by this point ;)
I see, now I understand what you meant before.
This is actually very similar to how the item networks in Integrated Tunnels work.
What you can do is exposing a slotless handler yourself, based on the item handlers that are connected with the network. If the connected positions exposes a slotless handler, you can use that directly, otherwise you can wrap it inside a DefaultSlotlessItemHandlerWrapper
.
Example of how it is done in Integrated Tunnels: https://github.com/CyclopsMC/IntegratedTunnels/blob/master-1.10/src/main/java/org/cyclops/integratedtunnels/core/network/ItemNetwork.java#L134-L171
You should however only expose that slotless handler if the capability has been registered (by CommonCapabilities), if you want to keep it an optional dependency.
You should also still keep using your regular item handler capability, for mods that do not support this new interface.
I don't know how to implement this behavior.
First and most importantly, I'd have to change basically the entire system of how I do things because I store everything in Lists of IItemHandler, and the slotless Item Handlers don't implement that so that's already not ideal to work with.
Secondly, the Item Interface works based on the concept of knowing the slot to insert items into and because of that knowing the inventory the item needs to be put into (for example, if you have two chests with 27 slots each, it would know for a number like 31 that it has to put the item into the second chest right away because the index gets shifted that way). If that isn't present, I'd have to loop through the entire system to try to find a spot for the items to go into, which would greatly reduce performance - right?
The other option would be to only allow SlotlessItemHandler -> SlotlessItemHandler interaction, which I personally think is hideous, and it would still not remove the problems of looping through the entire system every time and me having to rewrite this whole thing.
To me, implementing this seems like it would reduce performance more than it would fix performance issues with mods like Colossal Chests.
-
Yes, changes will be required. The slotless IH is indeed a significant change to the slot-based one. If no interface changes would be required, it would've been done already ;-)
-
Yes, you can do this with looping, unless you implement some kind of aggregate indexing mechanism on top of the list of slotless IH's.
Regarding the performance, don't forget that piping systems iterate over slots for regular item handlers. Looping is always present, currently it just doesn't exist explicitly in your code. So in case of the slotless IH, this looping cost is simply moved towards the capability provider, which will result in a performance boost if the provider does proper internal indexing. -
If I were you, I would support the following interactions:
IItemHandler
->IItemHandler
,ISlotlessItemHandler
->ISlotlessItemHandler
,ISlotlessItemHandler
->IItemHandler
.
The third one can easily be combined with the second one using theDefaultSlotlessItemHandlerWrapper
.
This seems like so much work >_>
I heard that in 1.12 all item handling will be completely revamped anyways, so this seems like a little bit of a waste of time to me :V
I looked at this a bit closer and I don't see a good way to integrate the way this slotless stuff wirks with the way my current system works, because the idea of slots existing is inherently baked into the system.
It's up to you.
I don't think modded 1.12 will come any time soon, and 1.10/1.11 will still be used for a while, so players will definitely benefit from a performance improvement.
It's definitely possible in your system, as I've implemented it in Integrated Tunnels as well, which works very similar to your system.
Did you actually look at my system?
It has all this stuff, and then it has this weird nightmarish way to cache everything and.. I don't really feel like changing any of it because it took so long to get working, and with non-insane sized chests, it works really well. >_>
But yes, I see your point in the performance and all that. It's just that I'm really dreading this.
Shouldn't I also implement IItemHandler -> ISlotlessItemHandler? Because otherwise, you couldn't input an item with a hopper into a Colossal chest using the Slotless Item Handler, right?
Implemented this with the last commit 4615a94.
Would be nice if you could look over it to see any fatal errors.
For my filtered machines, I stuck with my filter system and ignored the slotless item handler one, but for the Item Laser Relays, I just pass the filter on to following slotless item handlers.
This should allow items to be extracted -> inserted:
IItemHandler -> IItemHandler
SlotlessItemHandler -> SlotlessItemHandler
SlotlessItemHandler -> IItemHandler,
like you said.
I have found a problem with the way to implement filtered extractions with the slotless item handlers.
When my filter is set to whitelist certain items, I can just loop through the filter and use those filter items as the matchStack in the extractItem method.
But when my filter is set to blacklist certain items, what am I supposed to do then? If I just extract any item, it will always be the first available one in the inventory, and if extracting that one is disallowed, the system will stall. How do I work around that?
EDIT: Found out by speaking to way2muchnoise that this apparently isn't possible, and so I will stick with the way I am doing it now which is to fall back to the normal item handler whenever this occurs.
Great! I'll have a look at your code soon :)
IItemHandler -> ISlotlessItemHandler shouldn't be needed, because Colossal Chests supports the regular IItemHandler as well.
Indeed, blacklisting isn't supported at the moment, I'm wondering whether this should be included as a match flag, or if this should be part of a new interface/capability...
I'll think about it.