Ender IO Zoo

Ender IO Zoo

962k Downloads

[1.12.2] EnderIO tanks don't behave as expected (IFluidHandler#fill() method)

desht opened this issue ยท 12 comments

commented

Issue Description:

See TeamPneumatic/pnc-repressurized#138 for the original report.

What happens:

EnderIO's implementation of the IFluidHandler#fill() method appears to always return 0 (i.e. no fluid transferred) when filling a tank, even when the doFill parameter is false. We are using the fluid handler object returned by tileEntity.getCapability(CapabilityFluidHandler.FLUID_HANDLER_CAPABILITY, null); here, if that makes a difference (although if null isn't a valid side for EnderIO tanks, I would expect hasCapability to return false there).

What you expected to happen:

Calling handler.doFill(fluidStack, false) on an empty EnderIO tank, where fluidStack.amount = 1000 should return 1000, not 0.

Steps to reproduce:

  1. With PneumaticCraft: Repressurized installed, shift-right-click an empty EnderIO tank with the Amadron Tablet. Also shift-right-click a chest full of emeralds with the tablet.
  2. Right-click the tablet to open the trades GUI.
  3. The trades for "5 emeralds to 1000mB Lubricant" should be available at this point, but is not, since the tablet's test for the tank to receive 1000mB Lubricant failed, as described above.

Affected Versions (Do not use "latest"):

  • EnderIO: 1.12.2-5.0.20
  • EnderCore: 1.12.2-0.5.18
  • Minecraft: 1.12.2
  • Forge: 1.12.2-14.23.2.2654

Your most recent log file where the issue was present:

[pastebin/gist/etc link here]

commented

'null' is not a valid side, but there are to many mods out there that throw a hissy fit if there's nothing at the 'null' side. @McJty

PS:
image

commented

Fair enough. We can work sidedness into the Amadron tablet easily, I guess.

commented
/**
 * Determines if this object has support for the capability in question on the specific side.
 * The return value of this MIGHT change during runtime if this object gains or looses support
 * for a capability.
 *
 * Example:
 *   A Pipe getting a cover placed on one side causing it loose the Inventory attachment function for that side.
 *
 * This is a light weight version of getCapability, intended for metadata uses.
 *
 * @param capability The capability to check
 * @param facing The Side to check from:
 *   CAN BE NULL. Null is defined to represent 'internal' or 'self'
 * @return True if this object supports the capability.
 */
boolean hasCapability(@Nonnull Capability<?> capability, @Nullable EnumFacing facing);
commented

'null' is a valid input for capabilities. It means access not restricted by sidedness

commented

Agreed, but for a tank that requires sidedness to determine if the side it is filling from is disabled, it will need a sided access

commented

yes, but even then you can still support a fluid handler specific to side null which at least has the option to examine what fluid is in the tank. That way you have compatibility with mods like TOP or the RFTools liquid monitor. That's how Deep Resonance solves this. That mod also has tanks that have configurable sides for input/output but still it is possible to examine liquid content if you use null for side

commented

So a way of making the tank "read-only?"

commented

yes, at least if 'null' is given as the side. Just return an IFluidHandler that doesn't allow extraction/insertion but does return what the tank contains

commented

In the case of tanks you could in theory also opt for just allowing insertion/extraction if null side is given. This would make it compatible with mods that do remote access on liquids and can extract/insert them. But this is not really a requirement and I think it is ok if you don't want to do that

commented

and this is exactly what we are doing...and what PneumaticCraft: Repressurized did not expect.

Sry, McJty, I wanted to write "()" not "@". That's what I get for multitasking...

commented

I'm fine with updating PNC:R to have the Amadron tablet record the clicked side of the block as well, that's easy to do (and honestly more correct than the current behaviour which is a bit of a hold-over from the 1.7.10 port). So no complaints from me about the "wontfix" :)

commented

Yeah, that side parameter on the capabilities could use some improvement. There are so many cases that are all rolled into "null". It makes a semantic difference if a pipe is attached to a side or if the user clicked the block and just hit that side by chance. But mapping the later to null would confuse it with wireless liquid chargers...or mods that forget to add a side.