Skip to content

Conversation

@jshipley
Copy link
Contributor

@jshipley jshipley commented Oct 15, 2024

Changes to teapot to support picking up water or milk from any block with fluid handler capabilities.
Also added new config option to allow/disallow pickup from fluid handler blocks.

Milk (if it's enabled) is also supported in more cases.

Tested with Flopper, Cooking for Blockheads sink and cow bottle, and Mekanism fluid tank.

Compiled/tested against 1.19, but the changes should also work for the 1.20 branch.

Closes #46

Happy Modtoberfest 2024!

@jshipley
Copy link
Contributor Author

I did most of this before the 1.20.1 PR was merged. Please let me know if you want me to resubmit this for 1.20.1 instead of 1.19.2.

BlockEntity entity = world.getBlockEntity(pos);
Optional<IFluidHandler> fluidHandler = Optional.empty();
if (entity != null) {
fluidHandler = entity.getCapability(ForgeCapabilities.FLUID_HANDLER, side).resolve();
Copy link
Owner

@KnightMiner KnightMiner Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't check the block fluid handler; just make the teapot item expose a fluid handler. See vanilla water buckets as an example (we won't be able to use the same fluid handler exactly, but should be able to use something similar). We shouldn't have to change any of this pickup logic, and that has the added benefit of supporting UI based fluid tanks.

You might find this class from Tinkers' Construct useful for reference, here is how to use it. Just know that is for an empty-only handler, we want a fill only handler. For our sake we also don't need all that supplier stuff, could just statically create the fluid stack as long as it copies in the right spots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the fluid handler to the teapot item simplified the use() method quite a bit, but there are still blocks that aren't working without accessing the fluid handler.

The main blocks I found that weren't working were the MilkJarBlock and CowJarBlock from Cooking for Blockheads. The useItem() methods for those blocks specifically check for the bucket item, and the FluidUtil.tryPickUpFluid() method fails because there's no FluidState for the blocks.

So leaving a FluidUtil.tryFillContainer() attempt if FluidUtil.tryPickUpFluid() fails makes it work in cases where it would otherwise fail.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that sounds like a bug with the mod that adds MilkJarBlock, it would likely also break other mods of mine like Ceramics. We can probably leave that case as not working for now unless its easy to fix that without hardcoding/adding redundant logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably due to milk not being a real fluid in Vanilla. Maybe I'll look into submitting a PR for Cooking for Blockheads to see if it can be improved from that side.

@KnightMiner
Copy link
Owner

I did most of this before the 1.20.1 PR was merged. Please let me know if you want me to resubmit this for 1.20.1 instead of 1.19.2.

Either one is fine, I'll probably copy the PR back and forth once done as the code is mostly similar. Its probably a little easier if both PRs are on 1.20 but this is fine.

Fluid fluid = fluidState.getType();
// comparing with WATER fluid instead of WATER fluid tag, because many/most modded fluids are tagged as WATER
// to get water-like interactions even if the fluid is not water
if (fluid.isSame(Fluids.WATER)) {
Copy link
Owner

@KnightMiner KnightMiner Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also support milk if Forge's ForgeMod#MILK.isPresent() or whatever the check is to see that the milk fluid is enabled. In world pickup is not needed unless its easy to do, but definitely support it in the fluid handler I talk about in my other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between using the fluid handler on the teapot and using FluidUtil to pick up fluids, milk should be working without needing to specially check whether a milk fluid is enabled.

@KnightMiner
Copy link
Owner

As another thought on fluid handlers, might be nice if the filled teapots also act as fluid handlers, so you can dump them out into tanks. Only do it for the not-hot handlers, once its hot its a food now and is relevant to the other PR's domain.

@jshipley jshipley changed the base branch from 1.19 to 1.20 October 16, 2024 14:45
@jshipley
Copy link
Contributor Author

The only problem I see with adding empty-only fluid handlers to the (cold) filled teapots is that a teapot filled with milk will only be able to be emptied if fluid milk is enabled with other mods. So the water teapot and milk teapot will often have different behavior.

I think that the suggestion from #58 to make the cold teapots dumpable with animation, sound effects, and particles would probably be better. That also keeps the teapots from becoming too much like water buckets.

@jshipley
Copy link
Contributor Author

Also, the latest round of changes/testing have all been done on the 1.20 branch.

@KnightMiner
Copy link
Owner

I think that the suggestion from #58 to make the cold teapots dumpable with animation, sound effects, and particles would probably be better. That also keeps the teapots from becoming too much like water buckets

Thats reasonable. Lets stick with teapots being fill only and disallowing empty then.

@jshipley
Copy link
Contributor Author

I've cleaned up a couple of things and removed the last fluid transfer from TeapotItem::use()

@jshipley jshipley requested a review from KnightMiner October 23, 2024 20:08
Copy link
Owner

@KnightMiner KnightMiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one quick fix then this looks good to merge. Was sick last week so not much time to review this.

}
}

// use teapot like a bucket to get fluid, should work in most cases
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if you make the config lower than 1000 for the teapot capacity? If not we need to special case that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the way FluidUtil is handling it, it should be treated as a successful pickup as long as the item's fluid handler returns a non-zero filled amount.

The item fluid handler (FluidTeapotWrapper) uses the configured size. It will either "fill" 0 (fail) or teapotCapacity (success).

@KnightMiner KnightMiner merged commit f9a37fe into KnightMiner:1.20 Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Cross-mod compatibility feature] Take liquid from fluid tanks

2 participants