SleepyTrousers / EnderIO-1.5-1.12

http://enderio.com/
The Unlicense
728 stars 356 forks source link

Fluid Duplication interaction with Arcane Archives, Fluid Tank #5245

Open noobanidus opened 5 years ago

noobanidus commented 5 years ago

Greetings!

This issue comes to me from the Arcane Archives issue tracker (demonstration). I initially thought it was a programming issue with the Radiant Amphora, whose FluidItemStackHandler implementation is merely a link to the FluidTank of the associated Radiant Tank, but setting a breakpoint on the fill method and, sure enough, every time it was called, the doFill parameter was set to true.

I'm sure this could actually still be an issue on my end, with me failing to do something expected, but unfortunately I'm not sure what I might be expected to do: I don't have EnderIO actually in my dev environment unfortunately, so outside of knowing that either TileTank::fillEmptyContainer or EnderCore's FluidUtil::tryFillContainer are expecting to get some sort of other result that I'm not providing.

I know my code is pretty spaghetti and I'm probably missing a simple solution or something, but up until this point it appeared to function without issue, so I'm not sure what's gone wrong.

HenryLoenwind commented 5 years ago

Wo what's happening here is that your item has side effects in the world. It (or better: a copy of it) is part of a simulation that is aborted later because the output items could not be stacked, but the act of creating a filled version of the amphora changed the world outside the stack.

ItemStacks are funny that way. They have no concept of "being real" vs "being a 'may exist' copy" or even a "calculation copy". So you can never know if something that happens to a stack actually happens. Rule of thumb in Minecraft would be "if you don't get a world object, you don't change global game state".

noobanidus commented 5 years ago

It's not a side effect, it's a deliberate function of the item. Is there any way you can blacklist the item from the slot? I don't see any logical way to resolve it otherwise.

noobanidus commented 5 years ago

I'd like to suggest instead draining the itemstack once the "simulation" has failed, and not presuming that there are no consequences to calling "doFill" with simulate set to false -- i.e., properly resetting everything, even if it is just a copy. This should function correctly given a code change coming in the next release of Arcane Archives.

HellFirePvP commented 4 years ago

You do have a possible approach to not screw others over. You could make a copy of the itemstack, do a simulate=true to actually see if something works or not, and if it does, check your result with the simulated copy, i.e. "does it stack or not". If everything looks like it works, then you can actually perform those steps with simulate=false and actually perform the operations. So claiming "this is just the codebase's fault" is a bit shallow on your side when there is actual approaches to this problem.

edit: After a more careful consideration (and actually being at a dev environment to check the code), i am aware the issues the current fluid transfer capabilities have. As it stands now, it seems not doable to a handle fluid transfer simulation, transferring fluids into an item-fluidhandler from another fluidhandler with additional care for handling side effects to a full extent. One potential solution i see is to introduce a staging-slot where fluid transfer "is currently happening" - be it the user transferring fluids from the fluidtank into a stack of TE tanks, which fill dynamically up to their respective size, or any other fluidhandler like this one. Essentially, items would be set into that slot where fluid transfer was successful, however the item cannot be stacked into the output slot. Future transfers would attempt to fill into this slot's stack first or attempt to stack it into the output slot before looking at items in the input slot. That would take care of side effects while still retaining the potential complexity around filling a stack of empty cells and merging them back together in the output slot.

noobanidus commented 4 years ago

Another solution I thought of: keep the state of the output item and the second output slot, and, if the item fails to stack, drain the ItemStack capability and then store those states. Compare the items against them (or even set a flag that is only changed when the second output slot is changed) before attempting to fill again -- i.e., "last known bad state".

If draining an ItemStack capability is expensive, then surely repeatedly filling a copy every tick is also not inexpensive: this solution would reduce it to one fill operation, and, only if the stacking is not successful, one potential drain operation, instead of many fill operations per tick.