Railcraft

Railcraft

34M Downloads

Fluid duping when draining Railcraft Creosote Bottles into a Storage Terminal

rubensworks opened this issue ยท 9 comments

commented

This issue was originally reported at CyclopsMC/IntegratedTerminals#46

Description of the Bug

Railcraft Creosote Bottles are magically transformed into whatever fits into your fluid storage when shift-clicking them into the storage terminal's fluid tab. I tried this with a max-upgraded metal drum from "Barrels, Drums, Storage & More"
Also, if I click Creosote Oil fluid and then click an empty bottle, it takes 100 buckets of Creosote Oil out instead of 1.
Picking up an empty or filled bottle and clicking it into the terminal works as expected (i.e. transfers only 1)

I'd assume the same issue arises with Railcraft's other bottle, the Steam Bottle.

To Reproduce

  1. Place a Metal Drum (might work with other storages, too)
  2. Upgrade the Metal Drum using an Upgrade: Capacity x64
  3. Place a Fluid Interface on the Metal Drum
  4. Place a Logic Cable on the Fluid Interface
  5. Place a Storage Terminal on the Logic Cable
  6. Get a single Creosote Bottle
  7. Open the Storage Terminal
  8. Switch to the Fluid Storage tab
  9. Shift-click the Creosote Bottle
    • Expected: Metal Drum now contains 1 bucket of Creosote Oil
    • Actual: Metal Drum now contains 128 buckets of Creosote Oil
  10. If the Metal Drum isn't full, fill it to 128 buckets
  11. Click the Creosote Oil
  12. Click the Glass Bottle
    • Expected: Metal Drum now contains 127 bucket of Creosote Oil
    • Actual: Metal Drum now contains 28 buckets of Creosote Oil

Expected behavior

I expect Creosote Bottles to be worth exactly 1 bucket of Creosote Oil.
Not 100 and certainly not between 32768 and infinity :)

Screenshots & Video

Logs & Environment

  • Full log:
  • Crash report (If available):
$ md5sum {Integrated,Barrels,railcraft}*.jar
c1aff93e2ddcedda892e338dd1cc0950 *IntegratedCrafting-1.12.2-1.0.8.jar
82b15eaa0c2b4421408db074ff464374 *IntegratedDynamics-1.12.2-1.1.0.jar
66f8eef05a5347180992a58adcc3e878 *IntegratedTerminals-1.12.2-1.0.11.jar
4dcb9b2079097ca6ae15cd723f071f94 *IntegratedTunnels-1.12.2-1.6.10.jar
948d49b065fd9f07cc046111d93b0700 *BarrelsDrumsStorageAndMore-0.0.24.jar
b6f0592e8f5c24d07fe065f1c97c8fda *railcraft-12.0.0.jar

Suggested solution

I've done some debugging, and it looks like there are two problems with the FluidContainerCapabilityDispatcher in Railcraft that causes the problem above.

  1. getFluid always returns a valid FluidStack, even if the fluid handler has been drained and the container is an empty bottle.
  2. The drain methods are not overridden, and always allow draining, even if the container is an empty bottle.

Both methods should therefore have some check to see if the container item is the empty bottle.

commented

@rubensworks Where do you interact with the item's fluid handler capability in your code? Mind showing it to me? I guess you did not use IFluidHandlerItem where you should have

commented

Sure, here is the code on my end: https://github.com/CyclopsMC/IntegratedTerminals/blob/master-1.12/src/main/java/org/cyclops/integratedterminals/capability/ingredient/IngredientComponentTerminalStorageHandlerFluidStack.java#L148-L155
For reference, all of the fluid transfer logic is delegated to the Common Capabilities ingedient component API.

I guess you did not use IFluidHandlerItem where you should have

I do, otherwise I wouldn't have received RC's FluidContainerCapabilityDispatcher instance.

I pointed out the solution above, so I highly doubt there's a problem on my end though.

commented

After digging into your codebase (which is apparently as complex as gradle), I discovered that you are indeed handling Fluid Item Handlers incorrectly. Fluid Item Handlers were added to forge because the fluid item handler could expire and the backing item can change its type (causing a new capability attachment), unlike block entities that can be fluid handlers.

https://github.com/CyclopsMC/CommonCapabilities/blob/ee0db035a726a1b368a559ebbab744120a87039a/src/main/java/org/cyclops/commoncapabilities/IngredientComponents.java#L75

commented

I discovered that you are indeed handling Fluid Item Handlers incorrectly.

What makes you say that? I see nothing wrong with the code you referenced.
So you are not acknowledging the problems with RC's FluidContainerCapabilityDispatcher?

Fluid Item Handlers were added to forge because the fluid item handler could expire and the backing item can change its type (causing a new capability attachment), unlike block entities that can be fluid handlers.

I'm well aware of that, this isn't my first encounter with fluid capabilities...
I'm correctly using the resulting container here: https://github.com/CyclopsMC/IntegratedTerminals/blob/master-1.12/src/main/java/org/cyclops/integratedterminals/capability/ingredient/IngredientComponentTerminalStorageHandlerFluidStack.java#L160

commented

You need to refresh the container stack and the fluid item handler reference after performing a drain, but you are not doing that.

commented

You need to refresh the container stack and the fluid item handler reference after performing a drain, but you are not doing that.

The IFluidHandlerItem interface contract does not require the capability reference to be updated after every operation. It is allowed to perform multiple operations before getting the final container stack, which is supported by all other IFluidHandlerItem implementations I am aware of.

As such, Railcraft does not properly implement the IFluidHandlerItem interface. Above, I've explained how you can make the implementation comply to the interface.

commented

good note.

commented

Hi, I'm the person who originally reported this bug to Integrated Terminals.
I'm not sure if you guys agree on which mod needs to be changed yet, but I don't see a fix on either side, yet.
@liach I think I understand @rubensworks's suggestion well enough to make a PR, if you want me to.

commented

eh, problem with pr is that no one will merge it and even if i can merge, no one will release a version with the fix. need covertjaguar