refactor Drums,

[Spicierspace153]

Drums w/ buckets now work as intended in every single way

[chrombread]

quick suggestion

[danyadev]

Love the IFluidContainerItem support, but... I tried to fill a drum while holding a stack of Certus Quartz Tanks with 16000L water and they all just gone

tank.-.drum.mp4

[github-actions]

#56

[github-actions]

#57

[github-actions]

#99

[SuperSoupr]

Mostly nits but some fixes needed as well, otherwise LGTM 👍

[github-actions]

#102

[SuperSoupr]

Sorry I didn't think of this before but can you add a config for the size of the regular and bedrockium drums?

[github-actions]

#103

stale

[SuperSoupr]

Why ChatComponentTranslation? you aren't giving it a translation key...
either make it a translation key or replace with ChatComponentText

also the formatter puts a q after the number if it's 0 for some reaons

[SuperSoupr]

I don't know how to fix this, but there is a desync between the client and server here. The client doesn't get told how full the drum is when it's loaded. Not sure of how impactful this is but the onBlockActivated logic is wrong on the client because of it

[Justxs]

This method is HUGE and should be divided into smaller parts.
Here is how I would refactor the current code to improve readability (logic kept the same):

@Override
public boolean onBlockActivated(
    World world, int x, int y, int z,
    EntityPlayer player, int side,
    float hitX, float hitY, float hitZ
) {
    TileEntity te = world.getTileEntity(x, y, z);
    if (!(te instanceof TileEntityDrum drum)) {
        return false;
    }

    FluidTank tank = drum.tank;
    FluidStack fluid = tank.getFluid();
    ItemStack held = player.inventory.getCurrentItem();

    if (held == null) {
        return handleEmptyHand(world, player, tank);
    }

    Item item = held.getItem();

    return fluid == null
        ? handleEmptyDrum(world, x, y, z, player, tank, held, item)
        : handleFilledDrum(world, x, y, z, player, tank, fluid, held, item);
}

private boolean handleEmptyHand(World world, EntityPlayer player, FluidTank tank) {
    if (world.isRemote) {
        return false;
    }

    player.addChatComponentMessage(
        new ChatComponentTranslation(
            "%s %s",
            NumberFormat.DEFAULT.format(tank.getFluidAmount()),
            DrumConfig.unitToDisplay ? "mB" : "L"
        )
    );
    return true;
}

private boolean handleEmptyDrum(
    World world, int x, int y, int z,
    EntityPlayer player,
    FluidTank tank,
    ItemStack stack,
    Item item
) {
    if (item instanceof IFluidContainerItem container) {
        return fillDrumFromContainer(world, x, y, z, player, tank, stack, container);
    }

    if (item instanceof ItemBucket) {
        return fillDrumFromBucket(world, x, y, z, player, tank, stack);
    }

    return true;
}

private boolean fillDrumFromContainer(
    World world, int x, int y, int z,
    EntityPlayer player,
    FluidTank tank,
    ItemStack stack,
    IFluidContainerItem container
) {
    FluidStack fluid = container.getFluid(stack);
    if (fluid == null) {
        return true;
    }

    int fillAmount = Math.min(tank.getCapacity(), fluid.amount);
    FluidStack toFill = fluid.copy();
    toFill.amount = fillAmount;

    int filled = tank.fill(toFill, true);
    container.drain(stack, filled, true);

    if (!world.isRemote) {
        player.addChatComponentMessage(
            new ChatComponentTranslation(
                "message.drum.filled",
                filled,
                fluid.getLocalizedName()
            )
        );
    }

    world.markBlockForUpdate(x, y, z);
    return true;
}

private boolean fillDrumFromBucket(
    World world, int x, int y, int z,
    EntityPlayer player,
    FluidTank tank,
    ItemStack stack
) {
    FluidStack bucketFluid = FluidContainerRegistry.getFluidForFilledItem(stack);
    if (bucketFluid == null) {
        return false;
    }

    int filled = tank.fill(bucketFluid.copy(), true);
    if (filled <= 0) {
        return false;
    }

    if (!world.isRemote) {
        consumeItemStack(player, stack, new ItemStack(Items.bucket));
        player.addChatComponentMessage(
            new ChatComponentTranslation(
                "message.drum.filled",
                filled,
                bucketFluid.getLocalizedName()
            )
        );
    }

    world.markBlockForUpdate(x, y, z);
    return true;
}

private boolean handleFilledDrum(
    World world, int x, int y, int z,
    EntityPlayer player,
    FluidTank tank,
    FluidStack fluid,
    ItemStack stack,
    Item item
) {
    if (item instanceof IFluidContainerItem container) {
        return drainDrumIntoContainer(world, x, y, z, player, tank, fluid, stack, container);
    }

    if (item instanceof ItemBucket) {
        return handleBucketDrain(world, x, y, z, player, tank, fluid, stack);
    }

    return true;
}

[Justxs]

nesting could be reduced here

private static void consumeItemStack(
    EntityPlayer player,
    ItemStack hand,
    ItemStack result
) {
    InventoryPlayer inv = player.inventory;

    if (hand.stackSize == 1) {
        inv.setInventorySlotContents(inv.currentItem, result);
        finalizeInventoryChange(player);
        return;
    }

    hand.stackSize--;

    if (!inv.addItemStackToInventory(result)) {
        player.dropPlayerItemWithRandomChoice(result, false);
    }

    finalizeInventoryChange(player);
}

private static void finalizeInventoryChange(EntityPlayer player) {
    player.inventoryContainer.detectAndSendChanges();
    player.inventory.markDirty();
}