CyclopsMC / IntegratedCrafting

Craft stuff in Integrated Dynamics networks
MIT License
7 stars 6 forks source link

Crafting tasks reporting insufficient resources when maximum stack sizes are above 64 for an inventory #35

Closed Master00Maniac closed 5 years ago

Master00Maniac commented 5 years ago

Issue type:


Short description:

Using Integrated Dynamics, Tunnels, Terminals and Crafting, I've set up a small crafting system whose storage is primarily Arcane Archives (currently a WIP mod). ArcArc features 2 primary item storage methods, Troves and Chests. Both have odd issues when manually queuing a large batch craft with Integrated Crafting.

When attempting to craft items, it appears that the following happens when checking the inventories:

Radiant Chest- Only 64 items are checked per slot, meaning that at maximum the player only has access to 1/4 their total items for autocrafting when using radiant chests

Radiant Trove- Only 128 items appear to be visible for crafting tasks.

Anything beyond these amounts report insufficient items on the crafting job terminal. However, it should be noted that the storage terminal accurately reports the numbers of items contained in these inventories.

One of the ArcArc devs asked me to relay this:

The Radiant Chest has the same number of slots as a double chest, except each slot can hold up to 4x the maximum stack size to be stored per slot, using code from Nether Chest (used with permission and compatible license) which is accurately reported by the handler's getSlotLimit & getStackLimit. This should also be correct reflected by getStackInSlot. It's possibly just a case of "64" being hard-coded as the stack size limit instead of consulting the handler, but I'm not sure.

Steps to reproduce the problem:

  1. Connect an Integrated storage system to a single Radiant Chest, or Radiant Trove.
  2. Place 256 clay (item) inside the inventory
  3. request the crafting of 64 clay blocks

Expected behaviour:

64 clay blocks would be autocrafted


Versions:

Log file:

rubensworks commented 5 years ago

Thanks for reporting, I'll look into it as soon as possible!

noobanidus commented 5 years ago

Hi, I'm a dev of Arcane Archives! I asked Maniac to file this issue as I was too lazy I was doing a bunch of things and didn't want to forget.

For ease and convenience, the most recent version of ArcaneArchives (0.2.0-preview18-hotfix6) can be found here although it currently also depends on the most recent version of MysticalLib. It has an internal embedded dependency on a modified version of Guidebook (changes are being PR'd back into the main codebase) as jar-in-jar.

It's also currently building against forge 2836. If that causes problems for you/your environment, let me know and I should be able to excise the 2836-specific code and revert it back down to 2768 to produce a build for you. (I haven't checked to see what version the Integrated series compile against.)

Currently the mod has had no beta release and is therefore not searchable on CurseForge, but we're looking to have out 0.2.0 beta release Soon.

noobanidus commented 5 years ago

Alternately, the Radiant Chest code is used (with permission & license compatibility) from Nether Chest and theoretically Nether Chest itself should suffer from the same problems, if that's easier for you to use (as I see Crafting is 2781, I believe Nether Chest is compiling against and requiring Forge 2655).

rubensworks commented 5 years ago

Thanks for the information @duely!

rubensworks commented 5 years ago

@duely I just did some debugging, and it seems to be an issue in Forge's ItemStackHandler. As you can see at the linked line, the extracted items are limited to the max stack size, which will always be 64.

Unfortunately, I can't do anything about that on my end. My suggestion would be to extend ItemStackHandler, and override the extractItem method to take into account the changed stack sizes.

noobanidus commented 5 years ago

Oh, thanks for that! I didn't realise that it was actually a Forge issue. I'll modify this on our end before release.

rubensworks commented 5 years ago

Great, thanks! Also reporting this to Nether Chest, and closing this issue here.

GitHub
Build software better, together
GitHub is where people build software. More than 36 million people use GitHub to discover, fork, and contribute to over 100 million projects.
noobanidus commented 5 years ago

After having implemented this in my code, I feel that this is actually the wrong way to go about doing this, as it has had a number of unexpected consequences (including players now picking up the full 256 stack instead of just 64, which is very inconvenient).

A simple loop if the desired stack size is above the max stack size and extractItem returns below that, growing the original stack and repeatedly calling extractItem and then growing the original result by the size of that itemstack would seem like a more fitting approach. I'm not sure where in your code the issue is to make a specific suggestion, but so far I've had to patch the container code to make it more convenient for players interacting with chests.

rubensworks commented 5 years ago

t has had a number of unexpected consequences (including players now picking up the full 256 stack instead of just 64, which is very inconvenient).

Isn't this an issue with how extractItem is being called then? I would expect it to be called with a limit of 64 within GUIs.

A simple loop if the desired stack size is above the max stack size and extractItem returns below that, growing the original stack and repeatedly calling extractItem and then growing the original result by the size of that itemstack would seem like a more fitting approach.

Sure, that works great when you do the effective extraction. However, this is not compatible with simulated extraction, i.e., to check how much items can be effectively extracted. Unfortunately, getStackInSlot can not always be used for this, as this does not guarantee extraction.

noobanidus commented 5 years ago

Isn't this an issue with how extractItem is being called then? I would expect it to be called with a limit of 64 within GUIs.

Unfortunately not, the default value passed into extractItem is the result of slot.getStack()'s getCount function. I've added a patch to the slotClick function that fixes it.

I had a conversation with cpw about it and he basically said that there wasn't a contract indicating that the max stack size must be 64, but that we might run into a lot of trouble with things expecting 64.

I think my current fix will be sufficient for now then.

Sure, that works great when you do the effective extraction. However, this is not compatible with simulated extraction, i.e., to check how much items can be effectively extracted. Unfortunately, getStackInSlot can not always be used for this, as this does not guarantee extraction.

Excellent point! You are definitely correct in that case.

rubensworks commented 5 years ago

I haven't mentioned this yet, but it might also be interesting to look into the slotless item handler. By default, this interface should work with 64+ stacksizes. Integrated Tunnels/Crafting will always first check slotless item handlers, and fallback to regular item handlers, as the former will mostly be more efficient. This for example allows these mods to interact with Colossal Chests very efficiently.

I don't think it's really necessary in this case though, as it is mainly useful for handlers with massive amounts of slots. But perhaps it might be useful elsewhere.

Routhinator commented 4 years ago

I am hitting this problem now with YABBA barrels, and I am wondering if this is something that needs to be fixed in every mod that it affects or IC...

rubensworks commented 4 years ago

@Routhinator Yes, due to the limitations of Forge's item handler, this is something that needs manual integration between mods.